Skip to content

Refactor ItemsLayout handling: dynamic default + virtual view-managed subscriptions #29638

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

bhavanesh2001
Copy link
Contributor

@bhavanesh2001 bhavanesh2001 commented May 23, 2025

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!

Description of Change

This PR resolves #29619 by eliminating the memory leak caused by CollectionViewHandler2's subscription to ItemsLayout property changes and reworking the property change flow.

Alternative to #29635 , #28675

Problems

  • Previously, we used a static LinearItemsLayout.Vertical instance as the default for ItemsLayout
  • When the handler subscribed to property changes on this shared instance (without unsubscribing), it caused memory leaks (in the CV2 case)
  • Additionally, Android and Windows used WeakNotifyPropertyChangedProxy to work around this, but that added platform complexity and only masked the root cause.

Fix

  • The default ItemsLayout is now created per-instance using defaultValueCreator, not a static object.
  • The ItemsView (virtual view) subscribes directly to changes on its ItemsLayout and raises OnPropertyChanged("ItemsLayout") for relevant internal changes (Span, ItemSpacing, etc.).
  • This triggers the MapItemsLayout mapper, which applies platform-specific updates.
  • Removed WeakNotifyPropertyChangedProxy from Android and Windows for ItemsLayout changes.
  • All layout updates now flow through the mapper via the virtual view

Issues Fixed

Fixes #29619
Fixes #27666
Fixes #27667
Fixes #28656
Fixes #29696

@bhavanesh2001 bhavanesh2001 requested a review from a team as a code owner May 23, 2025 11:41
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label May 23, 2025
@bhavanesh2001 bhavanesh2001 marked this pull request as draft May 23, 2025 11:44
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

Can we add the CV2 to the leak tests ?

@jsuarezruiz jsuarezruiz added the area-controls-collectionview CollectionView, CarouselView, IndicatorView label May 26, 2025
@bhavanesh2001
Copy link
Contributor Author

bhavanesh2001 commented May 26, 2025

While enabling memory tests for CV2, I found that CarouselViewHandler2 is leaking 😢. It seems the cause is different from the issue we're addressing here — something else is triggering the leak.

Anyway, I’ll definitely look into it.

Aside from that, everything here looks great.

@rmarinho
Copy link
Member

/azp run

@dotnet dotnet deleted a comment from azure-pipelines bot May 28, 2025
@dotnet dotnet deleted a comment from jsuarezruiz May 28, 2025
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot May 28, 2025
@dotnet dotnet deleted a comment from PureWeen May 28, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot May 28, 2025
@dotnet dotnet deleted a comment from PureWeen May 28, 2025
@bhavanesh2001
Copy link
Contributor Author

bhavanesh2001 commented May 29, 2025

Only CarouselViewHandler2 memory tests are failing. #29719 should fix the leak.

I also noticed that CarouselView too uses a static instance as the default ItemsLayout. I'm updating it to use a per-instance value via defaultValueCreator and managing property changes in the VirtualView itself—just like we did with ItemsView for CollectionView.

Also, I need to write few UI tests to verify that property changes in ItemsLayout (e.g., ItemSpacing) are correctly reflected in the UI for both CollectionView and CarouselView

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@@ -187,7 +187,7 @@ public object PositionChangedCommandParameter
/// <summary>Bindable property for <see cref="ItemsLayout"/>.</summary>
public static readonly BindableProperty ItemsLayoutProperty =
BindableProperty.Create(nameof(ItemsLayout), typeof(LinearItemsLayout), typeof(ItemsView),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmarinho, shouldn't the declaring type here be typeof(CarouselView)?

@bhavanesh2001 bhavanesh2001 force-pushed the fix_collectionview_ItemsLayout branch from f11c626 to 37aba59 Compare May 29, 2025 17:05
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView community ✨ Community Contribution
Projects
None yet
3 participants