Fix TreeView expand #1924
Fix TreeView expand #1924
Conversation
msft-github-bot
added
the
needs-triage
kaiguo
added
area-TreeView
and removed
needs-triage
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -320,6 +320,14 @@ void TreeViewList::PrepareContainerForItemOverride(winrt::DependencyObject const | |||
| { | |||
| bool hasChildren = itemContainer.HasUnrealizedChildren() || itemNode->HasChildren(); | |||
| itemContainer.GlyphOpacity(hasChildren ? 1.0 : 0.0); | |||
| if (itemContainer.IsExpanded() != itemNode->IsExpanded()) { | |||
| const DispatcherHelper dispatcher{ *this }; | |||
There was a problem hiding this comment.
I'm curious why you need to run this though the dispatcher async.
Also, should'nt it be the other way ? we are creating the container based on the data, so I would think we need to set the container's value to what the node has.
There was a problem hiding this comment.
I'm curious why you need to run this though the dispatcher async.
It crashes if we don't use a dispatcher, I think it's still the same reason, changing expand state will change ListView's ItemsSource as we are adding/removing items from ListView, and this can't be done during layout.
Also, should'nt it be the other way ? we are creating the container based on the data, so I would think we need to set the container's value to what the node has.
Yeah this is confusing, in content mode, TreeViewItem (which is the container) holds the truth since we are using data binding, and in node mode it's the opposite, TreeViewNode holds the truth (at line 334 we are updating container based on TreeViewNode's state).
| @@ -331,7 +331,6 @@ void TreeViewItem::OnPropertyChanged(const winrt::DependencyPropertyChangedEvent | |||
| // The children have changed, validate and update GlyphOpacity | |||
| bool hasChildren = HasUnrealizedChildren() || treeViewNode->HasChildren(); | |||
| GlyphOpacity(hasChildren ? 1.0 : 0.0); | |||
| UpdateNodeIsExpandedAsync(*treeViewNode, IsExpanded()); | |||
There was a problem hiding this comment.
Do you still need to do it in the case where the container is already prepared and then the source gets set on the node after that?
There was a problem hiding this comment.
I think PrepareContainerForItem will get called every time we change ItemsSource? There were some odd behaviors when keeping them both in place. I guess it caused some race conditions because the operation is async...
|
🎉 Handy links: |
Description
We made changes in previous PR #1917 to update expand state when ItemsSource changes, it doesn't work as expected when ItemsSource is set in an async way. PrepareContainerForItemOverride is a better place to do this, this should keep the property value and actual expand state always in sync.
Motivation and Context
Fix #1790.
How Has This Been Tested?
Added more interaction tests.