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) {