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

Rename ProjectSettings macros to follow the singleton's rename #50350

Closed

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jul 10, 2021

The Globals singleton was renamed to ProjectSettings in 3.0. Additionally, this renames some of the macros to be more explicit (including for EditorSettings).

_DEF was renamed to _DEFAULT to avoid having the same length as _GET (which can look confusing at a glance).

This also adds comments above each macro, which will appear in many IDEs when hovering the macro names.

See #16863 (comment).

@Calinou Calinou removed request for a team July 10, 2021 18:35
@Calinou Calinou force-pushed the rename-projectsettings-macros branch from 90754a9 to 91f80de Compare July 10, 2021 18:35
The Globals singleton was renamed to ProjectSettings in 3.0.
Additionally, this renames some of the macros to be more explicit
(including for EditorSettings).

This also adds comments above each macro, which will appear in many IDEs
when hovering the macro names.
@Calinou Calinou force-pushed the rename-projectsettings-macros branch from 91f80de to 5f8663e Compare July 10, 2021 18:55
@Calinou Calinou requested review from a team as code owners July 10, 2021 18:55
@@ -510,19 +510,19 @@ Error ProjectSettings::_setup(const String &p_path, const String &p_main_pack, b
Error ProjectSettings::setup(const String &p_path, const String &p_main_pack, bool p_upwards) {
Error err = _setup(p_path, p_main_pack, p_upwards);
if (err == OK) {
String custom_settings = GLOBAL_DEF("application/config/project_settings_override", "");
String custom_settings = PROJECT_DEFAULT("application/config/project_settings_override", "");
Copy link
Member

Choose a reason for hiding this comment

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

GLOBAL_DEF is not short for GLOBAL_DEFAULT, its GLOBAL_DEFINE.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is a good argument for renaming it then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha I always thought it was define, not default, and was a bit confused reading the OP.

@reduz
Copy link
Member

reduz commented Jul 12, 2021

I am not sure myself, not against a rename, but what is proposed does not seem better than what it is already there. Maybe PROJECT_SETTING_DEF and PROJECT_SETTING_GET
EDITOR_SETTING_DEF and EDITOR_SETTING_GET

@EricEzaM
Copy link
Contributor

EricEzaM commented Jul 12, 2021

As a reminder, having project settings being able to be defined anywhere does not work so well with the doctool:

godotengine/godot-proposals#1339 (comment) and #48548 (comment)

[...] as a result of the EditorNode and related classes not being constructed [when running doctool], some project settings are actually left out of the documentation, such as editor/main_run_args which has it's GLOBAL_DEF in the EditorNode constructor, so when running the doctool it is never called.

Same problem occurs with EditorSettings, e.g. (e.g. EditorNode EDITOR_DEF("run/output/always_clear_output_on_play", true);) will never be in the documentation, even with #48548

@reduz
Copy link
Member

reduz commented Jul 12, 2021

@EricEzaM the general idea is that they are defined on initialization, but having them one in a place is quite messy, I prefer to have them closer to where they are relevant if possible.

@aaronfranke
Copy link
Member

@reduz What is messy about it? We already do this with editor settings.

@mhilbrunner
Copy link
Member

PR meeting: DEFAULT -> DEFINE

Also we're not against this, but we maybe should take the opportunity to modernise this
(@akien-mga, @aaronfranke @reduz had input, so maybe discuss further with them)

@Calinou Calinou closed this Jul 30, 2022
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.

5 participants