From 6e0f66f54c106a30675d564b8b1eb9a129afc706 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Sun, 1 Oct 2023 18:41:45 -0700 Subject: [PATCH] Make Placeholder a type that can be convered to Painter This removes the deprecated Composable variant of Placeholder. Users who require that functionality should instead use GlideSubcomposition. It's useful to remove this now because it allows for a much simpler Placholder interface and it unblocks us from moving most of GlideImage into GlideModifier. In turn that will let us expose GlideModifier as a real composable API component. --- ...deImageCustomDrawableTransformationTest.kt | 4 +- .../compose/GlideImageErrorTest.kt | 63 +------- .../compose/GlideImagePlaceholderTest.kt | 67 +------- .../integration/compose/test/expectations.kt | 66 +++++++- .../glide/integration/compose/GlideImage.kt | 151 +++++++----------- .../glide/integration/compose/Painter.kt | 9 ++ 6 files changed, 142 insertions(+), 218 deletions(-) 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 e51dbec7cd..8d78f21e98 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 @@ -34,7 +34,8 @@ class GlideImageCustomDrawableTransformationTest( // though it's only used by Parameters to create the test name. @Suppress("unused") private val name: String, ) { - @get:Rule val glideComposeRule = GlideComposeRule() + @get:Rule + val glideComposeRule = GlideComposeRule() @Test fun glideImage_nonBitmapDrawable_doesNotThrow() = runTest { @@ -98,6 +99,7 @@ private open class FakeDrawable : Drawable() { override fun draw(p0: Canvas) {} override fun setAlpha(p0: Int) {} override fun setColorFilter(p0: ColorFilter?) {} + @Deprecated("Deprecated in Java") override fun getOpacity(): Int = throw UnsupportedOperationException() } diff --git a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageErrorTest.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageErrorTest.kt index 83c6688850..bad1a24717 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageErrorTest.kt +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImageErrorTest.kt @@ -18,9 +18,9 @@ import org.junit.Test * 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() @@ -64,7 +64,7 @@ class GlideImageErrorTest { glideComposeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(failureResourceId)) + .assert(expectDisplayedPainter(context, failureResourceId)) } @Test @@ -81,7 +81,7 @@ class GlideImageErrorTest { glideComposeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedDrawable(failureDrawable)) + .assert(expectDisplayedPainter(failureDrawable)) } @Test @@ -98,33 +98,6 @@ class GlideImageErrorTest { glideComposeRule.onNodeWithContentDescription(description).assert(expectNoDrawable()) } - @Test - fun failureParameter_withComposable_displaysComposable() { - val failureResourceId = android.R.drawable.star_big_off - val description = "test" - glideComposeRule.setContent { - GlideImage( - 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), - ) - } - ) - } - - glideComposeRule - .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(failureResourceId)) - } - @Test fun failure_setViaFailureParameterWithResourceId_andRequestBuilderTransform_prefersFailureParameter() { val description = "test" @@ -141,7 +114,7 @@ class GlideImageErrorTest { glideComposeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(failureResourceId)) + .assert(expectDisplayedPainter(context, failureResourceId)) } @Test @@ -179,7 +152,7 @@ class GlideImageErrorTest { glideComposeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedDrawable(failureDrawable)) + .assert(expectDisplayedPainter(failureDrawable)) } @Test @@ -197,30 +170,4 @@ class GlideImageErrorTest { glideComposeRule.onNodeWithContentDescription(description).assert(expectNoDrawable()) } - - @Test - fun failure_setViaFailureParameterWithComposable_andRequestBuilderTransform_showsComposable() { - val description = "test" - val failureResourceId = android.R.drawable.star_big_off - glideComposeRule.setContent { - GlideImage( - model = null, - contentDescription = "other", - failure = - placeholder { - GlideImage( - model = null, - contentDescription = description, - failure = placeholder(failureResourceId), - ) - }, - ) { - it.error(android.R.drawable.btn_star) - } - } - - glideComposeRule - .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(failureResourceId)) - } } diff --git a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt index d6c5a3b0cb..25acb6f18d 100644 --- a/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt +++ b/integration/compose/src/androidTest/java/com/bumptech/glide/integration/compose/GlideImagePlaceholderTest.kt @@ -20,14 +20,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() @@ -78,7 +80,7 @@ class GlideImagePlaceholderTest { composeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(placeholderResourceId)) + .assert(expectDisplayedPainter(context, placeholderResourceId)) } @Test @@ -96,7 +98,7 @@ class GlideImagePlaceholderTest { composeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedDrawable(placeholderDrawable)) + .assert(expectDisplayedPainter(placeholderDrawable)) } @Test @@ -114,34 +116,6 @@ class GlideImagePlaceholderTest { composeRule.onNodeWithContentDescription(description).assert(expectNoDrawable()) } - @Test - fun loadingParameter_withComposable_displaysComposable() { - val waitModel = waitModelLoaderRule.waitOn(android.R.drawable.star_big_on) - val placeholderResourceId = android.R.drawable.star_big_off - val description = "test" - composeRule.setContent { - GlideImage( - 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), - ) - } - ) - } - - composeRule - .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(placeholderResourceId)) - } - @Test fun loading_setViaLoadingParameterWithResourceId_andRequestBuilderTransform_prefersLoadingParameter() { val description = "test" @@ -159,7 +133,7 @@ class GlideImagePlaceholderTest { composeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(placeholderResourceId)) + .assert(expectDisplayedPainter(context, placeholderResourceId)) } @Test @@ -179,7 +153,7 @@ class GlideImagePlaceholderTest { composeRule .onNodeWithContentDescription(description) - .assert(expectDisplayedDrawable(placeholderDrawable)) + .assert(expectDisplayedPainter(placeholderDrawable)) } @Test @@ -219,31 +193,4 @@ class GlideImagePlaceholderTest { composeRule.onNodeWithContentDescription(description).assert(expectNoDrawable()) } - - @Test - fun loading_setViaLoadingParameterWithComposable_andRequestBuilderTransform_showsComposable() { - val description = "test" - val waitModel = waitModelLoaderRule.waitOn(android.R.drawable.star_big_on) - val placeholderResourceId = android.R.drawable.star_big_off - composeRule.setContent { - GlideImage( - model = waitModel, - contentDescription = "other", - loading = - placeholder { - GlideImage( - model = waitModel, - contentDescription = description, - loading = placeholder(placeholderResourceId), - ) - }, - ) { - it.placeholder(android.R.drawable.btn_star) - } - } - - composeRule - .onNodeWithContentDescription(description) - .assert(expectDisplayedResource(placeholderResourceId)) - } } 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 32a991f9c7..ab22519445 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 @@ -1,16 +1,17 @@ -@file:OptIn(InternalGlideApi::class) - package com.bumptech.glide.integration.compose.test import android.content.Context import android.content.res.Resources import android.graphics.Bitmap -import android.graphics.drawable.Animatable import android.graphics.drawable.BitmapDrawable import android.graphics.drawable.Drawable import android.util.TypedValue -import androidx.compose.runtime.MutableState +import androidx.annotation.DrawableRes +import androidx.compose.ui.graphics.ImageBitmap +import androidx.compose.ui.graphics.asImageBitmap +import androidx.compose.ui.graphics.painter.BitmapPainter import androidx.compose.ui.graphics.painter.Painter +import androidx.compose.ui.res.imageResource import androidx.compose.ui.semantics.SemanticsPropertyKey import androidx.compose.ui.test.SemanticsMatcher import androidx.test.core.app.ApplicationProvider @@ -30,8 +31,10 @@ fun Int.dpToPixels() = ) .roundToInt() +@OptIn(InternalGlideApi::class) fun Int.bitmapSize() = context().resources.getDrawable(this, context().theme).size() +@OptIn(InternalGlideApi::class) fun Drawable.size() = (this as BitmapDrawable).bitmap.let { Size(it.width, it.height) } fun expectDisplayedResource(resourceId: Int) = @@ -39,17 +42,65 @@ fun expectDisplayedResource(resourceId: Int) = fun Drawable?.bitmapOrThrow(): Bitmap? = if (this == null) null else (this as BitmapDrawable).bitmap +@OptIn(InternalGlideApi::class) fun expectDisplayedDrawableSize(expectedSize: Size): SemanticsMatcher = expectDisplayedDrawable(expectedSize) { it?.size() } fun expectDisplayedDrawable(expectedValue: Drawable?): SemanticsMatcher = expectDisplayedDrawable(expectedValue.bitmapOrThrow(), ::compareBitmaps) { it.bitmapOrThrow() } + fun expectDisplayedPainter(expectedValue: Painter?): SemanticsMatcher = expectStateValue( - DisplayedPainterKey, expectedValue, {first, second -> first == second}, {value -> value} + DisplayedPainterKey, expectedValue, { first, second -> first == second }, { value -> value } ) +// These are hacks. We're relying on the ordering of expected.equals(actual) so that our +// DeepEqualsImageBitmap's equals method will be used. This doesn't implement the equals/hashcode +// contract :/ +fun expectDisplayedPainter(context: Context, @DrawableRes resourceId: Int): SemanticsMatcher { + val imageBitmap = DeepEqualsImageBitmap(ImageBitmap.imageResource(context.resources, resourceId)) + val painter = BitmapPainter(imageBitmap) + return expectDisplayedPainter(painter) +} + +fun expectDisplayedPainter(expectedValue: Drawable?): SemanticsMatcher { + val bitmapDrawable = expectedValue as BitmapDrawable + val imageBitmap = DeepEqualsImageBitmap(bitmapDrawable.bitmap.asImageBitmap()) + return expectDisplayedPainter(BitmapPainter(imageBitmap)) +} + +class DeepEqualsImageBitmap(base: ImageBitmap) : ImageBitmap by base { + override fun equals(other: Any?): Boolean { + val compare = other as? ImageBitmap ?: return false + return compare.width == width && compare.height == height + && compare.hasAlpha == hasAlpha + && compare.config == config + && compare.colorSpace == colorSpace + && equalPixels(this, other) + } + + private fun ImageBitmap.readPixels(): IntArray { + val pixels = IntArray(width * height) + readPixels(pixels) + return pixels + } + + private fun equalPixels(first: ImageBitmap, second: ImageBitmap): Boolean { + return first.readPixels().contentEquals(second.readPixels()) + } + + override fun hashCode(): Int { + var result = width.hashCode() + result *= 31 * height.hashCode() + result *= 31 * hasAlpha.hashCode() + result *= 31 * config.hashCode() + result *= 31 * colorSpace.hashCode() + result *= 31 * readPixels().contentHashCode() + return result + } +} + fun expectNoDrawable(): SemanticsMatcher = expectDisplayedDrawable(null) private fun compareBitmaps(first: Bitmap?, second: Bitmap?): Boolean { @@ -77,7 +128,7 @@ private fun expectStateValue( ): SemanticsMatcher = SemanticsMatcher("${key.name} = '$expectedValue'") { val value = transform(it.config.getOrElseNullable(key) { null }?.invoke()) - if (!compare(value, expectedValue)) { + if (!compare(expectedValue, value)) { throw AssertionError("Expected: $expectedValue, but was: $value") } true @@ -85,7 +136,8 @@ private fun expectStateValue( fun expectSameInstance(expectedDrawable: Drawable) = SemanticsMatcher("${DisplayedDrawableKey.name} = '$expectedDrawable'") { - val actualValue: Drawable? = it.config.getOrElseNullable(DisplayedDrawableKey) { null }?.invoke() + val actualValue: Drawable? = + it.config.getOrElseNullable(DisplayedDrawableKey) { null }?.invoke() if (actualValue !== expectedDrawable) { throw AssertionError("Expected: $expectedDrawable, but was: $actualValue") } 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 e88d409491..0b83bb659f 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 @@ -19,6 +19,7 @@ import androidx.compose.ui.layout.ContentScale import androidx.compose.ui.layout.Layout import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.platform.LocalInspectionMode +import androidx.compose.ui.res.painterResource import com.bumptech.glide.Glide import com.bumptech.glide.RequestBuilder import com.bumptech.glide.RequestManager @@ -60,10 +61,7 @@ public typealias RequestBuilderTransform = (RequestBuilder) -> RequestBuil * * @param loading A [Placeholder] that will be displayed while the request is loading. Specifically * it's used if the request is cleared ([com.bumptech.glide.request.target.Target.onLoadCleared]) or - * loading ([com.bumptech.glide.request.target.Target.onLoadStarted]. There's a subtle difference in - * behavior depending on which type of [Placeholder] you use. The resource and `Drawable` variants - * will be displayed if the request fails and no other failure handling is specified, but the - * `Composable` will not. + * loading ([com.bumptech.glide.request.target.Target.onLoadStarted]. * @param failure A [Placeholder] that will be displayed if the request fails. Specifically it's * used when [com.bumptech.glide.request.target.Target.onLoadFailed] is called. If * [RequestBuilder.error] is called in [requestBuilderTransform] with a valid [RequestBuilder] (as @@ -98,58 +96,33 @@ public fun GlideImage( // TODO(judds): Consider defaulting to load the model here instead of always doing so below. requestBuilderTransform: RequestBuilderTransform = { it }, ) { - val requestManager: RequestManager = LocalContext.current.let { remember(it) { Glide.with(it) } } + val context = LocalContext.current + val requestManager: RequestManager = remember(context) { Glide.with(context) } val requestBuilder = rememberRequestBuilderWithDefaults(model, requestManager, requestBuilderTransform, contentScale) - .let { loading?.apply(it::placeholder, it::placeholder) ?: it } - .let { failure?.apply(it::error, it::error) ?: it } - // 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 // isn't supported. - if (LocalInspectionMode.current && loading?.isResourceOrDrawable() == true) { + if (LocalInspectionMode.current && loading != null) { PreviewResourceOrDrawable(loading, contentDescription, modifier) 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) { - GlideSubcomposition(model, modifier, { requestBuilder }) { - if (state == RequestState.Loading && loadingComposable != null) { - loadingComposable() - } else if (state == RequestState.Failure && failureComposable != null) { - failureComposable() - } else { - Image( - painter, - contentDescription, - modifier, - alignment, - contentScale, - alpha, - colorFilter, - ) - } - } - } else { - SimpleLayout( - modifier - .glideNode( - requestBuilder, - contentDescription, - alignment, - contentScale, - alpha, - colorFilter, - transition, - loadingPlaceholder = loading?.maybePainter(), - errorPlaceholder = failure?.maybePainter(), - ) - ) - } + SimpleLayout( + modifier + .glideNode( + requestBuilder, + contentDescription, + alignment, + contentScale, + alpha, + colorFilter, + transition, + loadingPlaceholder = loading?.painter(), + errorPlaceholder = failure?.painter(), + ) + ) } /** @@ -310,16 +283,7 @@ private fun PreviewResourceOrDrawable( contentDescription: String?, modifier: Modifier, ) { - val painter = - when (loading) { - is Placeholder.OfDrawable -> loading.drawable.toPainter() - is Placeholder.OfResourceId -> - LocalContext.current.getDrawable(loading.resourceId).toPainter() - - is Placeholder.OfPainter -> loading.painter - is Placeholder.OfComposable -> - throw IllegalArgumentException("Composables should go through the production codepath") - } + val painter = loading.painter() Image( painter = painter, modifier = modifier, @@ -359,11 +323,11 @@ public fun placeholder(painter: Painter?): Placeholder = Placeholder.OfPainter(painter ?: ColorPainter(Color.Transparent)) /** - * Used to specify a [Composable] function to use in conjunction with [GlideImage]'s `loading` or - * `failure` parameter. + * This method does nothing! It's been deprecated for a while. * - * Providing a nested [GlideImage] is not recommended. Use [RequestBuilder.thumbnail] or - * [RequestBuilder.error] as an alternative. + * Use any of the other placeholder variants. If you need to have a composable placeholder status, + * consider using [GlideSubcomposition], but keep in mind it will force an expensive recomposition + * each time the load status changes. Do not use it in scrolling lists. */ @Deprecated( "Using this method forces recomposition when the image load state changes." + @@ -372,7 +336,7 @@ public fun placeholder(painter: Painter?): Placeholder = ) @ExperimentalGlideComposeApi public fun placeholder(composable: @Composable () -> Unit): Placeholder = - Placeholder.OfComposable(composable) + Placeholder.OfComposable() /** * Content to display during a particular state of a Glide Request, for example while the request is @@ -388,44 +352,47 @@ public fun placeholder(composable: @Composable () -> Unit): Placeholder = */ @ExperimentalGlideComposeApi public sealed class Placeholder { - internal class OfDrawable(internal val drawable: Drawable?) : Placeholder() - internal class OfResourceId(@DrawableRes internal val resourceId: Int) : Placeholder() - - internal class OfPainter(internal val painter: Painter) : Placeholder() - internal class OfComposable(internal val composable: @Composable () -> Unit) : Placeholder() - - internal fun isResourceOrDrawable() = - when (this) { - is OfDrawable -> true - is OfResourceId -> true - is OfComposable -> false - is OfPainter -> false + @Composable + internal abstract fun painter(): Painter + + /** This class does nothing, the functionality has been deprecated for a while. */ + internal class OfComposable() : Placeholder() { + private val painter = ColorPainter(Color.Transparent) + @Composable + override fun painter(): Painter { + return painter } + } - internal fun maybeComposable(): (@Composable () -> Unit)? = - when (this) { - is OfComposable -> this.composable - else -> null - } + @OptIn(ExperimentalGlideComposeApi::class) + internal class OfDrawable(val drawable: Drawable?) : Placeholder() { + private val painter: Painter = drawable.toPainter() - internal fun maybePainter() = - when (this) { - is OfPainter -> this.painter - else -> null - } + @Composable + override fun painter(): Painter = painter + } + + @OptIn(ExperimentalGlideComposeApi::class) + internal class OfResourceId(@DrawableRes val resourceId: Int) : Placeholder() { + private lateinit var _painter: Painter - internal fun apply( - resource: (Int) -> RequestBuilder, - drawable: (Drawable?) -> RequestBuilder - ): RequestBuilder = - when (this) { - is OfDrawable -> drawable(this.drawable) - is OfResourceId -> resource(this.resourceId) - // Clear out any previously set placeholder. - else -> drawable(null) + @Composable + override fun painter(): Painter { + if (!this::_painter.isInitialized) { + _painter = painterResource(resourceId) + } + return _painter } + } + + @OptIn(ExperimentalGlideComposeApi::class) + internal class OfPainter(private val painter: Painter) : Placeholder() { + @Composable + override fun painter(): Painter = painter + } } + @Composable private fun rememberRequestBuilderWithDefaults( model: Any?, diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/Painter.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/Painter.kt index 04477f8a7b..25294e6602 100644 --- a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/Painter.kt +++ b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/Painter.kt @@ -84,4 +84,13 @@ private class DrawablePainter( } } } + + override fun equals(other: Any?): Boolean { + val compare = other as? DrawablePainter ?: return false + return drawable == compare.drawable + } + + override fun hashCode(): Int { + return drawable.hashCode() + } }