maui icon indicating copy to clipboard operation
maui copied to clipboard

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

Open bhavanesh2001 opened this issue 6 months ago • 8 comments

[!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.

Supersedes to #29635 , #28675 , #29190

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 Fixes #28023

bhavanesh2001 avatar May 23 '25 11:05 bhavanesh2001

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.

bhavanesh2001 avatar May 26 '25 16:05 bhavanesh2001

/azp run

rmarinho avatar May 28 '25 13:05 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar May 28 '25 13:05 azure-pipelines[bot]

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

bhavanesh2001 avatar May 29 '25 06:05 bhavanesh2001

/azp run

rmarinho avatar May 29 '25 10:05 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar May 29 '25 10:05 azure-pipelines[bot]

/azp run

jsuarezruiz avatar May 30 '25 06:05 jsuarezruiz

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar May 30 '25 06:05 azure-pipelines[bot]

/azp run

PureWeen avatar Jun 17 '25 14:06 PureWeen

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Jun 17 '25 14:06 azure-pipelines[bot]

/rebase

rmarinho avatar Jul 10 '25 10:07 rmarinho

/azp run

rmarinho avatar Jul 10 '25 10:07 rmarinho

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Jul 10 '25 10:07 azure-pipelines[bot]

/azp run

PureWeen avatar Jul 11 '25 17:07 PureWeen

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Jul 11 '25 17:07 azure-pipelines[bot]

Is it realistic to assume that this will come in SR11, related to the milestone?

baaaaif avatar Aug 26 '25 11:08 baaaaif

https://github.com/dotnet/maui/pull/29683

PureWeen avatar Sep 11 '25 22:09 PureWeen