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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

paololazzari
Copy link
Contributor

Fixes #896

@rivo
Copy link
Owner

rivo commented Oct 5, 2023

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
Copy link
Contributor Author

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
Copy link
Owner

rivo commented Oct 5, 2023

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.

@rivo rivo closed this in 57ac381 Oct 5, 2023
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.

Add func (*TreeNode) GetParent
2 participants