Skip to content

Conversation

@lodetrick
Copy link
Contributor

See: #106164

These bugfixes are related but necessary for the above PR to function.

The first fix is in TabContainer. Currently TabContainer calls _update_margins in the add_child_notify method. Since it adds the TabBar as a child upon being allocated and the theme cache is not set when allocating the EditorBottomPanel class, this leads to a segfault as it is trying to access a nullptr. This PR fixes the bug by wrapping the theme access behind an if statement, removing many redundant ternaries.

The second fix is in TabBar's theme constants. The tab_separation theme variable did not work correctly where hidden tabs intermixed with non-hidden tabs could still add their separation even though they are not visible. This PR solves the issue by only adding the separation for non-hidden tabs.

@lodetrick lodetrick requested a review from a team as a code owner May 10, 2025 21:59
@AThousandShips AThousandShips added this to the 4.5 milestone May 11, 2025
Copy link
Contributor

@kitbdev kitbdev left a comment

Choose a reason for hiding this comment

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

The tab_separation changes also need to be done in the draw selected tab (below the draw unselected tab) and in TabBar::get_tab_rect.

It looks like the segfault only happened in a previous commit of #106164, so it is hard to debug. There is probably a deeper issue there that caused it. The current changes to TabContainer look fine.

@lodetrick lodetrick force-pushed the tab-control-fixes branch from 000302c to 87acf3d Compare May 12, 2025 08:12
@lodetrick
Copy link
Contributor Author

lodetrick commented May 12, 2025

Ok, I took a deeper look at how the tab_separation worked and realized that we should just always use the ofs_cache when drawing the tabs, we don't need to recalculate it and add more redundant code. I realized that a lot of the if statements were bandaid fixes that could be removed once the ofs_cache was correct. This ended up cleaning the code related to the tab_separation a lot, removing all but five instances of the variable in the code while still acting the same.

Known: Issue with clipping tabs that clips them at the wrong positions, looking into it. (Fixed)
Known: Same issue with interleaved hidden tabs but when dragging tabs, looking into it. (Fixed)

@lodetrick lodetrick force-pushed the tab-control-fixes branch 2 times, most recently from 1ad2dca to 5003d95 Compare May 12, 2025 23:31
@lodetrick
Copy link
Contributor Author

lodetrick commented May 12, 2025

Alright, here's what I did:

I reworked how it calculates where to place the dropping texture (the bar that shows up between tabs). Instead of calculating the average distance between tabs one apart, it merely adds and subtracts half the tab_separation, which is defined to be the distance between tabs. This removed a rather complicated bundle of if-statements and made the code structure closer to how it was before tab_separation was implemented.

I took the opportunity to add the get_previous_available and get_next_available public methods (making the select_previous_available use these internally), these methods are really useful in cases where there are hidden tabs interleaved. Bringing it back to #106164, these methods are necessary for the functionality to move tabs using buttons through the dock manager popup, as the bottom dock is structured to have interleaved hidden tabs. There are no docs for these yet, if this passes checks and is not urgent then I'll let a future PR decide if we want to expose these methods.

I also reworked the get_closest_tab_idx_to_point method. Previously it used the Vector2.distance function but since the TabBar only works on an axis we can just compute abs(tab_center.x - p_point.x). This is an easy performance increase, and if vertical tabs are implemented we only need to change the axis.

@lodetrick lodetrick force-pushed the tab-control-fixes branch from 5003d95 to 41a39de Compare May 13, 2025 00:26
@lodetrick lodetrick force-pushed the tab-control-fixes branch from 41a39de to 33078a3 Compare May 13, 2025 06:02
@lodetrick lodetrick force-pushed the tab-control-fixes branch from 33078a3 to 8e5389b Compare May 13, 2025 15:38
@lodetrick lodetrick force-pushed the tab-control-fixes branch from 8e5389b to 1359e06 Compare May 13, 2025 21:03
Copy link
Contributor

@kitbdev kitbdev left a comment

Choose a reason for hiding this comment

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

Looks good.

I think we should update the PR and commit title to be more accurate, since it does more than change the drawing logic. The segfault is more of a potential segfault too since it doesn't really happen in master. Something like "Fix TabBar hidden tabs handling".

@lodetrick lodetrick changed the title Fix Segfault and Incorrect Theme Drawing in Tab Controls Fix TabBar Hidden Tabs Handling May 14, 2025
@lodetrick lodetrick force-pushed the tab-control-fixes branch from 1359e06 to 5fbc8a6 Compare May 14, 2025 01:11
@Repiteo Repiteo merged commit 926cadb into godotengine:master May 14, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented May 14, 2025

Thanks!

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.

4 participants