-
Notifications
You must be signed in to change notification settings - Fork 809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: added pin layout screens for PSLab v6 #2625
base: development
Are you sure you want to change the base?
feat: added pin layout screens for PSLab v6 #2625
Conversation
Reviewer's Guide by SourceryThis pull request adds pin layout screens for PSLab v6, including front and back views with color-coded pin descriptions. It also updates the strings and colors resources to include the new pin layouts. Class diagram for PSLab v6 Pin Layout implementationclassDiagram
class PSLabPinLayoutFragment_v6 {
-List~PinDetails_v6~ pinDetails_v6
-Matrix matrix
-Matrix savedMatrix
+static boolean topside
+static PSLabPinLayoutFragment_v6 newInstance()
+onCreateView()
+onResume()
+onPause()
-populatePinDetails()
-getColor(int colorId)
+onTouch(View v, MotionEvent event)
-displayPinDescription(PinDetails_v6)
-spacing(MotionEvent)
-midPoint(PointF, MotionEvent)
}
class PinDetails_v6 {
-String name
-String description
-int colorID
+PinDetails_v6(String name, String description, int colorID)
+getName()
+getDescription()
+getColorID()
}
PSLabPinLayoutFragment_v6 --> PinDetails_v6 : uses
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @samruddhi-Rahegaonkar - I've reviewed your changes - here's some feedback:
Overall Comments:
- In PSLabPinLayoutFragment_v6's populatePinDetails method, the fourth square wave entry mistakenly uses getString(R.string.square_wave_3) instead of getString(R.string.square_wave_4). Update the string resource to ensure the correct label is used.
- The onTouch implementation uses getDrawingCache() to capture the color map bitmap, which is deprecated and can lead to inconsistent behavior. Consider using a more reliable method to capture the bitmap or compute the touched pixel from the resource.
- Calling System.gc() in onPause() is generally discouraged as it can adversely affect performance. Remove it or replace it with more targeted memory management techniques if necessary.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@samruddhi-Rahegaonkar Thanks for your work! So two things before we move forward with this:
|
503eb34
to
75944dc
Compare
Added a pin layout screens for PSLab v6.
75944dc
to
acfd78d
Compare
Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/13180329712/artifacts/2547716908 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samruddhi-Rahegaonkar Reviewed. Lots of work to be done here.
I may have missed more errors. Please kindly check all the pins again once.
@@ -86,6 +87,7 @@ public class MainActivity extends AppCompatActivity { | |||
private static final String TAG_INSTRUMENTS = "instruments"; | |||
private static final String TAG_ABOUTUS = "aboutUs"; | |||
private static final String TAG_PINLAYOUT = "pinLayout"; | |||
private static final String TAG_PINLAYOUT_V6 = "PINLAYOUTV6"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static final String TAG_PINLAYOUT_V6 = "PINLAYOUTV6"; | |
private static final String TAG_PINLAYOUT_V6 = "pinLayoutV6"; |
@@ -561,7 +589,6 @@ public void onReceive(Context context, Intent intent) { | |||
} | |||
} | |||
}; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file MainActivity.java has formatting issues. Kindly reformat the whole file as we have discussed earlier as well (Ctrl + A, Ctrl + Alt + L)
@@ -1,5 +1,5 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<resources> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file doesn't require any changes, does it ?
Kindly rollback this whole file, if it doesn't.
<color name="sq2">#D35FBC</color> | ||
<color name="sq3">#C837AB</color> | ||
<color name="sq4">#A02C89</color> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reformat, also removing the extra leading spaces at the end.
@@ -0,0 +1,272 @@ | |||
package io.pslab.fragment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file requires reformatting as well.
pinDetails_v6.add(new PinDetails_v6(getString(R.string.cs1), getString(R.string.cs1_description), getColor(R.color.cs1))); | ||
pinDetails_v6.add(new PinDetails_v6(getString(R.string.sdi), getString(R.string.sdi_description), getColor(R.color.sdi))); | ||
pinDetails_v6.add(new PinDetails_v6(getString(R.string.sck), getString(R.string.sck_description), getColor(R.color.sclk))); | ||
pinDetails_v6.add(new PinDetails_v6(getString(R.string.sdo), getString(R.string.sdo_description), getColor(R.color.sdo))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the color for this pin is configured wrongly in the image. Clicking this image causes the dialog for the SCK pin above to be opened.
<!-- Voltage Levels --> | ||
<string name="vdd_3_3v_description" translatable="false">VDD (+3.3V) power supply pin.</string> | ||
<string name="voltage_0_3_3v_description" translatable="false">Voltage level pin (0-3.3V).</string> | ||
<string name="voltage_plus_3_3v_description" translatable="false">Positive voltage level (+3.3V).</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a Positive Voltage Level.
<string name="voltage_plus_3_3v_description" translatable="false">Positive voltage level (+3.3V).</string> | |
<string name="voltage_plus_3_3v_description" translatable="false">Variable voltage pin (±3.3V).</string> |
|
||
<!-- Voltage Levels --> | ||
<string name="vdd_3_3v_description" translatable="false">VDD (+3.3V) power supply pin.</string> | ||
<string name="voltage_0_3_3v_description" translatable="false">Voltage level pin (0-3.3V).</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string name="voltage_0_3_3v_description" translatable="false">Voltage level pin (0-3.3V).</string> | |
<string name="voltage_0_3_3v_description" translatable="false">Variable voltage pin (0-3.3V).</string> |
<string name="vdd_3_3v_description" translatable="false">VDD (+3.3V) power supply pin.</string> | ||
<string name="voltage_0_3_3v_description" translatable="false">Voltage level pin (0-3.3V).</string> | ||
<string name="voltage_plus_3_3v_description" translatable="false">Positive voltage level (+3.3V).</string> | ||
<string name="voltage_plus_minus_5_0v_description" translatable="false">Dual voltage supply (±5.0V).</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string name="voltage_plus_minus_5_0v_description" translatable="false">Dual voltage supply (±5.0V).</string> | |
<string name="voltage_plus_minus_5_0v_description" translatable="false">Variable voltage (±5.0V).</string> |
<string name="voltage_0_3_3v_description" translatable="false">Voltage level pin (0-3.3V).</string> | ||
<string name="voltage_plus_3_3v_description" translatable="false">Positive voltage level (+3.3V).</string> | ||
<string name="voltage_plus_minus_5_0v_description" translatable="false">Dual voltage supply (±5.0V).</string> | ||
<string name="current_3ma_description" translatable="false">Current pin (3mA).</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<string name="current_3ma_description" translatable="false">Current pin (3mA).</string> | |
<string name="current_3ma_description" translatable="false">Current pin (<=3mA).</string> |
Fixes #2499
Summary by Sourcery
Add pin layout screens for PSLab v6.
New Features:
Tests: