Add support for vertical TabBar and side tab placement on TabContainer#116485
Add support for vertical TabBar and side tab placement on TabContainer#116485Quatumus wants to merge 1 commit into
TabBar and side tab placement on TabContainer#116485Conversation
TabBar and side tab placement on TabContainer
|
Hello,
If you are still interested in this feature, please rebase it, and I will review it. |
|
I am still interested in getting this implemented, I will do a rebase later today. |
3dfdb0d to
711137e
Compare
There was a problem hiding this comment.
The basics of it work.
Some issues:
POSITION_BOTTOMno longer flips the stylebox vertically
before:
after:
POSITION_RIGHTandPOSITION_LEFTprobably should rotate the stylebox 90 degrees to bring behavior in-line withPOSITION_BOTTOM(this is my opinion, we'll see what the maintainers think)- In every position, the
TabBarin theTabContainershould have a small gap to the left/top as was previously the case (see images above for reference) - Reintroduces the bug fixed in #118323
- Popup menu button location in
TabContainerinPOSITION_RIGHTandPOSITION_LEFTis wrong and does not update when tabs are added/removed
- With
POSITION_RIGHTandPOSITION_LEFT, the tabs don't properly respect maximum size
TabBar(standalone) minimum size computation is wrong when given a maximum height (when it works)
TabBar(standalone) completely breaks down when given a maximum width in vertical mode (in the image, the maximum width was even larger than the natural width)
Edit: Please remember to squash your commits. It's not a blocker for reviewing, but it is a blocker for merging.
635d73f to
55c0525
Compare
|
I fixed the issues you @StarryWorm found it should hopefully be good now. |
StarryWorm
left a comment
There was a problem hiding this comment.
Compiled locally on windows since GHA won't run automatically on first-time contributor PRs
All testing was done in a new project. Please let me know if any of these are not replicatable on your end.
All issues remain except the stylebox flip in POSITION_BOTTOM and tabs not respecting maximum height in POSITION_LEFT/POSITION_RIGHT
Remaining/new issues:
TabBarswitching from horizontal to vertical does not result in the same minimum size computations with a single child. The size should be the same in both cases since the horizontal/vertical modes should make 0 difference with a single child.
Setup: TabBar with one child with title = "Tab", horizontal mode size: (49, 40), vertical mode size: (45, 40). After switching to vertical I also had to reset size to get it to its final size of (45, 31). The needed reset might be due to the whole issue solved by #112741
Setup 2: same thing but with the 2D editor icon added, horizontal: (69, 31), vertical (69, 31), resets to (65, 31), but clips the tab (i.e. it is too big)
TabBar: Updating the max size doesn't automatically reflect in vertical modeTabBar#118323 bug still here
TabBarin vertical mode with a custom maximum width causesERROR: .\core/math/rect2.h:185 - Rect2 size is negative, this is not supported. Use Rect2.abs() to get a Rect2 with a positive size.on size reset and whatever this is
TabContainer: settingPOSITION_RIGHTorPOSITION_LEFTwith a maximum size clips them outside of the container. If the max size is "too small" it even triggers this errorERROR: .\core/math/rect2.h:185 - Rect2 size is negative, this is not supported. Use Rect2.abs() to get a Rect2 with a positive size.
- Follow up, after that resetting the maximum size causes this visual error (tabs are drawn underneath the panels). First image is
TabContainerselected, second isPanelchild selected:
TabContainerpopup menu button still inside of thePanelchild area instead of below the tabs
461bfa2 to
8bf6e87
Compare
|
Third time's a charm, I even had a friend test it out this time for bugs. |
There was a problem hiding this comment.
Looking a lot better.
Here's my test project so you can replicate these issues. All issues are compared against 4.7.dev4 which is the last time these two classes were touched (by me).
There is one issue that I think would need to be adressed in this PR which is not due to this PR:
- When creating a
TabBar, adding a tab to it, it keeps a height of 40px. However, resetting the size, the height shrinks to 31px.
The reason I bring this up is that it is the hidden cause behind the issue in my previous review whereby switching to vertical mode will then shrink it to 31px (its intended height in both horizontal and vertical modes).
This causes inconsistent behavior between horizontal and vertical modes, and switching to vertical will visually shrink the tab to 31px, but not its bounding box.
This is an issue in your PR, as the visual and bounding box are not aligned. The issue with vertical tabs clipping in the case of an icon is fixed though, which is amazing.
Remaining issues:
- Giving the
TabBara maximum width that is not excessively large results in the full tab not drawing within its bounding box
- In vertical mode, with clip tabs on and a large maximum height, the tabs draw outside of the bounding box (the bounding box should be fixed)
TabContainerhas some issues with the area it gives to its children when switching between side tabs and top/bottom tabs. It appears theirparent_maximum_size_cacheisn't being updated properly. Hiding and reshowing the container fixes the issue. It's very finnicky, mess around with the min and max sizes and toggle between states and it should happen.
Additional points:
- With side tabs, the popup button and/or scroll tabs buttons for
TabContainercreate a lot of dead space. I personally think they would be better at the bottom of the tabs (where panels 11 and 12 are in the image). @AdriaandeJongh WDYT?
This looks especially bad with few tabs and just the popup button
Once all the basics are fully figured out, I'll try a few more advanced cases (close buttons, etc).
|
I don't know how you do it but im glad we got people like you. Back to work I go. |
|
Other than fixing the clear problems, I also moved the side buttons to the bottom of the tabs as suggested. |
StarryWorm
left a comment
There was a problem hiding this comment.
New issue:
- Changing the single-tab
TabBarfrom horizontal to vertical now expands its bounding box vertically to 47px for some reason. You can see the rect expanding in the images below. The verticalTabBarstill visually shrinks to 31px but we have established that is not a bug, and I would agree.
| Horizontal | Vertical |
|---|---|
![]() |
![]() |
![]() |
![]() |
Mutated Issues:
- You fixed the maximum width issue the wrong way. Instead of clipping the bounding box to the size of the bounding box, it should expand the
TabBarto draw in the entire bounding box. This results in aTabBarwith a just large enough maximum size not drawing itself fully.
| Normal | Current PR state | Before latest fix |
|---|---|---|
![]() |
![]() |
Size2(69, 31) |
![]() |
![]() |
![]() |
- With vertical mode and a maximum height, the
TabBarshould expand up to that maximum height (with clip tabs on) before clipping tabs.- Side note: Currently this is handled in
get_minimum_size()but that should change in the future. See #118651.
- Side note: Currently this is handled in
In both images below, max size is significantly larger than needed for no clipping on both axes.
- You've also broken it for horizontal mode
|
Let's see how you manage to break it this time. |
|
I broke it by getting #118651 merged :) The tl:dr is, the auto-expanding behavior that used to be in Sorry, I didn't review your latest iteration. I knew I was going to break it, so I figured I might as well wait for that and then test once brought up-to-date. |
|
I did a rebase and hopefully nothing got broken while I was at it. |
…iner # Conflicts: # scene/gui/tab_bar.cpp # scene/gui/tab_container.cpp
StarryWorm
left a comment
There was a problem hiding this comment.
It appears I wasn't clear when I explained the new system to you and caused some confusion. Sorry about that.
TabBar::get_minimum_size() should return the true minimum size of the TabBar at all times. No expanding.
TabBar::get_desired_size() should return the largest size that would fit within the maximum size provided, i.e. the expanding up until max size behavior.
I would recommend looking at the changes I did for that PR (#118651) for tab_bar.cpp.
--
Also if you could, please, for the sake of reviewers, keep the methods in the order they were in (i.e. get_minimum_size starts at the top, line 50). For your new methods, add them after that. I know it sounds silly and unnecessary, but Git doesn't really like when things get moved around and changed at the same time, it makes reviewing the diff very hard. Take a look at the top of the tab_bar.cpp diff on your PR and you'll see what I mean.
We're looking at a pretty big PR, and this is causing a ton of extra diff noise which makes it look worse than it really is.









This PR adds a support for arranging tabs vertically with the TabBar and for placing the TabBar on the side with a TabContainer.
This would close godotengine/godot-proposals#1986 and closes godotengine/godot-proposals#5031