Handle invalid sizes in GlideImage.

bumptech · Nov 19, 2022 · 6cb9486 · 6cb9486
1 parent fab2aed
commit 6cb9486
Show file tree

Hide file tree

Showing 6 changed files with 173 additions and 19 deletions.
diff --git a/...idTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt b/...idTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt
@@ -22,6 +22,7 @@ import com.bumptech.glide.RequestBuilder
 import com.bumptech.glide.integration.compose.test.Constants
 import com.bumptech.glide.integration.compose.test.GlideComposeRule
 import com.bumptech.glide.integration.compose.test.assertDisplays
+import com.bumptech.glide.integration.compose.test.dpToPixels
 import com.bumptech.glide.integration.compose.test.onNodeWithDefaultContentDescription
 import com.bumptech.glide.integration.ktx.ExperimentGlideFlows
 import com.bumptech.glide.integration.ktx.Resource
@@ -161,7 +162,8 @@ class GlideImageDefaultTransformationTest {
     @DrawableRes resourceId: Int,
     transformation: (RequestBuilder<Drawable>) -> RequestBuilder<Drawable> = { it -> it },
   ): Drawable =
-    transformation(Glide.with(context).load(resourceId).override(WIDTH.px(), HEIGHT.px()))
+    transformation(
+      Glide.with(context).load(resourceId).override(WIDTH.dpToPixels(), HEIGHT.dpToPixels()))
       .loadRequiringSuccess()
 
   @Composable
@@ -186,11 +188,3 @@ class GlideImageDefaultTransformationTest {
     val SIZE_MODIFIER = Modifier.size(WIDTH.dp, HEIGHT.dp)
   }
 }
-
-fun Int.px() =
-  TypedValue.applyDimension(
-      TypedValue.COMPLEX_UNIT_DIP,
-      this.toFloat(),
-      Resources.getSystem().displayMetrics
-    )
-    .roundToInt()
diff --git a/...e/integration/compose/GlideComposeTest.kt → ...ide/integration/compose/GlideImageTest.kt b/...e/integration/compose/GlideComposeTest.kt → ...ide/integration/compose/GlideImageTest.kt
@@ -4,6 +4,7 @@ package com.bumptech.glide.integration.compose
 
 import android.content.Context
 import android.graphics.drawable.Drawable
+import androidx.compose.foundation.layout.Box
 import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.layout.size
 import androidx.compose.material.Text
@@ -20,7 +21,9 @@ 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.integration.compose.test.assertDisplays
 import com.bumptech.glide.integration.compose.test.bitmapSize
+import com.bumptech.glide.integration.compose.test.dpToPixels
 import com.bumptech.glide.integration.compose.test.expectDisplayedDrawable
 import com.bumptech.glide.integration.compose.test.expectDisplayedDrawableSize
 import com.bumptech.glide.integration.ktx.InternalGlideApi
@@ -29,7 +32,7 @@ import java.util.concurrent.atomic.AtomicReference
 import org.junit.Rule
 import org.junit.Test
 
-class GlideComposeTest {
+class GlideImageTest {
   private val context: Context = ApplicationProvider.getApplicationContext()
   @get:Rule val glideComposeRule = GlideComposeRule()
 
@@ -142,9 +145,140 @@ class GlideComposeTest {
     }
 
     glideComposeRule.waitForIdle()
-
     glideComposeRule
       .onNodeWithContentDescription(description)
       .assert(expectDisplayedDrawable(fullsizeDrawable))
   }
+
+  @Test
+  fun glideImage_withZeroSize_doesNotStartLoad() {
+    val description = "test"
+    glideComposeRule.setContent {
+      Box(modifier = Modifier.size(0.dp)) {
+        GlideImage(
+          model = android.R.drawable.star_big_on,
+          contentDescription = description,
+        )
+      }
+    }
+    glideComposeRule.waitForIdle()
+    glideComposeRule
+      .onNodeWithContentDescription(description)
+      .assertDisplays(null)
+  }
+
+
+  @Test
+  fun glideImage_withNegativeSize_doesNotStartLoad() {
+    val description = "test"
+    glideComposeRule.setContent {
+      Box(modifier = Modifier.size((-10).dp)) {
+        GlideImage(
+          model = android.R.drawable.star_big_on,
+          contentDescription = description,
+        )
+      }
+    }
+    glideComposeRule.waitForIdle()
+    glideComposeRule
+      .onNodeWithContentDescription(description)
+      .assertDisplays(null)
+  }
+
+  @Test
+  fun glideImage_withZeroWidth_validHeight_doesNotStartLoad() {
+    val description = "test"
+    glideComposeRule.setContent {
+      Box(modifier = Modifier.size(0.dp, 10.dp)) {
+        GlideImage(
+          model = android.R.drawable.star_big_on,
+          contentDescription = description,
+        )
+      }
+    }
+    glideComposeRule.waitForIdle()
+    glideComposeRule
+      .onNodeWithContentDescription(description)
+      .assertDisplays(null)
+  }
+
+  @Test
+  fun glideImage_withValidWidth_zeroHeight_doesNotStartLoad() {
+    val description = "test"
+    glideComposeRule.setContent {
+      Box(modifier = Modifier.size(10.dp, 0.dp)) {
+        GlideImage(
+          model = android.R.drawable.star_big_on,
+          contentDescription = description,
+        )
+      }
+    }
+    glideComposeRule.waitForIdle()
+    glideComposeRule
+      .onNodeWithContentDescription(description)
+      .assertDisplays(null)
+  }
+
+  @Test
+  fun glideImage_withZeroSize_thenValidSize_startsLoadWithValidSize() {
+    val description = "test"
+    val resourceId = android.R.drawable.star_big_on
+    val validSizeDp = 10
+    glideComposeRule.setContent {
+      val currentSize = remember { mutableStateOf(0.dp) }
+      fun swapSize() {
+        currentSize.value = validSizeDp.dp
+      }
+
+      TextButton(onClick = ::swapSize) { Text(text = "Swap") }
+      Box(modifier = Modifier.size(currentSize.value)) {
+        GlideImage(
+          model = resourceId,
+          contentDescription = description,
+        )
+      }
+    }
+    glideComposeRule.waitForIdle()
+    glideComposeRule.onNodeWithText("Swap").performClick()
+    glideComposeRule.waitForIdle()
+
+    glideComposeRule
+      .onNodeWithContentDescription(description)
+      .assert(expectDisplayedDrawableSize(Size(validSizeDp.dpToPixels(), validSizeDp.dpToPixels())))
+  }
+
+  @Test
+  fun glideImage_withZeroSize_thenMultipleValidSizes_startsLoadWithFirstValidSize() {
+    val description = "test"
+    val resourceId = android.R.drawable.star_big_on
+    val validSizeDps = listOf(10, 20, 30, 40)
+    glideComposeRule.setContent {
+      val currentSize = remember { mutableStateOf(0.dp) }
+      val currentSizeIndex = remember { mutableStateOf(0) }
+      fun swapSize() {
+        currentSize.value = validSizeDps[currentSizeIndex.value].dp
+        currentSizeIndex.value++
+      }
+
+      TextButton(onClick = ::swapSize) { Text(text = "Swap") }
+      Box(modifier = Modifier.size(currentSize.value)) {
+        GlideImage(
+          model = resourceId,
+          contentDescription = description,
+        )
+      }
+    }
+    repeat(validSizeDps.size) {
+      glideComposeRule.waitForIdle()
+      glideComposeRule.onNodeWithText("Swap").performClick()
+    }
+    glideComposeRule.waitForIdle()
+
+    val expectedSize = validSizeDps[0]
+    glideComposeRule
+      .onNodeWithContentDescription(description)
+      .assert(
+        expectDisplayedDrawableSize(Size(expectedSize.dpToPixels(), expectedSize.dpToPixels()))
+      )
+  }
 }
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
@@ -3,18 +3,29 @@
 package com.bumptech.glide.integration.compose.test
 
 import android.content.Context
+import android.content.res.Resources
 import android.graphics.Bitmap
 import android.graphics.drawable.BitmapDrawable
 import android.graphics.drawable.Drawable
+import android.util.TypedValue
 import androidx.compose.runtime.MutableState
 import androidx.compose.ui.semantics.SemanticsPropertyKey
 import androidx.compose.ui.test.SemanticsMatcher
 import androidx.test.core.app.ApplicationProvider
 import com.bumptech.glide.integration.compose.DisplayedDrawableKey
 import com.bumptech.glide.integration.ktx.InternalGlideApi
 import com.bumptech.glide.integration.ktx.Size
+import kotlin.math.roundToInt
 
-fun context(): Context = ApplicationProvider.getApplicationContext()
+private fun context(): Context = ApplicationProvider.getApplicationContext()
+
+fun Int.dpToPixels() =
+  TypedValue.applyDimension(
+    TypedValue.COMPLEX_UNIT_DIP,
+    this.toFloat(),
+    Resources.getSystem().displayMetrics
+  )
+    .roundToInt()
 
 fun Int.bitmapSize() = context().resources.getDrawable(this, context().theme).size()
 

diff --git a/...gration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/nodes.kt b/...gration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/nodes.kt
@@ -1,10 +1,14 @@
 package com.bumptech.glide.integration.compose.test
 
+import android.app.Application
+import android.content.Context
 import android.graphics.drawable.Drawable
+import androidx.annotation.DrawableRes
 import androidx.compose.ui.test.SemanticsNodeInteraction
 import androidx.compose.ui.test.assert
 import androidx.compose.ui.test.junit4.ComposeContentTestRule
 import androidx.compose.ui.test.onNodeWithContentDescription
+import androidx.test.core.app.ApplicationProvider
 
 object Constants {
   const val DEFAULT_DESCRIPTION = "test"
@@ -13,7 +17,11 @@ object Constants {
 fun ComposeContentTestRule.onNodeWithDefaultContentDescription() =
   onNodeWithContentDescription(Constants.DEFAULT_DESCRIPTION)
 
-fun SemanticsNodeInteraction.assertDisplays(drawable: Drawable) =
+fun SemanticsNodeInteraction.assertDisplays(@DrawableRes resourceId: Int) =
+  assertDisplays(ApplicationProvider.getApplicationContext<Application>().getDrawable(resourceId))
+
+
+fun SemanticsNodeInteraction.assertDisplays(drawable: Drawable?) =
   assert(expectDisplayedDrawable(drawable))
 
 fun SemanticsNodeInteraction.assertDisplaysInstance(drawable: Drawable) =

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
@@ -312,7 +312,10 @@ private fun rememberGlidePainter(
 @OptIn(InternalGlideApi::class)
 private fun Modifier.sizeObservingModifier(sizeObserver: SizeObserver): Modifier =
   this.layout { measurable, constraints ->
-    sizeObserver.setSize(constraints.inferredGlideSize())
+    val inferredSize = constraints.inferredGlideSize()
+    if (inferredSize != null) {
+      sizeObserver.setSize(inferredSize)
+    }
     val placeable = measurable.measure(constraints)
     layout(placeable.width, placeable.height) { placeable.place(0, 0) }
   }

diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/Sizes.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/Sizes.kt
@@ -32,8 +32,12 @@ internal fun RequestBuilder<out Any?>.overrideSize(): Size? =
 internal fun RequestBuilder<out Any?>.isOverrideSizeSet(): Boolean =
   overrideWidth.isValidGlideDimension() && overrideHeight.isValidGlideDimension()
 
-internal fun Constraints.inferredGlideSize(): Size =
-  Size(
-    if (hasBoundedWidth) maxWidth else Target.SIZE_ORIGINAL,
-    if (hasBoundedHeight) maxHeight else Target.SIZE_ORIGINAL,
-  )
+internal fun Constraints.inferredGlideSize(): Size? {
+  val width = if (hasBoundedWidth) maxWidth else Target.SIZE_ORIGINAL
+  val height = if (hasBoundedHeight) maxHeight else Target.SIZE_ORIGINAL
+  if (!width.isValidGlideDimension() || !height.isValidGlideDimension()) {
+    return null
+  }
+  return Size(width, height)
+}
+