Support loading resources from dynamic modules #3308
Conversation
Signed-off-by: Alex Saveau <[email protected]>
|
Is it guaranteed that a split will always contain the parent package name? Or could they theoretically have any arbitrary package name? It's definitely odd, but unless there's explicit framework support I'm not sure how you'd do better? |
|
Yeah, that's the issue 😞. Since it's just a package defined in the manifest, I'm pretty sure you can have it be whatever you want (though the official examples do follow this pattern). Sidenote: why are we throwing in the first place? Could we just return the original context if the new one isn't valid? |
|
@sjudd should we just not throw? It'll fail later on when it tries to load the resource anyway. |
|
What's the harm in throwing? Does it make a difference if the load's going to fail? |
|
Well the problem is that we throw too early since we don't know what the correct package for the resource is. I'm saying that if creating a new context fails, we just return the original context instead of throwing because loading the resource from that context may or may not fail. |
|
This issue has been automatically marked as stale because it has not had activity in the last seven days. It will be closed if no further activity occurs within the next seven days. Thank you for your contributions. |
|
@sjudd Do you think this PR is good as is? Or should I not throw as discussed above? |
|
@sjudd bump |
library/src/main/java/com/bumptech/glide/load/resource/drawable/ResourceDrawableDecoder.java
Outdated
Show resolved
Hide resolved
library/src/main/java/com/bumptech/glide/load/resource/drawable/ResourceDrawableDecoder.java
Show resolved
Hide resolved
|
I'd rather keep the exception for now I think. It's probably better to clearly fail when the resource can't be found, even if we might have found it, than silently return the wrong image if the resource id happens to correspond with a different image in the containing package. |
Signed-off-by: Alex Saveau <[email protected]>
|
Ohhh, I didn't think of that latter case, so SGTM for still throwing. 👍 Is this PR ready for purple then? |
|
LGTM, Thanks! |
* Glide_official/master: (405 commits) Fix an incorrect reference in RequestListener's javadoc. (bumptech#3352) Support loading resources from dynamic modules (bumptech#3308) Fix the annotation compiler test imports. Avoid API 28 gradle dependency warnings. Avoid new ArrayList(collection) in Registry Add a CustomTarget class Remove memegen-android-team from presubmit notifications. Make Giphy models implement equals/hashcode for in memory caching Remove applyInternal in RequestBuilder. Fix annotation processor overrides/tests and remove legacy support. Directly include RequestOptions in ReqestBuilder using CRGP Fix build inconsistencies from MOE import. Re-add empty resource class assertion to ResourceCacheGenerator Include Transcode class in cache key for cached resource classes. Automated g4 rollback of changelist 209077692. Add global/activity/fragment scoped RequestListener API to Glide. Add back assertion on empty resource classes in ResourceCacheGenerator Bump version to 4.9.0-SNAPSHOT Change README version to 4.8.0 Bump version to 4.8.0 ... # Conflicts: # library/build.gradle # library/src/main/java/com/bumptech/glide/Glide.java # library/src/main/java/com/bumptech/glide/RequestManager.java
Background
Dynamic modules use split APKs to distribute resources with the module instead of the base APK. This means that, for example, the runtime package may be
com.supercilex.robotscouterwhile the resource is located incom.supercilex.robotscouter.teams.These differences cause problems when Glide tries to load the resource because Glide will try to create a context from the
com.supercilex.robotscouter.teamspackage which doesn't really exist.Proposal
If resources are a subset of the context's package, use said context. Otherwise, try to create a new context.
@sjudd Can you think of a better solution? This is the best one I've come up with so far, but it seems hacky.