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.

Sign up for GitHub

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

Fix the bug of wrong aspect ratio of image cropper in some cases #4482

Closed
wants to merge 2 commits into from
Closed

Fix the bug of wrong aspect ratio of image cropper in some cases #4482

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ced150
Fix the bug of incorrect aspect ratio in some cases caused by the tim…
HHChaos Feb 10, 2022
1bfb9a0
Update
HHChaos Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions Microsoft.Toolkit.Uwp.UI.Controls.Media/ImageCropper/ImageCropper.Events.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,13 +166,21 @@ private void SourceImage_ManipulationDelta(object sender, ManipulationDeltaRoute

private void ImageCanvas_SizeChanged(object sender, SizeChangedEventArgs e)
{
if (Source == null)
if (Source == null || !IsValidRect(CanvasRect))
{
return;
}

UpdateImageLayout();
UpdateMaskArea();
if (_lazyInitImageLayoutAction != null)
{
_lazyInitImageLayoutAction.Invoke();
_lazyInitImageLayoutAction = null;
}
else
{
UpdateImageLayout();
UpdateMaskArea();
}
}
}
}
6 changes: 6 additions & 0 deletions Microsoft.Toolkit.Uwp.UI.Controls.Media/ImageCropper/ImageCropper.Logic.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@HHChaos HHChaos Mar 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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_ImageCanvas that contains the image, rather than the image itself.
  • We're calling InitImageLayout when 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.

Copy link
Member

@Arlodotexe Arlodotexe Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Member

@Arlodotexe Arlodotexe Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Moving additional findings to here in case I need to open a new PR for this.

Copy link
Member

Choose a reason for hiding this comment

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

PR #4720 opened with a much simpler fix

return;
}

if (Source != null)
{
_restrictedCropRect = new Rect(0, 0, Source.PixelWidth, Source.PixelHeight);
Expand Down
1 change: 1 addition & 0 deletions Microsoft.Toolkit.Uwp.UI.Controls.Media/ImageCropper/ImageCropper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public partial class ImageCropper : Control
private RectangleGeometry _outerGeometry;
private Geometry _innerGeometry;
private TimeSpan _animationDuration = TimeSpan.FromSeconds(0.3);
private Action _lazyInitImageLayoutAction;

/// <summary>
/// Initializes a new instance of the <see cref="ImageCropper"/> class.
Expand Down