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] Fix CollectionView.RemainingItemsThresholdReached #14391

Merged
merged 3 commits into from
May 5, 2023

Conversation

Foda
Copy link
Member

@Foda Foda commented Apr 4, 2023

Description of Change

  • Fixed an issue where the CollectionView.RemainingItemsThresholdReached event wasn't firing on Windows. This issue was caused by missing code in ItemsViewHandler.Windows to fire the event.

Issues Fixed

Fixes #8338
Fixes #9935 (dupe)
Fixes #12066 (dupe)

* Fixed an issue where the `CollectionView.RemainingItemsThresholdReached` event wasn't firing on Windows.
* Added a DeviceTest for the event
@Foda Foda added platform/windows 🪟 area-controls-collectionview CollectionView, CarouselView, IndicatorView labels Apr 4, 2023
@Foda Foda requested a review from jsuarezruiz April 4, 2023 18:38
Copy link
Member

@mandel-macaque mandel-macaque left a comment

Choose a reason for hiding this comment

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

Per-se good, I believe that we can achieve the same with a switch expr which would be more elegant, but it is not a reason to block a PR.

Comment on lines 431 to 432
case 0:
if (itemsViewScrolledEventArgs.LastVisibleItemIndex == ItemCount - 1)
Copy link
Member

Choose a reason for hiding this comment

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

Can this two statements be merged in a single switch expression?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup yup, actually I think the switch can be completely removed.

Comment on lines 435 to 436
default:
if (ItemCount - 1 - itemsViewScrolledEventArgs.LastVisibleItemIndex <= Element.RemainingItemsThreshold)
Copy link
Member

Choose a reason for hiding this comment

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

Same comment.

@Foda Foda requested a review from mandel-macaque April 7, 2023 17:34
@@ -423,6 +423,12 @@ internal void HandleScroll(ScrollViewer scrollViewer)
itemsViewScrolledEventArgs = ComputeVisibleIndexes(itemsViewScrolledEventArgs, layoutOrientaton, advancing);

Element.SendScrolled(itemsViewScrolledEventArgs);

if (Element.RemainingItemsThreshold > -1 &&
Copy link
Member

Choose a reason for hiding this comment

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

we should grab this value and use so we don't hit the BP twice

@rmarinho rmarinho merged commit 3188c91 into main May 5, 2023
@rmarinho rmarinho deleted the foda/collectionview branch May 5, 2023 10:56
@rmarinho rmarinho added the backport/suggested The PR author or issue review has suggested that the change should be backported. label May 5, 2023
rmarinho added a commit that referenced this pull request May 30, 2023
* fix(bug): Fix `RemainingItemsThresholdReached` on Windows

* Fixed an issue where the `CollectionView.RemainingItemsThresholdReached` event wasn't firing on Windows.
* Added a DeviceTest for the event

* Remove switch statement

* Update src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs

Co-authored-by: Rui Marinho <me@ruimarinho.net>

---------

Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>
Co-authored-by: Rui Marinho <me@ruimarinho.net>
@hartez hartez added the backport/approved After some discussion or review, this PR or change was approved to be backported. label May 31, 2023
@hartez
Copy link
Contributor

hartez commented Jun 1, 2023

/backport to net7.0

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

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

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

Applying: fix(bug): Fix `RemainingItemsThresholdReached` on Windows
Using index info to reconstruct a base tree...
M	src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs
M	src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.Windows.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.Windows.cs
CONFLICT (content): Merge conflict in src/Controls/tests/DeviceTests/Elements/CollectionView/CollectionViewTests.Windows.cs
Auto-merging src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix(bug): Fix `RemainingItemsThresholdReached` on Windows
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

github-actions bot commented Jun 1, 2023

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

hartez added a commit that referenced this pull request Jun 2, 2023
* fix(bug): Fix `RemainingItemsThresholdReached` on Windows

* Fixed an issue where the `CollectionView.RemainingItemsThresholdReached` event wasn't firing on Windows.
* Added a DeviceTest for the event

* Remove switch statement

* Update src/Controls/src/Core/Handlers/Items/ItemsViewHandler.Windows.cs

Co-authored-by: Rui Marinho <me@ruimarinho.net>

---------

Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>
Co-authored-by: Rui Marinho <me@ruimarinho.net>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
@samhouts samhouts added the fixed-in-8.0.0-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! label Aug 2, 2024
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-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! platform/windows 🪟
Projects
None yet
5 participants