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

[windows] Fixed CarouselView items rendering #16842

Merged
merged 6 commits into from
Aug 23, 2023
Merged

[windows] Fixed CarouselView items rendering #16842

merged 6 commits into from
Aug 23, 2023

Conversation

mauroa
Copy link
Contributor

@mauroa mauroa commented Aug 17, 2023

Description of Change

Added ArrangeOverride override and changed MeasureOverride in ItemContentControl to not call the base implementation when the ItemHeight and ItemWidth are already defined.

When the height and width of the items are already defined, it's likely that the base.MeasureOverride (WinUI) call reduces the original size, causing issues like the items in a CarouselView not fitting the container, which causes more items to be displayed at once. This fix makes sure that we always return the max height and width between the measured override and the existing items height and width. Also, adding ArrangeOverride from WinUI base class to control the layout correctly and completely on our side.

CarouselView before the fix:

CarouselBroken

CarouselView after the fix:

CarouselFixed

Also added Apium tests to validate that a single item is always shown in the carousel.

@mauroa mauroa self-assigned this Aug 17, 2023
@mauroa mauroa requested a review from a team as a code owner August 17, 2023 22:56
@mauroa mauroa changed the title Fixed CarouselView items rendering in Windows [windows] Fixed CarouselView items rendering Aug 17, 2023
@samhouts
Copy link
Member

I wonder if this will also fix these:

#13057
#12850

@samhouts samhouts added this to the .NET 8 GA milestone Aug 17, 2023
@Eilon Eilon added the area-controls-collectionview CollectionView, CarouselView, IndicatorView label Aug 17, 2023
@jknaudt21
Copy link
Contributor

jknaudt21 commented Aug 18, 2023

This is awesome! Considering that the control was super broken until now, this is a huge improvement. Some minor things to consider though:

  • Scrolling through the CarouselView flickers a bit and seems to be more sensitive when scrolling in one direction than the other. In the gif below I change scroll directions and the view began flickering. This was present before this PR so I doubt these changes had an effect.
    Scroll2
  • If I resize my screen, the CarouselView doesn't remain centered/move with the resize. If I resize long enough, I see (and can interact with) the other items in the view. Testing this out before this PR was impossible because the control was broken
    LongResize

@mauroa
Copy link
Contributor Author

mauroa commented Aug 18, 2023

I wonder if this will also fix these:

#13057 #12850

It should, however I need to review why it's not working that good when resizing the window, as JD pointed. Once that's resolved, it should fix the three issues (this one and the other two)

@mauroa
Copy link
Contributor Author

mauroa commented Aug 18, 2023

This is awesome! Considering that the control was super broken until now, this is a huge improvement. Some minor things to consider though:

  • Scrolling through the CarouselView flickers a bit and seems to be more sensitive when scrolling in one direction than the other. In the gif below I change scroll directions and the view began flickering. This was present before this PR so I doubt these changes had an effect.
    Scroll2
  • If I resize my screen, the CarouselView doesn't remain centered/move with the resize. If I resize long enough, I see (and can interact with) the other items in the view. Testing this out before this PR was impossible because the control was broken
    LongResize

@jknaudt21 I fixed the resizing issue, you can see it in the latest commit

@mauroa mauroa requested a review from jknaudt21 August 18, 2023 16:10
@mauroa
Copy link
Contributor Author

mauroa commented Aug 18, 2023

I wonder if this will also fix these:

#13057 #12850

@samhouts I just linked the other bugs too, after addressing JD's feedback

@jknaudt21
Copy link
Contributor

jknaudt21 commented Aug 18, 2023

@mauroa I think there are still a couple of rough edges with the resize changes you pushed.

Grey blocks appear when resizing and it doesn't re-center 100% of the time

I might also be nit-picking but it seems that maybe there's a lot of computation going on? The resize felt a bit less responsive.
Resize2

Sometimes resizing the screen very fast causes the app to crash with an NRE.

This seems to be easier to repro by resizing as soon as the page loads, so it might be that something isn't fully yet initialized? The failure happens in native UWP code, so it's going to be hard to debug 😅. The stack traces lead me to WindowsMessageManager.uwp.cs.
NRE

@samhouts samhouts added backport/suggested The PR author or issue review has suggested that the change should be backported. partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with labels Aug 21, 2023
@mauroa
Copy link
Contributor Author

mauroa commented Aug 21, 2023

@mauroa I think there are still a couple of rough edges with the resize changes you pushed.

Grey blocks appear when resizing and it doesn't re-center 100% of the time
Sometimes resizing the screen very fast causes the app to crash with an NRE.

I think we should track this grey item thing when resizing, as a separate issue to investigate. I can reproduce it before my changes and it probably affects other controls as well, it looks like a refreshing issue. Given this PR fixes three bugs and the refreshing issue is pre-existing, I would say we are safe to go and open a separate issue to track the remaining one.

jknaudt21
jknaudt21 previously approved these changes Aug 21, 2023
Copy link
Contributor

@jknaudt21 jknaudt21 left a comment

Choose a reason for hiding this comment

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

Approving so the fix can go in and then later polish the issue with resizing the screen.

mauroa and others added 6 commits August 22, 2023 11:26
…in ItemContentControl to not call base when the ItemHeight and ItemWidth are defined

When the height and width of the items are already defined, it's likely that the base.MeasureOverride (WinUI) call reduces the original size, causing issues like the items in a CarouselView not fitting the container, which causes more items to be displayed at once.
This fix makes sure that we always return the max height and width between the measured override and the existing items height and width.
Also, adding ArrangeOverride from WinUI base class to control the layout correctly and completely on our side.

Fixes Bug: #12567
@PureWeen PureWeen merged commit 862fa07 into main Aug 23, 2023
34 checks passed
@PureWeen PureWeen deleted the dev/mag/carousel branch August 23, 2023 13:13
@hartez hartez added the backport/approved After some discussion or review, this PR or change was approved to be backported. label Aug 23, 2023
@hartez
Copy link
Contributor

hartez commented Aug 29, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/6017941843

@github-actions
Copy link
Contributor

@hartez backporting to net7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: [windows] Added ArrangeOverride override and changed MeasureOverride in ItemContentControl to not call base when the ItemHeight and ItemWidth are defined
Applying: Added Appium test for CarouselView to validate fix for bug #12567
Applying: Fix RS0016 + added inheritdoc
error: sha1 information is lacking or useless (src/Controls/src/Core/Platform/Windows/CollectionView/ItemContentControl.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0003 Fix RS0016 + added inheritdoc
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@hartez an error occurred while backporting to net7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported. fixed-in-8.0.0-rc.1.9171 Look for this fix in 8.0.0-rc.1.9171 partner/cat 😻 this is an issue that impacts one of our partners or a customer our advisory team is engaged with platform/windows 🪟
Projects
None yet
6 participants