Use AppCompatResources.getDrawable for R.drawables

[FrancoisBlavoet]

Glide Version:
3.8.0-SNAPSHOT | 4.0.0-SNAPSHOT

Integration libraries:
none

Device/Android Version:
fails on anything < API 21

Issue details / Repro steps / Use case background:
If you try to use a VectorDrawable as a placeholder or error Drawable for Glide, you are going to get a crash on old devices.

In GenericRequest, Glide calls :
placeholderDrawable = context.getResources().getDrawable(placeholderResourceId);
We need to rely on the support lib in order to make this work.
Luckily appCompat-v7-24.2.0 has just added what we need : AppCompatResources.getDrawable()

This allows us to get a hook in how the support lib loads any resource, including vectors and get automatic caching for them the same way srcCompat already does.

Glide load line / GlideModule (if any) / list Adapter code (if any):

Glide.with(ctx)
     .load(model)
     .placeholder(R.drawable.some_vector)
     .error(R.drawable.some_other_vector)
     .into(view);

In order to do this, we would need to update the project in order to use compileSdk24
I am not sure that Robolectric 2 can target that API level, so it would also need to be updated to version 3
Is it something that could be done on the 3.0 branch ? I have no idea when the 4.0 is supposed to hit stable

I am not sure whether load(R.drawable.svg) needs the same treatment or not. I can't see a use case where you would prefer to load an SVG with Glide instead of just using the support lib. Except maybe if it is too large but I think that's an uncommon use case anyway.

[TWiStErRob]

I don't think we can pull v3 to 24+ support, it may also be too early to force v4 users to 24.2 too. Remember whatever methods Glide uses must be present on the classpath on the user's device. I think latest Glide should work with targetApi 19, compileSdk 19 as well, however not sure it does. That said, a reflective check for the existence of AppCompatResources would be a good compromise IMO.

<rant>Why, oh, why do we need yet another way of loading a drawable? Currently we have: Resources.getDrawable, Context.getDrawable, ResourcesCompat.getDrawable, ContextCompat.getDrawable, AppCompatResources.getDrawable, possibly more. Not to mention that some of these are deprecated and there are multiple overloads.</rant>

Regardless,

Glide.with(ctx)
     .load(model)
     .placeholder(AppCompatResources.getDrawable(R.drawable.some_vector))
     .error(AppCompatResources.getDrawable(R.drawable.some_other_vector))
     .into(view);

seems like a bearable workaround.

Robolectric version doesn't matter as long as it builds and checks for bugs. It's not a dependency for Glide users. v4 already uses 3.1. v4 stable is good question, @sjudd?

As usual I guess a PR is welcome, keeping the above constraints in mind.

---

I'm not sure what you're talking about with SVGs, Android/support doesn't support them as far as I know, you need a custom decoder for that. A use case when you prefer SVGs is when you don't want to convert to Android vectors, because of tool support I guess.

[FrancoisBlavoet]

Ok, it might be a bit early to rely on 24.2.0 (especially since I would not be surprised to see 24.2.1 in a week or so) but when the lib will be one month old, I think it will be an acceptable condition.

.error(AppCompatResources.getDrawable(R.drawable.some_other_vector))
That would work. Although ideally, I don't want to instantiate the error drawable until it is needed.

For the multiple getDrawable methods, it looks like ContextCompat & ResourcesCompat are part of support v4, whereas VectorDrawable backport is in support-v7.
It would probably make sense to only keep AppCompatResources.getDrawable in a later version of the support lib.

By SVG, I meant VectorDrawable, sorry.

I might implement this in a 3.0 fork then. We use VectorDrawables extensively at Deezer, including for placeholders and we have been waiting for AppCompatResources.getDrawable in order to be able to get rid of many rasters.

[TWiStErRob]

I don't think we can pull v3 to 24+ support

I meant that it may be much bigger work than anticipated. Not only need to target 24, you'll need Java 8, currently I think v3 builds on Java 6. Since Glide won't actually run by itself, it may be possible to get away with compiling against support 24 without targeting higher, and using AppCompatResources behind a Class.forName guard (static final cached of course) to make it work with earlier support versions.

I don't want to instantiate the error drawable until it is needed.

Yes, that's a good point, is it still to expensive to instantiate the error drawable once for each adapter and use a field reference: .error(this.preInflatedErrorVector)? Also you were talking about how it's caching stuff, in that case it shouldn't matter how many times you call it.

[FrancoisBlavoet]

Yes, that's a good point, is it still to expensive to instantiate the error drawable once

I meant that since the errorDrawable is only used in a corner case, I would like to only inflate it if it is necessary.
After looking at VectorDrawable's code, the cachedBitmap is only generated when the Drawable is drawn for the first time, so that's probably not that costly.
The paths are still read, but that should not be really costly in most cases.

The only remaining argument is that
.error(AppCompatResources.getDrawable(R.drawable.some_other_vector))
is ugly and error prone : if you replace a raster with a vector or forget to call AppCompatResources, you are going to get a crash on old devices.

I have already updated the project in order to compile with a JVM 8 on my end a while ago (and a colleague did the same some time ago in order to diagnose a Glide issue).
Many Android devs only use a jvm8 since it allows to never get a PermGenSpace crash and we both did not want to install an older JVM in order to compile Glide.
I can submit a PR for this if you want but again, if Glide 4 will soon hit stable it is probably not worth the hassle.

[TWiStErRob]

@sjudd just upped the support lib, but a feature detection is still needed in case someone uses an earlier one.

[FrancoisBlavoet]

nice !

I am a bit fuzzy on how transitive dependencies work with gradle, so I will have to toy around.
I am ok with using reflection to detect this method though :-)

I should probably create another issue to ask whether glide 4 is going to be published soon.

[TWiStErRob]

Transitive deps are simple: the higher level (less transitive hops) wins, #1464 is a good example (not support lib, but same principle):

// overrides transitive version from okhttp3-integration
compile 'com.github.bumptech.glide:glide:4.0.0-20160905.222409-202'

// transitively pulls in 4.0.0-SNAPSHOT which resolves to -203 build or later
// transitively pulls in okhttp3 3.0.1
compile 'com.github.bumptech.glide:okhttp3-integration:2.0.0-SNAPSHOT'

// overrides transitive version from okhttp3-integration
compile 'com.squareup.okhttp3:okhttp:3.4.1'

Create a sample project with the above dependencies and run gradle dependencies to see it in action. You can also just run that on any of your projects, I'm sure they all have different support lib versions.

[sjudd]

Looks like we fixed this in #1946.