refactor: simplify code and hide non-implemented instruments #3003
Conversation
This file contains hidden or 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
Contributor
Reviewer's GuideRefactors the instruments screen to use a unified instrument data model and index-based navigation, removes duplicated switch/case routing logic, and temporarily hides non-implemented instruments by omitting them from the unified list while updating search, filtering, and rendering to rely on the new structure. Sequence diagram for data-driven instrument navigationsequenceDiagram
actor User
participant InstrumentsScreen as InstrumentsScreenState
participant InstrumentData as _InstrumentData
participant Navigator
User->>InstrumentsScreen: tap instrument at listIndex
InstrumentsScreen->>InstrumentsScreen: _onItemTapped(listIndex)
InstrumentsScreen->>InstrumentData: instrument = _instrumentDatas[listIndex]
alt route already on top of stack
InstrumentsScreen->>Navigator: canPop(context)
InstrumentsScreen->>Navigator: popUntil(routeWithName(instrument.name))
else route not on top of stack
InstrumentsScreen->>Navigator: pushNamedAndRemoveUntil(instrument.name, isFirstRoute)
end
Class diagram for unified instrument data modelclassDiagram
class _InstrumentData {
+String heading
+String description
+String name
+_InstrumentData(String heading, String description, String name)
}
class _InstrumentsScreenState {
+List~int~ _filteredIndices
+AppLocalizations appLocalizations
+List~_InstrumentData~ _instrumentDatas
+void _onItemTapped(int index)
+void _filterInstruments(String query)
+void initState()
+Widget build(BuildContext context)
}
_InstrumentsScreenState --> _InstrumentData : uses
File-Level Changes
Assessment against 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}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Several
_InstrumentDataentries use the non-Desclocalization key for thedescription(e.g.appLocalizations.multimeterinstead ofmultimeterDesc), which will show the title text instead of the intended description; please switch these to the corresponding*Desckeys where applicable. - Consider making
_InstrumentData’s fieldsfinaland constructing_instrumentDatasas aconstlist to better express immutability and potentially enable small performance optimizations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several `_InstrumentData` entries use the non-`Desc` localization key for the `description` (e.g. `appLocalizations.multimeter` instead of `multimeterDesc`), which will show the title text instead of the intended description; please switch these to the corresponding `*Desc` keys where applicable.
- Consider making `_InstrumentData`’s fields `final` and constructing `_instrumentDatas` as a `const` list to better express immutability and potentially enable small performance optimizations.
## Individual Comments
### Comment 1
<location> `lib/view/instruments_screen.dart:92-101` </location>
<code_context>
+ _InstrumentData(appLocalizations.multimeter, appLocalizations.multimeter,
</code_context>
<issue_to_address>
**issue (bug_risk):** Descriptions for most instruments are using the title string instead of the dedicated `*Desc` localization, changing UI text compared to previous behavior.
In the new `_instrumentDatas` list, only `oscilloscope` still uses its `*Desc` value; the others now reuse the heading key for the description (e.g. `appLocalizations.multimeter` for both fields). This will make the description text identical to the heading and drop the explanatory copy. Please switch these to the corresponding `*Desc` localizations (e.g. `appLocalizations.multimeterDesc`, `logicAnalyzerDesc`, `sensorsDesc`, etc.) to match prior behavior.
</issue_to_address>
### Comment 2
<location> `lib/view/instruments_screen.dart:16-22` </location>
<code_context>
State<StatefulWidget> createState() => _InstrumentsScreenState();
}
+class _InstrumentData {
+ String heading;
+ String description;
+ String name;
+
+ _InstrumentData(this.heading, this.description, this.name);
+}
+
</code_context>
<issue_to_address>
**suggestion:** Consider making `_InstrumentData` immutable by using `final` fields.
Because `_InstrumentData` is only created in `initState` and then read, these fields don’t need to change. Making `heading`, `description`, and `name` `final` clarifies that the data is immutable and avoids accidental mutation, which is important for navigation/search data that can be hard to debug if modified unexpectedly.
```suggestion
class _InstrumentData {
final String heading;
final String description;
final String name;
const _InstrumentData(this.heading, this.description, this.name);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Build StatusBuild successful. APKs to test: https://github.com/fossasia/pslab-app/actions/runs/20215378713/artifacts/4865734429. ScreenshotsAndroid Screenshots
iPhone Screenshots
iPad Screenshots
|
||||||||||||||||||||||||
-1_instruments_screen.png?raw=true)
-2_nav_drawer.png?raw=true)
-3_accelerometer.png?raw=true)
-4_power_source.png?raw=true)
-5_multimeter.png?raw=true)
-6_wave_generator.png?raw=true)
-7_oscilloscope.png?raw=true)
I ❤️ immutable objects!
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Now that the list of instruments is driven by
_instrumentDatas, theinstrumentIconsarray inconstants.dartshould be updated to contain the same instruments in the same order (and with the removed instruments also removed or moved), otherwise the icons will be misaligned with the visible instruments (e.g.,soundMetercurrently reuses thegasSensoricon index). - For the commented-out instrument entries (
gasSensor,dustSensor), consider normalizing their futurenamevalues to match the other routes (i.e., include the leading/) so they can be safely uncommented later without introducing routing inconsistencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that the list of instruments is driven by `_instrumentDatas`, the `instrumentIcons` array in `constants.dart` should be updated to contain the same instruments in the same order (and with the removed instruments also removed or moved), otherwise the icons will be misaligned with the visible instruments (e.g., `soundMeter` currently reuses the `gasSensor` icon index).
- For the commented-out instrument entries (`gasSensor`, `dustSensor`), consider normalizing their future `name` values to match the other routes (i.e., include the leading `/`) so they can be safely uncommented later without introducing routing inconsistencies.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
I will create a separate issue for this.
Done in last commit. |
CloudyPadmal
approved these changes
Dec 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #2986
Changes
Screenshots / Recordings
Old Version
New Version
Checklist:
constants.dartor localization files instead of hard-coded values.dart formator the IDE formatter.flutter analyzeand tests run influtter test.Summary by Sourcery
Refactor the instruments screen to use a unified data model for instruments and data-driven navigation, while temporarily hiding non-implemented instruments from the list.
Bug Fixes:
Enhancements:
Summary by Sourcery
Refactor the instruments screen to use a single unified data model for instruments and simplify navigation and search logic while hiding instruments that are not yet implemented.
Bug Fixes:
Enhancements: