Skip to content

Conversation

@KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 15, 2025

Supersedes #106426

Adds a DOCK_SLOT_BOTTOM, which corresponds to bottom panel. All bottom panel controls are now docks. For compatibility, the old bottom editors are just wrapped in EditorDock.

This mostly brings changes to the dock popup, but also allows dragging docks between sides and bottom (currently only FileSystem):

godot.windows.editor.dev.x86_64_A0DP2XbvLh.mp4

Other than that, the current dock slot is now properly highlighted, slots unavailable for the current dock are darkened. The "Move to Bottom" button is replaced with a bottom dock slot.

I also added 3 new EditorDock properties: global (whether it appears in dock menu and can be closed), hidden (whether its tab is hidden) and title_color. All old bottom docks are non-global.

Dock's layout is now properly respected, i.e. you can make a dock that is horizontal only (in #106503 the LAYOUT_VERTICAL flag was effectively unused). I also added a LAYOUT_FLOATING flag, which determines whether the dock can float (all legacy bottom docks can't float).

In follow-ups I plan to update legacy docks to the new system, which would allow stuff like vertical ShaderEditor (it's already possible, but the editor is not adjusted for that, so the option is blocked). I will probably open a tracker for that.

A side-effect of these changes is that bottom docks can be rearranged, but I think it's not really a problem 🤔

Also for the future, I think EditorDockManager can be refactored, as it does too much. Some of its code can be moved to EditorDock and some of its code can be delegated to a new EditorDockContainer class extending TabContainer (the bottom panel would inherit it too). Also the main screen could potentially become a dock slot.

@KoBeWi KoBeWi added this to the 4.x milestone Jul 15, 2025
Copy link
Contributor

@lodetrick lodetrick left a comment

Choose a reason for hiding this comment

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

Initial thoughts. Regarding the popup interfering with the buttons I can fix that in the refactor.

Comment on lines 951 to 947
p_tab_container->set_popup(dock_context_popup);
p_tab_container->connect("pre_popup_pressed", callable_mp(dock_context_popup, &DockContextPopup::select_current_dock_in_dock_slot).bind(p_dock_slot));
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally chose not to have this popup appear for the bottom panel (because it is out of place with the other buttons), but it could also appear as one of the bottom panel's dedicated buttons

p_tab_container->set_use_hidden_tabs_for_min_size(true);
p_tab_container->get_tab_bar()->connect(SceneStringName(gui_input), callable_mp(this, &EditorDockManager::_dock_container_gui_input).bind(p_tab_container));
p_tab_container->hide();
p_tab_container->set_meta("dock_layout", p_layout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary with the array of DockSlots? I see what you mean about the need for EditorDockContainer.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a quick hack, the problem is that _move_dock() takes the new parent directly instead of slot ID. It requires some changes.

DockConstants::DockSlot occupied_slot = DockConstants::DOCK_SLOT_MAX;
TabBar *drop_tabbar = nullptr;

Color valid_drop_color;
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, this PR should be the one to introduce the invalid_drop_color and relevant layout checks in EditorDockDragHint (to visually tell the user that they can/cannot drag onto the bottom dock.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I just removed highlight if the slot can't be dropped to.

@arkology
Copy link
Contributor

Also, a side-effect of these changes is that bottom docks can be rearranged, but I think it's not really a problem

It will be set to default state (i.e. position) after selection "Reset layout" it top bar menu, right?

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 16, 2025

Currently no. Bottom editors are not part of default layout, their order is semi-random.

@arkology
Copy link
Contributor

Not sure this is intended :)

godot.windows.editor.x86_64_slot_HfoZb7nt9F.mp4

@KoBeWi

This comment was marked as resolved.

@arkology
Copy link
Contributor

arkology commented Oct 24, 2025

Something bad is happening with the bottom panel size:

BTW does it happen in master?
I suppose size changes because of #109915
UPD: Yes, it does... Should we treat this as a bug?

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 30, 2025

UPD: Yes, it does... Should we treat this as a bug?

Soo the problem is that previously when you selected Audio tab, the panel would expand height, but then shrink down once you select a smaller dock. Now the new height is permanent, and also bigger than before for some reason. So yeah, it is a bug.

@KoBeWi
Copy link
Member Author

KoBeWi commented Oct 30, 2025

I think I resolved all problems.
#106164 still needs to be rebased and merged, so this is going to be a draft for a bit more.

@KoBeWi KoBeWi marked this pull request as ready for review November 7, 2025 22:45
@KoBeWi KoBeWi requested review from a team as code owners November 7, 2025 22:45
@KoBeWi KoBeWi force-pushed the docking_abyss branch 2 times, most recently from 507418b to 2f66f2e Compare November 9, 2025 09:18
<member name="dock_shortcut" type="Shortcut" setter="set_dock_shortcut" getter="get_dock_shortcut">
The shortcut used to open the dock. This property can only be set before this dock is added via [method EditorPlugin.add_dock].
</member>
<member name="global" type="bool" setter="set_global" getter="is_global" default="true">
Copy link
Member

Choose a reason for hiding this comment

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

You may argue this needs to be named dock_global at this point, as you did prior, but I'm not sure either. So long as it's not is_global because we use verbs specifically for getter methods.

... Is calling this property global even appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be a better name?

The icon for the dock, as a texture. If specified, it will override [member icon_name].
</member>
<member name="dock_hidden" type="bool" setter="set_hidden" getter="is_hidden" default="false">
If [code]true[/code], the dock's tab will be hidden. If [member global] is [code]true[/code], the dock will not appear in the dock menu.
Copy link
Member

Choose a reason for hiding this comment

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

In another description you name this dock "Editor > Editor Docks", so this is a matter of consistency and clarity.

Suggested change
If [code]true[/code], the dock's tab will be hidden. If [member global] is [code]true[/code], the dock will not appear in the dock menu.
If [code]true[/code], the dock's tab will be hidden. If [member global] is [code]true[/code], the dock will not appear in the [b]Editor &gt; Editor Docks[/b] menu.

Although one could argue that the latter sentence should actually belong in global's description. I can't think of a nice way to make it fit.

Also maybe this should be reversed to dock_shown, instead? Just spitballing. Something about this API doesn't convince me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it could be renamed to active.

Copy link
Member

@Mickeon Mickeon Nov 9, 2025

Choose a reason for hiding this comment

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

"Active" would imply it begins processing something if set to true, as I've skimmed through the class reference to verify the consistency of it. Depending on how you look at it, it may be loosely appropriate. There's also RichTextLabel.scroll_active...

Is enabled too generic?

Copy link
Member Author

Choose a reason for hiding this comment

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

enabled is already used internally for something different.

@arkology
Copy link
Contributor

arkology commented Nov 10, 2025

UPD: Yes, it does... Should we treat this as a bug?

Soo the problem is that previously when you selected Audio tab, the panel would expand height, but then shrink down once you select a smaller dock. Now the new height is permanent, and also bigger than before for some reason. So yeah, it is a bug.

I've read the code (i.e. related changes after my PR) a bit. And looks like everything is completely broken after #106164.

Now the new height is permanent

Yes, this is my omission, I did not take into account all the cases. Will think how it can be fixed.

and also bigger than before for some reason

I suppose this is because of PR I've linked above. Also in current master restoring editor layout (split offset) at editor startup with active audio tab is also broken.

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 10, 2025

I've read the code (i.e. related changes after my PR) a bit. And looks like everything is completely broken after #106164.

The size bug is unrelated to #106164. I opened an issue: #112616

@arkology
Copy link
Contributor

I mean, the part that worked before

@KoBeWi
Copy link
Member Author

KoBeWi commented Nov 10, 2025

Before what? xd
The bigger size is part of the same issue.

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