Fix renaming a node to the name of its siblings causing exported NodePath to point to the wrong node

[nongvantinh]

Once text editing is complete, the method void SceneTreeEditor::_renamed() is triggered, followed by void SceneTreeDock::_node_prerenamed(Node *p_node, const String &p_new_name). Afterward, the method void SceneTreeDock::perform_node_renames(Node *p_base, HashMap<Node *, NodePath> *p_renames, HashMap<Ref<Animation>, HashSet<int>> *r_rem_anims) is called, which ultimately leads to line 1707 where the method p_base->set(propertyname, updated_variant); modifies the previously set value with the new Node's name. Next, the method void SceneTreeEditor::_rename_node(ObjectID p_node, const String &p_name) is triggered to update the TreeItem in SceneTreeEditor and the Node's name. However, the problem arises in line 962: n->set_name(p_name); when the Node's name is validated using data.parent->_validate_child_name(this, true);. It is then discovered that the new name conflicts with a sibling's name, prompting _generate_serial_child_name(p_child, name); to append a number to the renaming Node's name. Unfortunately, the value of the exported variable is not updated, causing the bug.

Fixes: #76192
Fixes: #76680

[ajreckof]

This a good fix. Unfortunately this only fixes it for basic renames and do not fix it for batch rename. You should do the same here to fix it for batch rename too

[ajreckof]

I would rename this function to prevalidate_child_name to make it clear what is the difference with the other function.

[ajreckof]

While reviewing this and working on #76376 at the same time, I realized Batch Rename was too fucked up for an easy fix and would need to wait for my proper fix in the linked PR. I think you should re-focus this PR solely on fixing the original bug with only simple renames. I'm so sorry as this was my fault for bringing up Batch rename in the first place.
The current state of the PR is non functional as it has a lot of problems with undo redo. I would advise you to remake it to its original state which was functional and in a mergeable state to fix the original bug for basic renames. the other possibility is to wait for my other PR to be merged then you will be able to make a proper fix for the original bug for both basic renames and batch renames.

[nongvantinh]

I agree with you. However, the current implementation of UndoRedo does not align with the node_prerename signal and the _rename_node method. Therefore, I have made updates to the PR to resolve the current issue. In case the team wishes to merge the original PR in the future, I have made sure to revert the changes.

The problem is:

godot/editor/rename_dialog.cpp

Line 603 in 64eeb04

scene_tree_editor->emit_signal(SNAME("node_prerename"), n, new_name);

The emit_signal method in scene_tree_editor does not work with UndoRedo. This is why it was not used with UndoRedo in the first place. When iterating through the nodes to perform a rename, it keeps notifying that it is going to change and tells the subscription's methods to update the NodePath accordingly. However, these changes are only made when undo_redo->commit_action(); is called.

[ajreckof]

now that #76376 is merged would you mind rebasing this to be included in 4.1 ? This should be similar to what you had done first just placed in the function _rename_node this would handle both normal renames and batch rename as both call this function now.

[ajreckof]

I like the idea of this PR. Would you mind just renaming the function to prevalidate_child_name as i suggested in a previous commentary?

[ajreckof]

This should be a bit above around line 981 to prevent renaming when the validated new name is the same as the current name. Take the previous line with it.

[AThousandShips]

Superseded by:

• Fix renaming a node to the name of its siblings breaking NodePath #87252