Avoid restarting requests unnecessarily

bumptech · Aug 12, 2023 · e9e2fa6 · e9e2fa6
1 parent 13130fb
commit e9e2fa6
Show file tree

Hide file tree

Showing 8 changed files with 278 additions and 112 deletions.
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)
       }

diff --git a/...ion/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageTest.kt b/...ion/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageTest.kt
@@ -1,22 +1,25 @@
-@file:OptIn(ExperimentalGlideComposeApi::class, InternalGlideApi::class)
-
 package com.bumptech.glide.integration.compose
 
 import android.content.Context
 import android.graphics.drawable.Drawable
+import androidx.compose.foundation.layout.Arrangement
 import androidx.compose.foundation.layout.Box
 import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.layout.size
+import androidx.compose.foundation.lazy.LazyRow
 import androidx.compose.material.Text
 import androidx.compose.material.TextButton
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.platform.LocalDensity
+import androidx.compose.ui.platform.testTag
 import androidx.compose.ui.test.assert
+import androidx.compose.ui.test.hasTestTag
 import androidx.compose.ui.test.onNodeWithContentDescription
 import androidx.compose.ui.test.onNodeWithText
 import androidx.compose.ui.test.performClick
+import androidx.compose.ui.test.performScrollToIndex
 import androidx.compose.ui.unit.dp
 import androidx.test.core.app.ApplicationProvider
 import com.bumptech.glide.Glide
@@ -38,9 +41,12 @@ import java.util.concurrent.atomic.AtomicReference
 import org.junit.Rule
 import org.junit.Test
 
+@OptIn(ExperimentalGlideComposeApi::class, InternalGlideApi::class)
 class GlideImageTest {
   private val context: Context = ApplicationProvider.getApplicationContext()
-  @get:Rule val glideComposeRule = GlideComposeRule()
+
+  @get:Rule
+  val glideComposeRule = GlideComposeRule()
 
   @Test
   fun glideImage_noModifierSize_resourceDrawable_displaysDrawable() {
@@ -100,6 +106,11 @@ class GlideImageTest {
       }
     }
 
+    // Precondition - ensure we loaded the first drawable.
+    glideComposeRule
+      .onNodeWithContentDescription(description)
+      .assert(expectDisplayedDrawable(firstDrawable))
+
     glideComposeRule.waitForIdle()
     glideComposeRule.onNodeWithText("Swap").performClick()
     glideComposeRule.waitForIdle()
@@ -173,7 +184,6 @@ class GlideImageTest {
       .assertDisplays(null)
   }
 
-
   @Test
   fun glideImage_withNegativeSize_doesNotStartLoad() {
     val description = "test"
@@ -323,4 +333,43 @@ class GlideImageTest {
 
     assertThat(onResourceReadyCounter.get()).isEqualTo(1)
   }
+
+  @Test
+  fun glideImage_whenDetachedAndReattached_rendersImage() {
+    val description = "test"
+    val testTag = "testTag"
+    glideComposeRule.setContent {
+      LazyRow(
+        horizontalArrangement = Arrangement.spacedBy(10.dp),
+        modifier = Modifier.testTag(testTag)
+      ) {
+        items(3) {
+          GlideImage(
+            model = android.R.drawable.star_big_on,
+            contentDescription = description + it,
+            modifier = Modifier.fillParentMaxSize()
+          )
+        }
+      }
+    }
+
+    // Scroll back and forth to trigger re-use of the GlideImages with the same
+    // parameters.
+    for (i in 0..2) {
+      glideComposeRule.onNode(hasTestTag(testTag)).performScrollToIndex(i)
+      glideComposeRule.waitForIdle()
+    }
+    glideComposeRule.onNode(hasTestTag(testTag)).performScrollToIndex(0)
+    glideComposeRule.waitForIdle()
+
+    val drawable = context.getDrawable(android.R.drawable.star_big_on)
+    // Make sure that all images are rendered
+    for (i in 0..2) {
+      glideComposeRule.onNode(hasTestTag(testTag)).performScrollToIndex(i)
+      glideComposeRule.waitForIdle()
+      glideComposeRule
+        .onNodeWithContentDescription(description + i)
+        .assert(expectDisplayedDrawable(drawable))
+    }
+  }
 }
diff --git a/integration/compose/src/main/java/com/bumptech/glide/ModelExtractor.kt b/integration/compose/src/main/java/com/bumptech/glide/ModelExtractor.kt
@@ -0,0 +1,4 @@
+package com.bumptech.glide
+
+internal val RequestBuilder<*>.internalModel
+  get() = model
diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideImage.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideImage.kt
@@ -11,7 +11,6 @@ import androidx.compose.runtime.collectAsState
 import androidx.compose.runtime.remember
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
-import androidx.compose.ui.draw.alpha
 import androidx.compose.ui.graphics.Color
 import androidx.compose.ui.graphics.ColorFilter
 import androidx.compose.ui.graphics.DefaultAlpha
@@ -20,11 +19,10 @@ import androidx.compose.ui.graphics.painter.BitmapPainter
 import androidx.compose.ui.graphics.painter.ColorPainter
 import androidx.compose.ui.graphics.painter.Painter
 import androidx.compose.ui.layout.ContentScale
+import androidx.compose.ui.layout.Layout
 import androidx.compose.ui.layout.layout
 import androidx.compose.ui.platform.LocalContext
 import androidx.compose.ui.platform.LocalInspectionMode
-import androidx.compose.ui.semantics.contentDescription
-import androidx.compose.ui.semantics.semantics
 import com.bumptech.glide.Glide
 import com.bumptech.glide.RequestBuilder
 import com.bumptech.glide.RequestManager
@@ -35,7 +33,6 @@ import com.bumptech.glide.integration.ktx.ImmediateGlideSize
 import com.bumptech.glide.integration.ktx.InternalGlideApi
 import com.bumptech.glide.integration.ktx.ResolvableGlideSize
 import com.bumptech.glide.integration.ktx.Resource
-import com.bumptech.glide.integration.ktx.Size
 import com.bumptech.glide.integration.ktx.Status
 import com.bumptech.glide.integration.ktx.flowResolvable
 import com.bumptech.glide.load.DataSource
@@ -116,8 +113,6 @@ public fun GlideImage(
       .let { loading?.apply(it::placeholder, it::placeholder) ?: it }
       .let { failure?.apply(it::error, it::error) ?: it }
 
-  val overrideSize: Size? = requestBuilder.overrideSize()
-  val size = rememberResolvableSize(overrideSize)
 
   // TODO(judds): It seems like we should be able to use the production paths for
   // resource / drawables as well as Composables. It's not totally clear what part of the prod code
@@ -127,6 +122,7 @@ public fun GlideImage(
     return
   }
 
+  // TODO(sam): Remove this branch when GlideSubcomposition has been out for a bit.
   val loadingComposable = loading?.maybeComposable()
   val failureComposable = failure?.maybeComposable()
   if (loadingComposable != null || failureComposable != null) {
@@ -148,11 +144,12 @@ public fun GlideImage(
       }
     }
   } else {
+    val size: ImmediateGlideSize? = requestBuilder.overrideSize()?.let { ImmediateGlideSize(it) }
     ModifierGlideImage(
       requestBuilder,
       size,
-      modifier,
       contentDescription,
+      modifier,
       alignment,
       contentScale,
       alpha,
@@ -440,15 +437,6 @@ public sealed class Placeholder {
     }
 }
 
-@OptIn(InternalGlideApi::class)
-@Composable
-internal fun rememberResolvableSize(
-  overrideSize: Size?,
-) =
-  remember(overrideSize) {
-    overrideSize?.let { ImmediateGlideSize(it) } ?: AsyncGlideSize()
-  }
-
 @Composable
 private fun rememberRequestBuilderWithDefaults(
   model: Any?,
@@ -488,20 +476,27 @@ private fun RequestBuilder<Drawable>.contentScaleTransform(
 @Composable
 private fun ModifierGlideImage(
   requestBuilder: RequestBuilder<Drawable>,
-  size: ResolvableGlideSize,
-  modifier: Modifier,
+  size: ImmediateGlideSize?,
   contentDescription: String?,
+  modifier: Modifier,
   alignment: Alignment,
   contentScale: ContentScale,
-  alpha: Float?,
+  alpha: Float,
   colorFilter: ColorFilter?
 ) {
-  Box(
+  Layout(
+    {},
     modifier
-      .glideNode(requestBuilder, size, contentScale, alignment, colorFilter)
-      .alpha(alpha ?: DefaultAlpha)
-      .then(Modifier.semantics {
-        contentDescription?.let { this.contentDescription = it }
-      })
-  )
+      .glideNode(
+        requestBuilder,
+        size,
+        contentDescription,
+        alignment,
+        contentScale,
+        alpha,
+        colorFilter,
+      )
+    ) { _, constraints ->
+    layout(constraints.minWidth, constraints.minHeight) {}
+  }
 }