-
Notifications
You must be signed in to change notification settings - Fork 658
EnableBtOnConnect #737
base: master
Are you sure you want to change the base?
EnableBtOnConnect #737
Conversation
This enables bt on connect with user intervention (adding general preference) It also tries to connect when starting if bt is enabled
The change itself looks good, but it breaks the DeviceCommunicationServiceTestCase. I'll try to fix it tomorrow unless someone beats me to it. You can run the tests with |
I didn't understand where it breaks, couldn't find it in Android Studio that is why I didn't fix it. The changes work as they are except tests. |
While appreciating the effort, I honestly believe we should not toggle the Bluetooth adapter from within the app, as this could:
For these reasons I - personally - am against merging this change. |
There is no way of enabling btr without user intervention. And we added an option for it, in case someone does not want it. |
We should also honor autoreconnect option before we auto connect on startup. Pressing Connect button does not check autoreconnect, it just sets autoreconnect and connects.
Thanks, I will have a look tomorrow! |
OK, I think we all agree that if GB is configured to launch on startup and to connect when BT is on, then it's really supposed to connect right after booting (scenario 1). Everything else would be a bug. So without your change, this scenario didn't work? A new scenario (2) is: Gadgetbridge is running, BT is off and you try to connect. That should pop up a confirmation box "Do you really want to turn on BT?". If confirmed, BT should be enabled and then a connection should be established. Did I get this right? Anything I forgot? |
Note that BT enabling should NOT be triggered automatically, through any other means than manually connecting to a device. Right? |
Scenario 1) is fine by me. Scenario 2) is in my opinion dangerous because "BT off" could mean many things we have little control over (e.g. BT is already powering on, but slow; BT is scheduled to be activated, but it has not happened yet; the ROM has some quirks for BT access like extra security (think privacy guard)). So I believe that the current situation where a toast is shown "Bluetooth is disabled" is the right compromise in this case. |
Regarding this specific pull request, I have the problem that the start command automatically triggers the connect. Currently (current master) the code already calls start from connect to ensure service is running. I am unaware of the reason because originally we had both start() and connect(), but with this change they would practically become a single status, and this should a change explicitly discussed, evaluated and approved. |
cpfeifer I agree on both scenarios, that is exactly what I had in mind. |
How would this pull request deal with Airplane Mode? Another option to respect it or to ignore it in the app's settings? |
This enables bt on connect with user intervention (adding general preference)
It also tries to connect when starting if bt is enabled