-
Notifications
You must be signed in to change notification settings - Fork 19
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
Bluetooth permissions functionality #218
Conversation
…PI 30 and below don't have a bluetooth scan permission, so we just generate a success early if they are in that API version.
…error with bluetooth plugin because they define the same permissions
…OS bluetooth permission
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.
I don't have any issues with these changes per-se except for the extraneous whitespace. And eventually we do want to incorporate the bluetooth scanning directly into e-mission-datacollection, particularly when we use the callbacks to modify the FSM.
However I don't think this should be strictly required - I would assume that the plugin would already have a method to request permissions. Or was it so old that it was written at a time without bluetooth permissions?
Also, where are the iOS changes?
@@ -200,6 +204,7 @@ public void run() { | |||
callbackContext.success(retVal); | |||
return true; | |||
} | |||
|
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.
extraneous whitespace
@louisg1337 can you please:
|
This is a question I can answer! From what I understand, the changes were in plugin.xml, line 216: <config-file target="*-Info.plist" parent="NSBluetoothAlwaysUsageDescription">
<string>We need Bluetooth access to interact with BLE beacons for the fleet version of the app.</string>
</config-file> This permission was then checked on the UI side, in this component. As we we understood it, the Classic plugin on iOS only worked with a preset list of peripherals (see screenshot below), and effectively did not scan. Because this wasn't functional, we opted to leave a "Classic Not Available" error message here! I can run another test on this branch if need be: the old code for iOS can be found here. |
indicate testing done (showing the new permissions) To answer your question about why we wrote our own permissions, the plugin did have a way to request permissions but it was too all over the place. It involved a lot more hassle as the permissions for the plugin requests multiple permissions that we don't need (locations + bluetooth connecting). We also will eventually need to incorporate the permissions for bluetooth so we figured why not just do it now. For testing done, I tested 3 cases on Android only. I tested what happens when a user, from a fresh install, tries to use the bluetooth scanner. The result is as follows. Clicking allow then takes you to the scanner page. After that, from a fresh install, I tried to deny the permissions. This is the result. Clicking the scanner again will pop up the previous permissions request. Then finally, I tried revoking the permissions to see if it would let you use the scanner, and it still pops up the same permission request as the first picture. remove the whitespace Just pushed a new commit with the whitespace removed, sorry about that! answer the question about iOS Katie hit the nail on the head! Because the classic didn't work the way we wanted on iOS, we chose to forego it. We did need that line in the |
Incorporated new functionality to request bluetooth permissions on Android.