Fix ItemsSource and TreeViewNode syncing #1202
Conversation
dev/TreeView/TreeViewNode.cpp
Outdated
| } | ||
|
|
||
| // Compare the actual content in collections when counts are equal | ||
| if (itemsSourceCount > 0) |
There was a problem hiding this comment.
This 'if' statement is not needed.
dev/TreeView/TreeViewNode.cpp
Outdated
| { | ||
| for (UINT32 i = 0; i < itemsSourceCount; i++) | ||
| { | ||
| if (Children().GetAt(i).Content() != m_itemsDataSource.GetAt(i)) |
There was a problem hiding this comment.
How about accessing and caching Children() exactly once at the top of the method?
dev/TreeView/TreeViewNode.cpp
Outdated
| { | ||
| for (UINT32 i = 0; i < itemsSourceCount; i++) | ||
| { | ||
| if (Children().GetAt(i).Content() != m_itemsDataSource.GetAt(i)) |
There was a problem hiding this comment.
Can Children().GetAt(i) be null?
There was a problem hiding this comment.
There's a if (childrenCount != itemsSourceCount) check at the top, so we shouldn't get here if GetAt(i) is null.
| @@ -2688,6 +2688,61 @@ public void TreeViewRootNodeBindingTest() | |||
| } | |||
| } | |||
|
|
|||
| [TestMethod] | |||
There was a problem hiding this comment.
Looks like the InteractionTests.TreeViewTests.TreeViewSwappingNodesTest_ContentMode test regressed.
There was a problem hiding this comment.
Looks like the InteractionTests.TreeViewTests.TreeViewSwappingNodesTest_ContentMode test regressed.
Just saw your reviews...
The test is setting ItemsSource to null and I was accessing itemsSource.Count without any check, which caused crash, should be fixed now.
jevansaks
added
the
release note
|
🎉 Handy links: |
Fixes #1182
In expand/collapse scenario, TreeViewNode.Children already contains the right content for ItemsSource, which means we don't need to clear and re-populate ItemsSource into children collection again. Re-populating for every expand can potentially create an infinite loop because updating TreeViewNode.Children collection triggers ListView layout updates, which can trigger ItemsSource update in some cases (when there are multiple levels of children and TreeViewItems got recycled), if we clean and re-populate children collection again it becomes an infinite loop.
Added a check to make sure we only update TreeViewNode children collection when the contents are actually different from ItemsSource.