From 99dae93eff973236db9d1c8a3178e556d210be18 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Sun, 2 Jul 2023 16:26:16 -0700 Subject: [PATCH] Move size resolution from GlideImage to GlidePainter --- .../glide/integration/compose/GlideImage.kt | 43 +++---------------- .../glide/integration/compose/GlidePainter.kt | 15 +++++-- .../glide/integration/compose/Sizes.kt | 14 +++--- .../bumptech/glide/integration/ktx/Flows.kt | 30 +++++++++---- .../glide/integration/ktx/FlowsTest.kt | 22 +++------- 5 files changed, 53 insertions(+), 71 deletions(-) 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 6778cf64d7..d6c9a6726c 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 @@ -13,7 +13,6 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.ColorFilter import androidx.compose.ui.graphics.DefaultAlpha 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.semantics.SemanticsPropertyKey @@ -35,9 +34,8 @@ import com.google.accompanist.drawablepainter.rememberDrawablePainter public typealias RequestBuilderTransform = (RequestBuilder) -> RequestBuilder /** - * Start a request by passing [model] to [RequestBuilder.load] using the given [requestManager] and - * then applying the [requestBuilderTransform] function to add options or apply mutations if the - * caller desires. + * Start a request by passing [model] to [RequestBuilder.load] and then applying the + * [requestBuilderTransform] function to add options or apply mutations if the caller desires. * * [alignment], [contentScale], [alpha], [colorFilter] and [contentDescription] have the same * defaults (if any) and function identically to the parameters in [Image]. @@ -51,10 +49,6 @@ public typealias RequestBuilderTransform = (RequestBuilder) -> RequestBuil * explicitly if you can. You may end up loading a substantially larger image than you need, which * will increase memory usage and may also increase latency. * - * If you provide your own [requestManager] rather than using this method's default, consider using - * [remember] at a higher level to avoid some amount of overhead of retrieving it each - * re-composition. - * * This method will inspect [contentScale] and apply a matching transformation if one exists. Any * automatically applied transformation can be overridden using [requestBuilderTransform]. Either * apply a specific transformation instead, or use [RequestBuilder.dontTransform]] @@ -111,7 +105,7 @@ public fun GlideImage( .let { failure?.apply(it::error, it::error) ?: it } val overrideSize: Size? = requestBuilder.overrideSize() - val (size, finalModifier) = rememberSizeAndModifier(overrideSize, modifier) + val size = rememberResolvableSize(overrideSize) // 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 @@ -124,7 +118,7 @@ public fun GlideImage( SizedGlideImage( requestBuilder = requestBuilder, size = size, - modifier = finalModifier, + modifier = modifier, contentDescription = contentDescription, alignment = alignment, contentScale = contentScale, @@ -233,25 +227,13 @@ public sealed class Placeholder { } } -@OptIn(InternalGlideApi::class) -private data class SizeAndModifier(val size: ResolvableGlideSize, val modifier: Modifier) - @OptIn(InternalGlideApi::class) @Composable -private fun rememberSizeAndModifier( +private fun rememberResolvableSize( overrideSize: Size?, - modifier: Modifier, ) = - remember(overrideSize, modifier) { - if (overrideSize != null) { - SizeAndModifier(ImmediateGlideSize(overrideSize), modifier) - } else { - val sizeObserver = SizeObserver() - SizeAndModifier( - AsyncGlideSize(sizeObserver::getSize), - modifier.sizeObservingModifier(sizeObserver) - ) - } + remember(overrideSize) { + overrideSize?.let { ImmediateGlideSize(it) } ?: AsyncGlideSize() } @Composable @@ -347,17 +329,6 @@ private fun rememberGlidePainter( return remember(requestBuilder, size) { GlidePainter(requestBuilder, size, scope) } } -@OptIn(InternalGlideApi::class) -private fun Modifier.sizeObservingModifier(sizeObserver: SizeObserver): Modifier = - this.layout { measurable, constraints -> - val inferredSize = constraints.inferredGlideSize() - if (inferredSize != null) { - sizeObserver.setSize(inferredSize) - } - val placeable = measurable.measure(constraints) - layout(placeable.width, placeable.height) { placeable.place(0, 0) } - } - internal val DisplayedDrawableKey = SemanticsPropertyKey>("DisplayedDrawable") internal var SemanticsPropertyReceiver.displayedDrawable by DisplayedDrawableKey diff --git a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlidePainter.kt b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlidePainter.kt index 5c1cf28436..adb81d1568 100644 --- a/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlidePainter.kt +++ b/integration/compose/src/main/java/com/bumptech/glide/integration/compose/GlidePainter.kt @@ -3,7 +3,6 @@ package com.bumptech.glide.integration.compose import android.graphics.drawable.BitmapDrawable import android.graphics.drawable.ColorDrawable import android.graphics.drawable.Drawable -import android.util.Log import androidx.compose.runtime.MutableState import androidx.compose.runtime.RememberObserver import androidx.compose.runtime.Stable @@ -20,7 +19,9 @@ import androidx.compose.ui.graphics.painter.BitmapPainter import androidx.compose.ui.graphics.painter.ColorPainter import androidx.compose.ui.graphics.painter.Painter import com.bumptech.glide.RequestBuilder +import com.bumptech.glide.integration.ktx.AsyncGlideSize import com.bumptech.glide.integration.ktx.ExperimentGlideFlows +import com.bumptech.glide.integration.ktx.ImmediateGlideSize import com.bumptech.glide.integration.ktx.InternalGlideApi import com.bumptech.glide.integration.ktx.Placeholder import com.bumptech.glide.integration.ktx.ResolvableGlideSize @@ -44,7 +45,7 @@ internal class GlidePainter @OptIn(InternalGlideApi::class) constructor( private val requestBuilder: RequestBuilder, - private val size: ResolvableGlideSize, + private val resolvableSize: ResolvableGlideSize, scope: CoroutineScope, ) : Painter(), RememberObserver { @OptIn(ExperimentGlideFlows::class) internal var status: Status by mutableStateOf(Status.CLEARED) @@ -59,7 +60,15 @@ constructor( override val intrinsicSize: Size get() = delegate?.intrinsicSize ?: Size.Unspecified + @OptIn(InternalGlideApi::class) override fun DrawScope.onDraw() { + when (resolvableSize) { + is AsyncGlideSize -> { + size.toGlideSize()?.let { resolvableSize.setSize(it) } + } + // Do nothing. + is ImmediateGlideSize -> {} + } delegate?.apply { draw(size, alpha, colorFilter) } } @@ -82,7 +91,7 @@ constructor( @OptIn(ExperimentGlideFlows::class, InternalGlideApi::class) private fun launchRequest() = this.scope.launch { - requestBuilder.flowResolvable(size).collect { + requestBuilder.flowResolvable(resolvableSize).collect { updateDelegate( when (it) { is Resource -> it.resource 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 cc56d4eb59..c59550f865 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 @@ -2,13 +2,12 @@ package com.bumptech.glide.integration.compose -import androidx.compose.ui.unit.Constraints import com.bumptech.glide.RequestBuilder import com.bumptech.glide.integration.ktx.InternalGlideApi import com.bumptech.glide.integration.ktx.Size import com.bumptech.glide.integration.ktx.isValidGlideDimension -import com.bumptech.glide.request.target.Target import kotlinx.coroutines.CompletableDeferred +import kotlin.math.roundToInt internal class SizeObserver { private val size = CompletableDeferred() @@ -32,12 +31,11 @@ internal fun RequestBuilder.overrideSize(): Size? = internal fun RequestBuilder.isOverrideSizeSet(): Boolean = overrideWidth.isValidGlideDimension() && overrideHeight.isValidGlideDimension() -internal fun Constraints.inferredGlideSize(): Size? { - val width = if (hasBoundedWidth) maxWidth else Target.SIZE_ORIGINAL - val height = if (hasBoundedHeight) maxHeight else Target.SIZE_ORIGINAL +internal fun androidx.compose.ui.geometry.Size.toGlideSize(): Size? { + val width = width.roundToInt(); + val height = height.roundToInt(); if (!width.isValidGlideDimension() || !height.isValidGlideDimension()) { - return null + return null; } - return Size(width, height) + return Size(width, height); } - diff --git a/integration/ktx/src/main/java/com/bumptech/glide/integration/ktx/Flows.kt b/integration/ktx/src/main/java/com/bumptech/glide/integration/ktx/Flows.kt index ad937cb796..a0ce7afa76 100644 --- a/integration/ktx/src/main/java/com/bumptech/glide/integration/ktx/Flows.kt +++ b/integration/ktx/src/main/java/com/bumptech/glide/integration/ktx/Flows.kt @@ -13,6 +13,7 @@ import com.bumptech.glide.request.target.Target import com.bumptech.glide.request.transition.Transition import com.bumptech.glide.requestManager import com.bumptech.glide.util.Util +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.channels.ProducerScope import kotlinx.coroutines.channels.awaitClose import kotlinx.coroutines.flow.Flow @@ -82,17 +83,18 @@ public fun RequestBuilder.flow( /** * Identical to [flow] with dimensions, except that the size is resolved asynchronously using - * [waitForSize]. + * [size]. * * If an override size has been set using [RequestBuilder.override], that size will be used instead - * and [waitForSize] may never be called. + * and [size] may never be called. * - * [Placeholder] values may be emitted prior to [waitForSize] returning. Similarly if + * [Placeholder] values may be emitted prior to [size] returning. Similarly if * [RequestBuilder.thumbnail] requests are present and have overridden sizes, [Resource] values for - * those thumbnails may also be emitted. [waitForSize] will only be used for requests where no + * those thumbnails may also be emitted. [size] will only be used for requests where no * [RequestBuilder.override] size is available. * - * If [waitForSize] does not return, this flow may never return values other than placeholders. + * If [size] never has [AsyncGlideSize.setSize] called, this flow may never return values other than + * placeholders. * * This function is internal only, intended primarily for Compose. The Target API provides similar * functionality for traditional Views. We could consider expanding the visibility if there are use @@ -101,8 +103,8 @@ public fun RequestBuilder.flow( @InternalGlideApi @ExperimentGlideFlows public fun RequestBuilder.flow( - waitForSize: suspend () -> Size, -): Flow> = flow(AsyncGlideSize(waitForSize)) + size: AsyncGlideSize, +): Flow> = flowResolvable(size) /** * Convert a load in Glide into a flow that emits placeholders and resources in the order they'd be @@ -276,7 +278,7 @@ private class FlowTarget( // begin immediately, shaving off some small amount of time. is AsyncGlideSize -> scope.launch { - val localResolvedSize = size.asyncSize() + val localResolvedSize = size.getSize() val callbacksToNotify: List synchronized(this) { resolvedSize = localResolvedSize @@ -387,6 +389,16 @@ public data class Size(val width: Int, val height: Int) { @InternalGlideApi public data class ImmediateGlideSize(val size: Size) : ResolvableGlideSize() @InternalGlideApi -public data class AsyncGlideSize(val asyncSize: suspend () -> Size) : ResolvableGlideSize() +public class AsyncGlideSize : ResolvableGlideSize() { + private val size = CompletableDeferred() + + public fun setSize(size: Size) { + this.size.complete(size) + } + + public suspend fun getSize(): Size { + return size.await() + } +} @InternalGlideApi public fun Int.isValidGlideDimension(): Boolean = Util.isValidDimension(this) diff --git a/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt b/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt index e13dc1886a..acee8028e4 100644 --- a/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt +++ b/integration/ktx/src/test/java/com/bumptech/glide/integration/ktx/FlowsTest.kt @@ -31,14 +31,11 @@ import com.google.common.truth.Correspondence import com.google.common.truth.IterableSubject import com.google.common.truth.Truth.assertThat import java.io.File -import java.lang.RuntimeException import java.util.concurrent.atomic.AtomicReference import kotlin.reflect.KClass import kotlin.test.assertFailsWith import kotlinx.coroutines.DelicateCoroutinesApi import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.channels.Channel -import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flow @@ -494,11 +491,13 @@ class FlowsTest { @Test fun flow_withAsyncSize_andOverrideSize_usesOverrideSize() = runTest { val requestedSizeReference = registerSizeCapturingFakeModelLoader() + val asyncSize = AsyncGlideSize() + asyncSize.setSize(Size(1, 2)) Glide.with(context) .load(FakeModel()) .override(200, 100) - .flow { Size(1, 2) } + .flow(asyncSize) .firstLoad() .toList() @@ -525,11 +524,7 @@ class FlowsTest { @Test fun flow_withAsyncSize_concreteSizeForThumbnail_startsMainRequestWhenAsyncSizeIsAvailable() = runTest { - val waitForThumbnailToFinishChannel = Channel() - val waitForThumbnailToFinishSize: suspend () -> Size = { - waitForThumbnailToFinishChannel.receive() - Size(100, 200) - } + val size = AsyncGlideSize() val result = Glide.with(context) @@ -538,9 +533,9 @@ class FlowsTest { Glide.with(context) .load(newImageFile()) .override(75, 50) - .listener(onSuccess { launch { waitForThumbnailToFinishChannel.send(true) } }) + .listener(onSuccess { launch { size.setSize(Size(100, 200)) } }) ) - .flow(waitForThumbnailToFinishSize) + .flow(size) .firstLoad() .toList() @@ -573,10 +568,7 @@ class FlowsTest { assertFailsWith { requestBuilder.flow() } } - private val delayForever: suspend () -> Size = { - delay(kotlin.time.Duration.INFINITE) - throw RuntimeException() - } + private val delayForever = AsyncGlideSize() private fun registerSizeCapturingFakeModelLoader(): AtomicReference { val result = AtomicReference()