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

Main thread assertion is redundant when using Glide with RemoteViews #310

Closed
abuddy opened this issue Jan 21, 2015 · 5 comments
Closed

Main thread assertion is redundant when using Glide with RemoteViews #310

abuddy opened this issue Jan 21, 2015 · 5 comments
Labels

Comments

@abuddy
Copy link

abuddy commented Jan 21, 2015

I'm using NotificationTarget with Glide 3.4.0.

I'm updating a notification from a service running on a background thread. Because of the Glides assertion it has to run on the main thread. RemoteViews however don't require the main thread.

Because of the requirement, the whole phone becomes unresponsive for about one to two seconds. That's noticeable when you have a widget in a notification with a button like playing/pausing music.

Here is how I call Glide:

Handler mainHandler = new Handler(Looper.getMainLooper());
Runnable myRunnable = new Runnable() {
    @Override
    public void run() {
        Glide.with(getApplicationContext())
                .load(targetUrl)
                .asBitmap()
                .into(new NotificationTarget(
                        getApplicationContext(),
                        notificationView,
                        R.id.icon,
                        notification,
                        NOTIFICATION));
    }
};
mainHandler.post(myRunnable);
@sjudd
Copy link
Collaborator

sjudd commented Jan 21, 2015

Can you add a stack trace or a link to the code of the assertion you're referring to? Most of the assertions exist to prevent deadlock.

That said, Targets are always called on the main thread, although you can start loads on a background thread. We could look in to changing that, but it might relatively straightforward to just change the remote views targets to post results to a background thread rather than trying to modify the framework.

If you have a use case and want to make that change, I'd love to see a pull request.

@abuddy
Copy link
Author

abuddy commented Jan 21, 2015

Sorry, there is no assertion, it has something to do with the Handler. Here is the relevant part of the stack trace.

java.lang.RuntimeException: Can't create handler inside thread that has not called Looper.prepare()
        at android.os.Handler.<init>(Handler.java:200)
        at android.os.Handler.<init>(Handler.java:128)
        at com.bumptech.glide.load.engine.ResourceRecycler.<init>(ResourceRecycler.java:13)
        at com.bumptech.glide.load.engine.Engine.<init>(Engine.java:90)
        at com.bumptech.glide.load.engine.Engine.<init>(Engine.java:58)
        at com.bumptech.glide.GlideBuilder.createGlide(GlideBuilder.java:178)
        at com.bumptech.glide.Glide.get(Glide.java:147)
        at com.bumptech.glide.RequestManager.<init>(RequestManager.java:59)
        at com.bumptech.glide.RequestManager.<init>(RequestManager.java:51)
        at com.bumptech.glide.manager.RequestManagerRetriever.getApplicationManager(RequestManagerRetriever.java:72)
        at com.bumptech.glide.manager.RequestManagerRetriever.get(RequestManagerRetriever.java:94)
        at com.bumptech.glide.Glide.with(Glide.java:587)

@abuddy
Copy link
Author

abuddy commented Jan 21, 2015

Here is what happens when I try to use Glide with calling Looper.prepare() beforehand.

Runnable runnable = new Runnable() {
    @Override
    public void run() {
        Looper.prepare();

        Glide.with(getApplicationContext())
                .load(targetUrl)
                .asBitmap()
                .into(new NotificationTarget(
                        getApplicationContext(),
                        notificationView,
                        R.id.icon,
                        notification,
                        NOTIFICATION));
    }
};

new Thread(runnable).start();

Stacktrace:

java.lang.IllegalArgumentException: You must call this method on the main thread
    at com.bumptech.glide.util.Util.assertMainThread(Util.java:115)
    at com.bumptech.glide.GenericRequestBuilder.into(GenericRequestBuilder.java:595)

@sjudd
Copy link
Collaborator

sjudd commented Jan 23, 2015

The Handler issue you mentioned was fixed in #295 and should be released in Glide 3.5 in the next couple of days. You can work around it (I think) by just calling Glide.get() once on the main thread somewhere prior to your background call.

The assertion in into() is currently required. The only way you can start loads on a background thread is to use a specific into() variant that takes a width and height and returns a Future.

Results have to be handled on the main thread to avoid deadlock (with the exception of the API I mentioned above), so even getting around the assertion you mention won't really solve your problem.

Again my inclination is that if setting the bitmap on the remote view is what is slow, you probably want to post it to a background thread inside NotificationTarget. The thread could for now just be a lazily initialized static variable.

Have you added timing to see if you can find out what exactly is slow? You could also try traceview, which wouldn't require building from source.

@abuddy
Copy link
Author

abuddy commented Mar 19, 2015

It indeed appears to be fixed. Using 3.5.2 now and I don't see the slow down. The issue can be closed. Sorry for not commenting on that earlier.

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

No branches or pull requests

3 participants