Avoid duplicating Modifiers in GlideModifer

bumptech · Sep 3, 2023 · 830fd11 · 830fd11
1 parent 59c2d9b
commit 830fd11
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 10 deletions.
diff --git a/...rc/androidTest/assets/glideImage_withDrawBehind_drawsImageOnTopOfBackground.png b/...rc/androidTest/assets/glideImage_withDrawBehind_drawsImageOnTopOfBackground.png
diff --git a/...on/compose/src/androidTest/assets/glideImage_withPadding_appliesPaddingOnce.png b/...on/compose/src/androidTest/assets/glideImage_withPadding_appliesPaddingOnce.png
diff --git a/...e/src/androidTest/assets/glideSubcomposition_withPadding_appliesPaddingOnce.png b/...e/src/androidTest/assets/glideSubcomposition_withPadding_appliesPaddingOnce.png
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
@@ -48,6 +48,9 @@ class GlideImageTest {
   @get:Rule
   val glideComposeRule = GlideComposeRule()
 
+  @get:Rule(order = 2)
+  val glideComposeRule = GlideComposeRule()
+
   @Test
   fun glideImage_noModifierSize_resourceDrawable_displaysDrawable() {
     val description = "test"

diff --git a/...se/src/androidTest/java/com/bumptech/glide/integration/compose/GlideSubcompositionTest.kt b/...se/src/androidTest/java/com/bumptech/glide/integration/compose/GlideSubcompositionTest.kt
@@ -3,22 +3,43 @@ package com.bumptech.glide.integration.compose
 import android.content.Context
 import androidx.compose.foundation.Image
 import androidx.compose.foundation.layout.Box
+import androidx.compose.foundation.layout.Spacer
+import androidx.compose.foundation.layout.aspectRatio
+import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.layout.size
+import androidx.compose.foundation.layout.width
+import androidx.compose.material.icons.Icons
+import androidx.compose.material.icons.filled.Email
+import androidx.compose.runtime.mutableStateOf
+import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
+import androidx.compose.ui.draw.drawBehind
+import androidx.compose.ui.geometry.Size
+import androidx.compose.ui.geometry.isUnspecified
+import androidx.compose.ui.graphics.Color
+import androidx.compose.ui.semantics.contentDescription
+import androidx.compose.ui.semantics.semantics
+import androidx.compose.ui.test.captureToImage
+import androidx.compose.ui.test.onNodeWithContentDescription
 import androidx.compose.ui.unit.dp
 import androidx.test.core.app.ApplicationProvider
 import com.bumptech.glide.Glide
 import com.bumptech.glide.integration.compose.test.GlideComposeRule
 import com.bumptech.glide.load.DataSource
+import com.bumptech.glide.test.compareToGolden
+import com.bumptech.glide.test.pxToDp
 import com.google.common.truth.Truth.assertThat
 import org.junit.Rule
 import org.junit.Test
+import org.junit.rules.TestName
 
 @OptIn(ExperimentalGlideComposeApi::class)
 class GlideSubcompositionTest {
-  val context: Context = ApplicationProvider.getApplicationContext()
+  private val context: Context = ApplicationProvider.getApplicationContext()
 
-  @get:Rule
+  @get:Rule(order = 1)
+  val testName = TestName()
+  @get:Rule(order = 2)
   val glideComposeRule = GlideComposeRule()
 
   @Test
@@ -162,5 +183,49 @@ class GlideSubcompositionTest {
     glideComposeRule.waitForIdle()
     assertThat(dataSource).isEqualTo(DataSource.MEMORY_CACHE)
   }
+
+  // See #5272
+  @Test
+  fun glideSubcomposition_withPadding_appliesPaddingOnce() {
+    glideComposeRule.setContent {
+      val lastSize = remember { mutableStateOf(Size.Unspecified) }
+
+      GlideSubcomposition(
+        model = null,
+        modifier = Modifier
+          .semantics {
+            contentDescription = "test"
+          }
+          .width(400.pxToDp())
+          .aspectRatio(1f)
+          .drawBehind {
+            if (lastSize.value.isUnspecified) {
+              lastSize.value = size
+              drawRect(Color.Blue)
+            } else if (lastSize.value != this.size) {
+              drawRect(Color.Red)
+            } else {
+              drawRect(Color.Blue)
+            }
+          }
+          .padding(80.pxToDp()),
+      ) {
+        when (state) {
+          RequestState.Failure -> Image(
+            imageVector = Icons.Default.Email,
+            contentDescription = "placeholder",
+            modifier = Modifier.width(400.pxToDp())
+          )
+          RequestState.Loading -> Spacer(modifier = Modifier.size(100.pxToDp()))
+          is RequestState.Success -> Image(painter = painter, contentDescription = null)
+        }
+      }
+    }
+    glideComposeRule.waitForIdle()
+    glideComposeRule.onNodeWithContentDescription("test")
+      .captureToImage()
+      .compareToGolden(testName.methodName)
+  }
 }
 
+
diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt
@@ -88,16 +88,14 @@ internal fun Modifier.glideNode(
     requestListener,
     draw,
     transitionFactory,
-  ) then
-      clipToBounds() then
+  )
+    .clipToBounds()
+    .semantics {
       if (contentDescription != null) {
-        semantics {
-          this@semantics.contentDescription = contentDescription
-          role = Role.Image
-        }
-      } else {
-        Modifier
+        this@semantics.contentDescription = contentDescription
       }
+      role = Role.Image
+    }
 }
 
 @ExperimentalGlideComposeApi