From 50525a82300afa8933e7fc56e3e3d8aa778e7790 Mon Sep 17 00:00:00 2001 From: Chris Date: Fri, 14 Aug 2020 14:45:08 +0100 Subject: [PATCH 1/3] Fix using CoilImage with an implicit size Currently CoilImage only works when you specific a size Modifier, since it waits for onPositioned to be called, and only starts a Coil request when it has a non-zero size. If no Modifier is provided, the CoilImage will have a zero size and we never execute a Coil request. Fixed by moving back to WithConstraints to move the request size logic to be based on the incming constraints, rather than the final size, which is probably more correct anyway. --- .../chrisbanes/accompanist/coil/Assertions.kt | 39 ------------ .../chrisbanes/accompanist/coil/CoilTest.kt | 40 +++++++++++-- .../dev/chrisbanes/accompanist/coil/Coil.kt | 60 ++++++++++++------- .../chrisbanes/accompanist/coil/Modifier.kt | 41 ------------- .../sample/coil/CoilBasicSample.kt | 10 ++++ 5 files changed, 83 insertions(+), 107 deletions(-) delete mode 100644 coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/Assertions.kt delete mode 100644 coil/src/main/java/dev/chrisbanes/accompanist/coil/Modifier.kt diff --git a/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/Assertions.kt b/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/Assertions.kt deleted file mode 100644 index 34e276d7e..000000000 --- a/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/Assertions.kt +++ /dev/null @@ -1,39 +0,0 @@ -/* - * Copyright 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package dev.chrisbanes.accompanist.coil - -import androidx.compose.ui.unit.Density -import androidx.compose.ui.unit.Dp -import androidx.compose.ui.unit.IntSize -import androidx.ui.test.SemanticsNodeInteraction -import org.junit.Assert - -fun SemanticsNodeInteraction.assertSize( - density: Density, - width: Dp, - height: Dp -): SemanticsNodeInteraction { - return assertSize(with(density) { IntSize(width.toIntPx(), height.toIntPx()) }) -} - -fun SemanticsNodeInteraction.assertSize( - expected: IntSize -): SemanticsNodeInteraction { - val node = fetchSemanticsNode("Assert size") - Assert.assertEquals(expected, node.size) - return this -} 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 240302be3..2a4660c6f 100644 --- a/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt +++ b/coil/src/androidTest/java/dev/chrisbanes/accompanist/coil/CoilTest.kt @@ -31,8 +31,12 @@ import androidx.compose.ui.unit.dp import androidx.core.net.toUri import androidx.test.filters.LargeTest import androidx.test.filters.SdkSuppress +import androidx.ui.test.assertHeightIsAtLeast +import androidx.ui.test.assertHeightIsEqualTo import androidx.ui.test.assertIsDisplayed import androidx.ui.test.assertPixels +import androidx.ui.test.assertWidthIsAtLeast +import androidx.ui.test.assertWidthIsEqualTo import androidx.ui.test.captureToBitmap import androidx.ui.test.createComposeRule import androidx.ui.test.onNodeWithTag @@ -105,7 +109,8 @@ class CoilTest { onNodeWithTag(CoilTestTags.Image) .assertIsDisplayed() - .assertSize(composeTestRule.density, 128.dp, 128.dp) + .assertWidthIsEqualTo(128.dp) + .assertHeightIsEqualTo(128.dp) } @Test @@ -125,7 +130,8 @@ class CoilTest { latch.await(5, TimeUnit.SECONDS) onNodeWithTag(CoilTestTags.Image) - .assertSize(composeTestRule.density, 128.dp, 128.dp) + .assertWidthIsEqualTo(128.dp) + .assertHeightIsEqualTo(128.dp) .assertIsDisplayed() .captureToBitmap() .assertPixels { Color.Red } @@ -154,7 +160,8 @@ class CoilTest { // Assert that the content is completely Red onNodeWithTag(CoilTestTags.Image) - .assertSize(composeTestRule.density, 128.dp, 128.dp) + .assertWidthIsEqualTo(128.dp) + .assertHeightIsEqualTo(128.dp) .assertIsDisplayed() .captureToBitmap() .assertPixels { Color.Red } @@ -167,7 +174,8 @@ class CoilTest { // Assert that the content is completely Blue onNodeWithTag(CoilTestTags.Image) - .assertSize(composeTestRule.density, 128.dp, 128.dp) + .assertWidthIsEqualTo(128.dp) + .assertHeightIsEqualTo(128.dp) .assertIsDisplayed() .captureToBitmap() .assertPixels { Color.Blue } @@ -209,6 +217,27 @@ class CoilTest { loadCompleteSignal.close() } + @Test + fun basicLoad_nosize() { + val latch = CountDownLatch(1) + + composeTestRule.setContent { + CoilImage( + data = resourceUri(R.raw.sample), + modifier = Modifier.testTag(CoilTestTags.Image), + onRequestCompleted = { latch.countDown() } + ) + } + + // Wait for the onRequestCompleted to release the latch + latch.await(5, TimeUnit.SECONDS) + + onNodeWithTag(CoilTestTags.Image) + .assertWidthIsAtLeast(1.dp) + .assertHeightIsAtLeast(1.dp) + .assertIsDisplayed() + } + @Test @SdkSuppress(minSdkVersion = 26) // captureToBitmap is SDK 26+ fun customGetPainter() { @@ -254,7 +283,8 @@ class CoilTest { // Assert that the layout is in the tree and has the correct size onNodeWithTag(CoilTestTags.Image) .assertIsDisplayed() - .assertSize(composeTestRule.density, 128.dp, 128.dp) + .assertWidthIsEqualTo(128.dp) + .assertHeightIsEqualTo(128.dp) } @OptIn(ExperimentalCoroutinesApi::class) 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 73a6c61ab..146ee5980 100644 --- a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt +++ b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt @@ -20,6 +20,7 @@ import android.graphics.drawable.Drawable import androidx.compose.foundation.Box import androidx.compose.foundation.Image import androidx.compose.runtime.Composable +import androidx.compose.runtime.Stable import androidx.compose.runtime.getValue import androidx.compose.runtime.launchInComposition import androidx.compose.runtime.remember @@ -28,6 +29,7 @@ import androidx.compose.runtime.state import androidx.compose.runtime.stateFor import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.WithConstraints import androidx.compose.ui.graphics.ColorFilter import androidx.compose.ui.graphics.ImageAsset import androidx.compose.ui.graphics.asImageAsset @@ -186,36 +188,45 @@ fun CoilImage( else -> null } - // Update the modifier, so that we listen for size changes to invoke the actor - val mod = modifier.onSizeChanged { size -> + WithConstraints(modifier) { + val lastRequestedSize = remember(requestActor) { MutableRef(IntSize.Zero) } + + val requestSize = IntSize( + width = if (constraints.hasBoundedWidth) constraints.maxWidth else Int.MAX_VALUE, + height = if (constraints.hasBoundedHeight) constraints.maxHeight else Int.MAX_VALUE + ) + 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 (lastRequestedSize.value != requestSize && + (r == null || shouldRefetchOnSizeChange(r, requestSize)) + ) { + requestActor.send(requestSize) + lastRequestedSize.value = requestSize } - } - if (painter == null) { - // If we don't have a result painter, we add a Box with our modifier - Box(mod) { - // If we don't have a result yet, we can show the loading content - // (if not null) - if (result == null && loading != null) { - loading() + if (painter == null) { + // If we don't have a result painter, we add a Box with our modifier + Box { + // If we don't have a result yet, we can show the loading content + // (if not null) + if (result == null && loading != null) { + loading() + } } + } else { + Image( + painter = painter, + contentScale = contentScale, + alignment = alignment, + colorFilter = colorFilter, + ) } - } else { - Image( - painter = painter, - contentScale = contentScale, - alignment = alignment, - colorFilter = colorFilter, - modifier = mod - ) } } +@Stable +private data class MutableRef(var value: T) + private fun CoilRequestActor( request: GetRequest ) = RequestActor { size -> @@ -224,6 +235,11 @@ private fun CoilRequestActor( // If the request has a sizeResolver set, we just execute the request as-is request } + size.height == Int.MAX_VALUE || size.width == Int.MAX_VALUE -> { + // If the size contains an 'infinite' dimension, we don't specific a size in the Coil + // request + request + } size != IntSize.Zero -> { // If we have a non-zero size, we can modify the request to include the size request.newBuilder() diff --git a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Modifier.kt b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Modifier.kt deleted file mode 100644 index 7069edd8b..000000000 --- a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Modifier.kt +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package dev.chrisbanes.accompanist.coil - -import androidx.compose.runtime.getValue -import androidx.compose.runtime.setValue -import androidx.compose.runtime.state -import androidx.compose.ui.Modifier -import androidx.compose.ui.composed -import androidx.compose.ui.onPositioned -import androidx.compose.ui.unit.IntSize - -/** - * [Modifier] which will invoke [onSizeChanged] whenever the size of the element changes. This - * will be called after positioning, similar to `Modifier.onPositioned`. - */ -internal fun Modifier.onSizeChanged( - onSizeChanged: (IntSize) -> Unit -) = composed { - var lastSize by state { null } - onPositioned { coordinates -> - if (coordinates.size != lastSize) { - lastSize = coordinates.size - onSizeChanged(coordinates.size) - } - } -} diff --git a/sample/src/main/java/dev/chrisbanes/accompanist/sample/coil/CoilBasicSample.kt b/sample/src/main/java/dev/chrisbanes/accompanist/sample/coil/CoilBasicSample.kt index d073992e8..352d86db4 100644 --- a/sample/src/main/java/dev/chrisbanes/accompanist/sample/coil/CoilBasicSample.kt +++ b/sample/src/main/java/dev/chrisbanes/accompanist/sample/coil/CoilBasicSample.kt @@ -126,6 +126,16 @@ private fun Sample() { }, modifier = Modifier.preferredSize(128.dp) ) + + // CoilImage with an implicit size + CoilImage( + data = randomSampleImageUrl(), + loading = { + Stack(Modifier.fillMaxSize()) { + CircularProgressIndicator(Modifier.gravity(Alignment.Center)) + } + } + ) } } } From d674256968bed24c6f82247c3a29b96dc3a65070 Mon Sep 17 00:00:00 2001 From: Chris Date: Fri, 14 Aug 2020 17:39:26 +0100 Subject: [PATCH 2/3] Address feedback --- .../java/dev/chrisbanes/accompanist/coil/Coil.kt | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) 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 146ee5980..a26367c70 100644 --- a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt +++ b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt @@ -192,8 +192,8 @@ fun CoilImage( val lastRequestedSize = remember(requestActor) { MutableRef(IntSize.Zero) } val requestSize = IntSize( - width = if (constraints.hasBoundedWidth) constraints.maxWidth else Int.MAX_VALUE, - height = if (constraints.hasBoundedHeight) constraints.maxHeight else Int.MAX_VALUE + width = if (constraints.hasBoundedWidth) constraints.maxWidth else UNSPECIFIED, + height = if (constraints.hasBoundedHeight) constraints.maxHeight else UNSPECIFIED ) val r = result @@ -224,6 +224,11 @@ fun CoilImage( } } +/** + * Value for a [IntSize] dimension, where the dimension is not specified or is unknown.s + */ +private const val UNSPECIFIED = -1 + @Stable private data class MutableRef(var value: T) @@ -235,9 +240,9 @@ private fun CoilRequestActor( // If the request has a sizeResolver set, we just execute the request as-is request } - size.height == Int.MAX_VALUE || size.width == Int.MAX_VALUE -> { - // If the size contains an 'infinite' dimension, we don't specific a size in the Coil - // request + size.width == UNSPECIFIED || size.height == UNSPECIFIED -> { + // If the size contains an unspecified dimension, we don't specify a size + // in the Coil request request } size != IntSize.Zero -> { From a476e3230aec9c442acbf66f9b614a3239cbfe27 Mon Sep 17 00:00:00 2001 From: Chris Banes Date: Mon, 17 Aug 2020 08:47:06 +0100 Subject: [PATCH 3/3] Fix typo --- coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 a26367c70..961f3245f 100644 --- a/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt +++ b/coil/src/main/java/dev/chrisbanes/accompanist/coil/Coil.kt @@ -225,7 +225,7 @@ fun CoilImage( } /** - * Value for a [IntSize] dimension, where the dimension is not specified or is unknown.s + * Value for a [IntSize] dimension, where the dimension is not specified or is unknown. */ private const val UNSPECIFIED = -1