Fix the bug of wrong aspect ratio of image cropper in some cases #4482
Conversation
…ing of updating the layout of the controls
|
Thanks HHChaos for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
requested review from
HHChaos
changed the title
michael-hawker
added
bug 🐛
|
Thanks @HHChaos! Glad you were able to find a fix. I added an issue for tracking as well. @jhariel8 want to try it out in your scenario to make sure it's working? You can find out how to use preview packages from https://aka.ms/wct/wiki/previewpackages |
|
@michael-hawker @HHChaos This fix worked perfectly for me! Thanks so much! |
There was a problem hiding this comment.
Thanks for validating @jhariel8! @Arlodotexe want to review sometime and then we merge this in?
| @@ -23,6 +23,12 @@ public partial class ImageCropper | |||
| /// <param name="animate">Whether animation is enabled.</param> | |||
| private void InitImageLayout(bool animate = false) | |||
| { | |||
| if (!IsValidRect(CanvasRect)) | |||
| { | |||
| _lazyInitImageLayoutAction = () => InitImageLayout(animate); | |||
There was a problem hiding this comment.
Something about this feels off...
@HHChaos were you able to pinpoint exactly what caused the original issue? Could we get some additional insight into the cause and the fix?
There was a problem hiding this comment.
I just noticed we don't do any gate on the control to be loaded. Maybe that'd be the better fix is waiting for layout to complete?
There was a problem hiding this comment.
@Arlodotexe The source of this bug is when ImageCropper is in the layout arranging phase, it may take more time if the available sizes are infinite, and because CanvasRect is not a valid size before the layout is completed, this bug may occur if the InitImageLayout method is called at this time.
@michael-hawker I have tested it, when loaded occurs, CanvasRect may still be an invalid value.
There was a problem hiding this comment.
This fix feels like it's patching over a slightly deeper issue.
Looking at the code, it appears that
- We're relying on the size of the
PART_ImageCanvasthat contains the image, rather than the image itself. - We're calling
InitImageLayoutwhen the source has changed, but not when the image is loaded.
If the above ends up being the cause of other issues down the road, we don't want to be band-aid patching them. When underlying issues get fixed that have been partially patched over, we need to clean up all those patches, and there are no notes or comments to tell us what code should be included in the cleanup.
To save time and headache later, I think we should look at fixing this by fixing the underlying problem.
There was a problem hiding this comment.
Looks like the source of the problem is that calling await imgCropper.LoadImageFromFile(file); doesn't wait for the image to load before completing.
var file = await StorageFile.GetFileFromApplicationUriAsync(new Uri("ms-appx:///Assets\\Screenshot 2021-03-16 083652.png"));
await imgCropper.LoadImageFromFile(file);
this.imgCropper.AspectRatio = 1.0;Adding a Task.Delay() with any value (minimum of 1) fixes the issue.
Notably, this issue also exists when do you
imgCropper.Source = writableBitmap;
imgCropper.AspectRatio = 1;Seems this scenario wasn't considered when designing the architecture for this control. I'll see if there's a better fix for this than lazy-loading. A few options come to mind...
There was a problem hiding this comment.
Moving additional findings to here in case I need to open a new PR for this.
There was a problem hiding this comment.
PR #4720 opened with a much simpler fix
Fixes #4483 #4478
Fix the issue of incorrect aspect ratio in some cases caused by the timing of updating the layout of the image cropper control.
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Other information