Remove GlidePainter in favor of Modifier nodes / Flows

Using a custom Modifier node instead of a Painter follows a recommendation from the Compose team. It will allow us or library users to compose Glide's modifier in the future for animations of other effects. For now we do not actually expose the Modifier directly. This change adds a GlideSubcomposition that uses' Glide's flows integration to allow subcomposition based on image load state. Subcomposition allows us to deprecate the expensive Composable placeholder variants and also allows users to implement complex layouts or animations. In a subsequent change we'll explore adding a default crossfade and/or exposing the Glide modifier node so that users of the library can compose modifiers themselves.
bumptech · Aug 12, 2023 · 60b567e · 60b567e
1 parent 8f6d645
commit 60b567e
Show file tree

Hide file tree

Showing 31 changed files with 1,071 additions and 360 deletions.
diff --git a/benchmark/src/androidTest/java/com/bumptech/glide/benchmark/BenchmarkFromCache.java b/benchmark/src/androidTest/java/com/bumptech/glide/benchmark/BenchmarkFromCache.java
@@ -2,6 +2,7 @@
 
 import android.app.Application;
 import android.graphics.Bitmap;
+import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.RawRes;
 import androidx.benchmark.BenchmarkState;
@@ -111,17 +112,17 @@ private void loadImageWithExpectedDataSource(
                   public boolean onLoadFailed(
                       @Nullable GlideException e,
                       Object model,
-                      Target<Bitmap> target,
+                      @NonNull Target<Bitmap> target,
                       boolean isFirstResource) {
                     return false;
                   }
 
                   @Override
                   public boolean onResourceReady(
-                      Bitmap resource,
-                      Object model,
+                      @NonNull Bitmap resource,
+                      @NonNull Object model,
                       Target<Bitmap> target,
-                      DataSource dataSource,
+                      @NonNull DataSource dataSource,
                       boolean isFirstResource) {
                     dataSourceRef.set(dataSource);
                     return false;

diff --git a/instrumentation/src/androidTest/java/com/bumptech/glide/MultiRequestTest.java b/instrumentation/src/androidTest/java/com/bumptech/glide/MultiRequestTest.java
@@ -68,17 +68,17 @@ public void thumbnail_onResourceReady_forPrimary_isComplete_whenRequestListenerI
                   public boolean onLoadFailed(
                       @Nullable GlideException e,
                       Object model,
-                      Target<Drawable> target,
+                      @NonNull Target<Drawable> target,
                       boolean isFirstResource) {
                     return false;
                   }
 
                   @Override
                   public boolean onResourceReady(
-                      Drawable resource,
-                      Object model,
+                      @NonNull Drawable resource,
+                      @NonNull Object model,
                       Target<Drawable> target,
-                      DataSource dataSource,
+                      @NonNull DataSource dataSource,
                       boolean isFirstResource) {
                     isPrimaryRequestComplete.set(target.getRequest().isComplete());
                     countDownLatch.countDown();
@@ -115,7 +115,7 @@ public void thumbnail_onLoadFailed_forPrimary_isNotRunningOrComplete_whenRequest
                   public boolean onLoadFailed(
                       @Nullable GlideException e,
                       Object model,
-                      Target<Drawable> target,
+                      @NonNull Target<Drawable> target,
                       boolean isFirstResource) {
                     Request request = target.getRequest();
                     isNeitherRunningNorComplete.set(!request.isComplete() && !request.isRunning());
@@ -125,10 +125,10 @@ public boolean onLoadFailed(
 
                   @Override
                   public boolean onResourceReady(
-                      Drawable resource,
-                      Object model,
+                      @NonNull Drawable resource,
+                      @NonNull Object model,
                       Target<Drawable> target,
-                      DataSource dataSource,
+                      @NonNull DataSource dataSource,
                       boolean isFirstResource) {
                     return false;
                   }

diff --git a/integration/compose/api/compose.api b/integration/compose/api/compose.api
@@ -3,6 +3,7 @@ public abstract interface annotation class com/bumptech/glide/integration/compos
 
 public final class com/bumptech/glide/integration/compose/GlideImageKt {
 	public static final fun GlideImage (Ljava/lang/Object;Ljava/lang/String;Landroidx/compose/ui/Modifier;Landroidx/compose/ui/Alignment;Landroidx/compose/ui/layout/ContentScale;FLandroidx/compose/ui/graphics/ColorFilter;Lcom/bumptech/glide/integration/compose/Placeholder;Lcom/bumptech/glide/integration/compose/Placeholder;Lkotlin/jvm/functions/Function1;Landroidx/compose/runtime/Composer;II)V
+	public static final fun GlideSubcomposition (Ljava/lang/Object;Landroidx/compose/ui/Modifier;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function3;Landroidx/compose/runtime/Composer;II)V
 	public static final fun placeholder (I)Lcom/bumptech/glide/integration/compose/Placeholder;
 	public static final fun placeholder (Landroid/graphics/drawable/Drawable;)Lcom/bumptech/glide/integration/compose/Placeholder;
 	public static final fun placeholder (Lkotlin/jvm/functions/Function2;)Lcom/bumptech/glide/integration/compose/Placeholder;
@@ -13,6 +14,11 @@ public abstract interface class com/bumptech/glide/integration/compose/GlidePrel
 	public abstract fun getSize ()I
 }
 
+public abstract interface class com/bumptech/glide/integration/compose/GlideSubcompositionScope {
+	public abstract fun getPainter ()Landroidx/compose/ui/graphics/painter/Painter;
+	public abstract fun getState ()Lcom/bumptech/glide/integration/compose/RequestState;
+}
+
 public abstract class com/bumptech/glide/integration/compose/Placeholder {
 	public static final field $stable I
 }
@@ -22,3 +28,29 @@ public final class com/bumptech/glide/integration/compose/PreloadKt {
 	public static final fun rememberGlidePreloadingData-u6VnWhU (ILkotlin/jvm/functions/Function1;JILjava/lang/Integer;Lkotlin/jvm/functions/Function2;Landroidx/compose/runtime/Composer;II)Lcom/bumptech/glide/integration/compose/GlidePreloadingData;
 }
 
+public abstract class com/bumptech/glide/integration/compose/RequestState {
+	public static final field $stable I
+}
+
+public final class com/bumptech/glide/integration/compose/RequestState$Failure : com/bumptech/glide/integration/compose/RequestState {
+	public static final field $stable I
+	public static final field INSTANCE Lcom/bumptech/glide/integration/compose/RequestState$Failure;
+}
+
+public final class com/bumptech/glide/integration/compose/RequestState$Loading : com/bumptech/glide/integration/compose/RequestState {
+	public static final field $stable I
+	public static final field INSTANCE Lcom/bumptech/glide/integration/compose/RequestState$Loading;
+}
+
+public final class com/bumptech/glide/integration/compose/RequestState$Success : com/bumptech/glide/integration/compose/RequestState {
+	public static final field $stable I
+	public fun <init> (Lcom/bumptech/glide/load/DataSource;)V
+	public final fun component1 ()Lcom/bumptech/glide/load/DataSource;
+	public final fun copy (Lcom/bumptech/glide/load/DataSource;)Lcom/bumptech/glide/integration/compose/RequestState$Success;
+	public static synthetic fun copy$default (Lcom/bumptech/glide/integration/compose/RequestState$Success;Lcom/bumptech/glide/load/DataSource;ILjava/lang/Object;)Lcom/bumptech/glide/integration/compose/RequestState$Success;
+	public fun equals (Ljava/lang/Object;)Z
+	public final fun getDataSource ()Lcom/bumptech/glide/load/DataSource;
+	public fun hashCode ()I
+	public fun toString ()Ljava/lang/String;
+}
+
diff --git a/integration/compose/build.gradle b/integration/compose/build.gradle
@@ -5,11 +5,11 @@ plugins {
 
 android {
     namespace 'com.bumptech.glide.integration.compose'
-    compileSdk 33
+    compileSdk 34
 
     defaultConfig {
         minSdk 21
-        targetSdk 33
+        targetSdk 34
 
         testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
     }

diff --git a/...java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt b/...java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt
@@ -1,5 +1,3 @@
-@file:OptIn(ExperimentalGlideComposeApi::class, ExperimentalCoroutinesApi::class)
-
 package com.bumptech.glide.integration.compose
 
 import android.graphics.Canvas
@@ -16,7 +14,6 @@ import com.bumptech.glide.integration.compose.test.Constants
 import com.bumptech.glide.integration.compose.test.GlideComposeRule
 import com.bumptech.glide.integration.compose.test.assertDisplaysInstance
 import com.bumptech.glide.integration.compose.test.onNodeWithDefaultContentDescription
-import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.runTest
 import org.junit.Rule
 import org.junit.Test
@@ -28,6 +25,7 @@ import org.junit.runners.Parameterized
  *
  * Transformable types are tested in [GlideImageDefaultTransformationTest].
  */
+@OptIn(ExperimentalGlideComposeApi::class)
 @RunWith(Parameterized::class)
 class GlideImageCustomDrawableTransformationTest(
   private val contentScale: ContentScale,
@@ -98,8 +96,8 @@ class GlideImageCustomDrawableTransformationTest(
 @Suppress("DeprecatedCallableAddReplaceWith")
 private open class FakeDrawable : Drawable() {
   override fun draw(p0: Canvas) {}
-  override fun setAlpha(p0: Int) = throw UnsupportedOperationException()
-  override fun setColorFilter(p0: ColorFilter?) = throw UnsupportedOperationException()
+  override fun setAlpha(p0: Int) {}
+  override fun setColorFilter(p0: ColorFilter?) {}
   @Deprecated("Deprecated in Java")
   override fun getOpacity(): Int = throw UnsupportedOperationException()
 }

diff --git a/...idTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt b/...idTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt
@@ -7,9 +7,7 @@
 package com.bumptech.glide.integration.compose
 
 import android.content.Context
-import android.content.res.Resources
 import android.graphics.drawable.Drawable
-import android.util.TypedValue
 import androidx.annotation.DrawableRes
 import androidx.compose.foundation.layout.size
 import androidx.compose.runtime.Composable
@@ -28,7 +26,6 @@ import com.bumptech.glide.integration.ktx.ExperimentGlideFlows
 import com.bumptech.glide.integration.ktx.Resource
 import com.bumptech.glide.integration.ktx.Status
 import com.bumptech.glide.integration.ktx.flow
-import kotlin.math.roundToInt
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.test.runTest

diff --git a/...ompose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageErrorTest.kt b/...ompose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageErrorTest.kt
@@ -1,5 +1,3 @@
-@file:OptIn(ExperimentalGlideComposeApi::class)
-
 package com.bumptech.glide.integration.compose
 
 import android.content.Context
@@ -18,9 +16,12 @@ import org.junit.Test
  * Avoids [com.bumptech.glide.load.engine.executor.GlideIdlingResourceInit] because we want to make
  * assertions about loads that have not yet completed.
  */
+@OptIn(ExperimentalGlideComposeApi::class)
+@Suppress("DEPRECATION") // Tests for a deprecated method
 class GlideImageErrorTest {
   private val context: Context = ApplicationProvider.getApplicationContext()
-  @get:Rule val glideComposeRule = GlideComposeRule()
+  @get:Rule
+  val glideComposeRule = GlideComposeRule()
 
   @Test
   fun requestBuilderTransform_withErrorResourceId_displaysError() {
@@ -105,16 +106,16 @@ class GlideImageErrorTest {
         model = null,
         contentDescription = "none",
         failure =
-          placeholder {
-            // Nesting GlideImage is not really a good idea, but it's convenient for this test
-            // because
-            // we can use our helpers to assert on its contents.
-            GlideImage(
-              model = null,
-              contentDescription = description,
-              failure = placeholder(failureResourceId),
-            )
-          }
+        placeholder {
+          // Nesting GlideImage is not really a good idea, but it's convenient for this test
+          // because
+          // we can use our helpers to assert on its contents.
+          GlideImage(
+            model = null,
+            contentDescription = description,
+            failure = placeholder(failureResourceId),
+          )
+        }
       )
     }
 
@@ -186,13 +187,13 @@ class GlideImageErrorTest {
         model = null,
         contentDescription = "other",
         failure =
-          placeholder {
-            GlideImage(
-              model = null,
-              contentDescription = description,
-              failure = placeholder(failureResourceId),
-            )
-          },
+        placeholder {
+          GlideImage(
+            model = null,
+            contentDescription = description,
+            failure = placeholder(failureResourceId),
+          )
+        },
       ) {
         it.error(android.R.drawable.btn_star)
       }

diff --git a/.../src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt b/.../src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt
@@ -1,5 +1,3 @@
-@file:OptIn(ExperimentalGlideComposeApi::class)
-
 package com.bumptech.glide.integration.compose
 
 import android.content.Context
@@ -21,11 +19,16 @@ import org.junit.Test
  * [com.bumptech.glide.integration.compose.test.GlideComposeRule] because we want to make assertions
  * about loads that have not yet completed.
  */
+@Suppress("DEPRECATION") // Tests for a deprecated method
+@OptIn(ExperimentalGlideComposeApi::class)
 class GlideImagePlaceholderTest {
   private val context: Context = ApplicationProvider.getApplicationContext()
-  @get:Rule(order = 1) val composeRule = createComposeRule()
-  @get:Rule(order = 2) val waitModelLoaderRule = WaitModelLoaderRule()
-  @get:Rule(order = 3) val tearDownGlide = TearDownGlide()
+  @get:Rule(order = 1)
+  val composeRule = createComposeRule()
+  @get:Rule(order = 2)
+  val waitModelLoaderRule = WaitModelLoaderRule()
+  @get:Rule(order = 3)
+  val tearDownGlide = TearDownGlide()
 
   @Test
   fun requestBuilderTransform_withPlaceholderResourceId_displaysPlaceholder() {
@@ -120,16 +123,16 @@ class GlideImagePlaceholderTest {
         model = waitModel,
         contentDescription = "none",
         loading =
-          placeholder {
-            // Nesting GlideImage is not really a good idea, but it's convenient for this test
-            // because
-            // we can use our helpers to assert on its contents.
-            GlideImage(
-              model = waitModel,
-              contentDescription = description,
-              loading = placeholder(placeholderResourceId),
-            )
-          }
+        placeholder {
+          // Nesting GlideImage is not really a good idea, but it's convenient for this test
+          // because
+          // we can use our helpers to assert on its contents.
+          GlideImage(
+            model = waitModel,
+            contentDescription = description,
+            loading = placeholder(placeholderResourceId),
+          )
+        }
       )
     }
 
@@ -205,13 +208,13 @@ class GlideImagePlaceholderTest {
         model = waitModel,
         contentDescription = "other",
         loading =
-          placeholder {
-            GlideImage(
-              model = waitModel,
-              contentDescription = description,
-              loading = placeholder(placeholderResourceId),
-            )
-          },
+        placeholder {
+          GlideImage(
+            model = waitModel,
+            contentDescription = description,
+            loading = placeholder(placeholderResourceId),
+          )
+        },
       ) {
         it.placeholder(android.R.drawable.btn_star)
       }