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

Jump to bottom

Vector drawable can't be used as error drawable #1267

Closed
jakubkrolewski opened this issue Jun 14, 2016 · 5 comments
Closed

Vector drawable can't be used as error drawable #1267

jakubkrolewski opened this issue Jun 14, 2016 · 5 comments
Assignees
@sjudd
Labels
bug enhancement
Milestone
4.3

Comments

@jakubkrolewski
Copy link

jakubkrolewski commented Jun 14, 2016 •
edited by TWiStErRob

  1. Create a vector drawable in xml
  2. Refer the drawable id in the error method
  3. Run the app on Android < 5.0
  4. Make sure main image is not loaded and the error drawable is displayed
  5. The app crashes with:
android.content.res.Resources$NotFoundException: File res/drawable/no_image_placeholder.xml from drawable resource ID #0x7f020080
   at android.content.res.Resources.loadDrawable(Resources.java:3376)
   at android.content.res.Resources.getDrawable(Resources.java:1872)
   at com.bumptech.glide.request.GenericRequest.getErrorDrawable(GenericRequest.java:409)
   at com.bumptech.glide.request.GenericRequest.setErrorPlaceholder(GenericRequest.java:399)
   at com.bumptech.glide.request.GenericRequest.onException(GenericRequest.java:548)
   at com.bumptech.glide.load.engine.EngineJob.handleExceptionOnMainThread(EngineJob.java:183)
   at com.bumptech.glide.load.engine.EngineJob.access$200(EngineJob.java:22)
   at com.bumptech.glide.load.engine.EngineJob$MainThreadCallback.handleMessage(EngineJob.java:204)
   at android.os.Handler.dispatchMessage(Handler.java:98)
   at android.os.Looper.loop(Looper.java:146)
   at android.app.ActivityThread.main(ActivityThread.java:5653)
   at java.lang.reflect.Method.invokeNative(Native Method)
   at java.lang.reflect.Method.invoke(Method.java:515)
   at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1291)
   at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1107)
   at dalvik.system.NativeStart.main(Native Method)
Caused by: org.xmlpull.v1.XmlPullParserException: Binary XML file line #2: invalid drawable tag vector
   at android.graphics.drawable.Drawable.createFromXmlInner(Drawable.java:986)
   at android.graphics.drawable.Drawable.createFromXml(Drawable.java:930)
   at android.content.res.Resources.loadDrawable(Resources.java:3372)
   at android.content.res.Resources.getDrawable(Resources.java:1872) 
   at com.bumptech.glide.request.GenericRequest.getErrorDrawable(GenericRequest.java:409) 
   at com.bumptech.glide.request.GenericRequest.setErrorPlaceholder(GenericRequest.java:399) 
   at com.bumptech.glide.request.GenericRequest.onException(GenericRequest.java:548) 
   at com.bumptech.glide.load.engine.EngineJob.handleExceptionOnMainThread(EngineJob.java:183) 
   at com.bumptech.glide.load.engine.EngineJob.access$200(EngineJob.java:22) 
   at com.bumptech.glide.load.engine.EngineJob$MainThreadCallback.handleMessage(EngineJob.java:204) 
   at android.os.Handler.dispatchMessage(Handler.java:98) 
   at android.os.Looper.loop(Looper.java:146) 
   at android.app.ActivityThread.main(ActivityThread.java:5653) 
   at java.lang.reflect.Method.invokeNative(Native Method) 
   at java.lang.reflect.Method.invoke(Method.java:515) 
   at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1291) 
   at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1107) 
   at dalvik.system.NativeStart.main(Native Method)

The problem is that GenericRequestgets resources from the application context, not the provided context. All the AppCompat classes (like AppCompatActivity) can return the VectorEnabledTintResourcesclass, which supports vector drawable. Unfortunately, there's no AppCompatApplication, so the Applicationclass always returns just plain Resourcesclass which doesn't support vector drawables before Android 5.0.

Glide Version:
3.7.0

Integration libraries:
okhttp3-integration:1.4.0

Device/Android Version:
GT-P5210 Android 4.4.2
@TWiStErRob
Copy link
Collaborator

TWiStErRob commented Jun 14, 2016 •
edited

A simple workaround would be to get the drawable yourself: .error(ContextCompat.getDrawable(getContext(), R.drawable.no_image_placeholder)).

Tip: you can save the request builder into a field and reuse it in an adapter to prevent continuously calling these (see Glide.with(...).from*).

@TWiStErRob TWiStErRob added the bug label Jun 14, 2016
@TWiStErRob
Copy link
Collaborator

TWiStErRob commented Jun 14, 2016 •
edited

I took a deeper look, checked the flow of Context from user to GenericRequest and it seems to me that if we are not paranoid in RequestManager the wrapped activity context would flow nicely to where you need it. The problem is that it would require a full analysis of other escape paths of that context to see if they're not kept once the Activity is destroyed.

You said there's no AppCompatApplication, but you could simply create your own. Wouldn't it "just work" if you copied AppCompatActivity.getResources method into your Application class?

@sjudd what's your take on this?

@jakubkrolewski
Copy link
Author

jakubkrolewski commented Jun 14, 2016 •
edited by TWiStErRob

The AppCompatApplication (full source below) is what I'm using right now, so the bug is no a blocker for me. However, the provided Contextcan be in fact some custom instance of ContextWrapper, so getting resources from it directly would be a more bulletproof solution.

/**
 * Based on {@link android.support.v7.app.AppCompatActivity}
 */
public class AppCompatApplication extends MultiDexApplication {

    private Resources resources;

    @Override
    public void onConfigurationChanged(Configuration newConfig) {
        super.onConfigurationChanged(newConfig);
        if (resources != null) {
            DisplayMetrics newMetrics = super.getResources().getDisplayMetrics();
            resources.updateConfiguration(newConfig, newMetrics);
        }
    }

    @Override
    public Resources getResources() {
        if (resources == null && VectorEnabledTintResources.shouldBeUsed()) {
            resources = new VectorEnabledTintResources(this, super.getResources());
        }
        return resources == null ? super.getResources() : resources;
    }
}

@sjudd
Copy link
Collaborator

sjudd commented Jun 14, 2016

Themes are a sign we ought to use the provided context to load placeholders, not the application. We will have to be careful to avoid leaking activities unnecessarily, but that already can happen (if you use something like SimpleTarget) and we do try to guard against it by cancelling/restarting requests when activities and fragments are destroyed/re-created, so hopefully it won't be too bad.

@sjudd sjudd added this to the 4.3 milestone Oct 7, 2017
@sjudd
Copy link
Collaborator

sjudd commented Oct 18, 2017

So I think I have a more complete fix for this in 4.3 which will allow automatic application of themes and handle custom ContextWrappers for error/fallback/placeholder. The original issue I think was actually fixed a while ago in bd9e265.

@sjudd sjudd closed this as completed in bbb25af Oct 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sjudd sjudd

Labels
bug enhancement
Projects
None yet
Milestone
4.3
Development

No branches or pull requests
3 participants
@sjudd @TWiStErRob @jakubkrolewski