Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TreeView selection state for unexpanded items #597

Merged
merged 2 commits into from
Nov 5, 2022

Conversation

klondikedragon
Copy link
Contributor

Resolves #578

Pre-launch Checklist

  • I have updated CHANGELOG.md with my changes
  • I have run "dart format ." on the project
  • I have added/updated relevant documentation

@@ -276,7 +276,7 @@ class TreeViewItem with Diagnosticable {
bool hasFalse = false;
bool hasTrue = false;

for (final child in children.build(assignInternalProperties: false)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we build the children here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build method only returns visible children, and the selection state also needs to consider children that are not visible because they are underneath a non-expanded item. Also, updateSelected is being called on all parent nodes for any TreeViewItem whose selection state has changed in a depth-first fashion (or in the case of the initial load, for all items). If updateSelected recursively builds all children again, it will make the traversal cost of updating selection state O(n^2) in the size of the tree rather than O(n). This could be a big difference for really large trees.

I've added some more documentation to updateSelected to indicate the proper way to call it, for future references. All current callers already do follow this pattern.

item.executeForAllParents((parent) => parent
  ?.updateSelected(widget.deselectParentWhenChildrenDeselected))

@bdlukaa bdlukaa merged commit 79b5036 into bdlukaa:master Nov 5, 2022
@klondikedragon klondikedragon deleted the fix/treeview-expanded branch November 6, 2022 03:41
@klondikedragon klondikedragon restored the fix/treeview-expanded branch November 10, 2022 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TreeView initial value on selected.
2 participants