diff --git a/integration/compose/api/compose.api b/integration/compose/api/compose.api index 9ceb583721..3ce4f4b4a3 100644 --- a/integration/compose/api/compose.api +++ b/integration/compose/api/compose.api @@ -20,7 +20,6 @@ public final class com/bumptech/glide/integration/compose/GlideImageKt { public static final fun placeholder (I)Lcom/bumptech/glide/integration/compose/Placeholder; public static final fun placeholder (Landroid/graphics/drawable/Drawable;)Lcom/bumptech/glide/integration/compose/Placeholder; public static final fun placeholder (Landroidx/compose/ui/graphics/painter/Painter;)Lcom/bumptech/glide/integration/compose/Placeholder; - public static final fun placeholder (Lkotlin/jvm/functions/Function2;)Lcom/bumptech/glide/integration/compose/Placeholder; } public abstract interface class com/bumptech/glide/integration/compose/GlidePreloadingData { @@ -35,6 +34,7 @@ public abstract interface class com/bumptech/glide/integration/compose/GlideSubc public abstract class com/bumptech/glide/integration/compose/Placeholder { public static final field $stable I + public abstract fun painter (Landroidx/compose/runtime/Composer;I)Landroidx/compose/ui/graphics/painter/Painter; } public final class com/bumptech/glide/integration/compose/PreloadKt { 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..f954322a3f 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, @@ -358,22 +322,6 @@ public fun placeholder(@DrawableRes resourceId: Int): Placeholder = 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. - * - * Providing a nested [GlideImage] is not recommended. Use [RequestBuilder.thumbnail] or - * [RequestBuilder.error] as an alternative. - */ -@Deprecated( - "Using this method forces recomposition when the image load state changes." + - " If you require this behavior use GlideSubcomposition instead", - level = DeprecationLevel.WARNING -) -@ExperimentalGlideComposeApi -public fun placeholder(composable: @Composable () -> Unit): Placeholder = - Placeholder.OfComposable(composable) - /** * Content to display during a particular state of a Glide Request, for example while the request is * loading or if the request fails. @@ -388,44 +336,38 @@ 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 + public abstract fun painter(): 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 + } - 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) + @OptIn(ExperimentalGlideComposeApi::class) + internal class OfResourceId(@DrawableRes val resourceId: Int) : Placeholder() { + private lateinit var _painter: Painter + + @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() + } }