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 methods to TreeItem to freely move them between parents #3607

Closed
CsloudX opened this issue Nov 28, 2021 · 3 comments · Fixed by godotengine/godot#77446
Closed

Add methods to TreeItem to freely move them between parents #3607

CsloudX opened this issue Nov 28, 2021 · 3 comments · Fixed by godotengine/godot#77446
Milestone

Comments

@CsloudX
Copy link

CsloudX commented Nov 28, 2021

Describe the project you are working on

A simple no-game application

Describe the problem or limitation you are having in your project

I want change my tree like this:

|-TreeRoot
  |-NodeA
    |-Child
  |-NodeB

-->

|-TreeRoot
  |-NodeA
  |-NodeB
    |-Child

In current situation, I must add a temp node for NodeB, then move Child, then delete the temp node.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I think should remove move_after and move_before method for TreeItem, replece this by add_child and move_child,
where was two advantages:

  1. It can resolve my problem.
  2. It well like Node's methods, all api was same.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

/

If this enhancement will not be used often, can it be worked around with a few lines of script?

This can be worked arouond, but it not particulary convenient

Is there a reason why this should be core and not an add-on in the asset library?

/

@YuriSizov YuriSizov changed the title Change TreeItem some methods Add methods to TreeItem to freely move them between parents Nov 28, 2021
@YuriSizov
Copy link
Contributor

It well like Node's methods, all api was same.

That's not necessarily a good thing. We don't want to create an assumption that TreeItem is a node and behaves anything like node. Also, Tree API generally benefits from specific methods, as specific methods can be easily optimized. So I am against removing the two existing methods, but a new generic method to add a node to (the end of) a parent can be introduced.

@trollodel
Copy link

Continuing from godotengine/godot#46773#issuecomment-979779187, I think that there are 2 different issues described in the OP:

  • There isn't a single function call to reparent an item when the new parent has no children.
  • TreeItem move methods aren't consistent with the Node ones.

The first one is a limitation that I didn't consider in godotengine/godot#46773, and it should be fixed. However, a method that puts the item as the last children is quite slow, especially when there are several children (TBH, it can be optimized by using the existing TreeItem children cache, but that cache is not always available).
So, I would prefer a method that inserts the item as the first child, or something that works when

The second one is a consistency vs usability issue. Not only the move_* methods are optimized, but they are more flexible than Node.move_child. For example, the move_* methods handle reparenting transparently.
So, I agree with @pycbouh that they shouldn't be removed.

@CsloudX
Copy link
Author

CsloudX commented Nov 29, 2021

Agree, maybe only need a new add_item method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants