Allow to set CroppedRegion in ImageCropper #2846
Conversation
|
|
|
|
|
|
||
| set | ||
| { | ||
| _currentCroppedRect = value; |
There was a problem hiding this comment.
We need to check the value of _currentCroppedRect to ensure it is valid (related to _restrictedCropRect and MinCropSize), otherwise the saved image will be incorrect.
There was a problem hiding this comment.
In addition, we have to consider AspectRatio(KeepAspectRatio and UsedAspectRatio are used in the code).
There was a problem hiding this comment.
Good point. What do you suggest to do when the provided rect doesn't satisfy the requisites?
Thinking out loud: We could modify it so it respects the minimum size and aspect ratio, but it would be hard for the user of the control to predict what the resulting rect will be. Maybe we should just drop it when the value is not valid? But since this is a setter there will be no failure indication, unless we throw and exception which might be too aggressive.
What do you think about discarding the setter and replacing it with
bool TrySetCroppedRegion(Rect r)returning true if the rect satisfies the requirements, and false if the rect is discarded?
There was a problem hiding this comment.
I agree. It's a great idea！
| } | ||
|
|
||
| // Reject regions larger than the original picture | ||
| if (rect.Width > _restrictedCropRect.Width || rect.Height > _restrictedCropRect.Height) |
There was a problem hiding this comment.
Thanks, but don't forget to check rect.X and rect.Y.
There was a problem hiding this comment.
Oops right didn't realize they can have different offsets, fixing this line
There was a problem hiding this comment.
@arcadiogarcia did this piece get resolved properly? Is that the next section with the Left and Bottom checks? Just wanted to make sure before approving.
There was a problem hiding this comment.
@michael-hawker Yes, that new condition should reject all regions that step outside the original picture.
Issue: #2845
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The ImageCropper control doesn't allow to set the CroppedRegion
What is the new behavior?
The ImageCropper allows to set the CroppedRegion, so the app can specify a default crop region.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information