-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS] Fixed KeepLastItemInView for CV1 and added support for KeepLastItemInView for CV2 #28720
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for the ItemsUpdatingScrollMode property "KeepLastItemInView" for CollectionView on iOS. Key changes include updating the LayoutFactory2 methods to accept the new parameter, modifying the custom UICollectionViewCompositionalLayout to enforce scrolling behavior, and updating the ItemsViewHandler2 and CollectionViewHandler2 to pass the new parameter.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/Controls/src/Core/Handlers/Items2/iOS/LayoutFactory2.cs | Updated method signatures to include ItemsUpdatingScrollMode and added logic in the custom layout to scroll to the last item |
src/Controls/src/Core/Handlers/Items2/ItemsViewHandler2.iOS.cs | Replaced the previous direct assignment with a call to UpdateLayout for propagating ItemsUpdatingScrollMode |
src/Controls/src/Core/Handlers/Items2/CollectionViewHandler2.iOS.cs | Updated layout factory calls to pass the ItemsUpdatingScrollMode parameter |
Comments suppressed due to low confidence (3)
src/Controls/src/Core/Handlers/Items2/iOS/LayoutFactory2.cs:12
- The method signature now includes the new parameter 'itemsUpdatingScrollMode', which is a breaking change in the public API. Please ensure related documentation and tests are updated accordingly.
public static UICollectionViewLayout CreateList(LinearItemsLayout linearItemsLayout, LayoutGroupingInfo groupingInfo, LayoutHeaderFooterInfo headerFooterInfo, ItemsUpdatingScrollMode itemsUpdatingScrollMode)
src/Controls/src/Core/Handlers/Items2/ItemsViewHandler2.iOS.cs:120
- Replacing the direct assignment of ItemsUpdatingScrollMode with an UpdateLayout() call may introduce side effects; ensure that tests cover the correct propagation of ItemsUpdatingScrollMode.
handler.UpdateLayout();
src/Controls/src/Core/Handlers/Items2/CollectionViewHandler2.iOS.cs:157
- Ensure tests verify that the ItemsUpdatingScrollMode value is correctly propagated to the layout factory, including in fallback scenarios.
var itemsUpdatingScrollMode = ItemsView.ItemsUpdatingScrollMode;
{ | ||
base.FinalizeCollectionViewUpdates(); | ||
|
||
if (_itemsUpdatingScrollMode == ItemsUpdatingScrollMode.KeepLastItemInView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overriding FinalizeCollectionViewUpdates to scroll to the last item is a significant behavior change; please ensure tests verify the accuracy of this scrolling behavior.
Copilot uses AI. Check for mistakes.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
} | ||
else | ||
{ | ||
collectionView.ScrollToItem(lastIndexPath, UICollectionViewScrollPosition.Right, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should look at RTL here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that the current implementation already supports RTL. I've included it in the UITest to be sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test ? Also can we make sure if RTL is on it scrolls correctly
Thanks
sure! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test is failing for Mac and CV1 KeepLastItemInViewShouldWork
@rmarinho I've added a commit that fixes it for cv1 |
This also fixes issue #18029 |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test failure seems consistent
https://dev.azure.com/xamarin/public/_build/results?buildId=140014&view=ms.vss-test-web.build-test-results-tab&runId=4027466&resultId=100127&paneView=debug
LayoutPassesShouldNotIncrease
@albyrock87 I've modified your UI test. Could you please have a look at this pr and let me know if I should have modified the test? |
78132f5
to
12b4197
Compare
Now my question is: you haven't touched 25671.xaml in any way, so how is it possible that the performance changed so much? 25671 is not using the flag you're fixing here, so I would expect no changes there. Also, CV2 has a major performance decrease which seems even more strange. |
12b4197
to
ba5f11c
Compare
Implements ItemsUpdatingScrollMode.KeepLastItemInView for CollectionView on iOS by updating layout creation and scroll logic. Adds internal ItemsUpdatingScrollMode property to ItemsLayout. Includes new test case and UI test for issue dotnet#28720 to verify correct behavior.
Implements ItemsUpdatingScrollMode.KeepLastItemInView for CollectionView on iOS by updating layout creation and scroll logic. Adds internal ItemsUpdatingScrollMode property to ItemsLayout. Includes new test case and UI test for issue dotnet#28720 to verify correct behavior.
Closed in favour of #31104 |
Implements ItemsUpdatingScrollMode.KeepLastItemInView for CollectionView on iOS by updating layout creation and scroll logic. Adds internal ItemsUpdatingScrollMode property to ItemsLayout. Includes new test case and UI test for issue dotnet#28720 to verify correct behavior.
Implements ItemsUpdatingScrollMode.KeepLastItemInView for CollectionView on iOS by updating layout creation and scroll logic. Adds internal ItemsUpdatingScrollMode property to ItemsLayout. Includes new test case and UI test for issue dotnet#28720 to verify correct behavior.
Issues Fixed
Fixes #28716
Fixes #18029
Fixes #31085
Screen.Recording.2025-04-01.at.01.17.56.mov
Screen.Recording.2025-04-01.at.01.15.22.mov