Use of listener function in AsyncImage leads to unexpected recomposition

[panpf]

Describe the bug In AsyncImage, ImageRequest can be passed as a parameter to AsyncImage. If the listener function is called when constructing ImageRequest, the twice-created ImageRequest will always return false through equals comparison, and eventually a recomposition will occur unexpectedly.

To Reproduce This problem can be reproduced with slight modifications based on your example.

// sample.compose.MainActivity.kt

@Composable
private fun ListScreen(
    gridState: LazyStaggeredGridState,
    images: List<Image>,
    onImageClick: (Image, MemoryCache.Key?) -> Unit,
) {
    val context = LocalContext.current
    val numColumns = remember(context) { numberOfColumns(context) }

    // Add 1: Reading the scroll state here can trigger the recomposition of the LazyVerticalStaggeredGrid level
    LaunchedEffect(gridState.isScrollInProgress) {
        // do something
    }

    LazyVerticalStaggeredGrid(
        columns = StaggeredGridCells.Fixed(numColumns),
        state = gridState,
        modifier = Modifier.testTag("list"),
    ) {
        items(images) { image ->
            // Scale the image to fit the width of a column.
            val size = with(LocalDensity.current) {
                image
                    .calculateScaledSize(LocalContext.current, numColumns)
                    .run { DpSize(width.toDp(), height.toDp()) }
            }

            // Intentionally not a state object to avoid recomposition.
            var placeholder: MemoryCache.Key? = null

            AsyncImage(
                model = ImageRequest.Builder(LocalContext.current)
                    .data(image.uri)
                    .parameters(image.parameters)
                    // Add 2: Disabling the memory cache here can clearly observe the recomposition and cause the image to be reloaded.
                    .memoryCachePolicy(CachePolicy.DISABLED)
                    // Add 3: The listener function is called here to make the created ImageRequest different from the previous one.
                    .listener { _, _ ->

                    }
                    .build(),
                placeholder = ColorPainter(Color(image.color)),
                error = ColorPainter(Color.Red),
                onSuccess = { placeholder = it.result.memoryCacheKey },
                contentDescription = null,
                contentScale = ContentScale.Crop,
                modifier = Modifier
                    .size(size)
                    .clickable { onImageClick(image, placeholder) },
            )
        }
    }
}

Logs/Screenshots

https://github.com/coil-kt/coil/assets/3250512/cd2e445a-987f-4673-beaf-9f8c0dc53a8f

Version coil: 2.5.0 compose: 1.5.4

As demonstrated in the above video, when the finger is pressed or raised when the sliding list is read, 'gridState.isScrollInProgress' is read, which causes the reorganization of all items. During the reorganization of the items, the two ImageRequests before and after the item reorganization cause equals to be false due to their different listeners. Eventually causing the AsyncImage to recomposition and reload the image.

I can temporarily solve this problem by using remember when creating the ImageRequest as follows:

@Composable
private fun ListScreen(
    gridState: LazyStaggeredGridState,
    images: List<Image>,
    onImageClick: (Image, MemoryCache.Key?) -> Unit,
) {
    val context = LocalContext.current
    val numColumns = remember(context) { numberOfColumns(context) }

    // Add 1: Reading the scroll state here can trigger the recomposition of the LazyVerticalStaggeredGrid level
    LaunchedEffect(gridState.isScrollInProgress) {
        // do something
    }

    LazyVerticalStaggeredGrid(
        columns = StaggeredGridCells.Fixed(numColumns),
        state = gridState,
        modifier = Modifier.testTag("list"),
    ) {
        items(images) { image ->
            // Scale the image to fit the width of a column.
            val size = with(LocalDensity.current) {
                image
                    .calculateScaledSize(LocalContext.current, numColumns)
                    .run { DpSize(width.toDp(), height.toDp()) }
            }

            // Intentionally not a state object to avoid recomposition.
            var placeholder: MemoryCache.Key? = null

            val request = remember(image.uri) {
                ImageRequest.Builder(context)
                    .data(image.uri)
                    .parameters(image.parameters)
                    // Add 2: Disabling the memory cache here can clearly observe the reorganization and cause the image to be reloaded.
                    .memoryCachePolicy(CachePolicy.DISABLED)
                    // Add 3: The listener function is called here to make the created ImageRequest different from the previous one.
                    .listener { _, _ ->

                    }
                    .build()
            }
            AsyncImage(
                model = request,
                placeholder = ColorPainter(Color(image.color)),
                error = ColorPainter(Color.Red),
                onSuccess = { placeholder = it.result.memoryCacheKey },
                contentDescription = null,
                contentScale = ContentScale.Crop,
                modifier = Modifier
                    .size(size)
                    .clickable { onImageClick(image, placeholder) },
            )
        }
    }
}

But I feel like this isn't the right way to solve this problem and I'd love to hear your thoughts!

[colinrtwhite]

Good catch! I think the best way to fix this is to add a key: String = model.hashCode().toString() param to AsyncImage, which we can use to determine whether we should launch a new request. Additionally, this also makes it more explicit (and gives more control) to the end user about when a new ImageRequest will be launched. Other composables use a similar key pattern.

[panpf]

I think the way to increase the dedicated key is not appropriate, because this will increase the complexity of AsyncImage and make it difficult to understand its behavior. For example, when using the user settings to build ImageRequest, the user settings must also be synchronized to the key to make it automatically recomposition when the user settings change.

Therefore, it makes more sense to let the compose framework decide whether to recomposition based on ImageRequest's equals equality method. listener and target can be removed in equals, because generally listener and target do not affect how to load images, and there is no relying on ImageRequest equals in the View version.

[colinrtwhite]

@panpf The key param would be optional and default to the model's hashCode so I'm not sure how it would increase the complexity of using AsyncImage. Any changes to the model would change its hashCode so it wouldn't need to be synchronized unless you set a custom value.

I don't think we should change ImageRequest's equals method to remove its listeners. As a value class I'd expect all its properties to be used in the equals/hashCode implementations.

[panpf]

I think there are two conditions that must be met for adding key:

1. The key cannot be model.hashCode().toString(), because the model may be ImageRequest, and the hashCode of ImageRequest still contains the listener. The key can only be 'uri+bitmapConfig+colorSpace+precision+...', which should be similar to the cache key. This can avoid being affected by the listener and can be automatically reorganized when bitmapConfig and other configuration changes.
2. You need to let compose ignore the model parameter when judging reorganization (when the model is ImageRequest), but there seems to be no way to do this at present.

Since point 2 cannot be satisfied, this solution may not be implemented.

I have other solution:

• Solution 1. Disable the use of the listener function in AsyncImage, and then provide the listenenr parameter by the AsyncImage function, similar to the transform parameter
• Solution 2. Create a new ImageRequest wrapper class. Its equals method ignores the listener attribute of ImageRequest. This class is only used in the AsyncImage function, as follows

@Stable
class ComposeImageRequest(val request: ImageRequest) {

    override fun equals(other: Any?): Boolean {
        if (this === other) return true
        return other is ComposeImageRequest &&
            request.context == other.request.context &&
            request.data == other.request.data
            // ... Ignore listener
    }

    override fun hashCode(): Int {
        var result = request.context.hashCode()
        result = 31 * result + request.data.hashCode()
        // Ignore listener
        return result
    }
}

fun ImageRequest.toCompose(): ComposeImageRequest = ComposeImageRequest(this)

@Composable
fun AsyncImage(
    model: Any?,
    contentDescription: String?,
    imageLoader: ImageLoader,
    modifier: Modifier = Modifier,
    transform: (State) -> State = DefaultTransform,
    onState: ((State) -> Unit)? = null,
    alignment: Alignment = Alignment.Center,
    contentScale: ContentScale = ContentScale.Fit,
    alpha: Float = DefaultAlpha,
    colorFilter: ColorFilter? = null,
    filterQuality: FilterQuality = DefaultFilterQuality,
) {
    require(model !is ImageRequest) {
        "Compose version must use ComposeImageRequest"
    }
// ...
}

[colinrtwhite]

@panpf We can't require users to wrap ImageRequest as it would break many existing call sites. Also I think wrapping ImageRequest isn't very intuitive or ergonomic. Determining the right "key" to use is also slightly more complicated as ImageRequest can have custom parameters passed through Parameters (or Extras in Coil 3.x) which can influence the key.

I'm thinking we might want to support passing some kind of ImageRequestKeyFactory to AsyncImage where the factory is (ImageRequest) -> Any. We can have our own DefaultImageRequestKeyFactory, but allow setting a custom factory if preferred.

[panpf]

@colinrtwhite ImageRequestKeyFactory cannot solve the problem, because you still have to pass ImageRequest to AsyncImage, and the Compose framework will decide whether to recomposition through the eauals method of ImageRequest.

Maybe it’s time to refactor AsyncImage. ImageRequest’s listener is only applicable to the view system. In compose, we should use AsyncImageState to support the listener, as follows:

// New AsyncImage
@Composable
fun AsyncImage(
    state: AsyncImageState,
    contentDescription: String?,
    modifier: Modifier = Modifier,
    transform: (State) -> State = DefaultTransform,
    onState: ((State) -> Unit)? = null,
    alignment: Alignment = Alignment.Center,
    contentScale: ContentScale = ContentScale.Fit,
    alpha: Float = DefaultAlpha,
    colorFilter: ColorFilter? = null,
    filterQuality: FilterQuality = DefaultFilterQuality,
) {
   state.contentScale = contentScale
   // ...
}

// Old AsyncImage
@Composable
fun AsyncImage(
    model: Any?,
    contentDescription: String?,
    imageLoader: ImageLoader,
    modifier: Modifier = Modifier,
    transform: (State) -> State = DefaultTransform,
    onState: ((State) -> Unit)? = null,
    alignment: Alignment = Alignment.Center,
    contentScale: ContentScale = ContentScale.Fit,
    alpha: Float = DefaultAlpha,
    colorFilter: ColorFilter? = null,
    filterQuality: FilterQuality = DefaultFilterQuality,
) = AsyncImage(
    state = rememberAsyncImageState(model, imageLoader),
    contentDescription = contentDescription,
    modifier = modifier,
    transform = transformOf(placeholder, error, fallback),
    onState = onStateOf(onLoading, onSuccess, onError),
    alignment = alignment,
    contentScale = contentScale,
    alpha = alpha,
    colorFilter = colorFilter,
    filterQuality = filterQuality
)

@Composable
fun rememberAsyncImageState(model: Any?, imageLoader: ImageLoader): AsyncImageState {
    require(request.listener == null) {
        "listener is not supported, please use AsyncImageState.loadState instead"
    }
    require(request.target == null) {
        "target is not supported"
    }
    return remember(imageLoader, model) {
        AsyncImageState(imageLoader, model)
    }
}

@Stable
class AsyncImageState internal constructor(
    val imageLoader: ImageLoader,
    val model: Any?,
) : RememberObserver {

    val loadState: LoadState? by mutableStateOf(null)

    // Execute ImageRequest in AsyncImageState, AsyncImagePainter draws the image by reading loadState

   fun restart() {
      // When loading fails, the user can click the button and call the restart method to reload the image.
   }

   sealed interface LoadState {
        data object Started : LoadState
        data class Success(val result: DisplayResult.Success) : LoadState
        data class Error(val result: DisplayResult.Error) : LoadState
        data object Canceled : LoadState
    }
}

The new architecture adopts the style of standard Compose components, which is not only compatible with the old API to the greatest extent, but also solves the problem of being unable to re-execute the request when the request fails through AsyncImageState.restart()

[colinrtwhite]

Ah sorry - I misunderstood. You're right that adding a key factory param to AsyncImage won't fix AsyncImage being unskippable due to the imageLoader and model params. I was thinking about ways to fix how changing listener also launches a new image request (which is also an issue).

To make AsyncImage skippable we can mark ImageLoader as stable using something similar to this or maybe the new stability config file once it's supported for libraries.

Making model stable is tougher since it's Any?. I'm not sure but it might be best to add overloads for built in supported params that are stable (string, uri, etc.) and show a light lint warning if a user uses the Any? overload.

[colinrtwhite]

Took a stab at a similar solution to what you suggested here. It's currently targeted to the 3.x branch, but might make sense to backport to the 2.x branch after it's tested in the 3.x alphas.

[panpf]

My implementation is ready, it's here ( https://github.com/panpf/sketch/blob/main/sketch-compose-core/src/main/kotlin/com/github/panpf/sketch/compose/AsyncImage.kt )

There are several important changes:

1. Use the https://github.com/skydoves/compose-stable-marker library to mark DisplayRequest (coil-like ImageRequest) and Sketch (coil-like ImageLoader) as stable, and they are confirmed to be valid by the compose compiler report ( https://github.com/panpf/sketch/blob/83a1e074d564498775947656c9fbfeef1396a0ab/sketch-core/src/main/kotlin/com/github/panpf/sketch/request/DisplayRequest.kt#L76 )
2. It is prohibited to use the listener and target attributes of request to completely avoid misuse ( https://github.com/panpf/sketch/blob/83a1e074d564498775947656c9fbfeef1396a0ab/sketch-compose-core/src/main/kotlin/com/github/panpf/sketch/compose/AsyncImageState.kt#L178 )
3. Move the state and painter properties from AsyncImagePainter to AsyncImageState, and AsyncImageState provides state and painter externally and replaces listener and target ( https://github.com/panpf/sketch/blob/83a1e074d564498775947656c9fbfeef1396a0ab/sketch-compose-core/src/main/kotlin/com/github/panpf/sketch/compose/AsyncImageState.kt#L117 )

[colinrtwhite]

Going to close this out as it should be fixed in 2.6.0 with EqualityDelegate.