Skip to content

feat(dock): make tab bar size configurable - #2972

Open
hipstersmoothie wants to merge 3 commits into
longbridge:mainfrom
hipstersmoothie:feat/configurable-dock-tab-size
Open

feat(dock): make tab bar size configurable#2972
hipstersmoothie wants to merge 3 commits into
longbridge:mainfrom
hipstersmoothie:feat/configurable-dock-tab-size

Conversation

@hipstersmoothie

@hipstersmoothie hipstersmoothie commented Sep 5, 2026

Copy link
Copy Markdown

NOTE: This was completely written by AI but the issue is real. I was not able to find a way to set the height of the tab bar in any way.

Summary

  • add DockSkin::tab_size and DockSkin::set_tab_size so applications can size dock-owned tab bars
  • honor custom pixel sizes in Tab, including the outer height, inner surface, and proportional horizontal spacing
  • add a rendered dock test proving a taller tab bar leaves correspondingly less panel content

Motivation

TabBar implements Sizable, but Size::Size(Pixels) currently falls through to the medium tab metrics. In addition, applications cannot pass a size to the TabBar constructed internally by DockSkin. 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:

skin.set_tab_size(px(40.), cx);

Testing

  • cargo test -p gpui-component — 421 unit tests and all compatibility suites pass
  • cargo fmt --all -- --check — passes
  • cargo clippy -p gpui-component --all-targets — no diagnostics in changed files
  • cargo clippy ... -- -D warnings is currently blocked by two pre-existing clippy::nonminimal_bool diagnostics in crates/base/src/calendar.rs:131 under Rust 1.95

AI 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.

TabBar already followed DockSkin::tab_size; the auto-style title used
when a group has one visible panel stayed hardcoded at 30px.
@huacnlee

huacnlee commented Sep 6, 2026

Copy link
Copy Markdown
Member

NOTE: This was completely written by AI but the issue is real. I was not able to find a way to set the height of the tab bar in any way.

Don't worry about this, we have totally accept AI written codes, even 100%.

@huacnlee huacnlee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 widthNumberInputthis.min_w(size) (crates/component/src/input/number_input.rs:150)
  • a row height — Size::table_row_height
  • a font size — input_text_sizesize * 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:90inner_paddings: height * 0.375
  • crates/component/src/tab/tab_bar.rs:352default_gap: height * 0.375
  • crates/component/src/tab/tab_bar.rs:372 — segmented padding_x: height * 0.125
  • crates/component/src/tab/tab_bar.rs:388 — underline gap: 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_smrems(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.
@hipstersmoothie

Copy link
Copy Markdown
Author

Thanks for the review — this push follows that diagnosis.

  • Dock and tab chrome heights now follow window.rem_size(), same idea as Icon. At a 16px rem the numbers are unchanged (32px tab bar, 30px title, 29px closed bottom strip).
  • set_tab_size(Size) is gone. The optional override is set_tab_bar_height(Option<Pixels>): a pixel in, a pixel out, no Size enum. Padding, radius, and type still come from the named size steps.
  • Title bars and tab bars share that override. Closed bottom docks use DockAreaRenderer::closed_bottom_extent so a taller bar is not clipped by CLOSED_BOTTOM_STRIP.
  • Docs in both locales and the dock story cover the setter.

cargo test -p gpui-component --lib dock:: and tab:: pass. cargo clippy -p gpui-component --all-targets -- -D warnings is clean; gpui-base still has the existing clippy::nonminimal_bool in calendar.rs.

@huacnlee huacnlee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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_height and TabBar::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.

@huacnlee

huacnlee commented Sep 9, 2026

Copy link
Copy Markdown
Member

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:

  • First, a focused fix for the original clipping problem, with regression tests and only the changes needed to make the default behavior correct.
  • Then, if there is still a concrete need, a separate PR for height customization or broader sizing improvements, with its own use cases and design discussion.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants