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.

Sign up for GitHub

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

Jump to bottom

Fix path typo for editor def capitalize_properties #19568

Merged
merged 1 commit into from
Jun 15, 2018
Merged

Fix path typo for editor def capitalize_properties #19568

merged 1 commit into from
Jun 15, 2018

Conversation

guilhermefelipecgs
Copy link
Contributor

No description provided.

akien-mga
akien-mga reviewed Jun 15, 2018
View reviewed changes
editor/editor_node.cpp
@@ -1381,7 +1381,7 @@ void EditorNode::_edit_current() {
return;
}

bool capitalize = bool(EDITOR_DEF("interface/editor/capitalize_properties", true));
bool capitalize = bool(EDITOR_GET("interface/inspector/capitalize_properties"));
Copy link
Member

Choose a reason for hiding this comment

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

Why is there both interface/inspector/capitalize_properties and interface/editor/capitalize_properties?

Copy link
Member

Choose a reason for hiding this comment

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

$ rg "interface/.*/capitalize_properties"
editor/inspector_dock.cpp
556:    inspector->set_enable_capitalize_paths(bool(EDITOR_DEF("interface/editor/capitalize_properties", true)));

editor/editor_node.cpp
1384:   bool capitalize = bool(EDITOR_DEF("interface/editor/capitalize_properties", true));
4639:   EDITOR_DEF("interface/inspector/capitalize_properties", true);

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like it should also be interface/inspector in inspector_dock.cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is there both interface/inspector/capitalize_properties and interface/editor/capitalize_properties?

interface/editor/capitalize_properties is wrong, if you look in code, both are used in inspector.

@akien-mga akien-mga added this to the 3.1 milestone Jun 15, 2018
akien-mga
akien-mga reviewed Jun 15, 2018
View reviewed changes
editor/inspector_dock.cpp Outdated
@@ -553,7 +553,7 @@ InspectorDock::InspectorDock(EditorNode *p_editor, EditorData &p_editor_data) {
inspector->set_v_size_flags(Control::SIZE_EXPAND_FILL);
inspector->set_use_doc_hints(true);
inspector->set_hide_script(false);
inspector->set_enable_capitalize_paths(bool(EDITOR_DEF("interface/editor/capitalize_properties", true)));
inspector->set_enable_capitalize_paths(bool(EDITOR_GET("interface/editor/capitalize_properties")));
inspector->set_use_folding(!bool(EDITOR_DEF("interface/editor/disable_inspector_folding", false)));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also rename this to interface/inspector/disable_folding while at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, It seems I forgot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

akien-mga
akien-mga reviewed Jun 15, 2018
View reviewed changes
editor/inspector_dock.cpp Outdated
@@ -553,7 +553,7 @@ InspectorDock::InspectorDock(EditorNode *p_editor, EditorData &p_editor_data) {
inspector->set_v_size_flags(Control::SIZE_EXPAND_FILL);
inspector->set_use_doc_hints(true);
inspector->set_hide_script(false);
inspector->set_enable_capitalize_paths(bool(EDITOR_DEF("interface/editor/capitalize_properties", true)));
inspector->set_enable_capitalize_paths(bool(EDITOR_GET("interface/inspector/capitalize_properties")));
inspector->set_use_folding(!bool(EDITOR_DEF("interface/editor/disable_inspector_folding", false)));
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned, I think this one should also get s/editor/inspector/ for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

(and then drop "inspector" from the property name, so interface/inspector/disable_folding)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

akien-mga
akien-mga reviewed Jun 15, 2018
View reviewed changes
editor/inspector_dock.cpp Outdated
inspector->set_enable_capitalize_paths(bool(EDITOR_DEF("interface/editor/capitalize_properties", true)));
inspector->set_use_folding(!bool(EDITOR_DEF("interface/editor/disable_inspector_folding", false)));
inspector->set_enable_capitalize_paths(bool(EDITOR_GET("interface/inspector/capitalize_properties")));
inspector->set_use_folding(!bool(EDITOR_DEF("interface/inspector/disable_inspector_folding", false)));
Copy link
Member

Choose a reason for hiding this comment

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

I still think the second _inspector is redundant... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, maybe, I'll test it here and if it's not necessary I'll remove than.

Copy link
Contributor Author

@guilhermefelipecgs guilhermefelipecgs Jun 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You'are referring to interface/inspector/capitalize_properties right?

Copy link
Member

@akien-mga akien-mga Jun 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No, I'm just saying that "interface/inspector/disable_inspector_folding" (line 557 which I'm commenting on) should be "interface/inspector/disable_folding".

"disable_folding" is enough if we're already in the "inspector" category, no need to restate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood now, I fixed disable_folding to update without need to restart godot too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed disable_folding to update without need to restart godot too.

I made some changes just to guarantee that performance will not be affected.

@guilhermefelipecgs guilhermefelipecgs force-pushed the fix_editor_def branch 2 times, most recently from 1a8a5e1 to 1172c7a Compare June 15, 2018 14:40
akien-mga
akien-mga reviewed Jun 15, 2018
View reviewed changes
editor/editor_node.cpp Outdated
@@ -1437,6 +1438,7 @@ void EditorNode::_edit_current() {
if (current_obj->is_class("ScriptEditorDebuggerInspectedObject")) {
editable_warning = TTR("This is a remote object so changes to it will not be kept.\nPlease read the documentation relevant to debugging to better understand this workflow.");
capitalize = false;
folding = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why enable folding for the debugger inspector? You typically want to see values in the debugger, and I think it's better to keep it using the same setting as the normal inspector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's do the opposite, I'll fix this name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it's more legible I hope.

@akien-mga akien-mga merged commit 4c41f6c into godotengine:master Jun 15, 2018
@guilhermefelipecgs guilhermefelipecgs deleted the fix_editor_def branch June 15, 2018 16:11
@YuriSizov YuriSizov mentioned this pull request Aug 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
bug topic:editor
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
2 participants
@guilhermefelipecgs @akien-mga