Skip to content
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

Always deliver scan/ranging results on the main thread #149

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

Conversation

samskiter
Copy link

UNTESTED, but hopefully not too complex.

(apologies for screwing up the indentation style)

@davidgyoung
Copy link
Member

As discussed in #148 this is a breaking change, so it cannot be merged until a 3.0 release.

@samskiter
Copy link
Author

just to add another vote to this, I just hit:

android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.

After adjusting some ui from the didRangeBeaconsInRegion callback

@marcosalis
Copy link
Contributor

I agree that this is a valuable change to the library behaviour. I think that the most common user case for the callbacks is to perform operations that will eventually modify the UI, so it makes perfect sense to receive the callbacks from the main thread (and this is the approach that most well known third-party libraries take). If the caller needs to perform long running operations, it makes sense that they spawn their own Executors, or thread, or whatever the threading model of their application requires.
What's more, AltBeacon (as far as I could see in the documentation) doesn't specify if it's thread safe, so the first instinct would be to only access it from the main thread (and expect callbacks on the same thread).

@@ -44,6 +47,19 @@ public BeaconIntentProcessor() {

@Override
protected void onHandleIntent(Intent intent) {
//Make sure we always deliver results on the main thread
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onHandleIntent() always gets executed on a worker thread, as this is the expected behaviour of an IntentService (see documentation).

It's probably better not to mess with Android callbacks by calling them directly.
It might be safer to instantiate the Handler in the onCreate() and add a new method that posts to the main thread the calls to the listeners.

@hashbrown
Copy link
Contributor

BTW @davidgyoung, you've already unintentionally made this breaking change in 2.12 (for Android 8), where you use the BeaconLocalBroadcastProcesser to handle Range and Monitor notifications that are intents captured from a BroadcastReceiver. Since the intents are always handled on the main thread, and BeaconLocalBoradcasProcessor is not an IntentService as before, all Ranging callbacks are actually received by the application on its main thread.

Personally I'd like to see this fixed in the 2.X series since it is a breaking api change, and has at least been detrimental to my app. Also, if this PR is accepted, can we please configure this behavior? I do not want to receive these on the main thread.

@davidgyoung
Copy link
Member

I had not intended such a change, no. At this point I agree that documentation of the existing behavior and configurability is probably the best option.

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

Successfully merging this pull request may close these issues.

5 participants