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

Add ability to rename groups in the GroupsEditor #62659

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Jul 3, 2022

QoL change to the Groups editor:

group_renaming

Previously user must press Manage Groups to call the dialog to change it. I didnt checked how it would interact with #60965. If that PR would superseeds it, that would be good, if not, it can be used on its own.

Also added a check to prevent redundanly calling create_action(TTR("Rename Group")); if the group name was unchanged.

@@ -217,19 +217,14 @@ void GroupDialog::_add_group_text_changed(const String &p_new_text) {
}

void GroupDialog::_group_renamed() {
TreeItem *renamed_group = groups->get_edited();
TreeItem *renamed_group = groups->get_selected();
Copy link
Member Author

Choose a reason for hiding this comment

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

This was changed to prevent bug:
group_renaming_bug

@Chaosus Chaosus requested review from akien-mga and Paulb23 July 3, 2022 09:00
@Chaosus Chaosus force-pushed the editor_group_rename_option branch from 881370c to 54ea4fc Compare July 3, 2022 09:14
@Chaosus Chaosus marked this pull request as draft July 3, 2022 09:26
@Chaosus Chaosus marked this pull request as ready for review July 3, 2022 09:41
@akien-mga
Copy link
Member

We discussed this in a PR review meeting, and there's one issue with this approach. The group dialog (which you get when clicking "Manage Groups") lets you modify groups for all nodes in these groups, so when renaming a group there, it gets renamed for all nodes.

While this PR changes the Node-specific group dock, and renaming a group there should not affect other nodes. So this should be changed to only change the group for the selected Node (i.e. creating a new group instead of renaming an existing one that might be shared by others).

@Chaosus Chaosus force-pushed the editor_group_rename_option branch from 54ea4fc to 2efbba7 Compare July 5, 2022 16:03
@Chaosus
Copy link
Member Author

Chaosus commented Jul 5, 2022

@akien-mga Done, I think

@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2022

There is a bug that happens when you rename a group from an inherited scene:
godot windows editor dev x86_64_Qz5dyKXNJF

(here's a rebased patch btw)

@Chaosus
Copy link
Member Author

Chaosus commented Oct 4, 2022

@KoBeWi ah, it's just rebasing - I thought you've fixed a bug :)

@Chaosus
Copy link
Member Author

Chaosus commented Oct 4, 2022

@KoBeWi that bug is not related to this PR (at least directly).

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Approved in PR review meeting.

@akien-mga akien-mga merged commit bc7981d into godotengine:master Oct 11, 2022
@akien-mga
Copy link
Member

Thanks!

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.

None yet

4 participants