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

[BUG] - ScaleType Problem #220

Closed
PiotrBandurski opened this issue Sep 26, 2021 · 10 comments · Fixed by #322
Closed

[BUG] - ScaleType Problem #220

PiotrBandurski opened this issue Sep 26, 2021 · 10 comments · Fixed by #322

Comments

@PiotrBandurski
Copy link

🚨🚨 Before open the ticket please check if you can reproduce the bug in the sample code

  • Lib Version 3.3.5

Describe the bug
I think that scale type is not working (Or I'm doing something wrong). When I have CropImageView with ratio for example 3x5 and I want to crop square image and im setting ScaleType to CENTER_CROP I would expect that preview of image will be filled (like in android ImageView).

To Reproduce
Steps to reproduce the behavior:

  1. In extended_activity.xml add app:cropScaleType="centerCrop" to <com.canhub.cropper.CropImageView
  2. Run sample app
  3. click "SAMPLE OF CUSTOM ACTIVITY"
  4. Pick/take square photo
  5. Go to crop screen
  6. CropImageView is not filled

Expected behavior
Image should fill whole CropImageView

Media

Smartphone (please complete the following information):

  • Device: xiaomi mi a 2 lite
  • OS: Android 9

Additional context
But maybe Im doing something wrong :D

@Canato
Copy link
Member

Canato commented Sep 27, 2021

Hey @PiotrBandurski Welcome!

And much thanks for using the template, helps a lot!

The issue here is because our CropImageView is a FrameLayout that contains a ImageView

So in the code if we do imageView.scaleType = ImageView.ScaleType.CENTER_CROP it works!
But for be able to do from the xml we need to create a BindAdapter!
Is something small that need some testing, I can do by the end of the week, but if you wanna open a PR for it, please fell free.

Will add some tags in case someone else wanna help

@PiotrBandurski
Copy link
Author

No problem, my pleasure :)

When I set
imageView.scaleType = ImageView.ScaleType.CENTER_CROP, it fills image view but I'm not able to exceed crop borders (darker image on screenshoot) I can create pull request with it but can you point me where to fix issue with crop borders?

@Canato
Copy link
Member

Canato commented Sep 27, 2021

Strange behaviour, will dig it a little @@

@PiotrBandurski
Copy link
Author

I tried to fix CropOverlayView but it is too complex for me

@Canato
Copy link
Member

Canato commented Sep 29, 2021

Will try to check over weekend, this is annoying @@

@Canato
Copy link
Member

Canato commented Sep 30, 2021

@PiotrBandurski this is a bug that we have in other places that I will need top fix anyway =|

Can open the PR and I test it, will try to fix the bug on the weekend

@Marchuck
Copy link

Marchuck commented Jan 5, 2022

Hello folks 👋
I faced similar issue with centerCrop when I was migrating from legacy to this new library.
When I compared old Java code to the new Kotlin one, I spotted there's tiny change in applyImageMatrix source

there's no preceding if for scaleType like it was before, so current part:

mImageMatrix.mapRect(cropRect)
if (center) {
    // set the zoomed area to be as to the center of cropping window as possible
    mZoomOffsetX = ...

should be

mImageMatrix.mapRect(cropRect)
if (mScaleType == ScaleType.CENTER_CROP && center && !animate) {
    mZoomOffsetX = 0f
    mZoomOffsetY = 0f
} else if (center) {
    // set the zoomed area to be as to the center of cropping window as possible
    mZoomOffsetX = ...

hope it helped 🤞

@Canato
Copy link
Member

Canato commented Jan 18, 2022

@Marchuck could you please open a PR with the fix? Thanks!

@scana
Copy link
Contributor

scana commented Mar 3, 2022

@Canato @Marchuck this is actually not the only part of code that is missing.

The thing is that together with @Marchuck we were using forked version of the old library and difference mentioned by him is actually a fix that we have implemented on our own (and never shared 🤦)

I'll open a PR for it.

@Canato
Copy link
Member

Canato commented Mar 3, 2022

Thanks @scana =D

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.

4 participants