fix: Consider ordering list of sensors alphabetically #2597 #2599
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix fossasia#2597 Alphabetical-order-sensor
Reviewer's Guide by SourceryThis PR introduces alphabetical ordering to the list of displayed sensors in the SensorActivity. It also updates the Gradle dependencies to specific versions. Sequence diagram for sensor detection and display flowsequenceDiagram
participant User
participant SA as SensorActivity
participant PS as PopulateSensors
participant I2C
User->>SA: Click Auto Scan
SA->>PS: execute()
PS->>I2C: scan()
I2C-->>PS: detectedAddresses
PS->>PS: Process detected sensors
PS->>PS: Sort sensors alphabetically
PS-->>SA: Update UI with sorted list
SA-->>User: Display sorted sensor list
Class diagram showing changes to SensorActivityclassDiagram
class SensorActivity {
-I2C i2c
-ScienceLab scienceLab
-Map<Integer, List<String>> sensorAddr
-List<String> dataName
-ArrayAdapter<String> adapter
-ListView lvSensor
+onCreate(Bundle)
-addSensorToMap(int address, String sensorName)
}
class PopulateSensors {
-List<Integer> detectedAddresses
+doInBackground()
+onPostExecute()
}
SensorActivity *-- PopulateSensors
note for SensorActivity "Changed sensorAddr from Map<Integer, String>
to Map<Integer, List<String>> to handle
duplicate addresses"
note for PopulateSensors "Renamed 'data' to 'detectedAddresses'
and improved sensor detection logic"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @samruddhi-Rahegaonkar - I've reviewed your changes - here's some feedback:
Overall Comments:
- The gradle changes need attention: 1) Downgrading com.android.application from 8.7.3 to 8.5.1 should be justified or reverted 2) IndicatorSeekBar dependency is specified twice with different version formats
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 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.
Comment on lines
180
to
189
| protected void onPostExecute(Void aVoid) { | ||
| super.onPostExecute(aVoid); | ||
|
|
||
| StringBuilder tvData = new StringBuilder(); | ||
| if (data != null) { | ||
| for (Integer myInt : data) { | ||
| if (myInt != null && sensorAddr.get(myInt) != null) { | ||
| dataAddress.add(String.valueOf(myInt)); | ||
| if (scienceLab.isConnected() && detectedAddresses != null) { | ||
| for (Integer address : detectedAddresses) { | ||
| if (sensorAddr.containsKey(address)) { | ||
| dataName.addAll(sensorAddr.get(address)); | ||
| } | ||
| } | ||
|
|
||
| for (final String s : dataAddress) { | ||
| tvData.append(s).append(":").append(sensorAddr.get(Integer.parseInt(s))).append("\n"); | ||
| } | ||
|
|
||
| } else { | ||
| tvData.append(getResources().getString(R.string.sensor_not_connected)); | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): The connected state message is incorrect - it always shows 'not_connected' even when scienceLab.isConnected() is true
This could confuse users about the actual connection state of their device. Consider showing the list of detected sensors and their addresses when connected.
Comment on lines
+191
to
+199
| // Add all sensors, even if not detected | ||
| for (List<String> sensors : sensorAddr.values()) { | ||
| for (String sensor : sensors) { | ||
| if (!dataName.contains(sensor)) { | ||
| dataName.add(sensor); | ||
| } | ||
| } | ||
| } | ||
| Collections.sort(dataName); |
There was a problem hiding this comment.
suggestion: The UI doesn't distinguish between detected and available sensors, which could be misleading
Consider visually indicating which sensors are actually detected vs which ones are just potentially available, perhaps using different styles or grouping in the ListView.
Suggested implementation:
// Create map of sensor names to their detection status
Map<String, Boolean> sensorStatus = new HashMap<>();
// First add all available sensors as undetected
for (List<String> sensors : sensorAddr.values()) {
for (String sensor : sensors) {
sensorStatus.put(sensor, false);
}
}
// Mark detected sensors
if (detectedAddresses != null) {
for (int address : detectedAddresses) {
if (sensorAddr.containsKey(address)) {
for (String sensor : sensorAddr.get(address)) {
sensorStatus.put(sensor, true);
}
}
}
}
// Create sorted list of sensor names
List<String> sortedSensors = new ArrayList<>(sensorStatus.keySet());
Collections.sort(sortedSensors);
// Create and set custom adapter
SensorListAdapter adapter = new SensorListAdapter(this, sortedSensors, sensorStatus);
listView.setAdapter(adapter);
if (scienceLab.isConnected()) {You'll need to create a new SensorListAdapter class that extends ArrayAdapter<String>. Here's a template for it:
public class SensorListAdapter extends ArrayAdapter<String> {
private final Map<String, Boolean> sensorStatus;
public SensorListAdapter(Context context, List<String> sensors, Map<String, Boolean> status) {
super(context, android.R.layout.simple_list_item_1, sensors);
this.sensorStatus = status;
}
@Override
public View getView(int position, View convertView, ViewGroup parent) {
View view = super.getView(position, convertView, parent);
TextView text = view.findViewById(android.R.id.text1);
String sensor = getItem(position);
// Style based on detection status
if (sensorStatus.get(sensor)) {
text.setTextColor(ContextCompat.getColor(getContext(), android.R.color.black));
text.setTypeface(null, Typeface.NORMAL);
} else {
text.setTextColor(ContextCompat.getColor(getContext(), android.R.color.darker_gray));
text.setTypeface(null, Typeface.ITALIC);
}
return view;
}
}Also ensure you have a ListView with id listView in your layout XML file.
|
@samruddhi-Rahegaonkar If you have rebased your changes over the commits in #2589, then let's work on merging that one first and then work on this one. It's a nice practice though to always create new branches from a single branch that remains updated with the upstream ( |
|
@AsCress yes! |
marcnause
changed the title
marcnause
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix : Consider ordering list of sensors alphabetically #2597
Summary by Sourcery
Bug Fixes: