From 17635e3b7d21cd5987e6be1508b1ad11596de3d7 Mon Sep 17 00:00:00 2001 From: Chris Banes Date: Thu, 9 Jul 2020 14:00:08 +0100 Subject: [PATCH] Stop refetching on every size change by default Currently we invoke Coil on every size change, which is correct but a lot of wasted work most of the time. We no longer re-fetch on every size change. If developers want to customize this, they can use the new `shouldRefetchOnSizeChange` lambda parameter to allow re-fetching as they see fit. --- .../chrisbanes/accompanist/coil/CoilTest.kt | 34 +++++++++++++++++++ .../dev/chrisbanes/accompanist/coil/Coil.kt | 32 ++++++++++++----- .../chrisbanes/accompanist/coil/Crossfade.kt | 8 +++++ 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt b/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt index 8eed0e0a6..beb3660f9 100644 --- a/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt +++ b/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt @@ -48,6 +48,7 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.TestCoroutineDispatcher import kotlinx.coroutines.test.runBlockingTest +import kotlinx.coroutines.withTimeoutOrNull import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -175,6 +176,39 @@ class CoilTest { loadCompleteSignal.close() } + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun basicLoad_changeSize() { + val loadCompleteSignal = Channel(Channel.UNLIMITED) + val sizeFlow = MutableStateFlow(128.dp) + + composeTestRule.setContent { + val size = sizeFlow.collectAsState() + CoilImage( + data = resourceUri(R.drawable.blue_rectangle), + modifier = Modifier.preferredSize(size.value).testTag(CoilTestTags.Image), + onRequestCompleted = { loadCompleteSignal.offer(Unit) } + ) + } + + // Await the first load + runBlocking { + loadCompleteSignal.receive() + } + + // Now change the size + sizeFlow.value = 256.dp + + // Await the potential second load (which shouldn't come) + runBlocking { + val result = withTimeoutOrNull(3000) { loadCompleteSignal.receive() } + assertThat(result).isNull() + } + + // Close the signal channel + loadCompleteSignal.close() + } + @Test @SdkSuppress(minSdkVersion = 26) // captureToBitmap is SDK 26+ fun customGetPainter() { diff --git a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt index e6d308e9b..cf8e1d609 100644 --- a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt +++ b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt @@ -58,6 +58,8 @@ import coil.request.GetRequestBuilder * @param getFailurePainter Optional builder for the [Painter] to be used to draw the failure * loading result. Passing in `null` will result in falling back to the default [Painter]. * @param loading Content to be displayed when the request is in progress. + * @param shouldRefetchOnSizeChange Lambda which will be invoked when the size changes, allowing + * optional re-fetching of the image. Return true to re-fetch the image. * @param onRequestCompleted Listener which will be called when the loading request has finished. */ @Composable @@ -70,6 +72,7 @@ fun CoilImage( getSuccessPainter: @Composable ((SuccessResult) -> Painter)? = null, getFailurePainter: @Composable ((ErrorResult) -> Painter?)? = null, loading: @Composable (() -> Unit)? = null, + shouldRefetchOnSizeChange: (currentResult: RequestResult, size: IntSize) -> Boolean = defaultRefetchOnSizeChangeLambda, onRequestCompleted: (RequestResult) -> Unit = emptySuccessLambda ) { CoilImage( @@ -89,6 +92,7 @@ fun CoilImage( getSuccessPainter = getSuccessPainter, getFailurePainter = getFailurePainter, loading = loading, + shouldRefetchOnSizeChange = shouldRefetchOnSizeChange, modifier = modifier ) } @@ -110,6 +114,8 @@ fun CoilImage( * @param getFailurePainter Optional builder for the [Painter] to be used to draw the failure * loading result. Passing in `null` will result in falling back to the default [Painter]. * @param loading Content to be displayed when the request is in progress. + * @param shouldRefetchOnSizeChange Lambda which will be invoked when the size changes, allowing + * optional re-fetching of the image. Return true to re-fetch the image. * @param onRequestCompleted Listener which will be called when the loading request has finished. */ @Composable @@ -122,9 +128,16 @@ fun CoilImage( getSuccessPainter: @Composable ((SuccessResult) -> Painter)? = null, getFailurePainter: @Composable ((ErrorResult) -> Painter?)? = null, loading: @Composable (() -> Unit)? = null, + shouldRefetchOnSizeChange: (currentResult: RequestResult, size: IntSize) -> Boolean = defaultRefetchOnSizeChangeLambda, onRequestCompleted: (RequestResult) -> Unit = emptySuccessLambda ) { - var result by stateFor(request.data) { null } + // GetRequest does not support object equality (as of Coil v0.10.1) so we can not key any + // remember calls using the request itself. For now we just use the [data] field, but + // ideally this should use [request] to track changes in size, transformations, etc too. + // See: https://github.com/coil-kt/coil/issues/405 + val requestKey = request.data ?: request + + var result by stateFor(requestKey) { null } // This may look a little weird, but allows the launchInComposition callback to always // invoke the last provided [onRequestCompleted]. @@ -139,11 +152,7 @@ fun CoilImage( val callback = state { onRequestCompleted } callback.value = onRequestCompleted - // GetRequest does not support object equality (as of Coil v0.10.1) so we can not key the - // remember() using the request itself. For now we just use the [data] field, but - // ideally this should use [request] to track changes in size, transformations, etc too. - // See: https://github.com/coil-kt/coil/issues/405 - val requestActor = remember(request.data) { CoilRequestActor(request) } + val requestActor = remember(requestKey) { CoilRequestActor(request) } launchInComposition(requestActor) { // Launch the Actor @@ -176,9 +185,14 @@ fun CoilImage( else -> null } + // Update the modifier, so that we listen for size changes to invoke the actor val mod = modifier.onSizeChanged { size -> - // When the size changes, send it to the request actor - requestActor.send(size) + val r = result + if (r == null || shouldRefetchOnSizeChange(r, size)) { + // If we don't have a result yet, or the size has changed and we should re-fetch, + // send it to the request actor + requestActor.send(size) + } } if (painter == null) { @@ -295,6 +309,8 @@ internal fun defaultSuccessPainterGetter(result: SuccessResult): Painter { internal val emptySuccessLambda: (RequestResult) -> Unit = {} +internal val defaultRefetchOnSizeChangeLambda: (RequestResult, IntSize) -> Boolean = { _, _ -> false } + internal fun Drawable.toImageAsset(fallbackSize: IntSize = IntSize.Zero): ImageAsset { return toBitmap( width = if (intrinsicWidth > 0) intrinsicWidth else fallbackSize.width, diff --git a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Crossfade.kt b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Crossfade.kt index d353977ff..cece4ac45 100644 --- a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Crossfade.kt +++ b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Crossfade.kt @@ -67,6 +67,8 @@ private const val DefaultTransitionDuration = 1000 * @param crossfadeDuration The duration of the crossfade animation in milliseconds. * @param getFailurePainter Optional builder for the [Painter] to be used to draw the failure * loading result. Passing in `null` will result in falling back to the default [Painter]. + * @param shouldRefetchOnSizeChange Lambda which will be invoked when the size changes, allowing + * optional re-fetching of the image. Return true to re-fetch the image. * @param onRequestCompleted Listener which will be called when the loading request has finished. */ @Composable @@ -78,6 +80,7 @@ fun CoilImageWithCrossfade( crossfadeDuration: Int = DefaultTransitionDuration, getFailurePainter: @Composable ((ErrorResult) -> Painter?)? = null, loading: @Composable (() -> Unit)? = null, + shouldRefetchOnSizeChange: (currentResult: RequestResult, size: IntSize) -> Boolean = defaultRefetchOnSizeChangeLambda, onRequestCompleted: (RequestResult) -> Unit = emptySuccessLambda ) { CoilImage( @@ -88,6 +91,7 @@ fun CoilImageWithCrossfade( getFailurePainter = getFailurePainter, loading = loading, modifier = modifier, + shouldRefetchOnSizeChange = shouldRefetchOnSizeChange, onRequestCompleted = onRequestCompleted ) } @@ -109,6 +113,8 @@ fun CoilImageWithCrossfade( * @param crossfadeDuration The duration of the crossfade animation in milliseconds. * @param getFailurePainter Optional builder for the [Painter] to be used to draw the failure * loading result. Passing in `null` will result in falling back to the default [Painter]. + * @param shouldRefetchOnSizeChange Lambda which will be invoked when the size changes, allowing + * optional re-fetching of the image. Return true to re-fetch the image. * @param onRequestCompleted Listener which will be called when the loading request has finished. */ @Composable @@ -120,6 +126,7 @@ fun CoilImageWithCrossfade( crossfadeDuration: Int = DefaultTransitionDuration, getFailurePainter: @Composable ((ErrorResult) -> Painter?)? = null, loading: @Composable (() -> Unit)? = null, + shouldRefetchOnSizeChange: (currentResult: RequestResult, size: IntSize) -> Boolean = defaultRefetchOnSizeChangeLambda, onRequestCompleted: (RequestResult) -> Unit = emptySuccessLambda ) { CoilImage( @@ -129,6 +136,7 @@ fun CoilImageWithCrossfade( getSuccessPainter = { crossfadePainter(it, durationMs = crossfadeDuration) }, getFailurePainter = getFailurePainter, loading = loading, + shouldRefetchOnSizeChange = shouldRefetchOnSizeChange, modifier = modifier, onRequestCompleted = onRequestCompleted )