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

Fix batch rename for unique name and empty name #78292

Conversation

ajreckof
Copy link
Member

Fixes #78290
Fixes #78291

@ajreckof ajreckof force-pushed the Fix-batch-rename-for-unique-name-and-empty-name- branch 2 times, most recently from ac3a396 to 9d1192f Compare June 15, 2023 22:39
@YeldhamDev YeldhamDev added this to the 4.1 milestone Jun 16, 2023
@akien-mga akien-mga requested a review from KoBeWi June 19, 2023 21:20
@ajreckof ajreckof force-pushed the Fix-batch-rename-for-unique-name-and-empty-name- branch from 9d1192f to b9af386 Compare June 20, 2023 21:23
@ajreckof
Copy link
Member Author

Implemented what kobewi suggested with both it now look like this.
Capture d’écran 2023-06-20 à 23 22 22

KoBeWi
KoBeWi previously approved these changes Jun 20, 2023
@KoBeWi KoBeWi dismissed their stale review June 21, 2023 00:21

sorry I just found a rather important bug xd

@KoBeWi
Copy link
Member

KoBeWi commented Jun 21, 2023

The approach for appending errors needs to be reworked, as it's possible to stack errors from multiple nodes:
image

@YuriSizov YuriSizov changed the title Fix batch rename for unique name and empty name. Fix batch rename for unique name and empty name Jun 21, 2023
@ajreckof ajreckof force-pushed the Fix-batch-rename-for-unique-name-and-empty-name- branch from b9af386 to 63119e5 Compare June 21, 2023 11:42
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased on top of master cb73a6e), it works.

However, I can only confirm the first case (empty node name) is resolved, as I can't test the second issue. I never see the Batch Rename option in the context menu when selecting several nodes if both are scene-unique, and this also applies in master.

Approving nonetheless, as this is an improvement over the existing situation.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 22, 2023

I never see the Batch Rename option in the context menu when selecting several nodes

Batch Rename does not appear if you select a parent and child node together. Likely a bug.

@ajreckof
Copy link
Member Author

ajreckof commented Jun 22, 2023

I never see the Batch Rename option in the context menu when selecting several nodes

Batch Rename does not appear if you select a parent and child node together. Likely a bug.

Indeed See #76069

@akien-mga akien-mga requested a review from KoBeWi June 23, 2023 06:45
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 23, 2023
@YuriSizov
Copy link
Contributor

Besides a review from @KoBeWi this also needs a rebase.

@ajreckof ajreckof force-pushed the Fix-batch-rename-for-unique-name-and-empty-name- branch 3 times, most recently from 30420e9 to e9d630d Compare July 7, 2023 22:06
@ajreckof
Copy link
Member Author

ajreckof commented Jul 7, 2023

So I rebased this. The conflicting fix was about making sure it wouldn't raise an error when renaming a node to the same name when it is an unique name in owner. I moved this fix to the new places from this PR

@ajreckof ajreckof force-pushed the Fix-batch-rename-for-unique-name-and-empty-name- branch 2 times, most recently from da02b0b to 98a0fe1 Compare July 8, 2023 01:11
@YuriSizov
Copy link
Contributor

I think the added meta should be prefixed with "_" by convention. Otherwise, this seems to address @KoBeWi's concern, albeit in a rather awkward way. Can't we set the meta and then compose a line by checking for each possible key, instead of doing this nested if thing?


// we previously made sure name is not the same as current name so that it wonn't complain about already used unique name when not changing name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// we previously made sure name is not the same as current name so that it wonn't complain about already used unique name when not changing name
// We previously made sure name is not the same as current name so that it won't complain about already used unique name when not changing name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Poke @ajreckof, would be good to fix this before merging.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 31, 2023

I think the added meta should be prefixed with "_" by convention.

This convention is irrelevant for internal editor nodes.

@ajreckof ajreckof force-pushed the Fix-batch-rename-for-unique-name-and-empty-name- branch from 98a0fe1 to 4909396 Compare August 3, 2023 16:27
@akien-mga akien-mga merged commit 179e3d6 into godotengine:master Aug 3, 2023
@akien-mga
Copy link
Member

Thanks!

@ajreckof ajreckof deleted the Fix-batch-rename-for-unique-name-and-empty-name- branch August 5, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch rename won't check for duplicate unique name Batch rename to empty string will behave strangely.
6 participants