Allow easily renaming multiple nodes #69087

KoBeWi · 2022-11-24T01:37:44Z

I wanted this for a long time. The batch rename dialog is an overkill for such basic operation 😒

Putting as draft, because undo is broken.

KoBeWi · 2022-11-24T12:59:32Z

Undo error fixed, but I had to do a little refactoring. Previously SceneTreeEditor's items held NodePath in their metadata. Now they use ObjectID. I added some helper methods to handle that easier.

If something was accessing these nodes from outside scene_tree_editor.cpp, it would be now broken, so hopefully it wasn't used anywhere 🙃

AttackButton · 2022-12-23T15:34:00Z

Is it possible to rename an interleaved selection? Like:

node2, node6, node8, node9 -> Test, Test2, Test3, Test4?

KoBeWi · 2022-12-23T16:52:49Z

ajreckof · 2023-04-14T15:52:17Z

So I took a look at it, the code seems great and straight forward, I don't have any problem with the implementation. But while doing the worst i could to make it break I did found a few bug and strange behaviour I'll try to order them from most important to sidenotes that are not really important and might only be a personnal point of view.

When editing multiple node. ~~StringPath~~NodePath will be badly updated which isn't a case when editing element one at a time. But this also happens with Batch edit so maybe it is okay to have a bug with this here too. (I'm not sure it has been reported yet for Batch Edit). it feels like the rename function is not designed to have multiple rename in the same frame
You can edit outside the list by going to a node with the arrows. This is not a problem in general but will make strange behavior especially since there is not enough readability. For example, if you select a node you rename it you then go down with the arrow and rename this other node and unfortunately the previous node is also renamed at the same time. I think if the node is outside of the selection it should not rename selected nodes. 
When changing the name of one node to an empty string it will rename the node to the name of its class but this behavior was not reimplemented in multi edit so it will just rename everyone from the name of the class being edited instead of their own class.
Edit disappear from popup when multiple node are selected since we are adding a particular behavior on this it might be a good idea to make it reappear maybe with a slightly modified name. Simple Batch Edit? Multi-edit ?
After multi editing the inspector will edit the first node instead of the batch of node
When modifying the name to the name of the focused node it won't rename any of the node as the name of the focused node didn't change. It might be problematic if someone is editing the property of a batch of node and then want to rename the nodes and just paste a value and nothing happens. Right now you have to either unselect the node modify and then reselect your node to continue with your edition of those nodes or set the value to something arbitrary and then setting to the value you want. 
Right now it feels like we are only editing the focused node. I’m not sure how to improve it :(

KoBeWi · 2023-04-14T16:23:17Z

What is StringPath 🤔
See this issue: Arrow keys doesn't select objects on the scene hierarchy #12544 If you move within nodes of the same type, the nodes are correctly deselected
Makes sense
Makes sense
The inspector is consistent with what is selected after rename, so it's not really a problem
I'll see if it can be fixed easily
The PR was inspired by Windows' file explorer, which also behaves like this

ajreckof · 2023-04-14T16:36:20Z

sorry for first one it is NodePath. I will make a different bug issue. I think the solution does not need to be in this PR as the problem is also present in Batch Edit and most likely the solution should be in both. I'll look if the issue already exists. If it does not I will create it. If it does I will update this with a link to it. (Edit : I couldn't find one so i created one here Batch Rename try to update NodePath but fails which results in invalid NodePath error #76070)
okay so it was just another bug
N/A
N/A
N/A
N/A
That's why i placed it last as i didn't have any idea how to make it better and didn't have any other example of similar behaviour, if you feel it is easily understandable by most people than i think it's okay

KoBeWi · 2023-04-14T17:42:44Z

I resolved 3, 4 and 6

KoBeWi · 2023-06-15T11:25:40Z

Rebased after #76376
I reworked the code using the new methods and it's now simpler.

I tested different cases and it works correctly, but seems to break node paths in very specific cases .-. Maybe it's something that could be fixed later, idk.
EDIT: Seems like it's what #76575 fixes.

ajreckof

So reviewing your code made me see some short comings of #76376 I put some review but most likely this PR will be better after I fixed the shortcomings I found.

I tested different cases and it works correctly, but seems to break node paths in very specific cases .-. Maybe it's something that could be fixed later, idk.

Most likely this is this issue #76192. There is a PR but it has to be rebased on my last changes on this part.
Edit : you found it while I was reviewing ^__^

ajreckof · 2023-06-15T20:41:44Z

editor/gui/scene_tree_editor.cpp

+void SceneTreeEditor::rename_node(Node *p_node, const String &p_name, TreeItem *p_item) {
+ TreeItem *item;
+ if (p_item) {
+ item = p_item; // During batch rename the paths may change, so using _find() is unreliable.
+ } else {
+ item = _find(tree->get_root(), p_node->get_path());
+ }


You should just iterate through the node in reverse order so that every node still find it's item as this is what is done for batch rename currently. For this you can just replace push_back by push push_front at line 1029.

I know this is not great as nodes will be numbered from bottom to top and would need fixing but I think fixing this is another PR job. For now this PR should aim to keep consistency with what batch rename does.

editor/gui/scene_tree_editor.cpp

ajreckof · 2023-06-15T22:23:37Z

editor/gui/scene_tree_editor.cpp

+void SceneTreeEditor::_renamed(TreeItem *p_item, TreeItem *p_batch_item, Node *p_node) {
+ Node *n;
+ if (p_node) {
+ n = p_node; // During batch rename the paths may change, so using metadata is unreliable.
+ } else {
+ n = get_node(p_item->get_metadata(0));
+ }
+ ERR_FAIL_NULL(n);
+
+ String new_name;
+ if (p_batch_item) {
+ if (!p_batch_item->get_meta(SNAME("use_class"), false)) {
+ new_name = p_batch_item->get_text(0);
+ }
+ } else {
+ new_name = p_item->get_text(0);
+ }


once the lower part is handled by rename_node (#78292).I think the rest of this function can be integrated into the _edited function this will make clearer and more readable code.

KoBeWi · 2024-03-01T21:07:17Z

@ajreckof Would you be interested in doing the changes you requested? I rebased and tested the PR and it still works. If I understand correctly, it might have some inconsistencies with batch rename, but right now it's in mergable state and this can be improved in a follow-up.

ajreckof · 2024-03-01T21:14:24Z

Yes it shouldn't take me long maybe an hour or so I think. Your rebased PR should work but will most likely recreate bugs that were fixed on batch rename and simple rename.

KoBeWi · 2024-03-01T21:25:37Z

Which bugs?

editor/gui/scene_tree_editor.cpp

ajreckof · 2024-03-02T12:29:31Z

I'd rather make the naming correct and your changes + related fixes can be done in a follow up.

That seems okay I would still include my suggestion of removing the _renamed function and calling directly the rename_node function with the right parameter as what is done in _renamed is already done in rename node

KoBeWi · 2024-03-02T12:32:40Z

Uh can you make a separate patch for that? >_>

ajreckof · 2024-03-02T13:25:45Z

of course here is the updated patch https://github.com/KoBeWi/godot/compare/million_of_names...ajreckof:godot:millions-of-names?expand=1

Edit : This patch also fix a bug with your implementation that prevent renaming with multi-rename multiple nodes that are unique name as unique name is checked before adapting to the parent.

editor/gui/scene_tree_editor.cpp

nongvantinh · 2024-03-02T16:10:50Z

I think this feature could coexist with the existing batch rename dialog #15928. However, from a UX perspective, it would be beneficial if, instead of just displaying one LineEdit for the first selected node, we could display LineEdits for all selected nodes and update changes as we type. However, the limitation of the current system only allows editing one node at a time. Therefore, I believe it is impossible to achieve.

Mickeon · 2024-03-02T16:24:18Z

we could display LineEdits for all selected nodes and update changes as we type

Indeed, it's not possible without some nasty hacks and workarounds. Not really an issue in my opinion, but it may hurt discoverability.

For this to be more discoverable both Rename and Batch Rename should be available on the popup menu when selecting multiple nodes but that remains to be seen later.

ajreckof · 2024-03-02T16:54:42Z

For this to be more discoverable both Rename and Batch Rename should be available on the popup menu when selecting multiple nodes but that remains to be seen later.

strange see how I proposed it here #69087 (comment) and it was said to be resolved here #69087 (comment) but effectively when I test it isn't appearing

I think this feature could coexist with the existing batch rename dialog #15928. However, from a UX perspective, it would be beneficial if, instead of just displaying one LineEdit for the first selected node, we could display LineEdits for all selected nodes and update changes as we type. However, the limitation of the current system only allows editing one node at a time. Therefore, I believe it is impossible to achieve.

see for better context : #69087 (comment)

KoBeWi · 2024-03-02T18:10:14Z

I fixed the missing Rename option for multiple nodes.

Calinou

Tested locally (rebased on top of master f2045ba), it mostly works as expected.

Only entries that are located below the currently focused TreeItem (see the gray focus outline) are renamed, so if you select from bottom to top, only one entry will be renamed.

simplescreenrecorder-2024-03-02_20.46.46.mp4

Considering there's been talk to hide this gray focus outline in Tree, I suggest not changing the behavior based on where the focused item is and always rename all selected items.

KoBeWi · 2024-03-02T23:29:51Z

Fixed.

ajreckof · 2024-03-03T00:29:57Z

When the root is selected it will not rename it and start from the next one.

Co-authored-by: ajreckof <tbonhoure@ymail.Com>

KoBeWi · 2024-03-03T12:16:33Z

#69087 (comment)

Calinou

Tested locally, it works as expected now (including when nodes at different levels are selected, or when both parent and children are selected).

akien-mga · 2024-03-04T19:45:58Z

Thanks!

KoBeWi added enhancement topic:editor usability labels Nov 24, 2022

KoBeWi added this to the 4.x milestone Nov 24, 2022

KoBeWi force-pushed the million_of_names branch from 380ce0b to a03291e Compare November 24, 2022 12:59

KoBeWi marked this pull request as ready for review November 24, 2022 12:59

KoBeWi force-pushed the million_of_names branch 2 times, most recently from 020f1c3 to cfb866c Compare November 24, 2022 14:33

KoBeWi force-pushed the million_of_names branch from cfb866c to be58d7f Compare March 18, 2023 02:05

KoBeWi force-pushed the million_of_names branch from be58d7f to 7769406 Compare April 13, 2023 16:15

KoBeWi force-pushed the million_of_names branch from 7769406 to 9063ab9 Compare April 14, 2023 17:41

KoBeWi requested a review from a team as a code owner April 14, 2023 17:41

KoBeWi force-pushed the million_of_names branch from 9063ab9 to a3d06d2 Compare June 15, 2023 11:21

ajreckof requested changes Jun 15, 2023

View reviewed changes

KoBeWi mentioned this pull request Oct 26, 2023

Add foreign validation warning for rename actions. #84022

Merged

KoBeWi mentioned this pull request Feb 19, 2024

Change default visual indication for the Tree cursor #88537

Draft

KoBeWi force-pushed the million_of_names branch from a3d06d2 to d86014e Compare March 1, 2024 20:14

KoBeWi requested review from a team as code owners March 1, 2024 20:14

AThousandShips reviewed Mar 2, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Show resolved Hide resolved

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

KoBeWi mentioned this pull request Mar 2, 2024

Implement batch rename functionality for FileSystemDock #89063

Open

KoBeWi force-pushed the million_of_names branch from f4439ea to 9fc1d6e Compare March 2, 2024 14:12

AThousandShips changed the title ~~Allow to easily rename multiple nodes~~ Allow easily renaming multiple nodes Mar 2, 2024

AThousandShips reviewed Mar 2, 2024

View reviewed changes

editor/gui/scene_tree_editor.cpp Outdated Show resolved Hide resolved

KoBeWi force-pushed the million_of_names branch from 9fc1d6e to 3d9ae25 Compare March 2, 2024 18:09

Calinou reviewed Mar 2, 2024

View reviewed changes

KoBeWi force-pushed the million_of_names branch from 3d9ae25 to 520b347 Compare March 2, 2024 23:29

Allow to easily rename multiple nodes

ffadba0

Co-authored-by: ajreckof <tbonhoure@ymail.Com>

KoBeWi force-pushed the million_of_names branch from 520b347 to ffadba0 Compare March 3, 2024 12:16

akien-mga requested review from ajreckof and Calinou March 4, 2024 12:16

akien-mga modified the milestones: 4.x, 4.3 Mar 4, 2024

Calinou approved these changes Mar 4, 2024

View reviewed changes

ajreckof approved these changes Mar 4, 2024

View reviewed changes

akien-mga merged commit 9f4a764 into godotengine:master Mar 4, 2024
16 checks passed

KoBeWi deleted the million_of_names branch March 4, 2024 19:46

raulsntos mentioned this pull request Jul 31, 2024

Fix some legacy code #84944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow easily renaming multiple nodes #69087

Allow easily renaming multiple nodes #69087

KoBeWi commented Nov 24, 2022

KoBeWi commented Nov 24, 2022 •

edited

Loading

AttackButton commented Dec 23, 2022

KoBeWi commented Dec 23, 2022

ajreckof commented Apr 14, 2023 •

edited

Loading

KoBeWi commented Apr 14, 2023

ajreckof commented Apr 14, 2023 •

edited

Loading

KoBeWi commented Apr 14, 2023

KoBeWi commented Jun 15, 2023 •

edited

Loading

ajreckof left a comment •

edited

Loading

ajreckof Jun 15, 2023

ajreckof Jun 15, 2023

KoBeWi commented Mar 1, 2024

ajreckof commented Mar 1, 2024

KoBeWi commented Mar 1, 2024

ajreckof commented Mar 2, 2024

KoBeWi commented Mar 2, 2024

ajreckof commented Mar 2, 2024 •

edited

Loading

nongvantinh commented Mar 2, 2024 •

edited

Loading

Mickeon commented Mar 2, 2024 •

edited

Loading

ajreckof commented Mar 2, 2024 •

edited

Loading

KoBeWi commented Mar 2, 2024

Calinou left a comment •

edited

Loading

KoBeWi commented Mar 2, 2024

ajreckof commented Mar 3, 2024

KoBeWi commented Mar 3, 2024

Calinou left a comment

akien-mga commented Mar 4, 2024

Allow easily renaming multiple nodes #69087

Allow easily renaming multiple nodes #69087

Conversation

KoBeWi commented Nov 24, 2022

KoBeWi commented Nov 24, 2022 • edited Loading

AttackButton commented Dec 23, 2022

KoBeWi commented Dec 23, 2022

ajreckof commented Apr 14, 2023 • edited Loading

KoBeWi commented Apr 14, 2023

ajreckof commented Apr 14, 2023 • edited Loading

KoBeWi commented Apr 14, 2023

KoBeWi commented Jun 15, 2023 • edited Loading

ajreckof left a comment • edited Loading

Choose a reason for hiding this comment

ajreckof Jun 15, 2023

Choose a reason for hiding this comment

ajreckof Jun 15, 2023

Choose a reason for hiding this comment

KoBeWi commented Mar 1, 2024

ajreckof commented Mar 1, 2024

KoBeWi commented Mar 1, 2024

ajreckof commented Mar 2, 2024

KoBeWi commented Mar 2, 2024

ajreckof commented Mar 2, 2024 • edited Loading

nongvantinh commented Mar 2, 2024 • edited Loading

Mickeon commented Mar 2, 2024 • edited Loading

ajreckof commented Mar 2, 2024 • edited Loading

KoBeWi commented Mar 2, 2024

Calinou left a comment • edited Loading

Choose a reason for hiding this comment

KoBeWi commented Mar 2, 2024

ajreckof commented Mar 3, 2024

KoBeWi commented Mar 3, 2024

Calinou left a comment

Choose a reason for hiding this comment

akien-mga commented Mar 4, 2024

KoBeWi commented Nov 24, 2022 •

edited

Loading

ajreckof commented Apr 14, 2023 •

edited

Loading

ajreckof commented Apr 14, 2023 •

edited

Loading

KoBeWi commented Jun 15, 2023 •

edited

Loading

ajreckof left a comment •

edited

Loading

ajreckof commented Mar 2, 2024 •

edited

Loading

nongvantinh commented Mar 2, 2024 •

edited

Loading

Mickeon commented Mar 2, 2024 •

edited

Loading

ajreckof commented Mar 2, 2024 •

edited

Loading

Calinou left a comment •

edited

Loading