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.

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

rememberAsyncImagePainter doesn't render correctly with Roborazzi/Paparazzi #1910

Open
colinrtwhite opened this issue Oct 30, 2023 · 4 comments
Labels
help wanted Issues that are up for grabs + are good candidates for community PRs

Comments

@colinrtwhite
Copy link
Member

colinrtwhite commented Oct 30, 2023

Describe the bug
rememberAsyncImagePainter doesn't render properly when used with Roborazzi and Paparazzi. The issue appears to be related to the fact that it it needs to wait for a valid drawSize, which is set in DrawScope.onDraw immediately before the painter is then drawn. AsyncImage avoids this timing issue by using ConstraintsSizeResolver to listen to its own composable constraints, which are set before the draw stage.

Interestingly, Paparazzi and Roborazzi both render rememberAsyncImagePainter incorrectly, but differently. When intrinsicContentSize is set to a fixed value this fixes the issue on Roborazzi, but not Paparazzi. It's possible there are different root causes, but this needs to be debugged further.

Work-around
It's possible to work-around this issue by setting any custom size:

rememberAsyncImagePainter(
    model = ImageRequest.Builder(LocalContext.current)
        .data("https://www.example.com/image.jpg")
        .size(Size.ORIGINAL)
        .build(),
    contentDescription = null,
)

To Reproduce
See RoborazziComposeTest.rememberAsyncImagePainter and PaparazziTest.rememberAsyncImagePainter in this repository.

Version
Reproduced with Coil 2.4.0, Roborazzi 1.6.0, and Paparazzi 1.3.1.

Related: cashapp/paparazzi#1123

@takahirom
Copy link

takahirom commented Nov 1, 2023

I'm not sure about the issue with Roborazzi. Does "to specify an immediate size" mean we shouldn't use ".size(Size.ORIGINAL)"? Could you let me know if this is correct?

Expected Actual
coil test RoborazziComposeTest rememberAsyncImagePainter coil test RoborazziComposeTest rememberAsyncImagePainter

@colinrtwhite
Copy link
Member Author

colinrtwhite commented Nov 9, 2023

@takahirom Yep, the expected + actual are what I'm seeing too. Part of the reason AsyncImage works, but rememberAsyncImagePainter doesn't is rememberAsyncImagePainter has to grab the size for the image request here during onDraw. This occurs after intrinsicSize is called, so intrinsicSize is initially Size.Unspecified. This gets updated to the painter's size after onDraw which seems to be OK on-device but is too late for the Roborazzi capture.

Paparazzi is doing something differently as it doesn't render the image at all - even if intrinsicSize is hardcoded to an exact pixel size.

Ideally size(Size.ORIGINAL) isn't necessary - it's a work-around that skips waiting for the canvas size in onDraw.

@colinrtwhite colinrtwhite added the help wanted Issues that are up for grabs + are good candidates for community PRs label Nov 16, 2023
@kartik-prakash
Copy link

kartik-prakash commented Jan 22, 2024

Any updates here? This has become a huge blocker for us. The workaround mentioned to use Size.ORIGINAL doesn't seem to work for us.

@colinrtwhite
Copy link
Member Author

@kartik-prakash I haven't had a chance to take a look at this further as I'm focused on the 3.x work. If you're able to track down the issue locally, I'd accept a PR that patches the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Issues that are up for grabs + are good candidates for community PRs
Projects
None yet
Development

No branches or pull requests

3 participants