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

In Create New Scene dialog derive the default root node name based on editor/naming/node_name_casing #79756

Merged
merged 1 commit into from
Aug 2, 2023

Conversation

sfreed141
Copy link
Contributor

Fixes #79751

This PR updates the Create New Scene dialog to respect the editor/naming/node_name_casing project setting for the root node name (when it's left empty). I also updated the placeholder text to be more accurate, as well as add a tooltip to the line edit control with more information (so users can discover the project setting if they aren't aware of it yet)

@sfreed141
Copy link
Contributor Author

(force push was to fix style issue)

@Sauermann Sauermann added this to the 4.2 milestone Jul 21, 2023
@dalexeev
Copy link
Member

The static method Node::adjust_name_casing() with the functionality already exists.

godot/scene/main/node.cpp

Lines 1199 to 1209 in 6588a4a

String Node::adjust_name_casing(const String &p_name) {
switch (GLOBAL_GET("editor/naming/node_name_casing").operator int()) {
case NAME_CASING_PASCAL_CASE:
return p_name.to_pascal_case();
case NAME_CASING_CAMEL_CASE:
return p_name.to_camel_case();
case NAME_CASING_SNAKE_CASE:
return p_name.to_snake_case();
}
return p_name;
}

Also note that the to_pascal_case() method does not respect specific abbreviations (e.g. Node2D is converted to Node2d). See #67219 for details.

@sfreed141
Copy link
Contributor Author

sfreed141 commented Jul 22, 2023

Thanks for the info @dalexeev, I'll update the PR to use Node::adjust_name_casing!

Also note that the to_pascal_case() method does not respect specific abbreviations (e.g. Node2D is converted to Node2d)

I was wondering about these edge cases too, but since it's quite simple to change the name after the fact (or in the Create New Scene dialog) I'm not concerned about it. This PR should still improve the majority of cases (since the current workflow essentially requires you to rename everything manually, assuming a difference in naming convention between filenames and node names).

One way to address that could be to show the converted name as placeholder text in realtime (as the scene name is modified). This way a user would be able to see if the name is converted as expected and, if needed, manually enter a name. Thoughts?

@dalexeev
Copy link
Member

String new_name = p_parent->validate_child_name(child);
if (GLOBAL_GET("editor/naming/node_name_casing").operator int() != NAME_CASING_PASCAL_CASE) {
new_name = adjust_name_casing(new_name);
}
child->set_name(new_name);

The built-in classes are already named in PascalCase with the correct capitalization, so it's probably justified to do nothing for the PascalCase option.

CC @KoBeWi

@sfreed141
Copy link
Contributor Author

sfreed141 commented Jul 23, 2023

@dalexeev I'm confused what you mean here. This PR is unrelated to the capitalization of built-in classes. Perhaps a screenshot will make this more clear:
image
image
This PR is automatically adjusting the Root Name to respect the node_name_casing option (the prior behavior was copying the Scene Name).

Also I pushed an update to use adjust_name_casing and show the derived name in the placeholder text (the above screenshot includes this change. Notice the placeholder text in Root Name reflects the result of adjust_name_casing).

@dalexeev
Copy link
Member

@sfreed141 Sorry, I confused this with another issue, since I don't use the dialog, but just press the plus button to add an empty scene tab. Then I think that it makes sense to capitalize as is, with to_pascal_case(). Showing a preview of the generated name is a good idea, thank you!

Please squash your commits into one with a suitable message, according to the docs.

editor/scene_create_dialog.cpp Outdated Show resolved Hide resolved
editor/scene_create_dialog.cpp Outdated Show resolved Hide resolved
@sfreed141
Copy link
Contributor Author

Thanks for the feedback @dalexeev and @AThousandShips, the changes have been made and pushed.

dalexeev
dalexeev previously approved these changes Jul 24, 2023
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me and works great!

editor/scene_create_dialog.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member

KoBeWi commented Jul 25, 2023

Displaying final root name is something I had on my TODO list, so it's nice to see it being implemented.

I wonder if the name could be validated though. E.g. if you name your scene like this:
image
You will get this:
image
We already lack a warning if you put invalid root name that's going to be renamed, so it's something that could be fixed later though.

EDIT:
btw the original comment by dalexeev was to avoid this:
image
but I guess it's not a problem in custom scenes 🤔

@dalexeev
Copy link
Member

I wonder if the name could be validated though.

There is validate_node_name() method.

@sfreed141
Copy link
Contributor Author

@KoBeWi @dalexeev thanks for the suggestions, I'll incorporate those into this PR. A question though: if the derived name contains invalid values is there a precedent for how to display that? Right now I'm thinking of having the placeholder text start with a "⚠️" and have the tooltip say that invalid node name characters have been replaced with an "_". Thoughts?

@KoBeWi
Copy link
Member

KoBeWi commented Jul 26, 2023

You could add a yellow warning message in the validation panel.

@sfreed141
Copy link
Contributor Author

Updated. Here are some screenshots of the behavior now:
empty scene name
image
valid characters
image
invalid characters
image
image

@sfreed141
Copy link
Contributor Author

(forgot a period)

editor/scene_create_dialog.cpp Outdated Show resolved Hide resolved
editor/scene_create_dialog.cpp Outdated Show resolved Hide resolved
@sfreed141 sfreed141 force-pushed the root-node-casing-fix branch 2 times, most recently from a9ee082 to 472158f Compare July 27, 2023 17:21
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for your patience!

@YuriSizov YuriSizov requested a review from KoBeWi August 1, 2023 16:54
@akien-mga akien-mga merged commit 93c69a2 into godotengine:master Aug 2, 2023
13 checks passed
@akien-mga
Copy link
Member

akien-mga commented Aug 2, 2023

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants