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

Properly update nodepath with batch rename #76376

Merged

Conversation

ajreckof
Copy link
Member

@ajreckof ajreckof commented Apr 23, 2023

Fixes #76070
Fixes #76680
partially fixes #76679 (gives a warning after rename with invalid character but does not print a warning on the batch rename dialog box)

This changes the way undo redo was handled so that prerename signal is triggered just before the actual rename and not all at once at the start. This is important as prerename needs previous rename to have done so that Nodepath are valid.

Edit : this goes a bit further than the original commit as it was lacking in some places. It places in _rename_node everything that was in _renamed and would also apply for batch rename. This includes :

  • checking for invalid character
  • handling undo redo
  • emiting node_prerename

Unfortunatly this need this other PR #76688 to be merged first (it was added as first commit here as it is needed)

@fire fire changed the title properly update nodepath with batch rename Properly update nodepath with batch rename Apr 23, 2023
@Chaosus Chaosus added this to the 4.1 milestone Apr 24, 2023
@akien-mga akien-mga requested a review from KoBeWi April 25, 2023 13:39
@ajreckof ajreckof force-pushed the fix_NodePath_update_from_batch_rename branch from 33cb450 to a8dee4a Compare May 3, 2023 01:32
@ajreckof ajreckof requested a review from a team as a code owner May 3, 2023 01:32
@ajreckof ajreckof force-pushed the fix_NodePath_update_from_batch_rename branch from a8dee4a to 7f56b21 Compare May 3, 2023 02:41
@ajreckof ajreckof requested a review from a team as a code owner May 3, 2023 02:41
@KoBeWi
Copy link
Member

KoBeWi commented Jun 2, 2023

This PR introduces changes to the UndoRedo system. I think they are fine, but IMO they should be moved to a separate PR and the new arguments need to be documented.

@kleonc
Copy link
Member

kleonc commented Jun 2, 2023

This PR introduces changes to the UndoRedo system. I think they are fine, but IMO they should be moved to a separate PR and the new arguments need to be documented.

@KoBeWi From the PR description:

Unfortunatly this need this other PR #76688 to be merged first (it was added as first commit here as it is needed)

🙃

@KoBeWi
Copy link
Member

KoBeWi commented Jun 2, 2023

🙃

@akien-mga
Copy link
Member

Can be rebased now that the dependent PR has been merged.

@ajreckof ajreckof force-pushed the fix_NodePath_update_from_batch_rename branch from 7f56b21 to ba6e8e6 Compare June 14, 2023 09:21
@ajreckof
Copy link
Member Author

Rebased.

@ajreckof ajreckof force-pushed the fix_NodePath_update_from_batch_rename branch from ba6e8e6 to 263c356 Compare June 14, 2023 11:53
@KoBeWi
Copy link
Member

KoBeWi commented Jun 14, 2023

UndoRedo does not work anymore with batch rename.

@ajreckof ajreckof force-pushed the fix_NodePath_update_from_batch_rename branch from 263c356 to 5c15083 Compare June 14, 2023 13:17
@ajreckof
Copy link
Member Author

Yeah I think I messed up the rebase which made the commit_action disappear. I fixed it.

@akien-mga akien-mga merged commit 2c51eca into godotengine:master Jun 15, 2023
@akien-mga
Copy link
Member

Thanks!

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