Allow user to disable script editor and use external editor instead #73760
Conversation
|
The main question is what is the use case for disabling the script editor. There seems to be conflicted use cases:
|
nongvantinh
force-pushed
the
fix-73702
branch
from
2f6d07b to
f1ae881
Compare
nongvantinh
changed the title
nongvantinh
force-pushed
the
fix-73702
branch
from
f1ae881 to
b4d9880
Compare
nongvantinh
force-pushed
the
fix-73702
branch
from
b4d9880 to
0e2af99
Compare
|
Disabling Scripting does not disable Script Editor. Probably not intended (and inconsistent with how other nested features work). |
| @@ -49,6 +49,7 @@ class EditorFeatureProfile : public RefCounted { | |||
| enum Feature { | |||
| FEATURE_3D, | |||
| FEATURE_SCRIPT, | |||
| FEATURE_EDITOR_SCRIPT, | |||
There was a problem hiding this comment.
You need to also use BIND_ENUM_CONSTANT.
| Ref<EditorFeatureProfile> profile = feature_profile_manager->get_current_profile(); | ||
| if (profile->is_feature_disabled(EditorFeatureProfile::FEATURE_EDITOR_SCRIPT) && | ||
| !profile->is_feature_disabled(EditorFeatureProfile::FEATURE_SCRIPT)) { | ||
| if (main_plugin->get_name() == "Script" && current_res && !current_res->is_built_in() && (bool(EDITOR_GET("text_editor/external/use_external_editor")) || overrides_external_editor(current_obj))) { |
There was a problem hiding this comment.
There is 4 nested conditions here, maybe it could be simplified.
The first if (with == main_plugin) could be changed to early continue.
| @@ -49,6 +49,7 @@ class EditorFeatureProfile : public RefCounted { | |||
| enum Feature { | |||
| FEATURE_3D, | |||
| FEATURE_SCRIPT, | |||
| FEATURE_EDITOR_SCRIPT, | |||
There was a problem hiding this comment.
Adding FEATURE_EDITOR_SCRIPT here breaks compat, it should be added at the end of the enum.
|
While reviewing the PR and making revisions based on your suggestions, I noticed several underlying issues with this PR. Should I relocate FEATURE_EDITOR_SCRIPT to the bottom, it would require adjustments in many other places, as the current implementation does not account for the possibility of a main feature having multiple sub-main features. I will need more time to reconsider how to solve the problem in an elegant manner. Given my full-time job, it will take a little longer to complete, at the very least, not before the release of 4.1. My current idea is to solve the issue by utilizing an array or other suitable mechanisms. That's all I can think of. |
Fixes: #73702