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

Add option to disable translation of the crop window #79

Merged
merged 5 commits into from
Mar 4, 2021

Conversation

chriscoomber
Copy link
Contributor

close #78

Template 1 [New Feature]

Description:

Added new option to disable translating of the crop window by dragging the center.

Most of the changes here were boilerplate changes. I used the multiTouch option as a template to make sure I added the option to all the places it was required, such as XML, CropImageView and ActivityBuilder. I also added the toggle to the sample app.

The functional change is that, when this toggle is disabled, dragging from the center of the crop window does nothing.

Check list for the Code Reviewer:

  • CHANGELOG
  • README
  • Wiki
  • Version Number

@chriscoomber chriscoomber requested a review from a team as a code owner March 3, 2021 23:27
@chriscoomber
Copy link
Contributor Author

This conflicts very minorly with #76, so I suggest we merge than one first and then I'll fix up the conflicts.

@chriscoomber
Copy link
Contributor Author

Meta point: I realize that these features might not be popular - they're just something I need that I might as well share. I definitely want to use this library rather than rolling my own as it is high quality. This is the last feature that I need to add. Feel free to push back against this if you don't think this feature should be added - it's no problem for me to fork the repo.

@Canato
Copy link
Member

Canato commented Mar 4, 2021

@chriscoomber

Meta point: I realize that these features might not be popular - they're just something I need that I might as well share. I definitely want to use this library rather than rolling my own as it is high quality. This is the last feature that I need to add. Feel free to push back against this if you don't think this feature should be added - it's no problem for me to fork the repo.

I believe it is not a problem, since it can be useful for more people.

We just need to be sure that the default value keeps the expected behaviour.

Because of the possible misinterpretation of ImageTranslation how about we name the method lockCenter or disableCropCenterMove?

@chriscoomber
Copy link
Contributor Author

Because of the possible misinterpretation of ImageTranslation how about we name the method lockCenter or disableCropCenterMove?

I've renamed it centerMoveEnabled, and the XML attribute is cropCenterMoveEnabled.

@Canato Canato self-requested a review March 4, 2021 13:08
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.

Looking good!

Let me know, when is over.

Please don't forget to add setCenterMoveEnabled() into Sample/CameraFragment on the methods startCameraWithoutUri and startCameraWithUri

@chriscoomber
Copy link
Contributor Author

@Canato OK I think this is done. I can't see anything else that needs changing.

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 work!

@Canato Canato merged commit 245071e into CanHub:main Mar 4, 2021
@Canato Canato mentioned this pull request Mar 4, 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