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

Add editor singletons to expose more editor UI components #47520

Closed
wants to merge 7 commits into from

Conversation

trollodel
Copy link
Contributor

@trollodel trollodel commented Mar 31, 2021

Part of godotengine/godot-proposals#2537

This PR exposes every editor component (only the main ones, not subcomponents) through dedicated singletons.
The naming scheme follows this section, including the use of "workspace" instead of "main screen".

I took the opportunity to move some methods from EditorPlugin and EditorInterface to dedicated singletons to clean up that classes. As I pointed out in godotengine/godot-proposals#2537, the Godot editor is too big and complex to expose its API mostly through 2 classes.

Singletons:

  • EditorDocks
  • EditorBottomPanels
  • EditorWorkspaces
  • EditorTopBars
  • EditorPluginInterfaces (I don't like this name)
  • EditorScenes (may require a better name)
  • EditorInterface

Notes for reviewers:

  • Since this is going to be a big PR, I'd be verbose with commits and squash them when this became stable enough.
  • Docs are not done yet because I prefer that we agree on the new API.

@YuriSizov
Copy link
Contributor

YuriSizov commented Mar 31, 2021

Note for reviewers: since this is going to be a big PR, I'd be verbose with commits and squash them when this became stable enough.

I think it would be better to do this in steps, in several PRs.

"Expose all the editor UI" sounds impossible or at least controversial to begin with. But say exposing all the docks in a nice way is totally doable and I was eyeing it for some time now. As docks are already somewhat encapsulated in the EditorNode API and only need to be properly sanitized for what they expose of themselves. So if done correctly, I would be completely behind such PR, but other issues may not be that straightforward.

I would generally prefer to keep plugin API smaller and instead provide a more general help to navigating the editor UI, like naming more nodes.

@trollodel
Copy link
Contributor Author

I think it would be better to do this in steps, in several PRs.

Yeah, I wanted to do it too, but since that for only exposing the docks I need to change 33 files, it'll come with lots of PR that depends on each other.

"Expose all the editor UI"

I was worried that it is misleading, so I'll change it to something softer (the idea behind the PR doesn't change for now).

I would generally prefer to keep plugin API smaller and instead provide a more general help to navigating the editor UI, like naming more nodes.

Like godotengine/godot-proposals#1018? The issue with it is that if something change in the structure (like moving nodes) you lose the compatibility. Instead, if the interested component provides getters for relevant subcomponents, the problem came less frequently.

@trollodel trollodel changed the title Refractor editor plugin API in order to expose all the editor UI Refractor editor plugin API in order to expose more parts of the editor UI Mar 31, 2021
@YuriSizov
Copy link
Contributor

Like godotengine/godot-proposals#1018? The issue with it is that if something change in the structure (like moving nodes) you lose the compatibility. Instead, if the interested component provides getters for relevant subcomponents, the problem came less frequently.

Yes, frequently used parts or parts of higher interest should be exposed via specific, stable APIs. I was refering to your original title more, because to make the entirety of the UI tree more accessible the better option is to name nodes, even if they would break from time to time.

Generally I think that the approach should be:

  1. APIs for higher interest nodes
  2. Names for their children of lower interest
  3. Keep the rest as is, unnamed

@trollodel
Copy link
Contributor Author

Points 1 and 3 are exactly what I thought.
2 requires a naming scheme (as you pointed out in godotengine/godot-proposals#1018) and I don't plan to do it here.

@trollodel trollodel changed the title Refractor editor plugin API in order to expose more parts of the editor UI Add editor singletons to expose more editor UI components Apr 1, 2021
@trollodel
Copy link
Contributor Author

trollodel commented Apr 1, 2021

After thinking a bit, I decide to keep this PR only for exposing singleton as described in godotengine/godot-proposals#2537 and open other PRs after this.

@trollodel trollodel force-pushed the hackable_godot_editor branch 6 times, most recently from 17ab408 to cb1dd79 Compare April 7, 2021 19:31
@trollodel
Copy link
Contributor Author

trollodel commented Apr 8, 2021

I defined all the singletons for this PR, now it's time to give me some feedbacks 😉.

@trollodel trollodel force-pushed the hackable_godot_editor branch 2 times, most recently from 4242128 to f9f7390 Compare June 3, 2021 12:41
trollodel added 6 commits June 3, 2021 14:49
Remove docks getter from EditorInterface.
Add EditorDocks getter to EditorPlugin
Remove panel related methods from EditorPlugin
Add EditorBottomPanels getter to EditorPlugin
Add EditorWorkspaces getter to EditorPlugin
Remove plugin related methods from EditorPlugin
Add EditorPluginInterfaces getter to EditorPlugin
Register all the newly exposed classes
Fix tests (hopefully)
Add EditorTopBars getter to EditorPlugin
Add a missing getter
@trollodel trollodel force-pushed the hackable_godot_editor branch from f9f7390 to 21be17e Compare June 3, 2021 13:04
@trollodel
Copy link
Contributor Author

The implementation of this PR was not accepted in a PR meeting, and in the last live Q&A Akien says that a refractor of editor plugin API will be done after 4.0.
Since the release of Godot 4 is not incoming, I'm going to close this PR, as it's quite difficult to rebase.
I'm happy to redo it from scratch, but only when there will be interest on it.

@trollodel
Copy link
Contributor Author

Rejected in a discussion, the Godot contributor chat.
Part of this PR will be extracted to make a cleanup PR.

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.

3 participants