-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS] - Fixed the performance issue that occurred when updating the Spacing in CollectionView for both Vertical List and Grid layouts #29635
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
|
Hey there @@prakashKannanSf3972! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 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.
Pull Request Overview
This PR fixes a severe performance degradation in CollectionView updates by preventing the exponential accumulation of PropertyChanged event registrations. The key changes include unsubscribing existing event handlers before adding new ones, overriding DisconnectHandler to remove event handlers on disconnect, and updating the public API files to reflect these changes.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue27666.cs | Added UI test verifying issue behavior |
| src/Controls/tests/TestCases.HostApp/Issues/Issue27666.cs | Updated HostApp test for CollectionView spacing update |
| src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt | Updated public API with DisconnectHandler override |
| src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt | Updated public API with DisconnectHandler override |
| src/Controls/src/Core/Handlers/Items2/CollectionViewHandler2.iOS.cs | Modified event handler subscription logic and added DisconnectHandler override |
Comments suppressed due to low confidence (1)
src/Controls/tests/TestCases.HostApp/Issues/Issue27666.cs:91
- [nitpick] The UI test only triggers the spacing update without validating that the spacing has actually changed. Consider adding assertions to verify that the layout property is updated as expected.
void OnItemSpacingButtonClicked(object sender, EventArgs e)
| void OnItemSpacingButtonClicked(object sender, EventArgs e) | ||
| { | ||
| if (collectionView.ItemsLayout is LinearItemsLayout layout) | ||
| { | ||
| layout.ItemSpacing = layout.ItemSpacing == 0 ? 50 : 20; |
Copilot
AI
May 23, 2025
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.
[nitpick] Consider extracting the magic numbers (50 and 20) into named constants to improve code maintainability and clarity.
| void OnItemSpacingButtonClicked(object sender, EventArgs e) | |
| { | |
| if (collectionView.ItemsLayout is LinearItemsLayout layout) | |
| { | |
| layout.ItemSpacing = layout.ItemSpacing == 0 ? 50 : 20; | |
| const double DefaultItemSpacing = 50; | |
| const double AlternateItemSpacing = 20; | |
| void OnItemSpacingButtonClicked(object sender, EventArgs e) | |
| { | |
| if (collectionView.ItemsLayout is LinearItemsLayout layout) | |
| { | |
| layout.ItemSpacing = layout.ItemSpacing == 0 ? DefaultItemSpacing : AlternateItemSpacing; |
| *REMOVED*override Microsoft.Maui.Controls.Handlers.Compatibility.FrameRenderer.MovedToWindow() -> void | ||
| override Microsoft.Maui.Controls.Handlers.Compatibility.VisualElementRenderer<TElement>.MovedToWindow() -> void | ||
| override Microsoft.Maui.Controls.Handlers.Compatibility.VisualElementRenderer<TElement>.MovedToWindow() -> void | ||
| ~override Microsoft.Maui.Controls.Handlers.Items2.CollectionViewHandler2.DisconnectHandler(UIKit.UIView platformView) -> void |
Copilot
AI
May 23, 2025
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.
The introduction of a new override for DisconnectHandler in the public API could constitute a breaking change. Please ensure that the public documentation is updated accordingly and that consumers are aware of this update.
| *REMOVED*override Microsoft.Maui.Controls.Handlers.Compatibility.FrameRenderer.SetNeedsLayout() -> void | ||
| *REMOVED*override Microsoft.Maui.Controls.Handlers.Compatibility.FrameRenderer.MovedToWindow() -> void | ||
| override Microsoft.Maui.Controls.Handlers.Compatibility.VisualElementRenderer<TElement>.MovedToWindow() -> void | ||
| ~override Microsoft.Maui.Controls.Handlers.Items2.CollectionViewHandler2.DisconnectHandler(UIKit.UIView platformView) -> void |
Copilot
AI
May 23, 2025
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.
The introduction of a new override for DisconnectHandler in the public API could constitute a breaking change. Please ensure that the public documentation is updated accordingly and that consumers are aware of this update.
jsuarezruiz
left a comment
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.
@jsuarezruiz , The test RefreshShouldNotChangeSize is currently failing in the CI on iOS, however, the same test passes locally with both CV1 and CV2 handlers. Could you please re-run the CI to verify if the issue persists. |
PureWeen
left a comment
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.
Let's review
#29638
|
@prakashKannanSf3972 Could you rebase and fix the conflicts? Thanks in advance. |
aa190d6 to
ccc7a1d
Compare
@jsuarezruiz, The conflicts have been resolved. Thanks. |


Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Root Cause
SubscribeToItemsLayoutPropertyChangedsubscribes to thePropertyChangedevent to track changes in the layout.PropertyChangedevent handlers are added without removing the previously registered handlers.UpdateLayout(), causing cascading layout recalculations and severe performance degradation.Description of Change
ItemsLayout.PropertyChangedby invoking a dedicated cleanup method in DisconnectHandler() before the base disposal logic.SubscribeToItemsLayoutPropertyChanged().Tested the behaviour in the following platforms
Issues Fixed
Fixes #27666
Fixes #27667
Fixes #29619
Output
Before_Fix.mov
After_Fix.mov