From 89472a35df81ca8924ad31870ccd17a5a800674e Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Sun, 6 Nov 2022 19:07:04 -0800 Subject: [PATCH] Apply optional transformations for ContentScales Using optional trasnformations more closely matches the behavior of RequestBuilder when loading into Views. Using non-optional transformations will cause exceptions to be thrown if the loaded resource type cannot be transformed. In particular any non-Drawable / non-Bitmap resource will throw as will any Drawable type that cannot be converted into a simple Bitmap. Fixes #4943 --- ...deImageCustomDrawableTransformationTest.kt | 100 +++++++++- .../GlideImageDefaultTransformationTest.kt | 183 ++++++++++++++++++ .../integration/compose/test/expectations.kt | 10 + .../glide/integration/compose/test/nodes.kt | 18 ++ .../glide/integration/compose/GlideImage.kt | 4 +- 5 files changed, 311 insertions(+), 4 deletions(-) create mode 100644 integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt diff --git a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt index ab7e2bd8e3..2303e3477b 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageCustomDrawableTransformationTest.kt @@ -1,4 +1,100 @@ +@file:OptIn(ExperimentalGlideComposeApi::class, ExperimentalCoroutinesApi::class) + package com.bumptech.glide.integration.compose -class GlideImageCustomDrawableTransformationTest { -} \ No newline at end of file +import android.graphics.Canvas +import android.graphics.ColorFilter +import android.graphics.drawable.Animatable +import android.graphics.drawable.Drawable +import androidx.compose.foundation.layout.size +import androidx.compose.ui.Modifier +import androidx.compose.ui.geometry.Size +import androidx.compose.ui.layout.ContentScale +import androidx.compose.ui.layout.ScaleFactor +import androidx.compose.ui.unit.dp +import com.bumptech.glide.integration.compose.test.Constants +import com.bumptech.glide.integration.compose.test.GlideComposeRule +import com.bumptech.glide.integration.compose.test.assertDisplaysInstance +import com.bumptech.glide.integration.compose.test.onNodeWithDefaultContentDescription +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.Parameterized + +/** + * Tests Issue #4943. + * + * Transformable types are tested in [GlideImageDefaultTransformationTest]. + */ +@RunWith(Parameterized::class) +class GlideImageCustomDrawableTransformationTest(private val contentScale: ContentScale) { + @get:Rule val glideComposeRule = GlideComposeRule() + + @Test + fun glideImage_nonBitmapDrawable_doesNotThrow() = runTest { + val customDrawable = FakeDrawable() + + glideComposeRule.setContent { + GlideImage( + model = customDrawable, + contentScale = contentScale, + contentDescription = Constants.DEFAULT_DESCRIPTION, + modifier = Modifier.size(100.dp, 200.dp) + ) + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplaysInstance(customDrawable) + } + + @Test + fun glideImage_animatableDrawable_doesNotThrow() = runTest { + val customDrawable = FakeAnimatableDrawable() + + glideComposeRule.setContent { + GlideImage( + model = customDrawable, + contentScale = contentScale, + contentDescription = Constants.DEFAULT_DESCRIPTION, + modifier = Modifier.size(200.dp, 100.dp) + ) + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplaysInstance(customDrawable) + } + + companion object { + @JvmStatic + @Parameterized.Parameters(name = "{0}: contentScale") + fun data() = arrayOf( + ContentScale.Crop, + ContentScale.FillBounds, + ContentScale.FillHeight, + ContentScale.FillWidth, + ContentScale.Fit, + ContentScale.Inside, + ContentScale.None, + object : ContentScale { + override fun computeScaleFactor(srcSize: Size, dstSize: Size): ScaleFactor = + ContentScale.Fit.computeScaleFactor(srcSize, dstSize) + }, + ) + } +} + +@Suppress("DeprecatedCallableAddReplaceWith") +private open class FakeDrawable : Drawable() { + override fun draw(p0: Canvas) { + } + override fun setAlpha(p0: Int) = throw UnsupportedOperationException() + override fun setColorFilter(p0: ColorFilter?) = throw UnsupportedOperationException() + @Deprecated("Deprecated in Java") + override fun getOpacity(): Int = throw UnsupportedOperationException() +} + +private class FakeAnimatableDrawable : FakeDrawable(), Animatable { + override fun start() {} + override fun stop() {} + override fun isRunning(): Boolean = throw UnsupportedOperationException() +} 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 new file mode 100644 index 0000000000..2106a12a51 --- /dev/null +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageDefaultTransformationTest.kt @@ -0,0 +1,183 @@ +@file:OptIn(ExperimentalCoroutinesApi::class, ExperimentGlideFlows::class, + ExperimentalGlideComposeApi::class) + +package com.bumptech.glide.integration.compose + +import android.content.Context +import android.content.res.Resources +import android.graphics.drawable.Drawable +import android.util.TypedValue +import androidx.annotation.DrawableRes +import androidx.compose.foundation.layout.size +import androidx.compose.runtime.Composable +import androidx.compose.ui.Modifier +import androidx.compose.ui.layout.ContentScale +import androidx.compose.ui.unit.dp +import androidx.test.core.app.ApplicationProvider +import com.bumptech.glide.Glide +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.onNodeWithDefaultContentDescription +import com.bumptech.glide.integration.ktx.ExperimentGlideFlows +import com.bumptech.glide.integration.ktx.Resource +import com.bumptech.glide.integration.ktx.Status +import com.bumptech.glide.integration.ktx.flow +import kotlin.math.roundToInt +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.junit.Rule +import org.junit.Test + +/** + * Non-transformable types are tested in [GlideImageCustomDrawableTransformationTest] + */ +class GlideImageDefaultTransformationTest { + private val context: Context = ApplicationProvider.getApplicationContext() + @get:Rule val glideComposeRule = GlideComposeRule() + + @Test + fun glideImage_withContentScaleNone_noTransformation_doesNotApplyTransformation() = runTest { + val resourceId = android.R.drawable.star_big_on + val expectedDrawable = loadExpectedDrawable(resourceId) + + glideComposeRule.setContent { + ContentScaleGlideImage( + model = resourceId, + contentScale = ContentScale.None, + ) + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplays(expectedDrawable) + } + + @Test + fun glideImage_withContentScaleFit_noTransformation_appliesCenterInsideTransformation() = runTest { + val resourceId = android.R.drawable.star_big_on + val expectedDrawable = loadExpectedDrawable(resourceId) { it.centerInside() } + + glideComposeRule.setContent { + ContentScaleGlideImage( + model = resourceId, + contentScale = ContentScale.Fit, + ) + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplays(expectedDrawable) + } + + @Test + fun glideImage_withContentScaleFit_explicitTransformation_usesExplicitTransformation() = runTest { + val resourceId = android.R.drawable.star_big_on + val expectedDrawable = loadExpectedDrawable(resourceId) { it.centerCrop() } + + glideComposeRule.setContent { + ContentScaleGlideImage( + model = resourceId, + contentScale = ContentScale.Fit, + ) { it.centerCrop() } + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplays(expectedDrawable) + } + + @Test + fun glideImage_withContentScaleInside_noTransformation_appliesCenterInsideTransformation() = runTest { + val resourceId = android.R.drawable.star_big_on + val expectedDrawable = loadExpectedDrawable(resourceId) { it.centerInside() } + + glideComposeRule.setContent { + ContentScaleGlideImage( + model = resourceId, + contentScale = ContentScale.Inside, + ) + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplays(expectedDrawable) + } + + @Test + fun glideImage_withContentScaleInside_explicitTransformation_usesExplicitTransformation() = runTest { + val resourceId = android.R.drawable.star_big_on + val expectedDrawable = loadExpectedDrawable(resourceId) { it.centerCrop() } + + glideComposeRule.setContent { + ContentScaleGlideImage( + model = resourceId, + contentScale = ContentScale.Inside, + ) { it.centerCrop() } + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplays(expectedDrawable) + } + + + @Test + fun glideImage_withContentScaleCrop_noTransformation_appliesCenterCropTransformation() = runTest { + val resourceId = android.R.drawable.star_big_on + val expectedDrawable = loadExpectedDrawable(resourceId) { it.centerCrop() } + + glideComposeRule.setContent { + ContentScaleGlideImage( + model = resourceId, + contentScale = ContentScale.Crop, + ) + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplays(expectedDrawable) + } + + @Test + fun glideImage_withContentScaleCrop_explicitTransformation_usesExplicitTransformation() = runTest { + val resourceId = android.R.drawable.star_big_on + val expectedDrawable = loadExpectedDrawable(resourceId) { it.centerInside() } + + glideComposeRule.setContent { + ContentScaleGlideImage( + model = resourceId, + contentScale = ContentScale.Crop, + ) { it.centerInside() } + } + + glideComposeRule.onNodeWithDefaultContentDescription().assertDisplays(expectedDrawable) + } + + + private suspend fun RequestBuilder.loadRequiringSuccess() = + (this.flow().first { it.status == Status.SUCCEEDED} as Resource).resource + + private suspend fun loadExpectedDrawable( + @DrawableRes resourceId: Int, + transformation: (RequestBuilder) -> RequestBuilder = {it -> it}, + ): Drawable = + transformation(Glide.with(context).load(resourceId).override(WIDTH.px(), HEIGHT.px())) + .loadRequiringSuccess() + + @Composable + private fun ContentScaleGlideImage( + model: Any?, + contentScale: ContentScale, + requestBuilderTransform: RequestBuilderTransform = {it -> it}, + ) = + GlideImage( + model = model, + contentDescription = Constants.DEFAULT_DESCRIPTION, + modifier = SIZE_MODIFIER, + contentScale = contentScale, + requestBuilderTransform = requestBuilderTransform, + ) + + companion object { + const val WIDTH = 25 + // non-square + const val HEIGHT = 30 + + val SIZE_MODIFIER = Modifier.size(WIDTH.dp, HEIGHT.dp) + } +} + +fun Int.px() = + TypedValue.applyDimension( + TypedValue.COMPLEX_UNIT_DIP, this.toFloat(), Resources.getSystem().displayMetrics).roundToInt() \ No newline at end of file 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 0b0bfa4f7a..661aafafa5 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 @@ -63,3 +63,13 @@ private fun expectStateValue( } true } + +fun expectSameInstance(expectedDrawable: Drawable) = + SemanticsMatcher("${DisplayedDrawableKey.name} = '$expectedDrawable'") { + val actualValue: Drawable? = it.config.getOrElseNullable(DisplayedDrawableKey) { null }?.value + if (actualValue !== expectedDrawable) { + throw AssertionError("Expected: $expectedDrawable, but was: $actualValue") + } + true + } + 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 788edc35e5..6e60065110 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,2 +1,20 @@ package com.bumptech.glide.integration.compose.test +import android.graphics.drawable.Drawable +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 + +object Constants { + const val DEFAULT_DESCRIPTION = "test" +} + +fun ComposeContentTestRule.onNodeWithDefaultContentDescription() = + onNodeWithContentDescription(Constants.DEFAULT_DESCRIPTION) + +fun SemanticsNodeInteraction.assertDisplays(drawable: Drawable) = + assert(expectDisplayedDrawable(drawable)) + +fun SemanticsNodeInteraction.assertDisplaysInstance(drawable: Drawable) = + assert(expectSameInstance(drawable)) \ No newline at end of file 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 46f23c4e66..ff8ea7644a 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 @@ -232,7 +232,7 @@ private fun RequestBuilder.contentScaleTransform( ): RequestBuilder { return when (contentScale) { ContentScale.Crop -> { - centerCrop() + optionalCenterCrop() } ContentScale.Inside, ContentScale.Fit -> { @@ -240,7 +240,7 @@ private fun RequestBuilder.contentScaleTransform( // decision given how unimportant Bitmap re-use is relative to minimizing texture sizes now. // So instead we'll do something different and prefer not to upscale, which means using // centerInside(). The UI can still scale the view even if the Bitmap is smaller. - centerInside() + optionalCenterInside() } else -> { this