Refactor editor EditorBottomPanel to be a TabContainer

[lodetrick]

See: godotengine/godot-proposals#12430, godotengine/godot-proposals#12402
Depends on #106263, #103478, #107395

Currently the Bottom Panel inherits from PanelContainer. This makes it hard to integrate with systems like the editor docks that need to make exceptions everywhere in their code for the bottom panel. This PR converts the type of EditorBottomPanel to a TabContainer. This removes some complexity inside the class as it essentially re-implemented the functionality, but more importantly allows future integrations with the dock system to be much more painless.

[MewPurPur]

Does this change anything for the user, like functionality or UI?

[lodetrick]

Does this change anything for the user, like functionality or UI?

Nothing intended in this PR. Any visual changes are likely bugs. This is just scaffolding for future PRs that would be able to integrate the bottom panel more tightly with the dock system. That would be when the users start to see a difference

Edit: This is no longer true, due to how the refactor needs to be implemented. See below for more details but the essence is that this is now a theme of the tabbar and so is limited by the theming options of said tabbar.

[KoBeWi]

I looked at the new BottomPanel's structure and it's basically the same... Still has internal PanelContainer and a ScrollContainer with Buttons. The TabContainer's TabBar is hidden and unused.

That looks hacky to me with no clear benefit. What complexity did you remove if you had to replace TabContainer's default behavior?

[lodetrick]

It was admittedly hacky to get the custom look of the TabBar and keeping it updated. Thats what the _notify methods are mainly for; updating the bottom bar to be consistent with how the TabContainer understands the tabs. The underlying logic and data is transferred to the TabContainer however, meaning that any TabContainer operation should be consistent with the EditorBottomPanel's exposed methods. The main complexity that was lost is that the BottomPanel no longer has to manage its children directly, it delegates that to the TabContainer, it instead focuses on how to update the buttons.

To be honest, most if not all of the complexity could be avoided if we were alright with not having the custom buttons and having the TabBar look on the bottom, but I felt that this refactor would be best to not change any visuals. The main point was to just change what class it inherited from. Of course I am willing to have your opinion on whether this complexity is worth it.

This is a brief mockup of how it would look like if it used the default TabContainer look

At one point I pursued creating a class that inherited from TabBar, I'll look into that again

[KoBeWi]

The main complexity that was lost is that the BottomPanel no longer has to manage its children directly

It didn't do anything more than toggling visibility, no?

I think if you want to refactor the bottom panel into TabContainer, it should become actual TabContainer. You can add a TabBar theme variation that makes it look like the current bottom panel's bar. Although that sounds like a change that should be discussed more.

Removing the buttons would also be useful if we eventually decided to improve docking system and allow moving tabs between docks and bottom panel.

[lodetrick]

Ok. I tried to create a theme that mimicked the look of the bottom panel and it does look good, the one constraint that I ran into is that the TabBar does not have a selected_hovered stylebox, or at least does not show the hovered stylebox when hovering over the selected tab. This would probably have to be introduced or changed if we are looking for visual parity with the current master. Also, the tabbar background's margins are not respected by the tabbar.

I think I agree that the theme override is the way to go now. I can draft a proposal once the theme is ready and I'm sure that this is feasible and able to accommodate for the buttons on the right side.

[lodetrick]

Alright, so I've gotten rid of the extraneous code and added a new theme override that mimics master's look pretty well, only having to compromise by having selected panels be "pushed down" because pressed panels can't be hovered. The bottom panel now looks like this:

The only "hack" that I needed to do was to override the TabContainer's _update_margins method in order to be able to set the margins of the tabbar so that the (version, pin, expand, etc.) buttons could fit on the right.

Along the way I also fixed a bug in the tab_separation theme variable where hidden tabs intermixed with non-hidden tabs could still add their separation even though they are not visible.

This refactor has drastically reduced the amount of code in the EditorBottomPanel class. That being said, I'm having trouble considering compatibility. The add_item method originally returned a button that plugins hid and toggled to their will. I ended up creating a dummy button that has signals connected to functionality that mirrors how it was used in the past.

I would love to deprecate the functions that have become wrappers for remove_child or move_child, but I'll let that be a discussion for a future PR.

[kitbdev]

• Also needs Add switch on hover to TabBar #103478 to keep the same behavior
• Allow TabContainers to position some child nodes in the tabs bar godot-proposals#2250 would be useful for the buttons
• Since the buttons could previously have different styles something like Adding support for styling the text on TabBar Tabs godot-proposals#9161 is needed too

More options for individual tabs is important to keep it the same, since it is already used for the Output and Debugger to show more information:

The tab bar and tab container bug fixes can be moved to a new PR, it would be easier to get them merged and keeps this one more focused.

[arkology]

Also tabs open on mouse press, not on release event. It feels strange (maybe not proper word, I'm not native English speaker. Maybe it would be more accurate to call it "unusual.")

[lodetrick]

Pulled from the updated #106263.

I fixed the sizing issues by setting a minimum size to something that I thought was reasonable. I also realized that there was a hack remaining (overriding _update_margins()), so I removed that by replacing it with a custom margin.

With regards to events on mouse pressed vs mouse released, I see what you mean about it being different than before, but that's how TabBar is coded so I'm not too sure how possible it would be to change, it would probably be a separate PR at least, it seems like a larger change (though I am curious why it is that way).

[arkology]

I fixed the sizing issues by setting a minimum size to something that I thought was reasonable.

I would prefer a more adaptive solution, but it's ok for now.

Found another bug – panel layout breaks when using Arabic (right-to-left language).

[lodetrick]

I fixed the sizing issues by setting a minimum size to something that I thought was reasonable.

I would prefer a more adaptive solution, but it's ok for now.

I fixed the issue in #108592 but didn't want to rebase over it just yet when this solution exists. The main reason why this issue surfaces is because the Output panel has an unusually small minimum width compared to the other panels. I can remove the minimum size line when that PR is merged

[KoBeWi]

Needs final (?) rebase. Also shouldn't the commits be squashed?

[KoBeWi]

So I'm updating #108647 and I have a problem with the rightside BoxContainer:

For whatever reason the TabBar ends perfectly before the icons, but I need more space due to menu icon. I can't figure out what ensures the TabBar size. It can be a coincidence 🙃

[lodetrick]

I ran into this same issue when writing #106426, and I solved it with this snippet:

void EditorBottomPanel::_theme_changed() {
	Ref<StyleBox> bottom_tabbar_style = get_theme_stylebox("tabbar_background", "BottomPanel")->duplicate();
	bottom_tabbar_style->set_content_margin(SIDE_RIGHT, bottom_hbox->get_minimum_size().x + bottom_tabbar_style->get_content_margin(SIDE_LEFT));
	add_theme_style_override("tabbar_background", bottom_tabbar_style);

It's been a while since I wrote it but essentially I modified the margins of tabbar_background to create the space necessary for the icon to not clip with anything. Another solution would be to just make it a button like the bell and version number instead of the built in tabcontainer use. Would you want me to include this in this PR?

Also relevant: #108647 (comment)

edit: this snippet is already part of this PR, I misremembered what it did.

[KoBeWi]

I ran into this same issue when writing #106426, and I solved it with this snippet:

Wait, so this PR has no such code? How does it even work? The TabBar already takes the icons into account, so it should be a matter of increasing the margin or whatever.

Another solution would be to just make it a button like the bell and version number instead of the built in tabcontainer use.

It would be better if the menu button was stil part of the TabContainer, so that all dock TabContainers are consistent.
I think a proper solution is adding support for side controls in TabContainer, but I guess I will hack it for now.

Would you want me to include this in this PR?

No, ideally this PR should not get any changes until merging. I already rebased my branch xd

[lodetrick]

It works by adding the button hbox as a child of the tab_bar within the tabcontainer so it doesn't get counted as one of the tabs, and then setting the margins to right, expand right. Speaking of that I think it might be possible to get this to work simply if you modify around line 208 in this PR of editor_bottom_panel.cpp so that it has an offset equal to the size of the button:

bottom_hbox->set_anchors_and_offsets_preset(Control::PRESET_RIGHT_WIDE);

[KoBeWi]

It works by adding the button hbox as a child of the tab_bar within the tabcontainer so it doesn't get counted as one of the tabs, and then setting the margins to right, expand right.

Yeah, but why does TabBar/TabContainer respect that margin? I checked with EditorDebugger, the TabContainer encompasses the buttons

but the TabBar is perfectly fit to them:

TabContainer will normally stretch the TabBar to full width.

btw, I actually found a bug 💀 The version goes out of bounds if editor is launched with closed bottom panel:

[lodetrick]

I think the core reason why this works is that nodes that aren't containers don't take their child positions and sizes into account when updating their minimum size. The hbox can be added as a child and repositioned to be at the far right of the tab_bar growing to the right, while not affecting the tab_bar size. Then it uses that code snippet I pasted earlier to move the far right of the tab_bar to be exactly the size of the hbox left, allowing the hbox to not be clipped and visible.

So the TabContainer technically doesn't know that the hbox exists, the bottom_panel just has code to update the margins based on the size of the hbox.

[KoBeWi]

Found it. It's this code in _theme_changed().

Ref<StyleBox> bottom_tabbar_style = get_theme_stylebox("tabbar_background", "BottomPanel")->duplicate();
bottom_tabbar_style->set_content_margin(SIDE_RIGHT, bottom_hbox->get_minimum_size().x + bottom_tabbar_style->get_content_margin(SIDE_LEFT));
add_theme_style_override("tabbar_background", bottom_tabbar_style);

EDIT:
That's the same code you quoted above 🤦‍♂️
So it was in this PR.

[arkology]

I can't find if this has already been said, but clicking the 3-dot button causes the editor to freeze and crush.

[KoBeWi]

The crash (I think it's the one):

CrashHandlerException: Program crashed
Engine version: Godot Engine v4.6.dev.custom_build (af6c45458ce9a6318e45f270772248f86f58bff2)
Dumping the backtrace. Please include this when reporting the bug on: https://github.com/godotengine/godot/issues
[0] Node::get_parent (C:\godot_source\scene\main\node.cpp:2083)
[1] Node::get_parent (C:\godot_source\scene\main\node.cpp:2083)
[2] EditorDockManager::get_dock_tab_container (C:\godot_source\editor\docks\editor_dock_manager.cpp:847)
[3] DockContextPopup::_update_buttons (C:\godot_source\editor\docks\editor_dock_manager.cpp:1238)
[4] DockContextPopup::select_current_dock_in_dock_slot (C:\godot_source\editor\docks\editor_dock_manager.cpp:1255)
[5] call_with_variant_args_helper<DockContextPopup,int,0> (C:\godot_source\core\variant\binder_common.h:223)
[6] call_with_variant_args<DockContextPopup,int> (C:\godot_source\core\variant\binder_common.h:337)
[7] CallableCustomMethodPointer<DockContextPopup,void,int>::call (C:\godot_source\core\object\callable_method_pointer.h:103)
[8] Callable::callp (C:\godot_source\core\variant\callable.cpp:57)
[9] CallableCustomBind::call (C:\godot_source\core\variant\callable_bind.cpp:150)
[10] Callable::callp (C:\godot_source\core\variant\callable.cpp:57)
[11] Object::emit_signalp (C:\godot_source\core\object\object.cpp:1342)
[12] Node::emit_signalp (C:\godot_source\scene\main\node.cpp:4167)
[13] Object::emit_signal<> (C:\godot_source\core\object\object.h:967)
[14] TabContainer::gui_input (C:\godot_source\scene\gui\tab_container.cpp:78)
[15] Control::_call_gui_input (C:\godot_source\scene\gui\control.cpp:1891)
[16] Viewport::_gui_call_input (C:\godot_source\scene\main\viewport.cpp:1729)
[17] Viewport::_gui_input_event (C:\godot_source\scene\main\viewport.cpp:1970)
[18] Viewport::push_input (C:\godot_source\scene\main\viewport.cpp:3485)
[19] Window::_window_input (C:\godot_source\scene\main\window.cpp:1835)
[20] call_with_variant_args_helper<Window,Ref<InputEvent> const &,0> (C:\godot_source\core\variant\binder_common.h:223)
[21] call_with_variant_args<Window,Ref<InputEvent> const &> (C:\godot_source\core\variant\binder_common.h:337)
[22] CallableCustomMethodPointer<Window,void,Ref<InputEvent> const &>::call (C:\godot_source\core\object\callable_method_pointer.h:103)
[23] Callable::callp (C:\godot_source\core\variant\callable.cpp:57)
[24] Callable::call<Ref<InputEvent> > (C:\godot_source\core\variant\variant.h:949)
[25] DisplayServerWindows::_dispatch_input_event (C:\godot_source\platform\windows\display_server_windows.cpp:4461)
[26] DisplayServerWindows::_dispatch_input_events (C:\godot_source\platform\windows\display_server_windows.cpp:4431)
[27] Input::_parse_input_event_impl (C:\godot_source\core\input\input.cpp:979)
[28] Input::flush_buffered_events (C:\godot_source\core\input\input.cpp:1262)
[29] DisplayServerWindows::process_events (C:\godot_source\platform\windows\display_server_windows.cpp:3822)
[30] OS_Windows::run (C:\godot_source\platform\windows\os_windows.cpp:2332)
[31] widechar_main (C:\godot_source\platform\windows\godot_windows.cpp:96)
[32] _main (C:\godot_source\platform\windows\godot_windows.cpp:122)
[33] main (C:\godot_source\platform\windows\godot_windows.cpp:136)
[34] WinMain (C:\godot_source\platform\windows\godot_windows.cpp:150)
[35] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[36] ShimMainCRTStartup (C:\godot_source\platform\windows\cpu_feature_validation.c:74)
[37] <couldn't map PC to fn name>
-- END OF C++ BACKTRACE --

get_current_tab_control() returns null when the bottom dock is closed.

I know I said it'd be better if you didn't make more changes, but this should be fixed, and tbh rebasing won't really be a problem.

[lodetrick]

Does that crash occur in this PR? I thought the 3-dot button was disabled in this one.

About that, if the bottom dock is closed, should it be allowed to show the popup? The popup is meant to rearrange the currently selected tab but if there is none then what tab would it rearrange? It could rearrange the previous tab but that's also not guaranteed to return a non-null pointer.

[arkology]

Does that crash occur in this PR? I thought the 3-dot button was disabled in this one.

Oops, sorry 😅 I downloaded PR artifacts from this PR and #108647 and looks like I mixed up everything. Silly me.

[KoBeWi]

The version button bug I mentioned in #106164 (comment) could be fixed though.