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

Allow to override editor settings per project #69012

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 22, 2022

This PR allows to override any editor setting for any project, so you can customize your editor per-project.

Closes godotengine/godot-proposals#1480

godot.windows.editor.dev.x86_64_49rzlQvaqS.mp4

@KoBeWi KoBeWi added this to the 4.x milestone Nov 22, 2022
@KoBeWi KoBeWi requested a review from a team as a code owner November 22, 2022 16:18
@adamscott

This comment was marked as resolved.

@KoBeWi KoBeWi force-pushed the settings_of_editor_project branch from a58fc36 to e552960 Compare November 22, 2022 16:24
@Mickeon
Copy link
Contributor

Mickeon commented Nov 22, 2022

To me it would make sense the text was either warning yellow or accent color. However, warning yellow would be akin to inherited properties in the Inspector, which is not quite the same concept. So, maybe the accent color, plus a symbol to the side of the Label, plus an additional paragraph to the tooltip explaining what is going on may be nice

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 22, 2022

The dialog uses standard inspector, so not sure how to display custom stuff without making too many specific changes.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 22, 2022

Well, what is it that you could do? To begin from something concrete.

@RedMser
Copy link
Contributor

RedMser commented Nov 22, 2022

Some things I noticed:

  • Possible solutions for better override UX
    1. As mentioned already, editor settings need some different styling if overridden (text color, ideally a button to jump to the corresponding override, maybe renaming the setting to Setting Name (overridden)?).
    2. Maybe the "Editor Overrides" project settings category could show up as a tab in Editor Settings dialog? So next to "General" have another tab for "Project Overrides" maybe? So the user doesn't have to wrangle two dialogs, since you can easily jump from one tab to another (maybe automatically do this after creating the override even).
    3. Below each editor setting that is overridden, show a second entry to edit the override's value directly (kinda like with the feature tag suffixes .debug etc.). Would most definitely need larger inspector changes to work properly, so let's file this under "future ideas for improvement".
  • Overridden settings should copy over the hint, hint string and documentation tooltip of the original editor setting.
  • Changing override value caused WARNING: Property not found: editor_overrides/interface/scene_tabs/display_close_button at: ProjectSettings::_get (core\config\project_settings.cpp:349) for me. This also happens when working with custom project settings so I guess it's fine.
  • The editor overrides API should probably be made available for GDScript, so plugins can more easily specify editor overrides (as was one of the use cases mentioned in the proposal).

@dalexeev
Copy link
Member

dalexeev commented Nov 23, 2022

I think this is a good feature, but there are two remarks:

  1. Two places to edit (Editor Settings and Project Settings), it looks confusing and not very user friendly. Even if these overrides are stored in project.godot (rather than, say, editor_settings_overrides.cfg), they should be hidden from the user.
  2. Now the whole project settings section (editor/*) is not needed and should be moved to Editor Settings.

Mockup:

@Mickeon
Copy link
Contributor

Mickeon commented Nov 23, 2022

Now the whole project settings section (editor/*) is not needed and should be moved to Editor Settings.

One thing at the time.

The look of the mock-up is... kind of interesting, although I'm not sure it's ideal for this. It's not like you'll see different names for "Override for _" in one project, so it just ends up being redundant.

@dalexeev
Copy link
Member

dalexeev commented Nov 23, 2022

Another option is to use two columns for global settings and overrides for the current project, like in git gui:

It's probably better for UX (it's not obvious to everyone that setting names have a context menu), but more cluttered.

It's not like you'll see different names for "Override for _" in one project, so it just ends up being redundant.

It could just be "Override for current project".

@Mickeon
Copy link
Contributor

Mickeon commented Oct 5, 2023

The more warning settings are added, the more necessary this feature feels.

@KoBeWi KoBeWi force-pushed the settings_of_editor_project branch from 62831b4 to 8ce35e8 Compare August 5, 2024 22:33
@KoBeWi KoBeWi requested review from a team as code owners August 5, 2024 22:33
Comment on lines -347 to -351
EditorSettings::get_singleton()->set_manually("interface/theme/preset", config.preset);
EditorSettings::get_singleton()->set_manually("interface/theme/accent_color", config.accent_color);
EditorSettings::get_singleton()->set_manually("interface/theme/base_color", config.base_color);
EditorSettings::get_singleton()->set_manually("interface/theme/contrast", config.contrast);
EditorSettings::get_singleton()->set_manually("interface/theme/draw_extra_borders", config.draw_extra_borders);
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 made the overrides apply permanently. Not sure if it's safe to remove, but these settings are fetched a few lines above, so it's super unlikely they will get modified here. Unless I'm missing something.

@KoBeWi KoBeWi force-pushed the settings_of_editor_project branch from 8ce35e8 to 3376148 Compare August 6, 2024 21:35
@KoBeWi KoBeWi requested a review from a team as a code owner August 6, 2024 21:35
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 6, 2024

Ok this is what I came up with:

godot.windows.editor.dev.x86_64_Zsd29jRBgO.mp4

(true is value of the override)

Just pushed the changes. The overrides are functional, but the goto button is not working yet. Also code needs cleanup.

@KoBeWi KoBeWi force-pushed the settings_of_editor_project branch 2 times, most recently from 55096fb to 7f5ba1f Compare August 14, 2024 19:33
@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 14, 2024

Finished. Feel free to test and review.

@KoBeWi KoBeWi force-pushed the settings_of_editor_project branch from 7f5ba1f to ba247bc Compare August 14, 2024 19:48
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Will compile and test this tomorrow but the code looks good

override_button = memnew(Button);
override_button->set_tooltip_text(TTR("Override this setting for the current project."));
container->add_child(override_button);
override_button->connect(SNAME("pressed"), callable_mp(this, &EditorSettingsPropertyWrapper::_create_override));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
override_button->connect(SNAME("pressed"), callable_mp(this, &EditorSettingsPropertyWrapper::_create_override));
override_button->connect(SceneStringName(pressed), callable_mp(this, &EditorSettingsPropertyWrapper::_create_override));

goto_button = memnew(Button);
goto_button->set_tooltip_text(TTR("Go to override in Project Settings."));
override_info->add_child(goto_button);
goto_button->connect(SNAME("pressed"), callable_mp(EditorNode::get_singleton(), &EditorNode::open_setting_override).bind(property), CONNECT_DEFERRED);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
goto_button->connect(SNAME("pressed"), callable_mp(EditorNode::get_singleton(), &EditorNode::open_setting_override).bind(property), CONNECT_DEFERRED);
goto_button->connect(SceneStringName(pressed), callable_mp(EditorNode::get_singleton(), &EditorNode::open_setting_override).bind(property), CONNECT_DEFERRED);

remove_button = memnew(Button);
remove_button->set_tooltip_text(TTR("Remove this override."));
override_info->add_child(remove_button);
remove_button->connect(SNAME("pressed"), callable_mp(this, &EditorSettingsPropertyWrapper::_remove_override));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
remove_button->connect(SNAME("pressed"), callable_mp(this, &EditorSettingsPropertyWrapper::_remove_override));
remove_button->connect(SceneStringName(pressed), callable_mp(this, &EditorSettingsPropertyWrapper::_remove_override));

void ProjectSettingsEditor::popup_for_override(const String &p_override) {
popup_project_settings();
tab_container->set_current_tab(0);
general_settings_inspector->set_current_section("editor_overrides/" + p_override.get_slice("/", 0));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
general_settings_inspector->set_current_section("editor_overrides/" + p_override.get_slice("/", 0));
general_settings_inspector->set_current_section(ProjectSettings::EDITOR_SETTING_OVERRIDE_PREFIX + p_override.get_slice("/", 0));

@marcinn
Copy link
Contributor

marcinn commented Oct 16, 2024

I would gently ask about risky or very subjective personal preferences not related to a project, like background color, text color, gizmo colors, etc. If they are saved in the project.godot file and replace my preferences, is there any way I can overwrite them again? Please think about collaborative work.

I understand and like the ability to override many editor settings, but I'm not sure if I'll be able to keep my work environment settings that are not related to the project (which is usually shared with others). If not, I would like to have possibility to lock settings important for me (but not related to the project), or another layer of override - a kind of rc file, which I will drop inside project dir and add to gitignore.

Finally I think that overriding everything is a very dangerous change (exploits in shared/public repositories, security vulnerabilities). You can set paths to exe files in Editor Settings. I'm pretty sure that I will able to commit malware and run it on someone's host through Godot Editor.

image

EDIT: my proposal is to prepare a curated list of safe and relevant to a project settings (see #98235 as a starting point, which is, for now, unsafe too)

EDIT2: Excluding risky settings should not be a solution. Any new setting will be potentially risky. We should implement "deny all, allow some" pattern here.

EDIT3: adding safe overrides in [editor] setting of project.godot is a good place to store per-project, IMO.

@Mickeon
Copy link
Contributor

Mickeon commented Oct 16, 2024

I wonder how the override.cfg file behaves with this, to resolve the concern raised above

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 16, 2024

I would gently ask about risky or very subjective personal preferences not related to a project, like background color, text color, gizmo colors, etc. If they are saved in the project.godot file and replace my preferences, is there any way I can overwrite them again? Please think about collaborative work.

Why would someone do that though? In collaborative work this stuff should be discussed in the team, so I expect people won't just randomly override other's settings. I mean, even right now you can e.g. disable or enable some GDScript warnings because you don't like them and this affects all team members.

I wonder how the override.cfg file behaves with this, to resolve the concern raised above

override.cfg should be supported normally, so a local file with overrides would be a solution if someone wants to have personal overrides for whatever reason. It can also "remove" overrides by setting their value to null.

Finally I think that overriding everything is a very dangerous change (exploits in shared/public repositories, security vulnerabilities). You can set paths to exe files in Editor Settings. I'm pretty sure that I will able to commit malware and run it on someone's host through Godot Editor.

Not sure what do you mean by that. It's already possible when you run a project from untrusted source. It can be a malicious tool script that runs in the main scene, or even a prepared Theme set as project's default. Running random projects is always a risk and this PR doesn't really make it any worse. Note that to run a malware executable, you'd need to distribute it together with the project.

@KoBeWi KoBeWi force-pushed the settings_of_editor_project branch from 8257de2 to 0b7c6e6 Compare October 16, 2024 22:27
@KoBeWi KoBeWi requested a review from a team as a code owner October 16, 2024 22:27
@marcinn
Copy link
Contributor

marcinn commented Oct 16, 2024

Why would someone do that though? I expect people won't just randomly override other's settings

I'd like too. But there are mistakes, jokes, attacks.

On the other side I'm using .vimrc which could be dangerous (but I haven't noticed issues, because nobody uses vim).
But vim has -Z (restricted) mode to avoid executing nasty things. It would be nice to have something similar...

It's already possible

Oh, you're right. Tool script with OS.execute is a even better attack vector. It can run anything just after auto reload (i.e. after switching to a malicious branch) :|

Note that to run a malware executable, you'd need to distribute it together with the project.

rm is on every host (as an example)

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 16, 2024

I'd like too. But there are mistakes, jokes, attacks.

The overrides only apply for the specific project and can't permanently "damage" your editor settings. If you are worried about jokes/attacks within your own project, the same applies to any other content. E.g. someone can replace the main character with a clown, as a joke. I don't see how it is different from trolling by overriding editor settings.

But vim has -Z (restricted) mode to avoid executing nasty things. It would be nice to have something similar...

See godotengine/godot-proposals#5010

@marcinn
Copy link
Contributor

marcinn commented Oct 16, 2024

See godotengine/godot-proposals#5010

Thanks. Subscribed.

I don't see how it is different from trolling by overriding editor settings

For me there is a difference between destroying my desk (overriding >my< preferences) and showing picture of a destroyed desk (a clown character).

@AThousandShips
Copy link
Member

Tested and works as expected, one thing I noticed though was that the button to remove the override was hard to click, it appears to only be working when clicking exactly on the cross, which makes it hard to use, otherwise it seems to work well!

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 17, 2024

That's the default button for deletable properties (same as e.g. in metadata), so it's unrelated to this PR.

@kitbdev
Copy link
Contributor

kitbdev commented Oct 31, 2024

Since the purpose of proposal is more about one user customizing the editor per project than using the same editor settings in a project with others, maybe the overridden editor settings should be saved separately from the project settings.
It could be in the Editor Settings folder or as a separate file in the .godot folder (similar to VSCode workspace settings). This way by default it is not shared with the rest of the project.

@marcinn
Copy link
Contributor

marcinn commented Nov 1, 2024

Since the purpose of proposal is more about one user customizing the editor per project than using the same editor settings in a project with others, maybe the overridden editor settings should be saved separately from the project settings.

For me, there are two cases:

  1. project settings related to coding standards (indentation, other linting rules, etc)
  2. user's private settings related to the project

The latter is not necessary at all - private settings are saved in editor settings already, and they are mostly global (cross project). Because our preferences (subjective settings) are generally same for all projects.

The whole thing is about settings important for collaborative work, which sometimes are related to the editor. IMHO some editor settings should be overridden by project settings. In the other words - in such cases the code editor should use project settings at highest priority. If this is not set in the project settings, the code editor should use the editor settings.

For your case (override your private preferences in project, just for you) the proposed 'override.cfg' will work if you add this file to .gitignore. But you will not be able to store code-related settings.

That's why I'm still not sure if these settings should be combined in one file. But yes, when used carefully it will fit our needs in most cases.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 2, 2024

maybe the overridden editor settings should be saved separately from the project settings.

That would require adding another hard-coded file in the project's main directory.

@marcinn
Copy link
Contributor

marcinn commented Nov 2, 2024

That would require adding another hard-coded file in the project's main directory.

There is already override.cfg. The code-related settings could be saved in project.godot.


Having both we can achieve flexibility. Use cases:

  1. override.cfg in git - for those who will override any settings per project, probably one-man-armies
  2. override.cfg added to .gitignore - for those who want custom editor settings, but the project team is larger than John Rambo
  3. code settings in project.godot - for those who want to customize code settings per project and want to set custom editor settings per project (p.2 - override.cfg w/gitignore); or for those who want just to configure important settings for collaborative work; (priority: code-related settings in project.godot < override.cfg < editor settings)

P1 and p2 will be solved by this PR. But having just only override.cfg will prevent from achieving p3.

Thanks, have a good day.

@Repiteo Repiteo removed the request for review from a team November 25, 2024 23:25
@akien-mga akien-mga self-requested a review December 17, 2024 20:33
@marcinn
Copy link
Contributor

marcinn commented Dec 25, 2024

(Not directly related to the issue)
Another example of editor settings, which should never be saved as a project setting, is export_path from export presets:
image
Every single export is trying to make changes in the repo.

@KoBeWi
Copy link
Member Author

KoBeWi commented Dec 25, 2024

Export path is not a setting.
Why would it change though? Usually you export to the same path.

@KoBeWi KoBeWi force-pushed the settings_of_editor_project branch from 0b7c6e6 to 79caf22 Compare December 25, 2024 09:30
@arkology
Copy link
Contributor

Most likely with this PR it will be possible to implement godotengine/godot-proposals#827 without overengineered hacks, just by 1) exposing feature profile as ordinary editor settings and 2) adding an option to replace pathes in settings with res:// (to be friendly for collaborative work on project)

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