Tree and TreeItem folding helpers and improvements#36926
Tree and TreeItem folding helpers and improvements#36926FeatherAntennae wants to merge 1 commit into
Conversation
HaSa1002
left a comment
HaSa1002
left a comment
There was a problem hiding this comment.
Just a few I thinks I saw
|
You should squash the last commit. But maybe even meld all of them into one, since they are all used to implement "Add Tree and TreeItem folding helpers" and add the details in the message body |
FeatherAntennae
force-pushed
the
gui-tree-folding-logic
branch
2 times, most recently
from
2051f30 to
eccae75
Compare
|
Sorry for the long delay reviewing this. This would likely need to rebased / retested on latest Would be good to get some input from @godotengine/gui-nodes folks familiar with the Tree API.
In this PR I don't see a reduction of said clutter, I assume this was meant to be PR'ed as a follow-up once agreed upon? It would be interesting to see how much these new methods help reduce clutter to be able to assess whether adding them is a good idea or not. |
|
As a matter of fact, I just came to this PR while triaging the least recently updated ones, but we just got a new PR today that seems to address some of the same ideas: #62666. Would be good to have a cross-review @AThousandShips. |
The main PR #33984 has more details. This one was split following discussion, to make it easier to review the changes to the
From what I can see, the idea is very similar. The main two differences are :
In any cases, if there's interest in this, I will have some free time this week that I could put on updating this and to look at a few of the other elements mentioned before in both PRs. I've been doing Engine code and C++ professionally for a year and a half now, so it'll be much faster. :) |
|
I went for shift as that was how Scene Tree does it, my original idea was to use right click before I realized it was already a feature |
|
My code should be ready to go and is in line with what Scene Tree does but since I missed this PR I'll let you see and leave it up to reviewers to decide which way to go |
|
No worries, if anything, it just shows that there's still interest in this. In fact, it seems a lot of love was put into making engine developers' life easier through various refactors recently, that's great! :)
Shift and alt were both hard-coded in various places, which is part of why I was going for this refactor. Most of the discussion for this was spread through old issues, PRs and a bit on the old IRC, so I don't know if the editor settled more toward one or the other since. I'll go through the source code later today to see. |
|
I couldn't even find any other instances of this folding behavior in the editor, I missed the one in Scene Tree when I originally made my proposal for this, failing to find this PR, the need arose from working on expanding the Help Search where I realized how helpful it would be to have recursive folding |
Tree class: - Added "set_collapsed_all(...)" method to allow collapsing or expanding the entire tree recursively. (Helper method to ensure it is done the same way by everyone, making use of the new recursive methods.) TreeItem class: - Added "has_expanded_child()" method to detect if at least one child of the TreeItem isn't collapsed(and has at least one child of it's own). - Added "is_in_active_branch()" method to detect if the TreeItem or at least one of its descendent is selected. - Added "set_collapsed_recursive(...)" method to collapse a TreeItem and its children recursively with possibility to ignore self and/or skip active items. Tree behaviour: - ALT + Click on a TreeItem's folding icon will call the TreeItem's method "toggle_collapsed_recursive()". Refactor: - Removed Scene Tree Dock custom recursive folding logic. It is now handled by the Tree directly. - Replaced Visual Shader Editor collapse / expand all logic with the one now included in TreeItem. - Replaced Script Editor Error Tree collapse / expand all logic with the one now included in TreeItem. Documentation: - Updated documentation with all new functions. - Added documentation for "uncollapse_tree()" function.
FeatherAntennae
force-pushed
the
gui-tree-folding-logic
branch
from
eccae75 to
ab61ff9
Compare
Alright, I redid all of this PR from scratch, much easier than trying to rebase after two years. Also did a quick refactor of a few places in the editor that were using these functions. All trees now supports them by default too. I did not replace the "collapse / expand all" in the Filesystem Dock yet, because it's scary spaghetti that I can't handle today. :P ALT + Click has the following behaviour:
(Essentially, it loops between the 3 states)
It also works everywhere else with a tree: |
|
Needs rebase after #62666, if some changes are still relevant. |
I do not mind rebasing again to contribute the differences between this and #62666, but the main part of what this was doing is done in that other PR, and it's already the second time this got done and forgotten while waiting for review. #62666 was a new, completely separate and very similar implementation that poped up, bringing attention to this here again. I rewrote everything since it was too old, except for the filesystem dock as it was more work and we were waiting for feedback on the approach, but the other implementation got merged before this one got the review it was waiting for. Most of what this does seems to have been implemented by #62666, except for two things that were decided based on discussion in the various issues, proposal, and other merge requests related to this one. I assumed the decision to merge the other PR meant that these differences were not wanted. What was different about my approach:
If these are wanted I can comme back to this, but I'd prefer this to be closed if it's going to be forgotten again because the differences are too small or because we're in the middle of the 4.0 beta. |
|
If that's the case then I think this can be closed. Moving the folding to Tree unified and removed duplicate code, so it was a straightforward change. The 2 extras you mentioned might warrant a proposal (and a new PR), because they change behavior. Thanks for your contribution nonetheless and sorry about the outcome .-. |
6 participants
A few small changes to the Tree and TreeItem classes.
The goal is to reduce clutter in many classes that each have their own (different) implementation of the same logic, while giving new and old behaviors of the UI trees better consistency across the editor.
For the discussion and details, see #33984.
This was separated from the #33984 PR, as the refactor is pretty big and is going to take a while to complete, but I think I pretty much nailed down the global changes needed to make everything compatible and consistent. I think some of these changes would help some other propositions and PR too. #33984 will be kept just to keep track of the refactor, but separating this will allow each part of the refactor to be it's own PR.
I completed the Tree and TreeItem documentation, and opened this PR for the changes to the Tree and TreeItem, including doc for the new methods. It already makes the click, alt-click and folding consistent to all trees by default.