fix: handle dust and gas sensor interactions#3143
Conversation
There was a problem hiding this comment.
Sorry @Irfan-del-droid, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Build StatusBuild successful. APKs to test: https://github.com/fossasia/pslab-app/actions/runs/23708113356/artifacts/6165535999. Screenshots |
|
The RCE warnings are pre-existing security advisories in the base repository, |
There was a problem hiding this comment.
Pull request overview
Adds placeholder navigation targets for Gas Sensor and Dust Sensor so the instruments menu no longer contains dead/unresponsive entries (part of resolving #3136), by wiring new screens into the app’s route table and instruments list.
Changes:
- Enabled Gas Sensor and Dust Sensor items in the instruments list and routed them to new screens.
- Added new placeholder screens for Gas Sensor and Dust Sensor with “not implemented” messaging.
- Removed the
push-event.ymlGitHub Actions workflow (push-based CI).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/view/instruments_screen.dart | Re-enables Gas/Dust sensor entries and points them to named routes. |
| lib/view/gas_sensor_screen.dart | New placeholder screen for Gas Sensor. |
| lib/view/dust_sensor_screen.dart | New placeholder screen for Dust Sensor. |
| lib/main.dart | Registers named routes for the new screens. |
| .github/workflows/push-event.yml | Deletes the push-triggered CI workflow. |
Comments suppressed due to low confidence (1)
.github/workflows/push-event.yml:1
- This PR deletes the entire
push-event.ymlworkflow, but the PR description is focused on Dust/Gas sensor navigation. Removing the push workflow can eliminate CI/builds on direct pushes to theflutterbranch; if this is intentional, it should be called out explicitly and/or replaced with an updated workflow. Otherwise, please revert this deletion and keep workflow changes in a separate PR.
| @override | ||
| void didChangeDependencies() { | ||
| _setPortraitOrientation(); | ||
| SystemChrome.setEnabledSystemUIMode(SystemUiMode.edgeToEdge); | ||
| super.didChangeDependencies(); | ||
| } | ||
|
|
There was a problem hiding this comment.
_setPortraitOrientation() and SystemChrome.setEnabledSystemUIMode(...) are invoked in both initState and didChangeDependencies, which is redundant and makes lifecycle behavior harder to reason about. Consider keeping this logic in a single lifecycle method (typically initState or a post-frame callback) unless there is a specific dependency-change scenario you need to handle.
| @override | |
| void didChangeDependencies() { | |
| _setPortraitOrientation(); | |
| SystemChrome.setEnabledSystemUIMode(SystemUiMode.edgeToEdge); | |
| super.didChangeDependencies(); | |
| } |
| @override | ||
| void didChangeDependencies() { | ||
| _setPortraitOrientation(); | ||
| SystemChrome.setEnabledSystemUIMode(SystemUiMode.edgeToEdge); | ||
| super.didChangeDependencies(); | ||
| } | ||
|
|
There was a problem hiding this comment.
_setPortraitOrientation() and SystemChrome.setEnabledSystemUIMode(...) are invoked in both initState and didChangeDependencies, which is redundant and makes lifecycle behavior harder to reason about. Consider keeping this logic in a single lifecycle method (typically initState or a post-frame callback) unless there is a specific dependency-change scenario you need to handle.
| @override | |
| void didChangeDependencies() { | |
| _setPortraitOrientation(); | |
| SystemChrome.setEnabledSystemUIMode(SystemUiMode.edgeToEdge); | |
| super.didChangeDependencies(); | |
| } |
| body: Center( | ||
| child: Padding( | ||
| padding: const EdgeInsets.all(24.0), | ||
| child: Column( | ||
| mainAxisAlignment: MainAxisAlignment.center, | ||
| children: [ | ||
| Icon( | ||
| Icons.air, | ||
| size: 120, | ||
| color: Colors.grey.shade400, | ||
| ), | ||
| const SizedBox(height: 32), | ||
| Text( | ||
| appLocalizations.gasSensor, | ||
| style: Theme.of(context).textTheme.headlineSmall?.copyWith( | ||
| fontWeight: FontWeight.bold, | ||
| color: Colors.black87, | ||
| ), | ||
| ), | ||
| const SizedBox(height: 16), | ||
| Text( | ||
| appLocalizations.screenNotImplemented, | ||
| textAlign: TextAlign.center, | ||
| style: Theme.of(context).textTheme.bodyLarge?.copyWith( | ||
| color: Colors.grey.shade600, | ||
| ), | ||
| ), | ||
| const SizedBox(height: 24), | ||
| Container( | ||
| padding: const EdgeInsets.all(16), | ||
| decoration: BoxDecoration( | ||
| color: Colors.blue.shade50, | ||
| borderRadius: BorderRadius.circular(8), | ||
| border: Border.all(color: Colors.blue.shade200), | ||
| ), | ||
| child: Column( | ||
| children: [ | ||
| Icon( | ||
| Icons.info_outline, | ||
| color: Colors.blue.shade700, | ||
| size: 32, | ||
| ), | ||
| const SizedBox(height: 8), | ||
| Text( | ||
| appLocalizations.gasSensorDesc, | ||
| textAlign: TextAlign.center, | ||
| style: TextStyle( | ||
| color: Colors.blue.shade900, | ||
| fontSize: 14, | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| const SizedBox(height: 32), | ||
| ElevatedButton.icon( | ||
| onPressed: () { | ||
| Navigator.pop(context); | ||
| }, | ||
| icon: const Icon(Icons.arrow_back), | ||
| label: Text(appLocalizations.close), | ||
| style: ElevatedButton.styleFrom( | ||
| padding: const EdgeInsets.symmetric( | ||
| horizontal: 24, | ||
| vertical: 12, | ||
| ), | ||
| ), | ||
| ), | ||
| ], | ||
| ), |
There was a problem hiding this comment.
GasSensorScreen and DustSensorScreen are nearly identical placeholder UIs (icon/title/message/desc/back button) with only colors/text differing. To avoid duplicated future updates (styling, localization, layout tweaks), consider extracting a shared reusable placeholder widget that takes the localized title/description, icon, and accent color as parameters.
| body: Center( | |
| child: Padding( | |
| padding: const EdgeInsets.all(24.0), | |
| child: Column( | |
| mainAxisAlignment: MainAxisAlignment.center, | |
| children: [ | |
| Icon( | |
| Icons.air, | |
| size: 120, | |
| color: Colors.grey.shade400, | |
| ), | |
| const SizedBox(height: 32), | |
| Text( | |
| appLocalizations.gasSensor, | |
| style: Theme.of(context).textTheme.headlineSmall?.copyWith( | |
| fontWeight: FontWeight.bold, | |
| color: Colors.black87, | |
| ), | |
| ), | |
| const SizedBox(height: 16), | |
| Text( | |
| appLocalizations.screenNotImplemented, | |
| textAlign: TextAlign.center, | |
| style: Theme.of(context).textTheme.bodyLarge?.copyWith( | |
| color: Colors.grey.shade600, | |
| ), | |
| ), | |
| const SizedBox(height: 24), | |
| Container( | |
| padding: const EdgeInsets.all(16), | |
| decoration: BoxDecoration( | |
| color: Colors.blue.shade50, | |
| borderRadius: BorderRadius.circular(8), | |
| border: Border.all(color: Colors.blue.shade200), | |
| ), | |
| child: Column( | |
| children: [ | |
| Icon( | |
| Icons.info_outline, | |
| color: Colors.blue.shade700, | |
| size: 32, | |
| ), | |
| const SizedBox(height: 8), | |
| Text( | |
| appLocalizations.gasSensorDesc, | |
| textAlign: TextAlign.center, | |
| style: TextStyle( | |
| color: Colors.blue.shade900, | |
| fontSize: 14, | |
| ), | |
| ), | |
| ], | |
| ), | |
| ), | |
| const SizedBox(height: 32), | |
| ElevatedButton.icon( | |
| onPressed: () { | |
| Navigator.pop(context); | |
| }, | |
| icon: const Icon(Icons.arrow_back), | |
| label: Text(appLocalizations.close), | |
| style: ElevatedButton.styleFrom( | |
| padding: const EdgeInsets.symmetric( | |
| horizontal: 24, | |
| vertical: 12, | |
| ), | |
| ), | |
| ), | |
| ], | |
| ), | |
| body: SensorPlaceholder( | |
| icon: Icons.air, | |
| title: appLocalizations.gasSensor, | |
| message: appLocalizations.screenNotImplemented, | |
| description: appLocalizations.gasSensorDesc, | |
| accentColor: Colors.blue, | |
| onClose: () { | |
| Navigator.pop(context); | |
| }, | |
| ), | |
| ); | |
| } | |
| } | |
| class SensorPlaceholder extends StatelessWidget { | |
| const SensorPlaceholder({ | |
| super.key, | |
| required this.icon, | |
| required this.title, | |
| required this.message, | |
| required this.description, | |
| required this.accentColor, | |
| required this.onClose, | |
| }); | |
| final IconData icon; | |
| final String title; | |
| final String message; | |
| final String description; | |
| final MaterialColor accentColor; | |
| final VoidCallback onClose; | |
| @override | |
| Widget build(BuildContext context) { | |
| return Center( | |
| child: Padding( | |
| padding: const EdgeInsets.all(24.0), | |
| child: Column( | |
| mainAxisAlignment: MainAxisAlignment.center, | |
| children: [ | |
| Icon( | |
| icon, | |
| size: 120, | |
| color: Colors.grey.shade400, | |
| ), | |
| const SizedBox(height: 32), | |
| Text( | |
| title, | |
| style: Theme.of(context).textTheme.headlineSmall?.copyWith( | |
| fontWeight: FontWeight.bold, | |
| color: Colors.black87, | |
| ), | |
| ), | |
| const SizedBox(height: 16), | |
| Text( | |
| message, | |
| textAlign: TextAlign.center, | |
| style: Theme.of(context).textTheme.bodyLarge?.copyWith( | |
| color: Colors.grey.shade600, | |
| ), | |
| ), | |
| const SizedBox(height: 24), | |
| Container( | |
| padding: const EdgeInsets.all(16), | |
| decoration: BoxDecoration( | |
| color: accentColor.shade50, | |
| borderRadius: BorderRadius.circular(8), | |
| border: Border.all(color: accentColor.shade200), | |
| ), | |
| child: Column( | |
| children: [ | |
| Icon( | |
| Icons.info_outline, | |
| color: accentColor.shade700, | |
| size: 32, | |
| ), | |
| const SizedBox(height: 8), | |
| Text( | |
| description, | |
| textAlign: TextAlign.center, | |
| style: TextStyle( | |
| color: accentColor.shade900, | |
| fontSize: 14, | |
| ), | |
| ), | |
| ], | |
| ), | |
| ), | |
| const SizedBox(height: 32), | |
| ElevatedButton.icon( | |
| onPressed: onClose, | |
| icon: const Icon(Icons.arrow_back), | |
| label: Text(AppLocalizations.of(context)!.close), | |
| style: ElevatedButton.styleFrom( | |
| padding: const EdgeInsets.symmetric( | |
| horizontal: 24, | |
| vertical: 12, | |
| ), | |
| ), | |
| ), | |
| ], |
| Widget build(BuildContext context) { | ||
| return CommonScaffold( | ||
| title: appLocalizations.dustSensor, | ||
| body: Center( | ||
| child: Padding( | ||
| padding: const EdgeInsets.all(24.0), | ||
| child: Column( | ||
| mainAxisAlignment: MainAxisAlignment.center, |
There was a problem hiding this comment.
DustSensorScreen duplicates the same placeholder layout used by GasSensorScreen. Consider extracting a shared reusable placeholder widget (title/description/icon/accent color) so future style or copy changes don't need to be made in multiple files.
- Create FeatureNotImplementedScreen reusable widget - Simplify GasSensorScreen and DustSensorScreen to use shared widget - Remove redundant didChangeDependencies calls - Keep lifecycle logic only in initState - Reduce code duplication by ~200 lines Addresses mentor feedback on code quality and maintainability
- Convert GasSensorScreen and DustSensorScreen to StatelessWidget - Use FeatureNotImplementedScreen reusable component - Remove ~100 lines of duplicated code per file - Maintain same UX with cleaner implementation
f4936a7 to
e7b6035
Compare
|
Closing in favour of #3244 |







-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)
Hi, I reviewed PR #3142 and noticed it seems to focus mainly on the Multimeter interaction and the web USB flow. I’m interested in contributing to the remaining parts of #3136, especially the Dust Sensor and Gas Sensor interactions.
A focused PR for the remaining Dust/Gas sensor issues, or. a follow-up PR covering the full remaining scope of #3136.
🐛 Issue Fixed
Resolves #3136 - Gas Sensor and Dust Sensor menu items were not clickable/visible
📋 Changes
🎯 UX Improvements
Every clickable UI element now either:
No element silently does nothing.
📁 Files Changed
lib/view/gas_sensor_screen.dart(NEW) - Gas Sensor placeholderlib/view/dust_sensor_screen.dart(NEW) - Dust Sensor placeholderlib/view/instruments_screen.dart- Uncommented sensor entrieslib/main.dart- Added routes✨ Key Features
🧪 Testing