Does `Tree:remove_node` remove all children of a node from the tree?

[miversen33]

The title may not be great so let me explain quick. Lets say I have a nui.tree object and I add a node with children to it (lets say that node is node1, and has children node10 and node11 under it). Now, I remove node1 from the tree

-- For simplicity, lets say node1's id is simply 'node1'
tree:remove_node('node1')

In this scenario, node10 and node11 are orphaned as their parent was removed from the tree.

Since the tree has references to the children in other places, that means (I believe) that they are still able to be retrieved if I query for them later even though their parent no longer exists and they are no orphaned branches.

Is there a "correct" way of removing an entire branch from a tree?

[MunifTanjim]

Hmm, the current behavior seems buggy. Currently the orphaned nodes are still accessible by their id.

So for example, if you have:

node1
- node10
- node11

The current behavior is:

tree:remove_node('node1') -- removes node1
tree:get_node('node1') -- returns `nil`
tree:get_node('node10') -- returns orphaned node10
tree:get_node('node11') -- returns orphaned node11

The correct behavior should be:

tree:remove_node('node1') -- removes node1, node10, node11
tree:get_node('node1') -- returns `nil`
tree:get_node('node10') -- returns `nil`
tree:get_node('node11') -- returns `nil`

Do you want to open a PR to fix it?

[miversen33]

I was having a bit of a moment last night as I was running into it, I was hoping I was just doing something wrong 🙃

I am actually using nui through neo-tree.nvim but as long as you work with me, I think I can get a PR thrown together for this.

I'll open a draft this afternoon and see if I can work through it, I don't imagine it will be too difficult. In theory this should be as simple as recursively walking the children (and children's children, etc) and removing all children from the tree right? That's how I was planning on handling this myself anyway

[MunifTanjim]

In theory this should be as simple as recursively walking the children (and children's children, etc) and removing all children from the tree right?

Yep, exactly that. The child ids are stored in node._child_ids, and the nodes are stored in self.nodes.by_id.

[miversen33]

That makes sense. Since remove_node's signature says it returns the removed node, what should happen with the child nodes? I'm just thinking from the perspective of a user using nui, should/would they care that the children are removed? Should the children be returned in some way with the removed node?

[MunifTanjim]

Let's just keep the current return behavior (i.e. just return a single node for node_id) unchanged.

Should the children be returned in some way with the removed node?

No need to do that for now.

If somebody reports an issue with use-case for it in future, we can collect the removed child nodes in a table removed_nodes_by_id and return it as a second return value.

[miversen33]

That works for me. If you could, take a peek at #228 and lemme know your thoughts. Its pretty simplistic but seems to be working for me. I can't get the unit tests to run on my local system (something about plenary not being located in pack/*/opt and symlinking that didn't work either lol

[MunifTanjim]

Thanks for taking care of it! The PR is merged 🎉