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

Expose NodeDock and its children #47767

Closed
wants to merge 1 commit into from

Conversation

trollodel
Copy link
Contributor

@trollodel trollodel commented Apr 10, 2021

Part of godotengine/godot-proposals#2537
Indipendent from #47520

Expose NodeDock, ConnectionDock and GroupsEditor. NodeDock was refactored to allow an arbitrary number of tabs that can be added by plugins (no more show_* methods).
ConnectionDock and GroupsEditor exposes their main subcomponents (both trees). Dialogs are not exposed.
Also, I did some little unrelated changes (including a bugfix).

Docs for newly exposed classes are added, but I'm not sure about the quality of them, especially the descriptions.
IMO the brief description should state what component is and where it is, and the description should explain in short how it is structured and how a plugin can modify it.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 18, 2021

I looked over the code and it looks fine. Needs rebase, so I can test it etc.

EDIT:
Also, how to get the editor's node dock from a plugin? I don't see any changes to EditorInterface.

@trollodel trollodel force-pushed the hackable_nodedock branch 2 times, most recently from 4d23379 to 9201781 Compare August 19, 2021 18:24
@trollodel
Copy link
Contributor Author

trollodel commented Aug 19, 2021

Also, how to get the editor's node dock from a plugin? I don't see any changes to EditorInterface.

That's expected. The idea behind this PR and #47985 is to expose relevant parts at the dock level, the dock itself should be exposed in a separate PR (#47520 is one possibility, but it was kinda rejected in a PR meeting).
Of course, I can expose the dock in EditorInterface here and delay the problem of exposing all the docks better (if it's really a problem).

@trollodel trollodel requested a review from a team as a code owner August 19, 2021 18:42
@KoBeWi
Copy link
Member

KoBeWi commented Aug 19, 2021

That's expected. The idea behind this PR and #47985 is to expose relevant parts at the dock level, the dock itself should be exposed in a separate PR

Simply exposing the class is rather useless as it can't be really used stand-alone from GDScript. For now I'd just add a method in EditorInterface, like get_node_dock() or something. If some other PR is going to expose it in a different way (which might not even happen), it can still be changed.

Anyways, there is a regression. Clicking group button in scene tree will only focus the node dock, instead of opening the relevant tab. Same with signal button.

@trollodel trollodel force-pushed the hackable_nodedock branch 2 times, most recently from 76ae75a to e2eb4d9 Compare August 23, 2021 17:52
@trollodel
Copy link
Contributor Author

Simply exposing the class is rather useless as it can't be really used stand-alone from GDScript. For now I'd just add a method in EditorInterface, like get_node_dock() or something. If some other PR is going to expose it in a different way (which might not even happen), it can still be changed.

Now that #47520 is closed, I expose it here.

Anyways, there is a regression. Clicking group button in scene tree will only focus the node dock, instead of opening the relevant tab. Same with signal button.

Should be fixed, it was throwing an error too.

Refractor NodeDock to allow an arbitrary number of tabs
Add the missing GroupDialog::_group_selected bind
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks ok now. I only did a technical review, the actual decision whether we should expose the dock is still to be made. Personally I don't think there is much harm. It might be useful for some plugins and the PR also includes some refactoring.

@YuriSizov YuriSizov requested review from a team and removed request for a team August 24, 2021 20:04
@reduz
Copy link
Member

reduz commented Aug 29, 2021

Several points on this:

  • I am against exposing editor internals for the sake of it. These should happen only with proper justification. As an example EditorInspector makes sense because you can reuse it a lot. NodeDock does is not justified.
  • Adding features should be majorly via EditorPlugin API, so we are not forced to keep compatibility every time the editor internals change.
  • Anything exposed from the editor to the editor plugins should begin with Editor, such as EditorInspector, EditorPlugin, etc. Classes without this naming convention should be renamed before exposed, if exposed.

@reduz
Copy link
Member

reduz commented Aug 29, 2021

Also by the way, the plan was for a while to do away with NodeDock and merge most of that functionality in the inspector, so accessing this is likely a bad idea.

@trollodel
Copy link
Contributor Author

Closing per @reduz comments and discussion on the Godot Contributor Chat.
I would extract some changes from this PR, but if the Node dock it's going to be removed it doesn't make any sense.

@YeldhamDev YeldhamDev removed the request for review from a team August 29, 2021 16:47
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