search

rivo / tview

Terminal UI library with rich, interactive widgets — written in Golang
MIT License
11.17k stars 574 forks source link

Add func (n *TreeNode) GetParent() #897

Closed paololazzari closed 1 year ago

paololazzari commented 1 year ago

Fixes https://github.com/rivo/tview/issues/896

rivo commented 1 year ago

The problem with this is that parent is a temporary variable (as you can see in its declaration comment) which is set during traversal of the tree. That means, it is not guaranteed to be set. For example, it will not be set for nodes that have not been traversed. It will also not be set if the tree has not been drawn yet. And it might even be wrong if you make changes to the tree structure before drawing it.

The correct implementation for a GetParent function is to walk the tree until you've found the node. Then you'll know the parent and can return it. This would work but it would be expensive, especially for large trees, so this fact must be described in the function comments.

If you want to provide such an implementation, feel free to submit one. As for the current PR, I don't think it's a good idea to return a temporary variable with almost no guarantees.

paololazzari commented 1 year ago

Thank you for the quick response @rivo

Why is parent a temporary variable though? why not have it be a normal variable? this could easily be set for the root node and for the other nodes it could be set when AddChild is called on them

rivo commented 1 year ago

I guess I should have anticipated this question. This is an architectural choice, to keep the number of variables that need to by synchronized as low as possible. It's the same with tview in general: Widgets like Flex or Grid will know their contained widgets but they will not know who contains them.

It's too easy to make mistakes and end up with an inconsistent data structure if references go both ways.

You might say "but it's quite easy to do it here", to which I'd say "it might be now but given that I have maintained this code for years and will maintain it for many years to come, I cannot be sure that a future change will lead to issues".

But as I said above, GetParent is possible and I would prefer that solution.