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

clear mRestoreCropWindowRect and mRestoreDegreesRotated when using setImageBitmap #69

Merged
merged 3 commits into from
Feb 27, 2021

Conversation

chriscoomber
Copy link
Contributor

#68

Template 2 [Bug]

Bug

Cause:

mRestoreCropWindowRect and mRestoreDegreesRotated weren't being cleared in all cases where a new image was set. They lingered on and when an image is next added they overwrote the crop rect with an old state one, which may have been related to a totally different image with different dimensions.

Reproduce

See linked issue

How the bug was solved:

Clear out these member variables a) When setting a new image, and b) When they are used to restore state. (Some attempt at both of these points had already been made - just not enough.)

Copy link
Member

@Canato Canato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for helping with this PR!

Would like to ask you to edit the CHANGELOG.md file
from this ## [unreleased 2.2.0] - to

## [unreleased 2.1.1] -

### Fixed
- CropImageView incorrectly restored on rotation[#68](https://github.com/CanHub/Android-Image-Cropper/issues/68)

And
(This is totally optional and don't need to do if you don't want or don't have the time)
Would be amazing to add this test case in our sample app!

We have the CropImageViewFragment class that already implement a CropImageView but didn't use the method setImageBitmap.

Would be very helpful to avoid breaking this again in the future, mainly when we change from java to kt!
Anyway, this is totally optional. Thanks for the help so far =D

Comment on lines 1196 to 1198
clearImageInt();
mRestoreCropWindowRect = null;
mRestoreDegreesRotated = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

What you think we add this both setting inside the clearImageInt() method?
Doing this, we can remove

      mRestoreCropWindowRect = null;
      mRestoreDegreesRotated = 0;

from method setImageUriAsync(uri)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, makes sense. I applied that suggestion.

I don't have time to look at the sample app, sorry.

These both need to be cleared when setting a new image via
setImageBitmap, just like how they are cleared via setImageUriAsync.

Also, the degrees need to be cleared immediately after they are used to
restore the degrees. At the moment they are left there, lurking. I don't
think this one is a bug since they are only used when
mRestoreCropWindowRect is not null (which is also odd?) but it's still
a code smell.
@chriscoomber chriscoomber requested a review from Canato February 26, 2021 23:40
Copy link
Member

@Canato Canato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Thanks for the lib improvements

@Canato Canato merged commit 2f74953 into CanHub:main Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants