From 966815704feedc6d524a9724c74b88a28ba8b40e Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Tue, 11 Oct 2022 15:36:47 -0700 Subject: [PATCH] Apply the current Theme for Uri and resource ids. Applying the Theme allows us to respect the Theme, including dark or light mode resources. We're making the same compromise here that we've made for other model types. Specifically we're only applying this behavior if the non-generic version of the method is used, ie the type of the model is known at compile time. We could try to do this at a lower level, but there's some additional risk of confusion about when or why the Theme is or isn't available. While this isn't perfectly consistent, the behavior can at least be well documented. There's also some risk that passing the Context's Resources / Theme classes to a background thread will result in transient memory leaks. I don't immediately see a direct link between either class and the enclosing Context, but it's hard to be certain. Progress towards #3751 --- .../java/com/bumptech/glide/DarkModeTest.java | 142 +++++++++++++++--- .../RememberGlidePreloadingDataTest.kt | 12 +- .../com/bumptech/glide/RequestBuilder.java | 50 +++++- .../java/com/bumptech/glide/load/Options.java | 8 +- .../drawable/ResourceDrawableDecoder.java | 20 ++- .../glide/request/BaseRequestOptions.java | 19 ++- 6 files changed, 214 insertions(+), 37 deletions(-) diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/DarkModeTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/DarkModeTest.java index ad093d31b4..4d128801ca 100644 --- a/instrumentation/src/androidTest/java/com/bumptech/glide/DarkModeTest.java +++ b/instrumentation/src/androidTest/java/com/bumptech/glide/DarkModeTest.java @@ -32,6 +32,7 @@ import com.bumptech.glide.test.ForceDarkOrLightModeActivity; import com.google.common.base.Function; import org.junit.Before; +import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; @@ -50,33 +51,54 @@ public void before() { assumeTrue(VERSION.SDK_INT >= VERSION_CODES.Q); } - // TODO(judds): The way we handle data loads in the background for resources is not Theme - // compatible. In particular, the theme gets lost when we convert the resource id to a Uri and - // we don't use the user provided theme. While ResourceBitmapDecoder and ResourceDrawableDecoder - // will use the theme, they're not called for most resource ids because those instead go through - // UriLoader, which just calls contentResolver.openStream. This isn't sufficient to use to theme. - // We could: - // 1. Avoid using contentResolver for android resource Uris and use ResourceBitmapDecoder instead. - // 2. #1 but only for non-raw resources which won't be themed - // 3. Always use Theme.getResources().openRawResource, which, despite the name, works find on - // Drawables and takes into account the theme. - // In addition we'd also need to consider just passing through the theme always, rather than only - // when it's specified by the user. Otherwise whether or not we'd obey dark mode would depend on - // the user also providing the theme from the activity. We'd want to try to make sure that doesn't - // leak the Activity. - @Test - public void load_withDarkModeActivity_usesLightModeDrawable() { + @Test + public void load_withDarkModeActivity_useDarkModeDrawable() { + runActivityTest( + darkModeActivity(), + R.raw.dog_dark, + activity -> Glide.with(activity).load(R.drawable.dog).override(Target.SIZE_ORIGINAL)); + } + + @Test + public void load_withDarkModeActivity_afterLoadingWithLightModeActivity_useDarkModeDrawable() { + // Load with light mode first. + runActivityTest( + lightModeActivity(), + R.raw.dog_light, + activity -> Glide.with(activity).load(R.drawable.dog).override(Target.SIZE_ORIGINAL)); + + // Then again with dark mode to make sure that we do not use the cached resource from the + // previous load. runActivityTest( darkModeActivity(), + R.raw.dog_dark, + activity -> Glide.with(activity).load(R.drawable.dog).override(Target.SIZE_ORIGINAL)); + } + + @Test + public void load_withDarkModeActivity_afterLoadingWithLightModeActivity_memoryCacheCleared_useDarkModeDrawable() { + // Load with light mode first. + runActivityTest( + lightModeActivity(), R.raw.dog_light, activity -> Glide.with(activity).load(R.drawable.dog).override(Target.SIZE_ORIGINAL)); + + // Then again with dark mode to make sure that we do not use the cached resource from the + // previous load. + runActivityTest( + darkModeActivity(), + R.raw.dog_dark, + activity -> { + Glide.get(context).clearMemory(); + return Glide.with(activity).load(R.drawable.dog).override(Target.SIZE_ORIGINAL); + }); } @Test - public void load_withDarkModeFragment_usesLightModeDrawable() { + public void load_withDarkModeFragment_usesDarkModeDrawable() { runFragmentTest( darkModeActivity(), - R.raw.dog_light, + R.raw.dog_dark, fragment -> Glide.with(fragment).load(R.drawable.dog).override(Target.SIZE_ORIGINAL)); } @@ -120,6 +142,35 @@ public void loadResourceNameUri_withDarkModeActivity_darkModeTheme_usesDarkModeD .theme(activity.getTheme())); } + @Test + public void loadResourceNameUri_withDarkModeActivity_usesDarkModeDrawable() { + runActivityTest( + darkModeActivity(), + R.raw.dog_dark, + activity -> + Glide.with(activity) + .load(newResourceNameUri(activity, R.drawable.dog)) + .override(Target.SIZE_ORIGINAL)); + } + + @Test + public void loadResourceNameUri_withDarkModeActivity_afterLightModeActivity_usesDarkModeDrawable() { + runActivityTest( + lightModeActivity(), + R.raw.dog_light, + activity -> + Glide.with(activity) + .load(newResourceNameUri(activity, R.drawable.dog)) + .override(Target.SIZE_ORIGINAL)); + runActivityTest( + darkModeActivity(), + R.raw.dog_dark, + activity -> + Glide.with(activity) + .load(newResourceNameUri(activity, R.drawable.dog)) + .override(Target.SIZE_ORIGINAL)); + } + @Test public void loadResourceIdUri_withDarkModeActivity_darkModeTheme_usesDarkModeDrawable() { runActivityTest( @@ -132,6 +183,17 @@ public void loadResourceIdUri_withDarkModeActivity_darkModeTheme_usesDarkModeDra .theme(activity.getTheme())); } + @Test + public void loadResourceIdUri_withDarkModeActivity_usesDarkModeDrawable() { + runActivityTest( + darkModeActivity(), + R.raw.dog_dark, + activity -> + Glide.with(activity) + .load(newResourceIdUri(activity, R.drawable.dog)) + .override(Target.SIZE_ORIGINAL)); + } + private static Uri newResourceNameUri(Context context, int resourceId) { Resources resources = context.getResources(); return newResourceUriBuilder(context) @@ -198,6 +260,28 @@ public void load_withApplicationContext_darkTheme_usesDarkModeDrawable() { .theme(input.getTheme())); } + @Ignore("TODO(#3751): Consider how to deal with themes applied for application context loads.") + @Test + public void load_withApplicationContext_lightTheme_thenDarkTheme_usesDarkModeDrawable() { + runActivityTest( + lightModeActivity(), + R.raw.dog_light, + input -> + Glide.with(input.getApplicationContext()) + .load(R.drawable.dog) + .override(Target.SIZE_ORIGINAL) + .theme(input.getTheme())); + + runActivityTest( + darkModeActivity(), + R.raw.dog_dark, + input -> + Glide.with(input.getApplicationContext()) + .load(R.drawable.dog) + .override(Target.SIZE_ORIGINAL) + .theme(input.getTheme())); + } + @Test public void loadResourceNameUri_withApplicationContext_darkTheme_usesDarkModeDrawable() { runActivityTest( @@ -210,6 +294,28 @@ public void loadResourceNameUri_withApplicationContext_darkTheme_usesDarkModeDra .theme(input.getTheme())); } + @Ignore("TODO(#3751): Consider how to deal with themes applied for application context loads.") + @Test + public void loadResourceNameUri_withApplicationContext_darkTheme_afterLightTheme_usesDarkModeDrawable() { + runActivityTest( + lightModeActivity(), + R.raw.dog_light, + input -> + Glide.with(input.getApplicationContext()) + .load(newResourceNameUri(input.getApplicationContext(), R.drawable.dog)) + .override(Target.SIZE_ORIGINAL) + .theme(input.getTheme())); + + runActivityTest( + darkModeActivity(), + R.raw.dog_dark, + input -> + Glide.with(input.getApplicationContext()) + .load(newResourceNameUri(input.getApplicationContext(), R.drawable.dog)) + .override(Target.SIZE_ORIGINAL) + .theme(input.getTheme())); + } + @Test public void loadResourceIdUri_withApplicationContext_darkTheme_usesDarkModeDrawable() { runActivityTest( diff --git a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/RememberGlidePreloadingDataTest.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/RememberGlidePreloadingDataTest.kt index 8ab6693df6..1b7c1fb83c 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/RememberGlidePreloadingDataTest.kt +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/RememberGlidePreloadingDataTest.kt @@ -121,7 +121,7 @@ class RememberGlidePreloadingDataTest { numberOfItemsToPreload = 1, fixedVisibleItemCount = 1, ) { data: Int, requestBuilder: RequestBuilder -> - requestBuilder.load(data) + requestBuilder.load(data).removeTheme() } TextButton(onClick = ::swapData) { Text(text = "Swap") } @@ -196,7 +196,7 @@ class RememberGlidePreloadingDataTest { numberOfItemsToPreload = 1, fixedVisibleItemCount = 1, ) { model, requestBuilder -> - requestBuilder.load(model) + requestBuilder.load(model).removeTheme() } } @@ -204,10 +204,16 @@ class RememberGlidePreloadingDataTest { // Wait for previous async image loads to finish glideComposeRule.waitForIdle() val nextPreloadModel: Drawable = - Glide.with(context).load(model).onlyRetrieveFromCache(true).submit().get() + Glide.with(context).load(model).removeTheme().onlyRetrieveFromCache(true).submit().get() assertThat(nextPreloadModel).isNotNull() } + // We're loading the same resource across two different Contexts. One is the Context from the + // instrumentation package, the other is the package under test. Each Context has it's own Theme, + // neither of which are equal to each other. So that we can verify an item is loaded into memory, + // we remove the themes from all requests that we need to have matching cache keys. + private fun RequestBuilder.removeTheme() = theme(null) + private companion object { const val model = android.R.drawable.star_big_on diff --git a/library/src/main/java/com/bumptech/glide/RequestBuilder.java b/library/src/main/java/com/bumptech/glide/RequestBuilder.java index 792972de17..52c5a46b92 100644 --- a/library/src/main/java/com/bumptech/glide/RequestBuilder.java +++ b/library/src/main/java/com/bumptech/glide/RequestBuilder.java @@ -1,11 +1,12 @@ package com.bumptech.glide; import static com.bumptech.glide.request.RequestOptions.diskCacheStrategyOf; -import static com.bumptech.glide.request.RequestOptions.signatureOf; import static com.bumptech.glide.request.RequestOptions.skipMemoryCacheOf; import android.annotation.SuppressLint; +import android.content.ContentResolver; import android.content.Context; +import android.content.res.Resources.Theme; import android.graphics.Bitmap; import android.graphics.drawable.Drawable; import android.net.Uri; @@ -603,6 +604,11 @@ public RequestBuilder load(@Nullable Drawable drawable) { * com.bumptech.glide.load.engine.DiskCacheStrategy#NONE} and/or {@link * com.bumptech.glide.request.RequestOptions#skipMemoryCache(boolean)} may be appropriate. * + *

If {@code string} is in fact a resource {@link Uri}, you should first parse it to a Uri + * using {@link Uri#parse(String)} and then pass the {@code Uri} to {@link #load(Uri)}. Doing so + * will ensure that we respect the appropriate theme / dark / light mode. As an alternative, you + * can also manually apply the current {@link Theme} using {@link #theme(Theme)}. + * * @see #load(Object) * @param string A file path, or a uri or url handled by {@link * com.bumptech.glide.load.model.UriLoader}. @@ -624,7 +630,21 @@ public RequestBuilder load(@Nullable String string) { * signature you create based on the data at the given Uri that will invalidate the cache if that * data changes. Alternatively, using {@link * com.bumptech.glide.load.engine.DiskCacheStrategy#NONE} and/or {@link - * com.bumptech.glide.request.RequestOptions#skipMemoryCache(boolean)} may be appropriate. + * com.bumptech.glide.request.RequestOptions#skipMemoryCache(boolean)} may be appropriate. The + * only exception to this is that if we recognize the given {@code uri} as having {@link + * ContentResolver#SCHEME_ANDROID_RESOURCE}, then we'll apply {@link AndroidResourceSignature} + * automatically. If we do so, calls to other {@code load()} methods will not override + * the automatically applied signature. + * + *

If {@code uri} has a {@link Uri#getScheme()} of {@link + * android.content.ContentResolver#SCHEME_ANDROID_RESOURCE}, then this method will add the + * {@link android.content.res.Resources.Theme} of the {@link Context} associated with this + * {@code requestBuilder} so that we can respect themeable attributes and/or light / dark mode. + * Any call to {@link #theme(Theme)} prior to this method call will be overridden. To avoid this, + * call {@link #theme(Theme)} after calling this method with either {@code null} or the + * {@code Theme} you'd prefer to use instead. Note that even if you change the + * theme, the {@link AndroidResourceSignature} will still be based on the {@link Context} + * theme. * * @see #load(Object) * @param uri The Uri representing the image. Must be of a type handled by {@link @@ -634,7 +654,22 @@ public RequestBuilder load(@Nullable String string) { @CheckResult @Override public RequestBuilder load(@Nullable Uri uri) { - return loadGeneric(uri); + return maybeApplyOptionsResourceUri(uri, loadGeneric(uri)); + } + + private RequestBuilder maybeApplyOptionsResourceUri( + @Nullable Uri uri, RequestBuilder requestBuilder) { + if (uri == null || !ContentResolver.SCHEME_ANDROID_RESOURCE.equals(uri.getScheme())) { + return requestBuilder; + } + return applyResourceThemeAndSignature(requestBuilder); + } + + private RequestBuilder applyResourceThemeAndSignature( + RequestBuilder requestBuilder) { + return requestBuilder + .theme(context.getTheme()) + .signature(AndroidResourceSignature.obtain(context)); } /** @@ -688,6 +723,13 @@ public RequestBuilder load(@Nullable File file) { * method, especially in conjunction with {@link com.bumptech.glide.load.Transformation}s with * caution for non-{@link Bitmap} {@link Drawable}s. * + *

This method will add the {@link android.content.res.Resources.Theme} of the {@link Context} + * associated with this {@code requestBuilder} so that we can respect themeable attributes and/or + * light / dark mode. Any call to {@link #theme(Theme)} prior to this method call will be + * overridden. To avoid this, call {@link #theme(Theme)} after calling this method with either + * {@code null} or the {@code Theme} you'd prefer to use instead. Note that even if you change the + * theme, the {@link AndroidResourceSignature} will still be based on the {@link Context} theme. + * * @see #load(Integer) * @see com.bumptech.glide.signature.AndroidResourceSignature */ @@ -695,7 +737,7 @@ public RequestBuilder load(@Nullable File file) { @CheckResult @Override public RequestBuilder load(@RawRes @DrawableRes @Nullable Integer resourceId) { - return loadGeneric(resourceId).apply(signatureOf(AndroidResourceSignature.obtain(context))); + return applyResourceThemeAndSignature(loadGeneric(resourceId)); } /** diff --git a/library/src/main/java/com/bumptech/glide/load/Options.java b/library/src/main/java/com/bumptech/glide/load/Options.java index e16b74d138..0212d90428 100644 --- a/library/src/main/java/com/bumptech/glide/load/Options.java +++ b/library/src/main/java/com/bumptech/glide/load/Options.java @@ -15,13 +15,19 @@ public void putAll(@NonNull Options other) { values.putAll((SimpleArrayMap, Object>) other.values); } - // TODO(b/234614365): Allow nullability. @NonNull public Options set(@NonNull Option option, @NonNull T value) { values.put(option, value); return this; } + // TODO(b/234614365): Expand usage of this method in BaseRequestOptions so that it's usable for + // other options. + public Options remove(@NonNull Option option) { + values.remove(option); + return this; + } + @Nullable @SuppressWarnings("unchecked") public T get(@NonNull Option option) { diff --git a/library/src/main/java/com/bumptech/glide/load/resource/drawable/ResourceDrawableDecoder.java b/library/src/main/java/com/bumptech/glide/load/resource/drawable/ResourceDrawableDecoder.java index 1c35989db8..c0ac8286b0 100644 --- a/library/src/main/java/com/bumptech/glide/load/resource/drawable/ResourceDrawableDecoder.java +++ b/library/src/main/java/com/bumptech/glide/load/resource/drawable/ResourceDrawableDecoder.java @@ -7,6 +7,7 @@ import android.content.res.Resources.Theme; import android.graphics.drawable.Drawable; import android.net.Uri; +import android.text.TextUtils; import androidx.annotation.DrawableRes; import androidx.annotation.NonNull; import androidx.annotation.Nullable; @@ -59,7 +60,8 @@ public ResourceDrawableDecoder(Context context) { @Override public boolean handles(@NonNull Uri source, @NonNull Options options) { - return source.getScheme().equals(ContentResolver.SCHEME_ANDROID_RESOURCE); + String scheme = source.getScheme(); + return scheme != null && scheme.equals(ContentResolver.SCHEME_ANDROID_RESOURCE); } @Nullable @@ -67,13 +69,17 @@ public boolean handles(@NonNull Uri source, @NonNull Options options) { public Resource decode( @NonNull Uri source, int width, int height, @NonNull Options options) { String packageName = source.getAuthority(); + if (TextUtils.isEmpty(packageName)) { + throw new IllegalStateException("Package name for " + source + " is null or empty"); + } Context targetContext = findContextForPackage(source, packageName); @DrawableRes int resId = findResourceIdFromUri(targetContext, source); - // We can't get a theme from another application. - Theme theme = options.get(THEME); - Preconditions.checkArgument( - targetContext.getPackageName().equals(packageName) || theme == null, - "Can't get a theme from another package"); + // Only use the provided theme if we're loading resources from our package. We can't get themes + // from other packages and we don't want to use a theme from our package when loading another + // package's resources. + Theme theme = + Preconditions.checkNotNull(packageName).equals(context.getPackageName()) + ? options.get(THEME) : null; Drawable drawable = theme == null ? DrawableDecoderCompat.getDrawable(context, targetContext, resId) @@ -82,7 +88,7 @@ public Resource decode( } @NonNull - private Context findContextForPackage(Uri source, String packageName) { + private Context findContextForPackage(Uri source, @NonNull String packageName) { // Fast path if (packageName.equals(context.getPackageName())) { return context; diff --git a/library/src/main/java/com/bumptech/glide/request/BaseRequestOptions.java b/library/src/main/java/com/bumptech/glide/request/BaseRequestOptions.java index 7a14e9f756..7e0ed46af5 100644 --- a/library/src/main/java/com/bumptech/glide/request/BaseRequestOptions.java +++ b/library/src/main/java/com/bumptech/glide/request/BaseRequestOptions.java @@ -421,11 +421,14 @@ public T theme(@Nullable Resources.Theme theme) { if (isAutoCloneEnabled) { return clone().theme(theme); } - // TODO(b/234614365): Allow the theme option to be null. - Preconditions.checkNotNull(theme); this.theme = theme; - fields |= THEME; - return set(ResourceDrawableDecoder.THEME, theme); + if (theme != null) { + fields |= THEME; + return set(ResourceDrawableDecoder.THEME, theme); + } else { + fields &= ~THEME; + return removeOption(ResourceDrawableDecoder.THEME); + } } /** @@ -558,6 +561,14 @@ public T set(@NonNull Option option, @NonNull Y value) { return selfOrThrowIfLocked(); } + T removeOption(@NonNull Option option) { + if (isAutoCloneEnabled) { + return clone().removeOption(option); + } + options.remove(option); + return selfOrThrowIfLocked(); + } + @NonNull @CheckResult public T decode(@NonNull Class resourceClass) {