-
Notifications
You must be signed in to change notification settings - Fork 246
fix: added bluetooth connection support for ubuntu #1497
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
base: development
Are you sure you want to change the base?
Conversation
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 fix the common build. Thanks |
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/18715870948/artifacts/4339685112. Screenshots |
mariobehling
left a comment
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.
Thanks for the Ubuntu/Linux Bluetooth support addition. The changes are meaningful and directionally correct. Below are issues that I believe must be addressed before merging, plus suggestions and a test plan.
✅ Must-Fix Before Merge
-
Hardcoded known-device MAC & constructing fake
ScanResultIn
badge_message_provider.dart’s_tryKnownDevices, there is:final knownDevices = ['5C:53:10:B7:AC:F6']; for (final id in knownDevices) { final mock = ScanResult( device: BluetoothDevice(id: id), advertisementData: /* some constructed AdvertisementData */, rssi: -50, ); completer.complete(ConnectState(mock, manager)); }
-
This is brittle: it embeds a specific device ID in the app code.
-
Creating a fake
ScanResultmay lead to inconsistent or incorrect state (e.g. using dummy RSSI, advertisement data) and may not map well to the UI or connection flow. -
Fix: Instead, either:
- Remove the known-device fallback altogether (default to scan-only).
- Or make the fallback list configurable (e.g. via a settings or developer mode), and avoid fabricating
ScanResult. If needed, refactor the connection API to accept a raw device ID or identifier in Linux context rather than forcing aScanResult.
-
-
Inconsistent use of
isCompletedsnapshot vs completer actual stateThe method signature is:
Future<bool> _tryKnownDevices(Completer<BleState?> completer, bool isCompleted)
You pass
isCompleted = completer.isCompletedinto the method, then inside you also checkcompleter.isCompleted. This duplication risks state divergence (the boolean captured may differ from the actual completer state later).- Fix: Remove the
bool isCompletedparameter. Inside the method, always checkcompleter.isCompletedto know if we should abort. Storing a snapshot is unnecessary and error-prone.
- Fix: Remove the
-
Ensure
stopScan()always runs on failure / exception pathsThe scan logic is somewhat complex with repeated
startScan()/stopScan()calls and delays. If an error is thrown in between, the Bluetooth adapter may remain scanning indefinitely.-
Fix: Wrap scan-start / scan-stop phases in
try/finallyblocks so thatstopScan()is guaranteed to run in failure paths. E.g.:try { await FlutterBluePlus.startScan(...); // scanning & waiting logic } finally { await FlutterBluePlus.stopScan(); }
-
-
Localization: Linux-specific messages not using l10n
In
BadgeMessageProvider, lines like:showToast('Bluetooth is not available on this system.');
are hardcoded English strings. Meanwhile, other branches use
l10n.xxx. All user-facing strings should be localized.- Fix: Move these messages into ARB / localization keys and replace with
l10n.<key>.
- Fix: Move these messages into ARB / localization keys and replace with
🔍 Suggested Enhancements / Robustness
- After the broad scan (fallback), filter discovered devices by expected service UUID(s) before attempting connect. This avoids connecting to irrelevant devices.
- Consolidate timeouts into constants and document them clearly (e.g. "initial scan up to 5 s, fallback up to 3 s").
- Use appropriate log levels (e.g.
logger.i,logger.w,logger.e) for major events, not justlogger.d. - If there is coupling of downstream logic to
ScanResultfields (e.g. advertisement data or name), refactor to make Linux connection flows agnostic to dummy vs real scan data.
samruddhi-Rahegaonkar
left a comment
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.
|
@nope3472 Just remove the hardcoded GUID. we are using it in connect_state.dart also it will be good. |
|
@nope3472 please address the comment of Samruddhi so we can merge this |
adityastic
left a comment
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.
Have a common interface with your functions exposed. Implement platform specific code into their own classes that extend the interface. Create a small manager which can look at the platform and instantiate the right implementation class for you. Which is then used across states/anywhere you like







-1_home_screen.png?raw=true)
-2_text_badge.png?raw=true)
-3_emoji_badge.png?raw=true)
-4_inverted_emoji_badge.png?raw=true)
-5_saved_badges.png?raw=true)
-6_saved_badges_clicked.png?raw=true)
-7_draw_badge.png?raw=true)
Fixes #1496
Changes
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.