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

Refactor code editors #28607

Open
KoBeWi opened this issue May 2, 2019 · 6 comments
Open

Refactor code editors #28607

KoBeWi opened this issue May 2, 2019 · 6 comments

Comments

@KoBeWi
Copy link
Member

KoBeWi commented May 2, 2019

Godot version:
3.1.x

Steps to reproduce:
script_text_editor.cpp
shader_editor_plugin.cpp
text_editor.cpp

What these 3 files have in common is that they implement literally the same thing 3 times. Some examples:

Same set of enums:

EDIT_UNDO,
EDIT_REDO,
EDIT_CUT,
EDIT_COPY,
EDIT_PASTE,
EDIT_SELECT_ALL,

EDIT_UNDO,
EDIT_REDO,
EDIT_CUT,
EDIT_COPY,
EDIT_PASTE,
EDIT_SELECT_ALL,

EDIT_UNDO,
EDIT_REDO,
EDIT_CUT,
EDIT_COPY,
EDIT_PASTE,
EDIT_SELECT_ALL,

Same edit menu:

edit_menu = memnew(MenuButton);
edit_menu->set_text(TTR("Edit"));
edit_menu->set_switch_on_hover(true);
edit_menu->get_popup()->set_hide_on_window_lose_focus(true);
edit_menu->get_popup()->add_shortcut(ED_GET_SHORTCUT("script_text_editor/undo"), EDIT_UNDO);
edit_menu->get_popup()->add_shortcut(ED_GET_SHORTCUT("script_text_editor/redo"), EDIT_REDO);
edit_menu->get_popup()->add_separator();

edit_menu = memnew(MenuButton);
edit_menu->set_text(TTR("Edit"));
edit_menu->get_popup()->connect("id_pressed", this, "_edit_option");
edit_hb->add_child(edit_menu);
edit_menu->get_popup()->add_shortcut(ED_GET_SHORTCUT("script_text_editor/undo"), EDIT_UNDO);
edit_menu->get_popup()->add_shortcut(ED_GET_SHORTCUT("script_text_editor/redo"), EDIT_REDO);
edit_menu->get_popup()->add_separator();

edit_menu = memnew(MenuButton);
edit_menu->set_text(TTR("Edit"));
edit_menu->set_switch_on_hover(true);
edit_menu->get_popup()->set_hide_on_window_lose_focus(true);
edit_menu->get_popup()->add_shortcut(ED_GET_SHORTCUT("script_text_editor/undo"), EDIT_UNDO);
edit_menu->get_popup()->add_shortcut(ED_GET_SHORTCUT("script_text_editor/redo"), EDIT_REDO);
edit_menu->get_popup()->add_separator();

Same menu actions:

case EDIT_MOVE_LINE_UP: {
shader_editor->move_lines_up();
} break;
case EDIT_MOVE_LINE_DOWN: {
shader_editor->move_lines_down();
} break;

case EDIT_MOVE_LINE_UP: {
code_editor->move_lines_up();
} break;
case EDIT_MOVE_LINE_DOWN: {
code_editor->move_lines_down();
} break;

case EDIT_MOVE_LINE_UP: {
code_editor->move_lines_up();
} break;
case EDIT_MOVE_LINE_DOWN: {
code_editor->move_lines_down();
} break;

There's muuuch more of it. And the worst part is when you want to add some feature to code editor. You basically need to repeat the code 3 times.

IMO the editors should be refactored to cut the repetitions as much as possible. There are differences here and there, but I bet it's possible to rewrite them in a way that doesn't repeat 80% of lines 3 times.

@Calinou
Copy link
Member

Calinou commented Jun 7, 2021

Closing in favor of #31739, which has more information about the issue at hand. Nonetheless, splitting into a code editing-focused class has already begun.

@Paulb23 did outstanding work to address this, with more to come in the future: #40973, #42775, #45393, #49238

@KoBeWi
Copy link
Member Author

KoBeWi commented Aug 6, 2021

Reopening, as the PR that will close #31739 doesn't resolve this issue. The refactoring work so far only touched CodeEdit and TextEdit, it's not related to editor classes.

@akien-mga
Copy link
Member

There have been improvements but there's still more work pending here. Won't happen for 4.0 though, and doesn't need to be tied to a specific milestone as it's internal refactoring.

@akien-mga akien-mga removed this from the 4.0 milestone Nov 21, 2022
@Paulb23
Copy link
Member

Paulb23 commented Apr 2, 2023

This will probably be my main focus for now, given TextEdit / CodeEdit are in a somewhat decent place for the time being.

Given this is going to be quite a large refactor, it would be ideal if as part of these changes we can hit several big features / proposals at the the same time.

This is the roughly the current class diagram:
image

Several pain points are:

The main feature, that is currently not possible that I'd like to tackle as part of this, is godotengine/godot-proposals/issues/414 allowing split views and having the same file open multiple times.

Secondly, the ShaderEditor is located where is is to allow viewing the scene and editing the Shader at the same time. If this become detachable (godotengine/godot-proposals/issues/28) then this assumption no longer holds. So allowing shaders to be opened in the ScriptEditor (godotengine/godot-proposals/issues/5071) would be neat.

Bear in mind this might change as I get into the details, I'm also not expecting to need to create all the classes on the following diagram but should serve as a guide / context for what I would like to move towards:
image

ScriptEditorManager will be the main view with the script list, managing multiple ScriptEditorWindows, any Shader / Script specific code can stay in their respective plugin classes.

ScriptEditorWindow will handle the split view. There will be at least one baked into the ScriptEditorManager, others could pop out into a separate windows.

Having the ScriptEditorResource will allow multiple ScriptEditors to sync changes. It also provides an abstraction layer so TextEditor can interact with several resource types without creating a specific editor for it. There still may be exceptions however, in these cases a new editor can extend the existing one's without duplicating everything.

@Gallilus

This comment was marked as resolved.

@Paulb23
Copy link
Member

Paulb23 commented Sep 17, 2023

Yes, still under heavy WIP, see: https://github.com/Paulb23/godot/tree/script-editor-resource

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

No branches or pull requests

5 participants