Simplify DeferredDispatch implementation and improve tests.

coil-kt · Jan 24, 2025 · 1b57d79 · 1b57d79
1 parent 63e7989
commit 1b57d79
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 82 deletions.
diff --git a/coil-compose-core/api/coil-compose-core.klib.api b/coil-compose-core/api/coil-compose-core.klib.api
@@ -184,8 +184,6 @@ final val coil3.compose.internal/coil3_compose_internal_AbstractContentPainterNo
 final val coil3.compose.internal/coil3_compose_internal_AsyncImageState$stableprop // coil3.compose.internal/coil3_compose_internal_AsyncImageState$stableprop|#static{}coil3_compose_internal_AsyncImageState$stableprop[0]
 final val coil3.compose.internal/coil3_compose_internal_ContentPainterElement$stableprop // coil3.compose.internal/coil3_compose_internal_ContentPainterElement$stableprop|#static{}coil3_compose_internal_ContentPainterElement$stableprop[0]
 final val coil3.compose.internal/coil3_compose_internal_ContentPainterNode$stableprop // coil3.compose.internal/coil3_compose_internal_ContentPainterNode$stableprop|#static{}coil3_compose_internal_ContentPainterNode$stableprop[0]
-final val coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineContext$stableprop // coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineContext$stableprop|#static{}coil3_compose_internal_DeferredDispatchCoroutineContext$stableprop[0]
-final val coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineDispatcher$stableprop // coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineDispatcher$stableprop|#static{}coil3_compose_internal_DeferredDispatchCoroutineDispatcher$stableprop[0]
 final val coil3.compose.internal/coil3_compose_internal_ForwardingCoroutineContext$stableprop // coil3.compose.internal/coil3_compose_internal_ForwardingCoroutineContext$stableprop|#static{}coil3_compose_internal_ForwardingCoroutineContext$stableprop[0]
 final val coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterElement$stableprop // coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterElement$stableprop|#static{}coil3_compose_internal_SubcomposeContentPainterElement$stableprop[0]
 final val coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterNode$stableprop // coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterNode$stableprop|#static{}coil3_compose_internal_SubcomposeContentPainterNode$stableprop[0]
@@ -211,8 +209,6 @@ final fun coil3.compose.internal/coil3_compose_internal_AbstractContentPainterNo
 final fun coil3.compose.internal/coil3_compose_internal_AsyncImageState$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_AsyncImageState$stableprop_getter|coil3_compose_internal_AsyncImageState$stableprop_getter(){}[0]
 final fun coil3.compose.internal/coil3_compose_internal_ContentPainterElement$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_ContentPainterElement$stableprop_getter|coil3_compose_internal_ContentPainterElement$stableprop_getter(){}[0]
 final fun coil3.compose.internal/coil3_compose_internal_ContentPainterNode$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_ContentPainterNode$stableprop_getter|coil3_compose_internal_ContentPainterNode$stableprop_getter(){}[0]
-final fun coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineContext$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineContext$stableprop_getter|coil3_compose_internal_DeferredDispatchCoroutineContext$stableprop_getter(){}[0]
-final fun coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineDispatcher$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_DeferredDispatchCoroutineDispatcher$stableprop_getter|coil3_compose_internal_DeferredDispatchCoroutineDispatcher$stableprop_getter(){}[0]
 final fun coil3.compose.internal/coil3_compose_internal_ForwardingCoroutineContext$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_ForwardingCoroutineContext$stableprop_getter|coil3_compose_internal_ForwardingCoroutineContext$stableprop_getter(){}[0]
 final fun coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterElement$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterElement$stableprop_getter|coil3_compose_internal_SubcomposeContentPainterElement$stableprop_getter(){}[0]
 final fun coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterNode$stableprop_getter(): kotlin/Int // coil3.compose.internal/coil3_compose_internal_SubcomposeContentPainterNode$stableprop_getter|coil3_compose_internal_SubcomposeContentPainterNode$stableprop_getter(){}[0]

diff --git a/coil-compose-core/src/commonMain/kotlin/coil3/compose/AsyncImagePainter.kt b/coil-compose-core/src/commonMain/kotlin/coil3/compose/AsyncImagePainter.kt
@@ -26,8 +26,7 @@ import coil3.compose.AsyncImagePainter.Companion.DefaultTransform
 import coil3.compose.AsyncImagePainter.Input
 import coil3.compose.AsyncImagePainter.State
 import coil3.compose.internal.AsyncImageState
-import coil3.compose.internal.DeferredDispatchCoroutineScope
-import coil3.compose.internal.launchUndispatched
+import coil3.compose.internal.launchWithDeferredDispatch
 import coil3.compose.internal.onStateOf
 import coil3.compose.internal.previewHandler
 import coil3.compose.internal.requestOf
@@ -148,10 +147,6 @@ private fun rememberAsyncImagePainter(
 class AsyncImagePainter internal constructor(
     input: Input,
 ) : Painter(), RememberObserver {
-    private val drawSize = MutableSharedFlow<Size>(
-        replay = 1,
-        onBufferOverflow = DROP_OLDEST,
-    )
     private var painter: Painter? by mutableStateOf(null)
     private var alpha: Float = DefaultAlpha
     private var colorFilter: ColorFilter? = null
@@ -162,6 +157,11 @@ class AsyncImagePainter internal constructor(
             field = value
         }
 
+    private val drawSize = MutableSharedFlow<Size>(
+        replay = 1,
+        onBufferOverflow = DROP_OLDEST,
+    )
+
     internal lateinit var scope: CoroutineScope
     internal var transform = DefaultTransform
     internal var onState: ((State) -> Unit)? = null
@@ -186,8 +186,8 @@ class AsyncImagePainter internal constructor(
     val input: StateFlow<Input> = inputFlow.asStateFlow()
 
     /** The latest [AsyncImagePainter.State]. */
-    private val _state: MutableStateFlow<State> = MutableStateFlow(State.Empty)
-    val state: StateFlow<State> = _state.asStateFlow()
+    private val stateFlow: MutableStateFlow<State> = MutableStateFlow(State.Empty)
+    val state: StateFlow<State> = stateFlow.asStateFlow()
 
     override val intrinsicSize: Size
         get() = painter?.intrinsicSize ?: Size.Unspecified
@@ -218,8 +218,8 @@ class AsyncImagePainter internal constructor(
 
     private fun launchJob() {
         val input = _input ?: return
-        // Observe the latest request and execute any emissions.
-        rememberJob = DeferredDispatchCoroutineScope(scope.coroutineContext).launchUndispatched {
+
+        rememberJob = scope.launchWithDeferredDispatch {
             val previewHandler = previewHandler
             val state = if (previewHandler != null) {
                 // If we're in inspection mode use the preview renderer.
@@ -296,9 +296,9 @@ class AsyncImagePainter internal constructor(
     }
 
     private fun updateState(state: State) {
-        val previous = _state.value
+        val previous = stateFlow.value
         val current = transform(state)
-        _state.value = current
+        stateFlow.value = current
         painter = maybeNewCrossfadePainter(previous, current, contentScale) ?: current.painter
 
         // Manually forget and remember the old/new painters.

diff --git a/coil-compose-core/src/commonMain/kotlin/coil3/compose/internal/DeferredDispatch.kt b/coil-compose-core/src/commonMain/kotlin/coil3/compose/internal/DeferredDispatch.kt
@@ -1,5 +1,3 @@
-@file:Suppress("NOTHING_TO_INLINE")
-
 package coil3.compose.internal
 
 import kotlin.coroutines.CoroutineContext
@@ -9,27 +7,29 @@ import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.CoroutineStart
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.InternalCoroutinesApi
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.Runnable
 import kotlinx.coroutines.launch
 
 /**
- * A [CoroutineScope] that does not dispatch until the [CoroutineDispatcher] in its
- * [CoroutineContext] changes.
+ * Launch [block] and defer dispatching until the context's [CoroutineDispatcher] changes.
  */
-internal fun DeferredDispatchCoroutineScope(
-    context: CoroutineContext,
-): CoroutineScope {
-    val originalDispatcher = context.dispatcher ?: Dispatchers.Unconfined
-    return CoroutineScope(DeferredDispatchCoroutineContext(context, originalDispatcher))
-}
+internal fun CoroutineScope.launchWithDeferredDispatch(
+    block: suspend CoroutineScope.() -> Unit,
+): Job = CoroutineScope(DeferredDispatchCoroutineContext(coroutineContext)).launch(
+    context = DeferredDispatchCoroutineDispatcher(
+        delegate = coroutineContext.dispatcher ?: Dispatchers.Unconfined,
+    ),
+    start = CoroutineStart.UNDISPATCHED,
+    block = block,
+)
 
 /**
  * A special [CoroutineContext] implementation that automatically enables
  * [DeferredDispatchCoroutineDispatcher] dispatching if the context's [CoroutineDispatcher] changes.
  */
-internal class DeferredDispatchCoroutineContext(
+private class DeferredDispatchCoroutineContext(
     context: CoroutineContext,
-    val originalDispatcher: CoroutineDispatcher,
 ) : ForwardingCoroutineContext(context) {
 
     override fun newContext(
@@ -43,20 +43,15 @@ internal class DeferredDispatchCoroutineContext(
                 (newDispatcher == null || newDispatcher == Dispatchers.Unconfined)
         }
 
-        return DeferredDispatchCoroutineContext(new, originalDispatcher)
+        return DeferredDispatchCoroutineContext(new)
     }
 }
 
-/** Launch [block] without dispatching. */
-internal inline fun CoroutineScope.launchUndispatched(
-    noinline block: suspend CoroutineScope.() -> Unit,
-) = launch(DeferredDispatchCoroutineDispatcher(Dispatchers.Unconfined), CoroutineStart.UNDISPATCHED, block)
-
 /**
  * A [CoroutineDispatcher] that delegates to [Dispatchers.Unconfined] while [unconfined] is true
  * and [delegate] when [unconfined] is false.
  */
-internal class DeferredDispatchCoroutineDispatcher(
+private class DeferredDispatchCoroutineDispatcher(
     private val delegate: CoroutineDispatcher,
 ) : CoroutineDispatcher() {
     private val _unconfined = atomic(true)

diff --git a/coil-compose-core/src/commonTest/kotlin/coil3/compose/internal/DeferredDispatchTest.kt b/coil-compose-core/src/commonTest/kotlin/coil3/compose/internal/DeferredDispatchTest.kt
@@ -1,7 +1,6 @@
 package coil3.compose.internal
 
 import coil3.ImageLoader
-import coil3.compose.AsyncImagePainter
 import coil3.decode.DataSource
 import coil3.decode.DecodeResult
 import coil3.decode.Decoder
@@ -26,73 +25,81 @@ import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.Runnable
 import kotlinx.coroutines.delay
-import kotlinx.coroutines.launch
 import kotlinx.coroutines.test.runTest
 import kotlinx.coroutines.withContext
 import okio.Buffer
 
 class DeferredDispatchTest : RobolectricTest() {
     private val testDispatcher = TestCoroutineDispatcher()
-    private val deferredDispatcher = DeferredDispatchCoroutineDispatcher(testDispatcher)
 
     @Test
-    fun `does not dispatch when unconfined=true`() = runTest {
-        deferredDispatcher.unconfined = true
+    fun `does not dispatch if dispatcher does not change`() = runTest {
+        withContext(testDispatcher) {
+            launchWithDeferredDispatch {
+                assertEquals(1, testDispatcher.dispatchCount)
 
-        withContext(deferredDispatcher) {
-            delay(100.milliseconds)
-            assertEquals(0, testDispatcher.dispatchCount)
+                delay(10.milliseconds)
+                withContext(EmptyCoroutineContext) {}
+
+                assertEquals(1, testDispatcher.dispatchCount)
+            }.join()
         }
     }
 
     @Test
-    fun `does dispatch when unconfined=false`() = runTest {
-        deferredDispatcher.unconfined = false
+    fun `does dispatch if dispatcher changes`() = runTest {
+        withContext(testDispatcher) {
+            launchWithDeferredDispatch {
+                assertEquals(1, testDispatcher.dispatchCount)
+
+                delay(10.milliseconds)
+                withContext(Dispatchers.Default) {}
 
-        withContext(deferredDispatcher) {
-            delay(100.milliseconds)
-            assertEquals(2, testDispatcher.dispatchCount)
+                assertEquals(2, testDispatcher.dispatchCount)
+            }.join()
         }
     }
 
-    /** This test emulates the context that [AsyncImagePainter] launches its request into. */
     @Test
-    fun `imageLoader does not dispatch if context does not change`() = runTest {
-        deferredDispatcher.unconfined = true
-
-        DeferredDispatchCoroutineScope(coroutineContext + deferredDispatcher).launch {
-            val imageLoader = ImageLoader(context)
-            val request = ImageRequest.Builder(context)
-                .data(Unit)
-                .fetcherFactory(TestFetcher.Factory())
-                .decoderFactory(TestDecoder.Factory())
-                .coroutineContext(EmptyCoroutineContext)
-                .build()
-            val result = imageLoader.execute(request)
-
-            assertIs<SuccessResult>(result)
-            assertEquals(0, testDispatcher.dispatchCount)
-        }.join()
+    fun `image loader does not dispatch if dispatcher does not change`() = runTest {
+        withContext(testDispatcher) {
+            launchWithDeferredDispatch {
+                assertEquals(1, testDispatcher.dispatchCount)
+
+                val imageLoader = ImageLoader(context)
+                val request = ImageRequest.Builder(context)
+                    .data(Unit)
+                    .fetcherFactory(TestFetcher.Factory())
+                    .decoderFactory(TestDecoder.Factory())
+                    .coroutineContext(EmptyCoroutineContext)
+                    .build()
+                val result = imageLoader.execute(request)
+
+                assertIs<SuccessResult>(result)
+                assertEquals(1, testDispatcher.dispatchCount)
+            }.join()
+        }
     }
 
-    /** This test emulates the context that [AsyncImagePainter] launches its request into. */
     @Test
-    fun `imageLoader does dispatch if context changes`() = runTest {
-        deferredDispatcher.unconfined = true
-
-        DeferredDispatchCoroutineScope(coroutineContext + deferredDispatcher).launch {
-            val imageLoader = ImageLoader(context)
-            val request = ImageRequest.Builder(context)
-                .data(Unit)
-                .fetcherFactory(TestFetcher.Factory())
-                .decoderFactory(TestDecoder.Factory())
-                .decoderCoroutineContext(Dispatchers.Default)
-                .build()
-            val result = imageLoader.execute(request)
-
-            assertIs<SuccessResult>(result)
-            assertEquals(1, testDispatcher.dispatchCount)
-        }.join()
+    fun `image loader does dispatch if dispatcher changes`() = runTest {
+        withContext(testDispatcher) {
+            launchWithDeferredDispatch {
+                assertEquals(1, testDispatcher.dispatchCount)
+
+                val imageLoader = ImageLoader(context)
+                val request = ImageRequest.Builder(context)
+                    .data(Unit)
+                    .fetcherFactory(TestFetcher.Factory())
+                    .decoderFactory(TestDecoder.Factory())
+                    .decoderCoroutineContext(Dispatchers.Default)
+                    .build()
+                val result = imageLoader.execute(request)
+
+                assertIs<SuccessResult>(result)
+                assertEquals(2, testDispatcher.dispatchCount)
+            }.join()
+        }
     }
 
     private class TestCoroutineDispatcher : CoroutineDispatcher() {