maui icon indicating copy to clipboard operation
maui copied to clipboard

[Windows] Add hot reload support for ListView

Open Foda opened this issue 1 year ago • 1 comments

Description of Change

This PR modifies the code in the CellControl for ListView to use the TemplatedItems property as the source of the Cell objects. Previously, we created a new Cell for each template, which meant that the cells were never wired up as logical children to the control (unless clicked on).

This fixes hot reload for templates used in the ListView. Note: this does not fix hot reload for the header and footer properties.

I've tested this with a normal list of data, and a grouped list of data. There was a few "gotchas" such as needing to invoke oldCell.Cleanup (otherwise we'd get elements added to 2 different parents). Another issue was that templates with Image will throw exceptions when their loading is cancelled while scrolling (I adjusted this logic to only throw if we didn't request to cancel it).

Finally, I've observed one remaining unsolved issue: a WinRT exception of "Unspecified Error" without any additional info if you have a big list and scroll really really fast. Any help to figure out this issue would be appreciated!

https://github.com/dotnet/maui/assets/890772/4c28aefb-40b1-497c-9fe9-7b2c015787cb

Issues Fixed

Fixes #13900 #18401

Foda avatar Jan 24 '24 17:01 Foda

maui.mp4 I updated my sample to include 1000 items on load. Since ListView is virtualized, it shows 74-ish items in the LVT, as it should, as it reflects what the template loads. When I scroll, items are added but never removed, so the VisualDiagnostic list always expands until every element is rendered. So I'm not sure if Virtualization is working in the underlying WinUI view and the abstracted Template view is out of step or if I'm missing some magic value to enable caching. I'm pretty sure this was happening before your change, but with your change, it's more clear since those cells are now showing up in LVT.

Likewise, if you add a new item to an ObservableCollection, items are being created (as far as I can tell) and rendered even when they are off-screen. The hooks for VisualDiagnostics should only be run when items are created and visible by the underlying view, so if these are being rendered, no matter if that item is visible and on screen, it could be that virtualization is broken.

I just took a look at this on the WinUI side (same grouped template), and the virtualization issue also seems to partially exist: resizing the window vertically adds more ListViewHeaderItem and ListViewItem items, but shrinking doesn't remove them.

I also observed that for 1000 item groups, both WinUI and the MAUI version only load/render a few ListViewHeaderItem, so that means that it's just not unloading the elements from the tree on the MAUI side:

WinUI MAUI (WinUI tree)
image image

Lemme try and figure out what on the WinUI side needs to be hooked for the element unload :)

Foda avatar Jan 25 '24 18:01 Foda

Shrinking or increasing the size of the viewport shouldn't affect the items. The ListView children are template representations of the cells contained within the list... I think.

I wrote a WinUI sample to show this: https://github.com/drasticactions/MauiRepros/tree/main/ListVirtualizationTest

This has a string list of X number of items. The LVT shows the inner children as around 100~ and as you scroll, it grows a little, but otherwise stays the same. If you use the Live Property Explorer to edit the grid background of one of the items, and start scrolling, you will see that item background be repeated in other cells. This, AFAIK, is correct. WinUIs list contains X number of cells, swapping out the template as you scroll. By us changing one of the child cells with LPE, this will effect multiple cells as they share templates and swap them out as you scroll. So the child list values should only go up or down when the templates literally need to be swapped for new ones that don't exist and a new cell has to be made.

So my WinUI example (AFAIK) should be operating correctly, and I think your WinUI example is too. This is making me think that the logic of SetupContent (and by extension, VisualDiagnostics.OnChildAdded) is wrong for WinUI,

https://github.com/dotnet/maui/blob/3c0ff6735c6b259a17cdef31f7ebce8dea1e0a30/src/Controls/src/Core/ListView/ListView.cs#L441-L453

If I have a WinUI ListView with an bound ObservableCollection with a ton of items, and add or remove an item from that list, that alone shouldn't add or remove a child from the LVT ListView children unless a new cell actually has been made. If you already filled out the ListView cell list, it won't change. With MAUI before your change, if I add a new item, it will always add a new LVT item, because that was the only way it was invoking that code, and it was never being invoked for the initial list of items.

drasticactions avatar Jan 26 '24 09:01 drasticactions

@drasticactions 2 weeks later, and I think I've sorted out all the mentioned issues. The control now works as you'd expect when setting CachingStrategy="RecycleElement" (only a set number of elements get cached, WinUI uses ~4x the viewport) and scrolling doesn't add more elements (a few might be added as the WinUI caching algo adjusts??). Adding/removing items to the backing list should no longer add/remove LVT items either.

I've also fixed all previous crashes due to items being added to 2x parents.

Foda avatar Feb 07 '24 00:02 Foda

/rebase

Foda avatar Feb 08 '24 17:02 Foda

Possibly fixed by https://github.com/dotnet/maui/issues/21981

PureWeen avatar Apr 23 '24 18:04 PureWeen

Closing this for now in lieu of other priorities.

If ListView isn't working for you on WinUI, please use CV and log any issues that you encounter.

PureWeen avatar May 19 '24 00:05 PureWeen