From b3294015ae9739ad434f2c274ca2104522735f67 Mon Sep 17 00:00:00 2001 From: Sam Judd Date: Sun, 20 Aug 2023 15:29:22 -0700 Subject: [PATCH] Influence layout using intrinsics in GlideNode --- .../glide/integration/compose/GlideImage.kt | 29 +++-- .../integration/compose/GlideModifier.kt | 116 +++++++++++++----- 2 files changed, 97 insertions(+), 48 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 3e79487bdf..c963961f79 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 @@ -34,29 +34,29 @@ public typealias RequestBuilderTransform = (RequestBuilder) -> RequestBuil * [alignment], [contentScale], [alpha], [colorFilter] and [contentDescription] have the same * defaults (if any) and function identically to the parameters in [Image]. * - * If you want to restrict the size of this [Composable], use the given [modifier]. If you'd like to - * force the size of the pixels you load to be different than the display area, use - * [RequestBuilder.override]. Often you can get better performance by setting an explicit size so - * that we do not have to wait for layout to fetch the image. If the size set via the [modifier] is + * Set the size this [Composable] using the given [modifier]. Use fixed sizes when you can for + * better performance and to avoid layout jank when images are loaded. If you cannot use a fixed + * size, try to at least set a bounded size. If the size set via the [modifier] is * dependent on the content, Glide will probably end up loading the image using * [com.bumptech.glide.request.target.Target.SIZE_ORIGINAL]. Avoid `SIZE_ORIGINAL`, implicitly or * 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. * - * 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]] + * You can force the size of the image you load to be different than the display area using + * [RequestBuilder.override]. * - * Transitions set via [RequestBuilder.transition] are currently ignored. - * - * Note - this method is likely to change while we work on improving the API. Transitions are one - * significant unexplored area. It's also possible we'll try and remove the [RequestBuilder] from - * the direct API and instead allow all options to be set directly in the method. + * [contentScale] will apply to both [loading] and [error] placeholders, as well as the the primary + * request. If you'd like different scaling behavior for placeholders vs the primary request, use + * [contentScale] to scale the placeholders and [requestBuilderTransform] to set a different + * `Transformation` for the image load. [contentScale] will also be inspected to apply a matching + * default `Transformation` if one exists. Any automatically applied `Transformation` based on + * [contentScale] can be overridden using [requestBuilderTransform] via [RequestBuilder.transform] + * or [RequestBuilder.dontTransform]. * * [requestBuilderTransform] is overridden by any overlapping parameter defined in this method if * that parameter is non-null. For example, [loading] and [failure], if non-null will be used in * place of any placeholder set by [requestBuilderTransform] using [RequestBuilder.placeholder] or - * [RequestBuilder.error]. + * [RequestBuilder.error]. Transitions set via [RequestBuilder.transition] are ignored. * * @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 @@ -80,7 +80,6 @@ public typealias RequestBuilderTransform = (RequestBuilder) -> RequestBuil */ // TODO(judds): the API here is not particularly composeesque, we should consider alternatives // to RequestBuilder (though thumbnail() may make that a challenge). -// TODO(judds): Consider how to deal with transitions. @ExperimentalGlideComposeApi @Composable public fun GlideImage( @@ -131,7 +130,7 @@ public fun GlideImage( alignment, contentScale, alpha, - colorFilter + colorFilter, ) } } 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 8c382fe8e2..4bcd596f10 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 @@ -2,7 +2,6 @@ package com.bumptech.glide.integration.compose import android.graphics.PointF import android.graphics.drawable.Drawable -import android.util.Log import androidx.compose.ui.Alignment import androidx.compose.ui.ExperimentalComposeUiApi import androidx.compose.ui.Modifier @@ -12,6 +11,7 @@ import androidx.compose.ui.graphics.ColorFilter import androidx.compose.ui.graphics.DefaultAlpha import androidx.compose.ui.graphics.drawscope.ContentDrawScope import androidx.compose.ui.graphics.drawscope.DrawScope +import androidx.compose.ui.graphics.drawscope.clipRect import androidx.compose.ui.graphics.drawscope.translate import androidx.compose.ui.graphics.painter.Painter import androidx.compose.ui.graphics.withSave @@ -36,6 +36,8 @@ import androidx.compose.ui.semantics.semantics import androidx.compose.ui.unit.Constraints import androidx.compose.ui.unit.IntOffset import androidx.compose.ui.unit.IntSize +import androidx.compose.ui.unit.constrainHeight +import androidx.compose.ui.unit.constrainWidth import androidx.compose.ui.unit.toSize import com.bumptech.glide.RequestBuilder import com.bumptech.glide.integration.ktx.AsyncGlideSize @@ -72,7 +74,7 @@ internal fun Modifier.glideNode( colorFilter: ColorFilter? = null, transitionFactory: Transition.Factory? = null, requestListener: RequestListener? = null, - draw: Boolean? = true, + draw: Boolean? = null, ): Modifier { return this then GlideNodeElement( requestBuilder, @@ -151,6 +153,7 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi private lateinit var requestBuilder: RequestBuilder private lateinit var contentScale: ContentScale private lateinit var alignment: Alignment + private lateinit var resolvableGlideSize: ResolvableGlideSize private var alpha: Float = DefaultAlpha private var colorFilter: ColorFilter? = null private var transitionFactory: Transition.Factory = DoNotTransition.Factory @@ -163,7 +166,6 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi // Only used for debugging private var drawable: Drawable? = null private var state: RequestState = RequestState.Loading - private var resolvableGlideSize: ResolvableGlideSize? = null private var placeholder: Painter? = null private var isFirstResource = true @@ -171,6 +173,9 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi private var placeholderPositionAndSize: CachedPositionAndSize? = null private var drawablePositionAndSize: CachedPositionAndSize? = null + private var hasFixedSize: Boolean = false + private var inferredGlideSize: com.bumptech.glide.integration.ktx.Size? = null + private var transition: Transition = DoNotTransition private fun RequestBuilder<*>.maybeImmediateSize() = @@ -199,6 +204,10 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi this.requestListener = requestListener this.draw = draw ?: true this.transitionFactory = transitionFactory ?: DoNotTransition.Factory + this.resolvableGlideSize = + requestBuilder.maybeImmediateSize() + ?: inferredGlideSize?.let { ImmediateGlideSize(it) } + ?: AsyncGlideSize() if (restartLoad) { clear() @@ -206,18 +215,6 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi // If we're not attached, we'll be measured when we eventually are attached. if (isAttached) { - // If we don't have a fixed size, we need a new layout pass to figure out how large the - // image should be. Ideally we'd retain the old resolved glide size unless some other - // modifier node had already requested measurement. Since we can't tell if measurement is - // requested, we can either re-use the old resolvableGlideSize, which will be incorrect if - // measurement was requested. Or we can invalidate resolvableGlideSize and ensure that it's - // resolved by requesting measurement ourselves. Requesting is less efficient, but more - // correct. - // TODO(sam): See if we can find a reasonable way to remove this behavior, or be more - // targeted. - if (requestBuilder.overrideSize() == null) { - invalidateMeasurement() - } launchRequest(requestBuilder) } } else { @@ -225,11 +222,17 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi } } + private val Size.isValidWidth + get() = this != Size.Unspecified && this.width.isValidDimension + + private val Size.isValidHeight + get() = this != Size.Unspecified && this.height.isValidDimension + private val Float.isValidDimension - get() = this > 0f + get() = this > 0f && isFinite() private val Size.isValid - get() = width.isValidDimension && height.isValidDimension + get() = isValidWidth && isValidHeight private fun Size.roundToInt() = IntSize(width.roundToInt(), height.roundToInt()) @@ -248,12 +251,12 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi val currentPositionAndSize = if (cache != null) { cache } else { - val srcWidth = if (painter.intrinsicSize.width.isValidDimension) { + val srcWidth = if (painter.intrinsicSize.isValidWidth) { painter.intrinsicSize.width } else { size.width } - val srcHeight = if (painter.intrinsicSize.height.isValidDimension) { + val srcHeight = if (painter.intrinsicSize.isValidHeight) { painter.intrinsicSize.height } else { size.height @@ -275,8 +278,10 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi ) } - translate(currentPositionAndSize.position.x, currentPositionAndSize.position.y) { - drawOne.invoke(this, currentPositionAndSize.size) + clipRect { + translate(currentPositionAndSize.position.x, currentPositionAndSize.position.y) { + drawOne.invoke(this, currentPositionAndSize.size) + } } return currentPositionAndSize } @@ -355,11 +360,10 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi } Preconditions.checkArgument(currentJob == null) currentJob = (coroutineScope + Dispatchers.Main.immediate).launch { - this@GlideNode.resolvableGlideSize = requestBuilder.maybeImmediateSize() ?: AsyncGlideSize() placeholder = null placeholderPositionAndSize = null - requestBuilder.flowResolvable(resolvableGlideSize!!).collect { + requestBuilder.flowResolvable(resolvableGlideSize).collect { val (state, drawable) = when (it) { is Resource -> { @@ -382,7 +386,11 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi updateDrawable(drawable) requestListener?.onStateChanged(requestBuilder.internalModel, drawable, state) this@GlideNode.state = state - invalidateDraw() + if (hasFixedSize) { + invalidateDraw() + } else { + invalidateMeasurement() + } } } } @@ -405,7 +413,6 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi private fun clear() { isFirstResource = true - resolvableGlideSize = null currentJob?.cancel() currentJob = null state = RequestState.Loading @@ -419,23 +426,66 @@ internal class GlideNode : DrawModifierNode, LayoutModifierNode, SemanticsModifi ): MeasureResult { placeholderPositionAndSize = null drawablePositionAndSize = null + hasFixedSize = constraints.hasFixedSize() + inferredGlideSize = constraints.inferredGlideSize() + when (val currentSize = resolvableGlideSize) { is AsyncGlideSize -> { - val inferredSize = constraints.inferredGlideSize() - if (inferredSize != null) { - currentSize.setSize(inferredSize) - } + inferredGlideSize?.also { currentSize.setSize(it) } } - // Do nothing. + is ImmediateGlideSize -> {} - null -> {} } - val placeable = measurable.measure(constraints) - return layout(constraints.maxWidth, constraints.maxHeight) { + val placeable = measurable.measure(modifyConstraints(constraints)) + return layout(placeable.width, placeable.height) { placeable.placeRelative(0, 0) } } + private fun Constraints.hasFixedSize() = hasFixedWidth && hasFixedHeight + + private fun modifyConstraints(constraints: Constraints): Constraints { + if (constraints.hasFixedSize()) { + return constraints.copy( + minWidth = constraints.maxWidth, + minHeight = constraints.maxHeight + ) + } + + val intrinsicSize = painter?.intrinsicSize ?: return constraints + + val intrinsicWidth = + if (constraints.hasFixedWidth) { + constraints.maxWidth + } else if (intrinsicSize.isValidWidth) { + intrinsicSize.width.roundToInt() + } else { + constraints.minWidth + } + + val intrinsicHeight = + if (constraints.hasFixedHeight) { + constraints.maxHeight + } else if (intrinsicSize.isValidHeight) { + intrinsicSize.height.roundToInt() + } else { + constraints.minHeight + } + + val constrainedWidth = constraints.constrainWidth(intrinsicWidth) + val constrainedHeight = constraints.constrainHeight(intrinsicHeight) + + val srcSize = Size(intrinsicWidth.toFloat(), intrinsicHeight.toFloat()) + val scaledSize = + srcSize * contentScale.computeScaleFactor( + srcSize, Size(constrainedWidth.toFloat(), constrainedHeight.toFloat()) + ) + + val minWidth = constraints.constrainWidth(scaledSize.width.roundToInt()) + val minHeight = constraints.constrainHeight(scaledSize.height.roundToInt()) + return constraints.copy(minWidth = minWidth, minHeight = minHeight) + } + override fun SemanticsPropertyReceiver.applySemantics() { displayedDrawable = { drawable } }