From 6cb9486abc77d2fd37eda7de3f5a01c88725682d Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Fri, 18 Nov 2022 22:48:31 -0800 Subject: [PATCH] Handle invalid sizes in GlideImage. --- .../GlideImageDefaultTransformationTest.kt | 12 +- ...{GlideComposeTest.kt => GlideImageTest.kt} | 138 +++++++++++++++++- .../integration/compose/test/expectations.kt | 13 +- .../glide/integration/compose/test/nodes.kt | 10 +- .../glide/integration/compose/GlideImage.kt | 5 +- .../glide/integration/compose/Sizes.kt | 14 +- 6 files changed, 173 insertions(+), 19 deletions(-) rename integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/{GlideComposeTest.kt => GlideImageTest.kt} (54%) diff --git a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt index 67f8076840..d97e4ade03 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt +++ b/integration/compose/src/androidTest/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) -> RequestBuilder = { 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/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideComposeTest.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageTest.kt similarity index 54% rename from integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideComposeTest.kt rename to integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageTest.kt index d51fcfd703..f8c727e362 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideComposeTest.kt +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/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/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/expectations.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/expectations.kt index 03c3f5f642..d409f21288 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/expectations.kt +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/expectations.kt @@ -3,9 +3,11 @@ 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 @@ -13,8 +15,17 @@ 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/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/nodes.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/nodes.kt index 6c7cf14c45..5999e501ab 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/test/nodes.kt +++ b/integration/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().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 index ff8ea7644a..b9ad825e6c 100644 --- 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 index 7e50d31992..cc56d4eb59 100644 --- 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.overrideSize(): Size? = internal fun RequestBuilder.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) +} +