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.
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
Use of listener function in AsyncImage leads to unexpected recomposition #1940
Comments
Good catch! I think the best way to fix this is to add a |
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. |
@panpf The I don't think we should change |
I think there are two conditions that must be met for adding key:
Since point 2 cannot be satisfied, this solution may not be implemented. I have other solution:
@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"
}
// ...
} |
@panpf We can't require users to wrap I'm thinking we might want to support passing some kind of |
@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() |
Ah sorry - I misunderstood. You're right that adding a key factory param to To make Making |
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. |
Going to close this out as it should be fixed in |
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.
Logs/Screenshots
Screen_recording_20231205_175255.mp4
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:
But I feel like this isn't the right way to solve this problem and I'd love to hear your thoughts!
The text was updated successfully, but these errors were encountered: