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

PicassoImage doesn't scale correctly when Modifier.aspectRatio() is used #118

Closed
alexjlockwood opened this issue Oct 13, 2020 · 2 comments · Fixed by #121
Closed

PicassoImage doesn't scale correctly when Modifier.aspectRatio() is used #118

alexjlockwood opened this issue Oct 13, 2020 · 2 comments · Fixed by #121

Comments

@alexjlockwood
Copy link
Contributor

I have a sample app that I was working on for a blog post.

In the picasso-bug branch, I use a PicassoImage like this (link to code):

    PicassoImage(
        data = imageUrl,
        modifier = modifier.aspectRatio(16f / 9f),
        fadeIn = true,
        contentScale = ContentScale.Crop,
    )

screencap-1602620231

I do not see this same behavior with CoilImage however. On the master branch, I am using this code:

    CoilImage(
        data = imageUrl,
        modifier = modifier.aspectRatio(16f / 9f),
        fadeIn = true,
        contentScale = ContentScale.Crop,
    )

screencap-1602620122

@chrisbanes
Copy link
Contributor

chrisbanes commented Oct 14, 2020

This is an interesting one.

So the issue is that I'm calling Picasso.get(...).resize(composableSize).onlyScaleDown(), which scales the bitmap to to fill the bounds (ignoring aspect ratio). This is very different to Coil's handling, which never changes the aspect ratio (much, much better imo).

Now, we two options: centerCrop() or centerInside(), and neither are perfect.

  • centerCrop() is the right solution if you're using ContentScale.Crop (it does the same thing), but it breaks the other ContentScales.
  • centerInside() maintains aspect ratio, but scales down. So images might be scaled smaller than the composable, and then scaled back up via the ContentScale which will look bad.
  • Ideally I want a centerOutside(), which will scale the image so that both dimensions are >= requested bounds (i.e. it covers the bounds), similar to Coil's Scale.FILL

I think I'll have to go with the correct solution: centerInside().

chrisbanes added a commit that referenced this issue Oct 14, 2020
* Add sample using aspect ratio + crop
* Allow Picasso to maintain aspect ratio when resizing

Closes #118
@alexjlockwood
Copy link
Contributor Author

thanks for the quick fix chris!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants