feat: ported barometer instrument #2743
Open
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
Reviewer's GuidePorts the barometer instrument end-to-end by adding a dedicated Flutter screen with gauge and chart UIs, integrating a BarometerStateProvider for live sensor streaming, refactoring existing stats widgets for dynamic unit and font support, and updating navigation routes and constants. Class diagram for Instrumentstats and StatItem refactorclassDiagram
class Instrumentstats {
+ double titleFontSize
+ double statFontSize
+ double maxValue
+ double minValue
+ double avgValue
+ String unit
+ Widget build(BuildContext context)
}
class StatItem {
+ String label
+ double value
+ double fontSize
+ Widget build(BuildContext context)
}
Instrumentstats --> StatItem : uses
File-Level Changes
Assessment against linked issues
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 @Yugesh-Kumar-S - I've reviewed your changes - here's some feedback:
- The hard-coded
case 6,case 7,case 8indices in InstrumentsScreen make navigation brittle—consider using named enums or a list of instrument routes instead of magic numbers. - The altitude-from-pressure formula is duplicated in both the provider and the chart widget; centralize that conversion in one place to avoid drift and maintenance overhead.
- You’re firing a Timer every second and calling notifyListeners even if the pressure hasn’t changed—consider updating the chart only on new sensor events or throttling UI rebuilds to improve performance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded `case 6`, `case 7`, `case 8` indices in InstrumentsScreen make navigation brittle—consider using named enums or a list of instrument routes instead of magic numbers.
- The altitude-from-pressure formula is duplicated in both the provider and the chart widget; centralize that conversion in one place to avoid drift and maintenance overhead.
- You’re firing a Timer every second and calling notifyListeners even if the pressure hasn’t changed—consider updating the chart only on new sensor events or throttling UI rebuilds to improve performance.
## Individual Comments
### Comment 1
<location> `lib/view/barometer_screen.dart:132` </location>
<code_context>
+ );
+ }
+
+ Widget altitudeTitleWidgets(
+ double value, TitleMeta meta, double maxPressure) {
+ final screenWidth = MediaQuery.of(context).size.width;
</code_context>
<issue_to_address>
Remove unused 'maxPressure' parameter
Since 'maxPressure' is not used in the function, please remove it from the parameter list to simplify the code.
Suggested implementation:
```
Widget altitudeTitleWidgets(
double value, TitleMeta meta) {
final screenWidth = MediaQuery.of(context).size.width;
```
You must also update all calls to `altitudeTitleWidgets` in this file to remove the third argument (`maxPressure`). For example, change:
```dart
altitudeTitleWidgets(value, meta, maxPressure)
```
to:
```dart
altitudeTitleWidgets(value, meta)
```
</issue_to_address>
### Comment 2
<location> `lib/providers/barometer_state_provider.dart:139` </location>
<code_context>
+ _dataCount > 0 ? _pressureSum / _dataCount : 0.0;
+
+ double getCurrentAltitude() => _pressureToAltitude(_currentPressure);
+ double getMinAltitude() =>
+ _pressureMin > 0 ? _pressureToAltitude(_pressureMax) : 0.0;
+ double getMaxAltitude() =>
+ _pressureMax > 0 ? _pressureToAltitude(_pressureMin) : 0.0;
</code_context>
<issue_to_address>
Incorrect guard condition in getMinAltitude
Consider checking _pressureMax > 0 or _dataCount > 0 instead, since _pressureMax is used in the calculation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/15756499018/artifacts/3362531181 |
AsCress
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.
Fixes #2742
Changes
Screenshots / Recordings
Simulated using virtual sensors
barometer.webm
Checklist:
strings.xml,dimens.xmlandcolors.xmlwithout hard coding any value.strings.xml,dimens.xmlorcolors.xml.Summary by Sourcery
Port the barometer instrument into the Flutter app by adding a dedicated screen, provider, and widget components, updating navigation, and enhancing existing stat widgets for dynamic units.
New Features:
Enhancements: