MaterialDesignInXAML / MaterialDesignInXamlToolkit

Google's Material Design in XAML & WPF, for C# & VB.Net.
http://materialdesigninxaml.net
MIT License
15.09k stars 3.42k forks source link

When the TreeListView expands too much content, scrolling to the top will shrink the content #3640

Open KashimuraBaka opened 2 months ago

KashimuraBaka commented 2 months ago

Bug explanation

When using the TreeViewList control, if too much data is expanded, scrolling to the top will automatically collapse all content, and it cannot maintain the expanded state. I thought it was my own problem, the same issue occurred during the demo attempt. 1

Version

5.1.0

corvinsz commented 1 month ago

This bug can be reproduced a little bit easier by following these steps:

  1. Add alot of items to the TreeListView
  2. Expand any item (preferably one at the top)
  3. Scroll far away from the expanded item
  4. Scroll back to the previous expanded item

MultiTreeViewBug

My theory

My theory is that the PrepareTreeListViewItem method is getting called twice, and the actual setting of the IsExpanded property is done asynchronusly via a Dispatcher. The first method call sets the IsExpanded to True, which is correct. The second method call sets it to False because the Dispatcher hasn't finished executing yet. When uncommenting the Dispatcher it works like expected, however I am not sure if that causes other side effects.

internal void PrepareTreeListViewItem(object? item, TreeListView treeListView, int level, bool isExpanded)
{
    Level = level;
    TreeListView = treeListView;

    //NB: This can occur as part of TreeListView.PrepareContainerForItemOverride
    //Because this can trigger additional collection changes we enqueue the operation
    //to occur after the current operation has completed.
    //Dispatcher.BeginInvoke(() =>
    //{
    if (GetTemplate() is HierarchicalDataTemplate { ItemsSource: { } itemsSourceBinding })
    {
        SetBinding(ChildrenProperty, itemsSourceBinding);
    }
    IsExpanded = isExpanded;
    //});

    DataTemplate? GetTemplate()
    {
        return ContentTemplate ??
               ContentTemplateSelector?.SelectTemplate(item, this) ??
               ContentPresenter?.Template;
    }
}

MultiTreeViewBug_Fixed

Keboo commented 1 month ago

This is "by design" but there is a compelling argument that the design may be flawed (or at least confusing).

First a little background. A big reason for creating this control was to have a tree view that supports UI virtualization (the default WPF tree view does not). By default, this tree view has UI virtualization enabled specifically for that reason. As you scroll the UI elements are being recycled to keep it nice and responsive. The problem comes because the IsExpanded property on the TreeListViewItem is not bound to anything in the item's view model, so once the UI element is recycled its previous state is lost.

The dispatcher change above is interesting however, it does introduce so issue with data binding from multiple threads specifically if the "children" are an observable collection that is being updated dynamically when the parent is expanded.

There are a couple solutions here: First, if virtualization is not desirable, then just switch back to the regular WPF TreeView. This one adds a lot of complexity to get the virtualization so only use it if you need it.

Second, bind the IsExpanded state to something in your item's view model. This allows your view model to maintain the state of the item, even when its UI container is recycled. Assuming the property on your view model is named IsExpanded then the XAML for this would look like:

<materialDesign:TreeListView >
  ...
  <materialDesign:TreeListView.ItemContainerStyle>
    <Style TargetType="materialDesign:TreeListViewItem" BasedOn="{StaticResource {x:Type materialDesign:TreeListViewItem}}">
      <Setter Property="IsExpanded" Value="{Binding IsExpanded}" />
    </Style>
  </materialDesign:TreeListView.ItemContainerStyle>
 ...
</materialDesign:TreeListView>

I would also be really interested in hearing thoughts on how this could be better documented or made more obvious for people. I am going to check in a sample of this into the repo as well.