From 9809d98fe43ae3d0593f3d710bb32af9bed72642 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Mon, 25 Sep 2023 21:16:43 -0700 Subject: [PATCH] Add a Painter variant of the placeholder APIs --- integration/compose/api/compose.api | 1 + .../compose/GlideImageErrorTest.kt | 20 +++ .../compose/GlideImagePlaceholderTest.kt | 22 ++++ .../integration/compose/test/expectations.kt | 10 +- .../glide/integration/compose/GlideImage.kt | 49 +++++--- .../integration/compose/GlideModifier.kt | 115 ++++++++++++++---- 6 files changed, 171 insertions(+), 46 deletions(-) diff --git a/integration/compose/api/compose.api b/integration/compose/api/compose.api index f4f4f6d59d..9ceb583721 100644 --- a/integration/compose/api/compose.api +++ b/integration/compose/api/compose.api @@ -19,6 +19,7 @@ public final class com/bumptech/glide/integration/compose/GlideImageKt { public static final fun GlideSubcomposition (Ljava/lang/Object;Landroidx/compose/ui/Modifier;Lkotlin/jvm/functions/Function1;Lkotlin/jvm/functions/Function3;Landroidx/compose/runtime/Composer;II)V 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; } 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 18bb29c5d9..83c6688850 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 @@ -7,6 +7,7 @@ import androidx.compose.ui.test.onNodeWithContentDescription import androidx.test.core.app.ApplicationProvider import com.bumptech.glide.integration.compose.test.GlideComposeRule import com.bumptech.glide.integration.compose.test.expectDisplayedDrawable +import com.bumptech.glide.integration.compose.test.expectDisplayedPainter import com.bumptech.glide.integration.compose.test.expectDisplayedResource import com.bumptech.glide.integration.compose.test.expectNoDrawable import org.junit.Rule @@ -143,6 +144,25 @@ class GlideImageErrorTest { .assert(expectDisplayedResource(failureResourceId)) } + @Test + fun failure_setViaFailureParameterWithPainter_andRequestBuilderTransform_prefersFailurePainter() { + val description = "test" + val failurePainter = context.getDrawable(android.R.drawable.star_big_off).toPainter() + glideComposeRule.setContent { + GlideImage( + model = null, + contentDescription = description, + failure = placeholder(failurePainter), + ) { + it.error(android.R.drawable.btn_star) + } + } + + glideComposeRule + .onNodeWithContentDescription(description) + .assert(expectDisplayedPainter(failurePainter)) + } + @Test fun failure_setViaFailureParameterWithDrawable_andRequestBuilderTransform_prefersFailureParameter() { val description = "test" 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 5e3b36f2b6..d6c5a3b0cb 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 @@ -7,6 +7,7 @@ import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onNodeWithContentDescription import androidx.test.core.app.ApplicationProvider import com.bumptech.glide.integration.compose.test.expectDisplayedDrawable +import com.bumptech.glide.integration.compose.test.expectDisplayedPainter import com.bumptech.glide.integration.compose.test.expectDisplayedResource import com.bumptech.glide.integration.compose.test.expectNoDrawable import com.bumptech.glide.testutil.TearDownGlide @@ -181,6 +182,27 @@ class GlideImagePlaceholderTest { .assert(expectDisplayedDrawable(placeholderDrawable)) } + @Test + fun loading_setViaLoadingParameterWithPainter_andRequestBuilderTransform_prefersLoadingParameter() { + val description = "test" + val waitModel = waitModelLoaderRule.waitOn(android.R.drawable.star_big_on) + val placeholderDrawable = context.getDrawable(android.R.drawable.star_big_off) + val placeholderPainter = placeholderDrawable.toPainter() + composeRule.setContent { + GlideImage( + model = waitModel, + contentDescription = description, + loading = placeholder(placeholderPainter), + ) { + it.placeholder(android.R.drawable.btn_star) + } + } + + composeRule + .onNodeWithContentDescription(description) + .assert(expectDisplayedPainter(placeholderPainter)) + } + @Test fun loading_setViaLoadingParameterWithNullDrawable_andRequestBuilderTransform_showsNoResource() { val description = "test" 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 0895394473..32a991f9c7 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 @@ -10,10 +10,12 @@ import android.graphics.drawable.BitmapDrawable import android.graphics.drawable.Drawable import android.util.TypedValue import androidx.compose.runtime.MutableState +import androidx.compose.ui.graphics.painter.Painter 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.compose.DisplayedPainterKey import com.bumptech.glide.integration.ktx.InternalGlideApi import com.bumptech.glide.integration.ktx.Size import kotlin.math.roundToInt @@ -43,10 +45,10 @@ fun expectDisplayedDrawableSize(expectedSize: Size): SemanticsMatcher = fun expectDisplayedDrawable(expectedValue: Drawable?): SemanticsMatcher = expectDisplayedDrawable(expectedValue.bitmapOrThrow(), ::compareBitmaps) { it.bitmapOrThrow() } -fun expectAnimatingDrawable(): SemanticsMatcher = - expectDisplayedDrawable(true) { - (it as Animatable).isRunning - } +fun expectDisplayedPainter(expectedValue: Painter?): SemanticsMatcher = + expectStateValue( + DisplayedPainterKey, expectedValue, {first, second -> first == second}, {value -> value} + ) fun expectNoDrawable(): SemanticsMatcher = expectDisplayedDrawable(null) 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 c963961f79..e88d409491 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 @@ -145,6 +145,8 @@ public fun GlideImage( alpha, colorFilter, transition, + loadingPlaceholder = loading?.maybePainter(), + errorPlaceholder = failure?.maybePainter(), ) ) } @@ -167,13 +169,10 @@ public interface GlideSubcompositionScope { @ExperimentalGlideComposeApi internal class GlideSubcompositionScopeImpl( - private val drawable: Drawable?, + maybePainter: Painter?, override val state: RequestState ) : GlideSubcompositionScope { - - override val painter: Painter - get() = drawable?.toPainter() ?: ColorPainter(Color.Transparent) - + override val painter: Painter = maybePainter ?: ColorPainter(Color.Transparent) } /** @@ -266,7 +265,7 @@ public fun GlideSubcomposition( remember(model, requestManager, requestBuilderTransform) { mutableStateOf(RequestState.Loading) } - val drawable: MutableState = remember(model, requestManager, requestBuilderTransform) { + val painter: MutableState = remember(model, requestManager, requestBuilderTransform) { mutableStateOf(null) } @@ -274,11 +273,11 @@ public fun GlideSubcomposition( remember(model, requestManager, requestBuilderTransform) { StateTrackingListener( requestState, - drawable + painter ) } - val scope = GlideSubcompositionScopeImpl(drawable.value, requestState.value) + val scope = GlideSubcompositionScopeImpl(painter.value, requestState.value) Box( modifier @@ -295,12 +294,12 @@ public fun GlideSubcomposition( @ExperimentalGlideComposeApi private class StateTrackingListener( val state: MutableState, - val drawable: MutableState + val painter: MutableState ) : RequestListener { - override fun onStateChanged(model: Any?, drawable: Drawable?, requestState: RequestState) { + override fun onStateChanged(model: Any?, painter: Painter?, requestState: RequestState) { state.value = requestState - this.drawable.value = drawable + this.painter.value = painter } } @@ -311,15 +310,18 @@ private fun PreviewResourceOrDrawable( contentDescription: String?, modifier: Modifier, ) { - val drawable = + val painter = when (loading) { - is Placeholder.OfDrawable -> loading.drawable - is Placeholder.OfResourceId -> LocalContext.current.getDrawable(loading.resourceId) + 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") } Image( - painter = remember(drawable) { drawable.toPainter() }, + painter = painter, modifier = modifier, contentDescription = contentDescription, ) @@ -348,6 +350,14 @@ public fun placeholder(drawable: Drawable?): Placeholder = Placeholder.OfDrawabl public fun placeholder(@DrawableRes resourceId: Int): Placeholder = Placeholder.OfResourceId(resourceId) +/** + * Used to specify a [Painter] to use in conjunction with [GlideImage]'s `loading` or `failure` + * parameters. + */ +@ExperimentalGlideComposeApi +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. @@ -380,6 +390,8 @@ public fun placeholder(composable: @Composable () -> Unit): Placeholder = 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() = @@ -387,6 +399,7 @@ public sealed class Placeholder { is OfDrawable -> true is OfResourceId -> true is OfComposable -> false + is OfPainter -> false } internal fun maybeComposable(): (@Composable () -> Unit)? = @@ -395,6 +408,12 @@ public sealed class Placeholder { else -> null } + internal fun maybePainter() = + when (this) { + is OfPainter -> this.painter + else -> null + } + internal fun apply( resource: (Int) -> RequestBuilder, drawable: (Drawable?) -> RequestBuilder diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt index 12c5232e17..a193f844b8 100644 --- a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt +++ b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlideModifier.kt @@ -65,7 +65,7 @@ import kotlin.math.roundToInt @ExperimentalGlideComposeApi internal interface RequestListener { - fun onStateChanged(model: Any?, drawable: Drawable?, requestState: RequestState) + fun onStateChanged(model: Any?, painter: Painter?, requestState: RequestState) } @ExperimentalGlideComposeApi @@ -79,6 +79,8 @@ internal fun Modifier.glideNode( transitionFactory: Transition.Factory? = null, requestListener: RequestListener? = null, draw: Boolean? = null, + loadingPlaceholder: Painter? = null, + errorPlaceholder: Painter? = null, ): Modifier { return this then GlideNodeElement( requestBuilder, @@ -89,6 +91,8 @@ internal fun Modifier.glideNode( requestListener, draw, transitionFactory, + loadingPlaceholder, + errorPlaceholder, ) .clipToBounds() .semantics { @@ -110,6 +114,8 @@ internal data class GlideNodeElement constructor( private val requestListener: RequestListener?, private val draw: Boolean?, private val transitionFactory: Transition.Factory?, + private val loadingPlaceholder: Painter?, + private val errorPlaceholder: Painter?, ) : ModifierNodeElement() { override fun create(): GlideNode { val result = GlideNode() @@ -127,6 +133,8 @@ internal data class GlideNodeElement constructor( requestListener, draw, transitionFactory, + loadingPlaceholder, + errorPlaceholder, ) } @@ -167,10 +175,12 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi private var requestListener: RequestListener? = null private var currentJob: Job? = null - private var painter: Painter? = null + private var primary: Primary? = null + + private var loadingPlaceholder: Painter? = null + private var errorPlaceholder: Painter? = null // Only used for debugging - private var drawable: Drawable? = null private var state: RequestState = RequestState.Loading private var placeholder: Painter? = null private var isFirstResource = true @@ -213,11 +223,18 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi requestListener: RequestListener?, draw: Boolean?, transitionFactory: Transition.Factory?, + loadingPlaceholder: Painter?, + errorPlaceholder: Painter?, ) { // Other attributes can be refreshed by re-drawing rather than restarting a request val restartLoad = !this::requestBuilder.isInitialized || requestBuilder != this.requestBuilder + // TODO(sam): Avoid restarting the entire load if we just change the placeholder. State + // management makes this complicated and this might not be a common use case, so we + // haven't yet done so. + || loadingPlaceholder != this.loadingPlaceholder + || errorPlaceholder != this.errorPlaceholder this.requestBuilder = requestBuilder this.contentScale = contentScale @@ -227,6 +244,8 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi this.requestListener = requestListener this.draw = draw ?: true this.transitionFactory = transitionFactory ?: DoNotTransition.Factory + this.loadingPlaceholder = loadingPlaceholder + this.errorPlaceholder = errorPlaceholder this.resolvableGlideSize = requestBuilder.maybeImmediateSize() ?: inferredGlideSize?.let { ImmediateGlideSize(it) } @@ -234,7 +253,7 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi if (restartLoad) { clear() - updateDrawable(null) + updatePrimary(null) // If we're not attached, we'll be measured when we eventually are attached. if (isAttached) { @@ -321,7 +340,7 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi } } - painter?.let { painter -> + primary?.painter?.let { painter -> drawContext.canvas.withSave { drawablePositionAndSize = drawOne(painter, drawablePositionAndSize) { size -> transition.drawCurrent.invoke(this, painter, size, alpha, colorFilter) @@ -347,7 +366,7 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi override fun onReset() { super.onReset() clear() - updateDrawable(null) + updatePrimary(null) } @OptIn(ExperimentGlideFlows::class) @@ -388,27 +407,37 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi placeholderPositionAndSize = null requestBuilder.flowResolvable(resolvableGlideSize).collect { - val (state, drawable) = + val (state, primary) = when (it) { is Resource -> { maybeAnimate(it) - Pair(RequestState.Success(it.dataSource), it.resource) + Pair(RequestState.Success(it.dataSource), Primary.PrimaryDrawable(it.resource)) } is Placeholder -> { - val drawable = it.placeholder val state = when (it.status) { Status.RUNNING, Status.CLEARED -> RequestState.Loading Status.FAILED -> RequestState.Failure Status.SUCCEEDED -> throw IllegalStateException() } - placeholder = drawable?.toPainter() + // Prefer the override Painters if set. + val painter = when (state) { + is RequestState.Loading -> loadingPlaceholder + is RequestState.Failure -> errorPlaceholder + is RequestState.Success -> throw IllegalStateException() + } + val primary = if (painter != null) { + Primary.PrimaryPainter(painter) + } else { + Primary.PrimaryDrawable(it.placeholder) + } + placeholder = primary.painter placeholderPositionAndSize = null - Pair(state, drawable) + Pair(state, primary) } } - updateDrawable(drawable) - requestListener?.onStateChanged(requestBuilder.internalModel, drawable, state) + updatePrimary(primary) + requestListener?.onStateChanged(requestBuilder.internalModel, primary.painter, state) this@GlideNode.state = state if (hasFixedSize) { invalidateDraw() @@ -419,17 +448,40 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi } } - private fun updateDrawable(drawable: Drawable?) { - this.drawable = drawable + private sealed class Primary { + class PrimaryDrawable(override val drawable: Drawable?) : Primary() { + override val painter = drawable?.toPainter() + override fun onUnset() { + drawable?.callback = null + drawable?.setVisible(false, false) + (drawable as? Animatable)?.stop() + } + + override fun onSet(callback: Drawable.Callback) { + drawable?.callback = callback + drawable?.setVisible(true, true) + (drawable as? Animatable)?.start() + } + } - this.drawable?.callback = null - this.drawable?.setVisible(false, false) - (this.drawable as? Animatable)?.stop() + class PrimaryPainter(override val painter: Painter?) : Primary() { + override val drawable = null + override fun onUnset() {} + override fun onSet(callback: Drawable.Callback) {} + } - painter = drawable?.toPainter() - drawable?.callback = callback - drawable?.setVisible(true, true) - (drawable as? Animatable)?.start() + abstract val painter: Painter? + abstract val drawable: Drawable? + + abstract fun onUnset() + + abstract fun onSet(callback: Drawable.Callback) + } + + private fun updatePrimary(newPrimary: Primary?) { + this.primary?.onUnset() + this.primary = newPrimary + newPrimary?.onSet(callback) drawablePositionAndSize = null } @@ -448,7 +500,7 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi currentJob?.cancel() currentJob = null state = RequestState.Loading - updateDrawable(null) + updatePrimary(null) } @OptIn(InternalGlideApi::class) @@ -484,7 +536,7 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi ) } - val intrinsicSize = painter?.intrinsicSize ?: return constraints + val intrinsicSize = primary?.painter?.intrinsicSize ?: return constraints val intrinsicWidth = if (constraints.hasFixedWidth) { @@ -509,8 +561,8 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi val srcSize = Size(intrinsicWidth.toFloat(), intrinsicHeight.toFloat()) val scaleFactor = contentScale.computeScaleFactor( - srcSize, Size(constrainedWidth.toFloat(), constrainedHeight.toFloat()) - ) + srcSize, Size(constrainedWidth.toFloat(), constrainedHeight.toFloat()) + ) if (scaleFactor == ScaleFactor.Unspecified) { return constraints } @@ -522,7 +574,8 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi } override fun SemanticsPropertyReceiver.applySemantics() { - displayedDrawable = { drawable } + displayedDrawable = { primary?.drawable } + displayedPainter = { primary?.painter } } override fun equals(other: Any?): Boolean { @@ -535,6 +588,8 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi && draw == other.draw && transitionFactory == other.transitionFactory && alpha == other.alpha + && loadingPlaceholder == other.loadingPlaceholder + && errorPlaceholder == other.errorPlaceholder } return false } @@ -548,6 +603,8 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi result = 31 * result + requestListener.hashCode() result = 31 * result + transitionFactory.hashCode() result = 31 * result + alpha.hashCode() + result = 31 * result + loadingPlaceholder.hashCode() + result = 31 * result + errorPlaceholder.hashCode() return result } } @@ -555,3 +612,7 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi internal val DisplayedDrawableKey = SemanticsPropertyKey<() -> Drawable?>("DisplayedDrawable") internal var SemanticsPropertyReceiver.displayedDrawable by DisplayedDrawableKey + +internal val DisplayedPainterKey = + SemanticsPropertyKey<() -> Painter?>("DisplayedPainter") +internal var SemanticsPropertyReceiver.displayedPainter by DisplayedPainterKey