Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use rememberCoroutineScope in AsyncImagePainter. #2220

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use rememberCoroutineScope in AsyncImagePainter. #2220

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4aef77c
Use rememberCoroutineScope in AsyncImagePainter.
colinrtwhite Apr 25, 2024
c9baeeb
Don't force Dispatchers.Main.immediate.
colinrtwhite Apr 25, 2024
80f46a5
Spotless.
colinrtwhite Apr 25, 2024
5791917
Use main immediate.
colinrtwhite Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 11 additions & 12 deletions coil-compose-core/src/commonMain/kotlin/coil3/compose/AsyncImagePainter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableFloatStateOf
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.rememberCoroutineScope
import androidx.compose.runtime.setValue
import androidx.compose.runtime.snapshotFlow
import androidx.compose.ui.geometry.Size
Expand Down Expand Up @@ -49,8 +50,8 @@ import kotlin.math.roundToInt
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.mapLatest
Expand Down Expand Up @@ -164,6 +165,7 @@ private fun rememberAsyncImagePainter(
validateRequest(request)

val painter = remember { AsyncImagePainter(request, state.imageLoader) }
painter.scope = rememberCoroutineScope { SupervisorJob() + Dispatchers.Main.immediate }
painter.transform = transform
painter.onState = onState
painter.contentScale = contentScale
Expand All @@ -183,8 +185,6 @@ class AsyncImagePainter internal constructor(
request: ImageRequest,
imageLoader: ImageLoader,
) : Painter(), RememberObserver {

private var rememberScope: CoroutineScope? = null
private val drawSize = MutableStateFlow(Size.Zero)

private var painter: Painter? by mutableStateOf(null)
Expand All @@ -204,6 +204,9 @@ class AsyncImagePainter internal constructor(
painter = value
}

internal lateinit var scope: CoroutineScope
private var rememberJob: Job? = null

internal var transform = DefaultTransform
internal var onState: ((State) -> Unit)? = null
internal var contentScale = ContentScale.Fit
Expand Down Expand Up @@ -246,11 +249,7 @@ class AsyncImagePainter internal constructor(
@OptIn(ExperimentalCoroutinesApi::class)
override fun onRemembered() {
// Short circuit if we're already remembered.
if (rememberScope != null) return

// Create a new scope to observe state and execute requests while we're remembered.
val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main.immediate)
rememberScope = scope
if (rememberJob != null) return
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be safer to use if(rememberJob?.isActive == true) return ?
Maybe the coroutine won't ever be cancelled without calling clear() though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep that's probably better 👍. Theoretically Compose could cancel the parent scope at any time.


// Manually notify the child painter that we're remembered.
(_painter as? RememberObserver)?.onRemembered()
Expand All @@ -264,7 +263,7 @@ class AsyncImagePainter internal constructor(
}

// Observe the current request and execute any emissions.
scope.launch {
rememberJob = scope.launch {
snapshotFlow { request }
.mapLatest { imageLoader.execute(updateRequest(it)).toState() }
.collect(::updateState)
Expand All @@ -282,8 +281,8 @@ class AsyncImagePainter internal constructor(
}

private fun clear() {
rememberScope?.cancel()
rememberScope = null
rememberJob?.cancel()
rememberJob = null
}

/** Update the [request] to work with [AsyncImagePainter]. */
Expand Down Expand Up @@ -319,7 +318,7 @@ class AsyncImagePainter internal constructor(
_painter = maybeNewCrossfadePainter(previous, current, contentScale) ?: current.painter

// Manually forget and remember the old/new painters if we're already remembered.
if (rememberScope != null && previous.painter !== current.painter) {
if (rememberJob != null && previous.painter !== current.painter) {
(previous.painter as? RememberObserver)?.onForgotten()
(current.painter as? RememberObserver)?.onRemembered()
}
Expand Down