Rework editor docks #106503

KoBeWi · 2025-05-16T19:46:22Z

Closes godotengine/godot-proposals#12435

There are some differences from the proposal. The new class is called EditorDock and inherits MarginContainer.

I still need to expose it. For now I changed all docks to use the new system.

EDIT:
Done. I made an example plugin that uses the new system:
dock-test.zip
It can be moved to bottom and supports saving layout.

Also supersedes #106428

lodetrick · 2025-05-16T21:23:40Z

This isn't an issue with this PR, but since we're changing how the dock customization works would it be possible to take #106428 into account? In my opinion it is better to have the brittle Texture2D to be the priority rather than the fallback because it would allow for overrides and the more dynamic StringName (it is able to be drawn in dark mode, for example) to be the fallback.

KoBeWi · 2025-05-16T23:11:51Z

Should be ready now. I included #106428 too.
Makes me wonder whether icon_name should be renamed to fallback_icon maybe?

lodetrick · 2025-05-16T23:27:08Z

I still think the icon_name has a use because it is a lot more flexible (it can be used in light or dark mode), maybe it could be icon -> override_icon and icon_name -> default_icon.

KeyboardDanni · 2025-05-17T00:34:28Z

I mentioned this already in the Godot Contributors Chat, but I'm concerned about every dock being added to the menubar under "Editor Docks". The problem is that this shows every possible context-sensitive dock to the user, even when they wouldn't normally be visible. For example, the "Tilemap" dock usually only appears when you are editing a Tilemap. I'm not exactly sure under what circumstances you'd want to access that dock when you're not editing a Tilemap. As goofy as the Microsoft Office Ribbon is, I feel that this is one aspect they did get right at least. In versions of Microsoft Office before 2007 the UI was crammed with a lot of toolbars, docks, selectable windows, that sort of thing, for features that aren't even active so they're just taking up space.

I could see the need to be able to access a dock that isn't context-relevant if you want to adjust where it is in the layout, but I'm not sure how best to approach that. Some programs temporarily show any stuff that would normally be hidden when you go into the "layout edit mode" but Godot doesn't really have that.

I do like the idea of being able to move docks that would normally be bottom-locked, and put them in other parts of the UI. But also a lot of bottom docks are situational, and them being at the bottom communicates that. So moving them elsewhere doesn't really fit the established paradigm.

KoBeWi · 2025-05-17T01:30:04Z

That's unrelated to this PR though. It only changes internals, the editor so far works the same as before.

lodetrick

The main content looked good from what I could see, although there were quite a few object leaks:

WARNING: 5 RIDs of type "CanvasItem" were leaked.
     at: _free_rids (servers/rendering/renderer_canvas_cull.cpp:2678)
ERROR: 6 RID allocations of type 'N10RendererRD14TextureStorage7TextureE' were leaked at exit.
WARNING: 8 RIDs of type "Texture" were leaked.
     at: finalize (servers/rendering/rendering_device.cpp:7164)
ERROR: 73 RID allocations of type 'PN18TextServerAdvanced22ShapedTextDataAdvancedE' were leaked at exit.
ERROR: 18 RID allocations of type 'PN18TextServerAdvanced12FontAdvancedE' were leaked at exit.
ERROR: 1 RID allocations of type 'PN18TextServerAdvanced27FontAdvancedLinkedVariationE' were leaked at exit.
WARNING: ObjectDB instances leaked at exit (run with --verbose for details).
     at: cleanup (core/object/object.cpp:2489)
ERROR: 2 resources still in use at exit (run with --verbose for details).
   at: clear (core/io/resource.cpp:637)

It might be an issue with master. I'll check. Yeah it has to do with master. Disregard above.

doc/classes/EditorPlugin.xml

doc/classes/EditorDock.xml

editor/plugins/editor_plugin.cpp

editor/plugins/version_control_editor_plugin.cpp

lodetrick · 2025-05-17T20:18:39Z

editor/plugins/editor_plugin.h

 	static void _bind_compatibility_methods();
+
+	void add_control_to_dock(DockSlot p_slot, Control *p_control, const Ref<Shortcut> &p_shortcut = nullptr);
+	void remove_control_from_docks(Control *p_control);


It might be an issue that these functions are protected now, as they were public in the past. Or should they have been protected from the start?

Seems like the compatibility methods are also protected. I just moved it into deprecated block.

It does not really matter, it only affects C++ modules that call plugins methods from outside the plugin, but there is no reason to do so.

Makes me curious why they were public to begin with

Protected methods are just rarely used in the engine. There are some common protected methods like notification() or bind_method() and then everything else is either private or public.

doc/classes/EditorDock.xml

lodetrick · 2025-05-17T21:31:59Z

doc/classes/EditorDock.xml

+		        title = "My Dock"
+		        icon = load("./dock_icon.png")
+
+		        var dock_content = preload("./dock_content.tscn").instantiate()


Suggested change

var dock_content = preload("./dock_content.tscn").instantiate()

var dock_content = preload("res://dock_content.tscn").instantiate()

That one is intended. The scene is supposed to be loaded from addon's directory and relative paths are supported in preload().

Thanks for the clarification, I'm not too familiar with plugins specifically

doc/classes/EditorDock.xml

editor/editor_dock.cpp

lodetrick

It looks good now from what I can see. Thanks for your effort!

lodetrick · 2025-05-19T08:25:25Z

It seems that changing the order of operations allowed plugins that don't set icons to show a broken icon:

Seen below, an empty icon_name would load the broken file icon, when we want the window icon.
p_dock->dock_icon.is_valid() ? p_dock->dock_icon : tab_container->get_editor_theme_icon(p_dock->icon_name);
Unfortunately to keep the new order of operations, I can't see a method that would reduce/keep the complexity of code.

kitbdev · 2025-05-19T01:58:14Z

editor/editor_dock_manager.cpp

-	p_dock->call("_set_dock_horizontal", true);
+	p_dock->at_bottom = true;
+	p_dock->previous_at_bottom = false;
+	p_dock->set_horizontal_layout(true);


The bottom dock is not necessarily horizontal (like when it is expanded or on tall screens) and the side docks are not always vertical (they can be resized freely when floating, and on wide screens).
So changing horizontal/vertical layout should instead be called during its resize when the horizontal size exceeds the vertical size.

I mimicked what FileSystem dock currently does. This can be changed later without breaking compatibility.

kitbdev · 2025-05-19T02:23:09Z

doc/classes/EditorDock.xml

+		@tool
+		extends EditorPlugin
+
+		# Define the dock class (can be in separate file).


Its kind of awkward needing to create a Node subclass in code that can't be part of the dock content scene, it make it harder to preview in the Editor when making the dock. We don't have interfaces, either. We should consider allowing Editor-only nodes in certain scenes in the 2d editor in the future.
This makes docks a bit more complex to create than the previous system.

EditorDock extends MarginContainer, which does not alter appearance of its children. Your dock scene will appear the same when added under dock.

True, I just mean it would be more convenient if the entire dock was in the scene instead of needing an EditorDock subclass separate from the dock content scene.
Like, if you want to reference children nodes in your EditorDock class (to change layout dir or something), you can't use unique names, or drag and drop the node into the script. And its more convenient to use the inspector for the EditorDock.
Actually, I can make a proposal afterwards.

kitbdev · 2025-05-19T02:38:54Z

editor/editor_dock.cpp

+#include "core/input/shortcut.h"
+#include "core/io/config_file.h"
+
+void EditorDock::_bind_methods() {


This should be at the bottom of the file, like most other cpp files.

It's the same order as in .h.

It's the same order, but most other files are not.
Well it doesn't really matter.

kitbdev · 2025-05-19T02:40:57Z

editor/editor_dock.cpp

+}
+
+String EditorDock::get_display_title() const {
+	return title.is_empty() ? get_name().operator String() : title;


Since the EditorNode is made in a script, the default name for the control is an internal name like @EditorDock@44105. This should use the class name or the child's (the content) name if this nodes name is generated, if possible.

kitbdev · 2025-05-19T02:56:21Z

editor/editor_dock.cpp

+void EditorDock::save_layout_to_config(Ref<ConfigFile> &p_layout, const String &p_section) const {
+	GDVIRTUAL_CALL(_save_layout_to_config, p_layout, p_section);
+}
+
+void EditorDock::load_layout_from_config(const Ref<ConfigFile> &p_layout, const String &p_section) {
+	GDVIRTUAL_CALL(_load_layout_from_config, p_layout, p_section);
+}


These expose all the settings that the Editor uses to save and load. While its not sensitive, it still probably shouldn't.
This can cause conflicts with the editors settings and settings with other docks. For example, if you try to make a generic "enabled" key, any other dock that tries that key will share it.
These could just take/give a Dictionary for the user to use for their settings and it can be saved to a key for that dock in the config file. ConfigFiles can still be used for internal docks, but it can call a different virtual function here.

Using the provided section ensures that you don't modify any unintended data. I could add a note in the docs.
We already have _get_window_layout(), which provides ConfigFile (and it's worse, because you don't get a dedicated section).

It uses the same section as the dock manager, so if you use a value like "dock_1" it will overwrite the existing value and mess up the layout.

Right, maybe I should make every dock get their dedicated section 🤔

kitbdev · 2025-05-19T03:44:36Z

editor/plugins/editor_plugin.h

+	void add_dock(EditorDock *p_dock, DockSlot p_slot);
+	void remove_dock(EditorDock *p_dock);


Should these be available in EditorInterface, so they are available for more than just plugins?

kitbdev · 2025-05-19T04:07:06Z

editor/editor_dock_manager.h

-	void remove_dock(Control *p_dock);
-
-	void set_dock_tab_icon(Control *p_dock, const Ref<Texture2D> &p_icon);
+	void add_dock(EditorDock *p_dock, DockSlot p_slot = DOCK_SLOT_NONE);


Instead of passing the dock slot in here, there can be a default dock slot property on the EditorDock. This way it can be determined by the dock itself.

But after the dock is added, should changing the property move it?

Probably not, the user should control where docks are after the first time they open.

kitbdev · 2025-05-19T14:39:50Z

doc/classes/EditorDock.xml

+	<tutorials>
+	</tutorials>


This should link to https://docs.godotengine.org/en/stable/tutorials/plugins/editor/making_plugins.html#a-custom-dock
And that page will need an update.

kitbdev · 2025-05-19T17:33:32Z

doc/classes/EditorDock.xml

+	<description>
+		EditorDock is a [Container] node that can be docked in one of the editor's dock slots. Docks are added by plugins to provide space for controls related to the plugin.
+		You can add a dock by using [method EditorPlugin.add_dock]. The dock can be customized by changing its properties.
+		[codeblock]


I think this example should be the simplest correct use of it, so its easy to get started. More complex examples can be in addition to this or in docs.godotengine.org/en/stable/tutorials/plugins/editor/making_plugins.html#a-custom-dock.

So something like:

[codeblock] @tool extends EditorPlugin # Dock reference. var dock # Plugin initialization. func _enter_tree(): dock = EditorDock.new() dock.title = "My Dock" dock.dock_icon = load("./dock_icon.png") var dock_content = preload("./dock_content.tscn").instantiate() dock.add_child(dock_content) add_dock(dock, EditorPlugin.DOCK_SLOT_RIGHT_UL) # Plugin clean-up. func _exit_tree(): remove_dock(dock) dock.queue_free() dock = null [/codeblock]

kitbdev · 2025-05-19T18:03:45Z

editor/editor_dock_manager.cpp

@@ -461,11 +467,11 @@ void EditorDockManager::save_docks_to_config(Ref<ConfigFile> p_layout, const Str
 		window_dump["window_screen"] = wrapper->get_window_screen();
 		window_dump["window_screen_rect"] = DisplayServer::get_singleton()->screen_get_usable_rect(screen);

-		String name = dock->get_name();
+		String name = dock->get_display_title();


We should not use the display title to save/load data. It's now pretty easy to change the title at any time for a dock, so this value cannot be relied on. Saving and loading needs this to be the same or it will not work.

The previous code was inconsistently using DockInfo's title (which could be empty). get_name() can't be used, because the dock might have title, but not name. Display title is the only consistent value; it's also very unlikely it's going to be changed, even if it's possible.

The previous code looks like it used the control's name and just ignored the title, since they always effectively had a name before.
I thought it was even encouraged now because the tab_style_changed signal emits when the title changes so it updates properly.
And though it's not a dock, the Debugger bottom panel changes it's display to Debugger (1) to show the error count dynamically.
We could just have another property to get a name for saving, and if it is empty fall back to the display title.

lodetrick · 2025-05-19T18:17:23Z

We should probably also clip the contents of the docks:

KoBeWi added this to the 4.x milestone May 16, 2025

KoBeWi added enhancement feature proposal topic:editor labels May 16, 2025

KoBeWi force-pushed the docker_exe branch 5 times, most recently from 441e53c to bb9c614 Compare May 16, 2025 23:05

KoBeWi marked this pull request as ready for review May 16, 2025 23:11

KoBeWi requested review from a team as code owners May 16, 2025 23:11

KoBeWi force-pushed the docker_exe branch from bb9c614 to da6a29e Compare May 17, 2025 01:35