Make Placeholder a type that can be convered to Painter

This removes the deprecated Composable variant of Placeholder. Users who require that functionality should instead use GlideSubcomposition. It's useful to remove this now because it allows for a much simpler Placholder interface and it unblocks us from moving most of GlideImage into GlideModifier. In turn that will let us expose GlideModifier as a real composable API component.
vigneshvg · Nov 3, 2023 · c06a85d · c06a85d
1 parent 75599b7
commit c06a85d
Show file tree

Hide file tree

Showing 6 changed files with 142 additions and 218 deletions.
diff --git a/...java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt b/...java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt
@@ -34,7 +34,8 @@ class GlideImageCustomDrawableTransformationTest(
   // though it's only used by Parameters to create the test name.
   @Suppress("unused") private val name: String,
 ) {
-  @get:Rule val glideComposeRule = GlideComposeRule()
+  @get:Rule
+  val glideComposeRule = GlideComposeRule()
 
   @Test
   fun glideImage_nonBitmapDrawable_doesNotThrow() = runTest {
@@ -98,6 +99,7 @@ private open class FakeDrawable : Drawable() {
   override fun draw(p0: Canvas) {}
   override fun setAlpha(p0: Int) {}
   override fun setColorFilter(p0: ColorFilter?) {}
+
   @Deprecated("Deprecated in Java")
   override fun getOpacity(): Int = throw UnsupportedOperationException()
 }

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
@@ -18,9 +18,9 @@ import org.junit.Test
  * 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()
 
@@ -64,7 +64,7 @@ class GlideImageErrorTest {
 
     glideComposeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(failureResourceId))
+      .assert(expectDisplayedPainter(context, failureResourceId))
   }
 
   @Test
@@ -81,7 +81,7 @@ class GlideImageErrorTest {
 
     glideComposeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedDrawable(failureDrawable))
+      .assert(expectDisplayedPainter(failureDrawable))
   }
 
   @Test
@@ -98,33 +98,6 @@ class GlideImageErrorTest {
     glideComposeRule.onNodeWithContentDescription(description).assert(expectNoDrawable())
   }
 
-  @Test
-  fun failureParameter_withComposable_displaysComposable() {
-    val failureResourceId = android.R.drawable.star_big_off
-    val description = "test"
-    glideComposeRule.setContent {
-      GlideImage(
-        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),
-          )
-        }
-      )
-    }
-
-    glideComposeRule
-      .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(failureResourceId))
-  }
-
   @Test
   fun failure_setViaFailureParameterWithResourceId_andRequestBuilderTransform_prefersFailureParameter() {
     val description = "test"
@@ -141,7 +114,7 @@ class GlideImageErrorTest {
 
     glideComposeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(failureResourceId))
+      .assert(expectDisplayedPainter(context, failureResourceId))
   }
 
   @Test
@@ -179,7 +152,7 @@ class GlideImageErrorTest {
 
     glideComposeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedDrawable(failureDrawable))
+      .assert(expectDisplayedPainter(failureDrawable))
   }
 
   @Test
@@ -197,30 +170,4 @@ class GlideImageErrorTest {
 
     glideComposeRule.onNodeWithContentDescription(description).assert(expectNoDrawable())
   }
-
-  @Test
-  fun failure_setViaFailureParameterWithComposable_andRequestBuilderTransform_showsComposable() {
-    val description = "test"
-    val failureResourceId = android.R.drawable.star_big_off
-    glideComposeRule.setContent {
-      GlideImage(
-        model = null,
-        contentDescription = "other",
-        failure =
-        placeholder {
-          GlideImage(
-            model = null,
-            contentDescription = description,
-            failure = placeholder(failureResourceId),
-          )
-        },
-      ) {
-        it.error(android.R.drawable.btn_star)
-      }
-    }
-
-    glideComposeRule
-      .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(failureResourceId))
-  }
 }
diff --git a/.../src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt b/.../src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt
@@ -20,14 +20,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()
 
@@ -78,7 +80,7 @@ class GlideImagePlaceholderTest {
 
     composeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(placeholderResourceId))
+      .assert(expectDisplayedPainter(context, placeholderResourceId))
   }
 
   @Test
@@ -96,7 +98,7 @@ class GlideImagePlaceholderTest {
 
     composeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedDrawable(placeholderDrawable))
+      .assert(expectDisplayedPainter(placeholderDrawable))
   }
 
   @Test
@@ -114,34 +116,6 @@ class GlideImagePlaceholderTest {
     composeRule.onNodeWithContentDescription(description).assert(expectNoDrawable())
   }
 
-  @Test
-  fun loadingParameter_withComposable_displaysComposable() {
-    val waitModel = waitModelLoaderRule.waitOn(android.R.drawable.star_big_on)
-    val placeholderResourceId = android.R.drawable.star_big_off
-    val description = "test"
-    composeRule.setContent {
-      GlideImage(
-        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),
-          )
-        }
-      )
-    }
-
-    composeRule
-      .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(placeholderResourceId))
-  }
-
   @Test
   fun loading_setViaLoadingParameterWithResourceId_andRequestBuilderTransform_prefersLoadingParameter() {
     val description = "test"
@@ -159,7 +133,7 @@ class GlideImagePlaceholderTest {
 
     composeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(placeholderResourceId))
+      .assert(expectDisplayedPainter(context, placeholderResourceId))
   }
 
   @Test
@@ -179,7 +153,7 @@ class GlideImagePlaceholderTest {
 
     composeRule
       .onNodeWithContentDescription(description)
-      .assert(expectDisplayedDrawable(placeholderDrawable))
+      .assert(expectDisplayedPainter(placeholderDrawable))
   }
 
   @Test
@@ -219,31 +193,4 @@ class GlideImagePlaceholderTest {
 
     composeRule.onNodeWithContentDescription(description).assert(expectNoDrawable())
   }
-
-  @Test
-  fun loading_setViaLoadingParameterWithComposable_andRequestBuilderTransform_showsComposable() {
-    val description = "test"
-    val waitModel = waitModelLoaderRule.waitOn(android.R.drawable.star_big_on)
-    val placeholderResourceId = android.R.drawable.star_big_off
-    composeRule.setContent {
-      GlideImage(
-        model = waitModel,
-        contentDescription = "other",
-        loading =
-        placeholder {
-          GlideImage(
-            model = waitModel,
-            contentDescription = description,
-            loading = placeholder(placeholderResourceId),
-          )
-        },
-      ) {
-        it.placeholder(android.R.drawable.btn_star)
-      }
-    }
-
-    composeRule
-      .onNodeWithContentDescription(description)
-      .assert(expectDisplayedResource(placeholderResourceId))
-  }
 }
diff --git a/.../compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/expectations.kt b/.../compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/expectations.kt
@@ -1,16 +1,17 @@
-@file:OptIn(InternalGlideApi::class)
-
 package com.bumptech.glide.integration.compose.test
 
 import android.content.Context
 import android.content.res.Resources
 import android.graphics.Bitmap
-import android.graphics.drawable.Animatable
 import android.graphics.drawable.BitmapDrawable
 import android.graphics.drawable.Drawable
 import android.util.TypedValue
-import androidx.compose.runtime.MutableState
+import androidx.annotation.DrawableRes
+import androidx.compose.ui.graphics.ImageBitmap
+import androidx.compose.ui.graphics.asImageBitmap
+import androidx.compose.ui.graphics.painter.BitmapPainter
 import androidx.compose.ui.graphics.painter.Painter
+import androidx.compose.ui.res.imageResource
 import androidx.compose.ui.semantics.SemanticsPropertyKey
 import androidx.compose.ui.test.SemanticsMatcher
 import androidx.test.core.app.ApplicationProvider
@@ -30,26 +31,76 @@ fun Int.dpToPixels() =
   )
     .roundToInt()
 
+@OptIn(InternalGlideApi::class)
 fun Int.bitmapSize() = context().resources.getDrawable(this, context().theme).size()
 
+@OptIn(InternalGlideApi::class)
 fun Drawable.size() = (this as BitmapDrawable).bitmap.let { Size(it.width, it.height) }
 
 fun expectDisplayedResource(resourceId: Int) =
   expectDisplayedDrawable(context().getDrawable(resourceId))
 
 fun Drawable?.bitmapOrThrow(): Bitmap? = if (this == null) null else (this as BitmapDrawable).bitmap
 
+@OptIn(InternalGlideApi::class)
 fun expectDisplayedDrawableSize(expectedSize: Size): SemanticsMatcher =
   expectDisplayedDrawable(expectedSize) { it?.size() }
 
 fun expectDisplayedDrawable(expectedValue: Drawable?): SemanticsMatcher =
   expectDisplayedDrawable(expectedValue.bitmapOrThrow(), ::compareBitmaps) { it.bitmapOrThrow() }
 
+
 fun expectDisplayedPainter(expectedValue: Painter?): SemanticsMatcher =
   expectStateValue(
-    DisplayedPainterKey, expectedValue, {first, second -> first == second}, {value -> value}
+    DisplayedPainterKey, expectedValue, { first, second -> first == second }, { value -> value }
   )
 
+// These are hacks. We're relying on the ordering of expected.equals(actual) so that our
+// DeepEqualsImageBitmap's equals method will be used. This doesn't implement the equals/hashcode
+// contract :/
+fun expectDisplayedPainter(context: Context, @DrawableRes resourceId: Int): SemanticsMatcher {
+  val imageBitmap = DeepEqualsImageBitmap(ImageBitmap.imageResource(context.resources, resourceId))
+  val painter = BitmapPainter(imageBitmap)
+  return expectDisplayedPainter(painter)
+}
+
+fun expectDisplayedPainter(expectedValue: Drawable?): SemanticsMatcher {
+  val bitmapDrawable = expectedValue as BitmapDrawable
+  val imageBitmap = DeepEqualsImageBitmap(bitmapDrawable.bitmap.asImageBitmap())
+  return expectDisplayedPainter(BitmapPainter(imageBitmap))
+}
+
+class DeepEqualsImageBitmap(base: ImageBitmap) : ImageBitmap by base {
+  override fun equals(other: Any?): Boolean {
+    val compare = other as? ImageBitmap ?: return false
+    return compare.width == width && compare.height == height
+        && compare.hasAlpha == hasAlpha
+        && compare.config == config
+        && compare.colorSpace == colorSpace
+        && equalPixels(this, other)
+  }
+
+  private fun ImageBitmap.readPixels(): IntArray {
+    val pixels = IntArray(width * height)
+    readPixels(pixels)
+    return pixels
+  }
+
+  private fun equalPixels(first: ImageBitmap, second: ImageBitmap): Boolean {
+    return first.readPixels().contentEquals(second.readPixels())
+  }
+
+  override fun hashCode(): Int {
+    var result = width.hashCode()
+    result *= 31 * height.hashCode()
+    result *= 31 * hasAlpha.hashCode()
+    result *= 31 * config.hashCode()
+    result *= 31 * colorSpace.hashCode()
+    result *= 31 * readPixels().contentHashCode()
+    return result
+  }
+}
+
 fun expectNoDrawable(): SemanticsMatcher = expectDisplayedDrawable(null)
 
 private fun compareBitmaps(first: Bitmap?, second: Bitmap?): Boolean {
@@ -77,15 +128,16 @@ private fun <ValueT, TransformedValueT> expectStateValue(
 ): SemanticsMatcher =
   SemanticsMatcher("${key.name} = '$expectedValue'") {
     val value = transform(it.config.getOrElseNullable(key) { null }?.invoke())
-    if (!compare(value, expectedValue)) {
+    if (!compare(expectedValue, value)) {
       throw AssertionError("Expected: $expectedValue, but was: $value")
     }
     true
   }
 
 fun expectSameInstance(expectedDrawable: Drawable) =
   SemanticsMatcher("${DisplayedDrawableKey.name} = '$expectedDrawable'") {
-    val actualValue: Drawable? = it.config.getOrElseNullable(DisplayedDrawableKey) { null }?.invoke()
+    val actualValue: Drawable? =
+      it.config.getOrElseNullable(DisplayedDrawableKey) { null }?.invoke()
     if (actualValue !== expectedDrawable) {
       throw AssertionError("Expected: $expectedDrawable, but was: $actualValue")
     }