Improve editor property capitalization #32734

Calinou · 2019-10-10T23:57:48Z

An hardcoded list of exceptions is now used to make property name capitalization more natural.

I'm not too happy with this implementation – maybe we should add an optional "friendly name" property hint in the end. Also, it takes more time to run the string replacements (30 µs on average for each string in a debug build, compared to 10 µs with the old method). Still, it's not run every frame so it should be fine performance-wise.

There are other improvements we can do, such as not capitalizing stop words like at, in and to. We could do this for the general-purpose String::capitalize() function in 4.0.

Preview

Look at the section and property names.

This closes godotengine/godot-proposals#1399.

editor/editor_sectioned_inspector.cpp

realkotob · 2019-10-11T12:41:02Z

String::capitalize() is pretty vague, usually (for e.g. in VSCode) the options are either UPPER CASE or Title Case, String::capitalize() could even refer to Sentence case :D

Could there be a way to make that clearer?

Calinou · 2019-10-11T13:41:49Z

@asheraryam Maybe it could be renamed to String::title_case() in 4.0.

aaronfranke · 2019-10-12T23:38:14Z

Since it's not strictly Title Case, it would make sense to call it something else. What about proper_case, since it fixes issues in order to make the capitalization proper.

aaronfranke

Would help fix #28748 if the Mono module uses this. CC @neikeq, subscribe so that you'll be notified. @Calinou Consider looking through #28748 for things to add to this list.

aaronfranke · 2019-11-15T21:46:25Z

editor/editor_node.cpp

+ capitalize_string_remaps["Gles 3"] = "GLES3";
+ capitalize_string_remaps["Hdr"] = "HDR";
+ capitalize_string_remaps["Hidpi"] = "hiDPI";
+ capitalize_string_remaps["Kb"] = "KB";


Where is "Kb" used? It might be a good idea to replace it with the binary prefix version "KiB". Also, by the time this will be merged (4.0), GLES3 will have been removed, consider removing it from this list.

aaronfranke · 2020-07-01T07:04:09Z

@Calinou Any update? This needs to be rebased on the latest master branch.

Also, I had an idea which might work for some situations. What if Godot's auto-capitalization had a rule where it only changed lowercase letters, so any pre-existing uppercase letters are kept as uppercase always? This would work for class names and any other situation where uppercase letters are used for acronyms in the name.

neikeq · 2020-07-01T09:49:22Z

I don't think this is the best solution. IMO we should make it possible to specify a descriptive name in addition to the property name. This way we wouldn't have to rely on the engine's hard-coded list to have the entries we want. It would also allow localization of those settings names.

Calinou · 2020-07-07T13:19:19Z

@neikeq Your proposal is definitely a better solution, but I don't know if we can afford adding yet another (optional) parameter to ProprertyInfo which already takes quite a lot of parameters.

Vivraan · 2020-08-17T17:18:19Z

Please merge, my eyes have found peace.

neikeq · 2020-08-18T18:13:43Z

editor/editor_node.cpp

@@ -4636,6 +4636,18 @@ void EditorNode::_scene_tab_changed(int p_tab) {
 editor_data.get_undo_redo().commit_action();
 }

+String EditorNode::capitalize_string(const String &p_string) const {
+
+ String capitalized_string = p_string.capitalize();


Is there a case where two different strings could result in the same capitalized string? If the answer is yes, then the current implementation is understandable. If not, then this can be optimized by only calling capitalize() if a remap could not be found below.

I agree, and this would remove the awkwardness of having remappings based on the wrongly capitalized property paths (which can change if we ever change what capitalize() does).

Instead, remap directly based on the property path substrings:

capitalize_string_remaps["s3tc"] = "S3TC";

neikeq

While I still think the solution I mentioned previously is great as it allows localization, I'm now in favour of merging this because:

Short-term it's the only solution as mine requires much more work.
I just realized I can re-use this capitalize_string in the C# bindings generator. I had already planned to create my own hard-coded remaps for generating better names (like AddiOSFramework instead of AddIosFramework). With the new capitalize_string I will no longer have to do that as I can re-use this.

Vivraan · 2020-08-19T07:30:57Z

While I still think the solution I mentioned previously is great as it allows localization, I'm now in favour of merging this because:

Short-term it's the only solution as mine requires much more work.

I just realized I can re-use this capitalize_string in the C# bindings generator. I had already planned to create my own hard-coded remaps for generating better names (like AddiOSFramework instead of AddIosFramework). With the new capitalize_string I will no longer have to do that as I can re-use this.

According to the .NET Capitalization Conventions, the latter is correct and should be preferred over the former.

aaronfranke · 2020-08-19T08:32:35Z

@Vivraan OS is an acronym, so it should be capitalized. Reading the document you linked, I'm confused as to why they chose to treat acronyms of 2 letters and acronyms of more than 2 letters differently, it seems inconsistent. An argument for AddIOSFramework could be made I suppose, but I don't like AddIosFramework.

Vivraan · 2020-08-19T08:36:37Z

@Vivraan OS is an acronym, so it should be capitalized. Reading the document you linked, I'm confused as to why they chose to treat acronyms of 2 letters and acronyms of more than 2 letters differently, it seems inconsistent. An argument for AddIOSFramework could be made I suppose, but I don't like AddIosFramework.

iOS is a three letter acronym, so it becomes AddIosFramework. I've digested the spec.
It is also widely followed (an exception could be Unity which has its own naming convention for internally developed packages).
On the other hand, something like macOS would become MacOS.

Consider BSD - AddBsdFramework.
Or ARM - AddArmFramework.

However, I have seen Godot use what you described as PascalCase (consider AABB instead of Aabb). We could have our own spec. Now, wouldn't that resolve all disputes?

Calinou · 2020-08-19T08:42:49Z

However, I have seen Godot use what you described as PascalCase. We could have our own spec. Now, wouldn't that resolve all disputes?

I think both "HttpRequest" and "HTTPRequest" can claim to be PascalCase. These just happen to be different flavors of PascalCase. Either way, Godot has settled on the latter, so I wouldn't change it in our API.

Vivraan · 2020-08-19T08:49:55Z

Here's a note on some camel case variants: https://wiki.c2.com/?CapitalizationRules

neikeq · 2020-08-19T16:05:04Z

Let's move the C# conversation somewhere else (feel free to open a godot-proposal where we can discuss the desired rules) as it's not the topic of this PR. If the names are not fine for C#, we can always make our own remap table as I initially had intended.

editor/editor_node.cpp

editor/editor_sectioned_inspector.cpp

akien-mga · 2021-01-07T09:48:30Z

editor/editor_node.cpp

@@ -4824,9 +4824,21 @@ void EditorNode::_scene_tab_changed(int p_tab) {
 editor_data.get_undo_redo().commit_action();
 }

+String EditorNode::capitalize_string(const String &p_string) const {


We should avoid putting all kinds of helpers in EditorNode as it makes it huge. Not sure where to put this though, maybe @EricEzaM has an idea since he's been reworking this code.

Looks like it can go into String itself.

No, in Godot solutions must be local and such editor-specific helper code is not something that should be part of the core String API (just like we wouldn't try to add it to std::string if we used the STL).

https://docs.godotengine.org/en/latest/community/contributing/best_practices_for_engine_contributors.html#solutions-must-be-local

An hardcoded list of exceptions is now used to make property name capitalization more natural.

reduz · 2021-08-30T20:12:16Z

I think this PR is mostly good, but I would not further bloat EditorNode, which is already incredibly bloated. Just add a new singleton like EditorStringCapitalization or similar and put stuff there.

akien-mga · 2022-03-10T20:10:04Z

Superseded by #58706.

Calinou added enhancement topic:editor labels Oct 10, 2019

Calinou force-pushed the improve-editor-property-capitalization branch from 26603b4 to b70aee2 Compare October 11, 2019 00:02

YeldhamDev reviewed Oct 11, 2019

View reviewed changes

editor/editor_sectioned_inspector.cpp Show resolved Hide resolved

Calinou force-pushed the improve-editor-property-capitalization branch from b70aee2 to 8f65925 Compare October 11, 2019 08:22

Calinou force-pushed the improve-editor-property-capitalization branch from 8f65925 to 94aa68f Compare October 11, 2019 13:42

Chaosus added this to the 4.0 milestone Nov 7, 2019

aaronfranke reviewed Nov 15, 2019

View reviewed changes

aaronfranke mentioned this pull request Nov 16, 2019

PascalCase naming inconsistencies with C# #28748

Closed

neikeq reviewed Aug 18, 2020

View reviewed changes

neikeq approved these changes Aug 18, 2020

View reviewed changes

Calinou mentioned this pull request Aug 22, 2020

Improve editor property capitalization godotengine/godot-proposals#1399

Closed

Calinou mentioned this pull request Oct 18, 2020

[Complex Text Layouts] Implement TextServer interface. #41100

Merged

Calinou force-pushed the improve-editor-property-capitalization branch from 94aa68f to 953381c Compare January 7, 2021 00:15