Skip to content
This repository has been archived by the owner on Feb 27, 2019. It is now read-only.

update how async permissions are handled #147

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

felix-dumit
Copy link
Contributor

Changed how async permissions are handled, as I was getting the main thread stuck waiting for an update sometimes.
Now the getResultsForConfig will wait for updates in the background before executing completion in main thread.

@felix-dumit felix-dumit mentioned this pull request Jan 14, 2016
@nickoneill
Copy link
Owner

This looks great, I like getting rid of the waitingFor trackers. What's the BeginAtCurrentState change do?

@felix-dumit
Copy link
Contributor Author

We're still using the waitingFor but not on the main thread anymore. I think we'll only be able to remove them if we move to a completion/promise model for these async permissions.

The BeginAtCurrentState is to address a potential issue if the alert is in the process of animating when the permissions are determined to be all enabled and the alert is told to dismiss. This basically makes the animation start from the current point it is in the screen instead of where it would be at the end of the opening animation (effectively it cancels the previous open animation)

@nickoneill
Copy link
Owner

Ah, interesting. Thanks!

Do you have a particular repro step for blocking the main thread so I can reproduce the issue and confirm the fix? I imagine it occurs with the bluetooth permission which I don't use pscope for frequently...

@felix-dumit
Copy link
Contributor Author

I spent a while wondering why the popup was never showing up in the example but the background modes were disabled :/

Anyways, I tried to reproduce the error in the example but I wasn't able.
I think what was happening though is that if the peripheralDidUpdateState took enough time to be called that show was called before it. It would then get stuck in that while loop and wouldn't allow the update state method to be called, since the BLE queue was main thread as well.

I think in my app, since I call bluetooth methods on startup, and also show the permissions scope alert on startup, it could happen that show was called before BLE had updated.

Actually I found a way to simulate the issue. Wrap the contents of the peripheralDidUpdate inside a dispatch_after block with a delay of like 5s (the time starts counting when the app opens because as soon as the permissions are added it fires the bluetooth status update, not when the alert is shown).
The alert will never show.

With these changes the alert will show up after ~5s of startup.

@felix-dumit
Copy link
Contributor Author

@nickoneill I've recently updated this further because I had found another condition in which it would get stuck waiting for bluetooth to update.

With these changes all the waiting operations are guaranteed to be off the main thread.

@bre7
Copy link
Collaborator

bre7 commented Apr 15, 2016

Background mode shouldn't be enabled in the library. Add a note in the README, under extra-requirements-for-permissions

@felix-dumit
Copy link
Contributor Author

@bre7 It's not for the library but only for the examples, it was already enabled for the swift example I enabled it for the objc-example to match that.

@bre7
Copy link
Collaborator

bre7 commented Apr 15, 2016

✋ my bad

@nickoneill
Copy link
Owner

Thanks for the additional changes @felix-dumit, I'll review next time we set up a release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants