Open tree branches when scrolling #6013
Conversation
Implement TODO action and update ScrollTo to open branches when scrolling to item
andydotxyz
left a comment
andydotxyz
left a comment
There was a problem hiding this comment.
Thanks for looking at this.
I think the expanding could be more efficient - instead of re-constructing the entire tree parent items you should be able to record the ids that lead to a child - then ensure that each of those ids are expanded. (though if you're using walk then onNode may need to be called after the child items in a branch scenario.
I would also factor this out into a separate method (maybe expandParents) so it's more readable and has a unit test.
🤔 I kinda tried that first time but miserably failed. will give it another shot and add a |
* add findPath internal method to find a path to a tree node * add expandBranches internal method to expand all branches leading to a node * use expandBranches in ScrollTo
|
@andydotxyz think I got it, hope this is more aligned with what you mentioned. |
andydotxyz
left a comment
andydotxyz
left a comment
There was a problem hiding this comment.
Thanks, this is looking good - a few cleanups and I think that the path slice should be ordered for avoidance of confusion
| t.Run("deeply nested node", func(t *testing.T) { | ||
| found, parents := tree.findPath("", "item_1_2_2") | ||
| assert.True(t, found) | ||
| assert.ElementsMatch(t, []string{"", "item_1", "item_1_2"}, parents) |
There was a problem hiding this comment.
Surely the order matters? The naming implies so - the word "path" implies ordering
| for _, child := range childUIDs(current) { | ||
| found, path := t.findPath(child, target) | ||
| if found { | ||
| return true, append(path, current) |
There was a problem hiding this comment.
I think this will add the path elements in reverse order, see note in tests too
| // Resize sets a new size for a widget. | ||
| // Opens the branches leading to a node | ||
| // | ||
| // Since: 2.8 |
There was a problem hiding this comment.
There is no since required for private methods.
| } | ||
|
|
||
| t.ensureOpenMap() | ||
| var opened []TreeNodeID |
There was a problem hiding this comment.
The need for this can be avoided if you call the callback function in the first loop.
There was a problem hiding this comment.
(just a bool instead to indicate if refresh is required)
| return | ||
| } | ||
|
|
||
| if len(parents) == 0 { |
There was a problem hiding this comment.
You could put this after an "or" for the !found to keep this shorter.
2 participants
Description:
Fixes the following code note:
This one was quite a bit out of my depth, and tried my best to find a somewhat clean way of doing it, therefore I don't have any smart reasoning as to why it was done like this other than a "this is how I managed to get it done"
let me know if you want any changes.
2025-11-06_20-11-1762456932.mp4
Checklist:
Where applicable: