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

Author: sjudd

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(

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<Drawable> ->
-          requestBuilder.load(data)
+          requestBuilder.load(data).removeTheme()
         }
 
       TextButton(onClick = ::swapData) { Text(text = "Swap") }
@@ -196,18 +196,24 @@ class RememberGlidePreloadingDataTest {
       numberOfItemsToPreload = 1,
       fixedVisibleItemCount = 1,
     ) { model, requestBuilder ->
-      requestBuilder.load(model)
+      requestBuilder.load(model).removeTheme()
     }
   }
 
   private fun assertThatModelIsInMemoryCache(@DrawableRes model: Int){
     // 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 <T> RequestBuilder<T>.removeTheme() = theme(null)
+
   private companion object {
     const val model = android.R.drawable.star_big_on
 

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<TranscodeType> load(@Nullable Drawable drawable) {
    * com.bumptech.glide.load.engine.DiskCacheStrategy#NONE} and/or {@link
    * com.bumptech.glide.request.RequestOptions#skipMemoryCache(boolean)} may be appropriate.
    *
+   * <p>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<TranscodeType> 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 <em>not</em> override
+   * the automatically applied signature.
+   *
+   * <p>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<TranscodeType> load(@Nullable String string) {
   @CheckResult
   @Override
   public RequestBuilder<TranscodeType> load(@Nullable Uri uri) {
-    return loadGeneric(uri);
+    return maybeApplyOptionsResourceUri(uri, loadGeneric(uri));
+  }
+
+  private RequestBuilder<TranscodeType> maybeApplyOptionsResourceUri(
+      @Nullable Uri uri, RequestBuilder<TranscodeType> requestBuilder) {
+    if (uri == null || !ContentResolver.SCHEME_ANDROID_RESOURCE.equals(uri.getScheme())) {
+      return requestBuilder;
+    }
+    return applyResourceThemeAndSignature(requestBuilder);
+  }
+
+  private RequestBuilder<TranscodeType> applyResourceThemeAndSignature(
+      RequestBuilder<TranscodeType> requestBuilder) {
+      return requestBuilder
+          .theme(context.getTheme())
+          .signature(AndroidResourceSignature.obtain(context));
   }
 
   /**
@@ -688,14 +723,21 @@ public RequestBuilder<TranscodeType> 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.
    *
+   * <p>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
    */
   @NonNull
   @CheckResult
   @Override
   public RequestBuilder<TranscodeType> load(@RawRes @DrawableRes @Nullable Integer resourceId) {
-    return loadGeneric(resourceId).apply(signatureOf(AndroidResourceSignature.obtain(context)));
+    return applyResourceThemeAndSignature(loadGeneric(resourceId));
   }
 
   /**

library/src/main/java/com/bumptech/glide/load/Options.java

@@ -15,13 +15,19 @@ public void putAll(@NonNull Options other) {
     values.putAll((SimpleArrayMap<Option<?>, Object>) other.values);
   }
 
-  // TODO(b/234614365): Allow nullability.
   @NonNull
   public <T> Options set(@NonNull Option<T> 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> T get(@NonNull Option<T> option) {

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,21 +60,26 @@ 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
   @Override
   public Resource<Drawable> 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<Drawable> 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;