feat(dock): make tab bar size configurable - #2972
Conversation
TabBar already followed DockSkin::tab_size; the auto-style title used when a group has one visible panel stayed hardcoded at 30px.
Don't worry about this, we have totally accept AI written codes, even 100%. |
huacnlee
left a comment
There was a problem hiding this comment.
Thanks for the report — the underlying problem is real, dock tab bars genuinely have no height escape hatch today. But I don't want to merge this API shape, and I think the diagnosis stops one level short of the actual cause.
1. tab_size does not name a size, it names a height
Size::Size(Pixels) has no single meaning in this library. Depending on the component it is:
- a square edge —
StyleSized::size_with→.size(px),Icon,Avatar,OtpInput,Button's icon box - a width —
NumberInput→this.min_w(size)(crates/component/src/input/number_input.rs:150) - a row height —
Size::table_row_height - a font size —
input_text_size→size * 0.875
This PR adds a fifth meaning ("dock tab outer height") behind a public setter called set_tab_size, and the only way a caller learns which one they got is by reading the impl. If the dock needs to size its chrome, the API should name the thing it sizes: tab_bar_height() -> Pixels / set_tab_bar_height(Pixels). A Pixels in, a Pixels out, no enum to disambiguate.
2. The value silently becomes widths
The number passed as a height is then used for horizontal spacing in four places:
crates/component/src/tab/tab.rs:90—inner_paddings:height * 0.375crates/component/src/tab/tab_bar.rs:352—default_gap:height * 0.375crates/component/src/tab/tab_bar.rs:372— segmentedpadding_x:height * 0.125crates/component/src/tab/tab_bar.rs:388— underlinegap:height * 0.5
So "tab size" is a height in one place and a width in four, and the ratios do not reproduce the scale they claim to generalize. Checking them against the built-in steps:
| ratio gives | actual | |
|---|---|---|
default_gap @ Large (36) |
13.5 | 16 |
inner_paddings @ Small (24) |
9 | 10 |
inner_paddings @ Large (36) |
13.5 | 16 |
underline gap @ Medium (36) |
18 | 16 |
underline gap @ Small (30) |
15 | 12 |
underline gap @ Large (44) |
22 | 20 |
Size::Size(px(36.)) is therefore not Size::Large, and only Size::Size(px(32.)) happens to land on Size::Medium for the default variant. A custom value lands between the named steps by a rule no caller can predict, so these constants are unreviewable — there is nothing to check them against.
3. The pass is partial, which confirms it is a height and not a size
Size::Size reaches the outer height and the horizontal padding, but falls through to the Medium arm for inner_margins (tab.rs:108), radius / inner_radius / tab_bar_radius (tab.rs:351-386), the label font (_ => this.text_sm()), and the icon (_ => this.size_4()). A 44px tab is a medium tab with air around it. That may well be the right behaviour — but then the parameter is a height, and calling it a size promises scaling it does not deliver.
4. The motivation points at a different bug
This makes dock tabs clip when an application increases its global text scale.
Labels are rem-based (text_sm → rems(0.875)), tab heights are literal px(). That mismatch is the bug: the box does not follow the text. The library already solves exactly this in Icon, which derives its size from window.rem_size() (crates/component/src/icon.rs:150).
Making the dock chrome's fixed heights rem-derived fixes the clipping for every application without new public API, and without asking apps to recompute a pixel height each time they change scale — which is what this PR leaves them doing. I'd rather see that change. If an explicit per-app override is still wanted afterwards, it can be added on top, as set_tab_bar_height(Pixels).
5. A third height table in render_title
crates/component/src/dock/tab_panel.rs:373-378 introduces its own 20/24/36/30 mapping, while TabVariant::height maps 20/24/36/32. Medium disagrees, so set_tab_size(px(32.)) — the value that equals today's default tab bar — silently makes the single-panel title bar 2px taller than leaving the default alone. Whatever the final shape, this height needs to come from one place.
6. A closed bottom dock still clips
crates/base/src/dock/dock_area.rs:1932:
/// A closed bottom dock keeps this much, so its tab bar stays clickable.
pub const CLOSED_BOTTOM_STRIP: Pixels = px(29.);With set_tab_size(px(44.)) that strip cuts off the tab bar it exists to preserve. Any height knob has to reach this constant too — another argument for fixing the scaling at the source rather than threading a value through the skin.
7. Docs and story
DockSkin's settings are documented in website/docs/dock.md and website/zh-CN/docs/dock.md; new public API has to land in both locales. There is also no story coverage for the new setting.
Minor
tab_size_changes_the_height_left_for_panel_content asserts medium_content - custom_content == px(12.), which encodes "the default bar is 32px" as an unexplained 12. Assert the two heights instead so a change to the default fails with a readable message.
Requesting changes. Happy to take a PR that makes the dock's fixed chrome heights follow rem_size; if we still want an explicit override after that, let's land it as set_tab_bar_height(Pixels) rather than overloading Size.
Tab labels are rem-based while chrome was a pixel constant, so a larger theme font clipped dock tabs. Scale those heights with rem_size, keep padding on named Size steps, and replace set_tab_size(Size) with set_tab_bar_height(Option<Pixels>). Closed bottom docks follow the same height so the strip still shows the bar.
|
Thanks for the review — this push follows that diagnosis.
|
huacnlee
left a comment
There was a problem hiding this comment.
The rem-scaling change addresses a real problem, and this revision incorporates much of the earlier feedback. However, the PR still combines too many separate design decisions for one fix: default scaling, exact pixel overrides, shared sizing for tab bars and single-panel titles, collapsed-dock sizing, and new public renderer and metric APIs.
Please narrow this PR to fixing the default chrome when the window's text scale increases. Tab labels and their surrounding layout should scale coherently without requiring applications to supply a pixel height.
For this revision, please:
- Keep the rem-based fix and validate ordinary dock tabs, single-panel titles, toolbar controls, and collapsed bottom docks together.
- Remove the explicit height override APIs (
DockSkin::set_tab_bar_heightandTabBar::with_tab_height) and the override plumbing through tabs and indicators. - Remove the override-driven renderer/API additions and public exports of fallback height metrics unless they are demonstrably necessary for the default scaling fix.
- Avoid turning the historical 29px/32px dimensions into a proportional rule for arbitrary heights. Related layout dimensions should have a clear structural reason, rather than being corrected through ratios.
- Keep the story focused on default scaling instead of demonstrating a fixed pixel override.
There is also a concrete problem with the new exact-height contract: in a rendered dock test with a 32px rem and set_tab_bar_height(Some(px(32.))), the chrome actually occupies 41px. The override reaches the tabs, but other controls still determine the overall height. The existing test resets rem to 16px before applying the override, so it misses this combination.
Custom tabs embedded in an application window's TitleBar are a separate composition problem. That specialized layout can use gpui-base::Tab / Tabs with application-owned geometry; it should not drive additional sizing policy into the standard Dock and TabBar. This does not dismiss the original dock clipping issue, which the library should fix.
My earlier review left room for a later pixel override, so I understand why it is included here. To make the scope explicit now: please leave height customization for a separate, independently justified proposal. I would prefer a smaller, coherent fix for default scaling over continuing to patch the interactions introduced by the override.
|
One clarification from my side: my earlier automated review may have steered this PR toward a broader solution than the original problem required. Some of the additional complexity follows suggestions I made, so that is not solely on this revision. Looking at the change as a whole, the bug fix and the broader sizing/API improvements have become intertwined, making the PR harder to review and move forward. I suggest separating them:
This should give us a smaller fix we can assess and land independently, without making it depend on agreement about a larger API design. There is no need to solve all of the historical sizing inconsistencies in this PR. |
Summary
DockSkin::tab_sizeandDockSkin::set_tab_sizeso applications can size dock-owned tab barsTab, including the outer height, inner surface, and proportional horizontal spacingMotivation
TabBarimplementsSizable, butSize::Size(Pixels)currently falls through to the medium tab metrics. In addition, applications cannot pass a size to theTabBarconstructed internally byDockSkin. This makes dock tabs clip when an application increases its global text scale.The default remains
Size::Medium, so existing docks are unchanged. Applications that need larger UI chrome can call:Testing
cargo test -p gpui-component— 421 unit tests and all compatibility suites passcargo fmt --all -- --check— passescargo clippy -p gpui-component --all-targets— no diagnostics in changed filescargo clippy ... -- -D warningsis currently blocked by two pre-existingclippy::nonminimal_booldiagnostics incrates/base/src/calendar.rs:131under Rust 1.95AI assistance
AI was used to inspect the component and dock rendering paths, draft the implementation and tests, and prepare this PR description. I reviewed the resulting API and diff and ran the test and formatting checks listed above.