Add basic tab group rendering for horizontal tabs#12089
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds the first horizontal tab-group rendering path, including grouped slots in the top tab bar, group headers, member-tab styling, and shared summary-icon selection with vertical tabs. The description includes visual evidence, and the spec context says no approved repository spec was available.
Concerns
- Collapsed horizontal groups stop rendering member tabs without replacing their saved tab-position anchors, so drag insertion can keep using stale member positions and produce insertion indices inside hidden groups.
- Horizontal group menu labels are derived from the vertical-tabs user preference rather than the actually rendered layout, so a horizontal header can show vertical wording when the vertical-tabs panel is closed.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| ctx, | ||
| )); | ||
|
|
||
| if !is_collapsed { |
There was a problem hiding this comment.
tab_position_id SavePosition entries and the header does not write a replacement. tab_insertion_index_for_cursor still reads those cached tab positions, so after collapsing a previously expanded group, stale member rects can return insertion indices inside the hidden group while this renderer only draws a ghost before the group. Dropping there can split the group/run; add a saved group/header rect and map collapsed groups to the correct boundary, or otherwise replace/ignore hidden member positions.
There was a problem hiding this comment.
This will be resolved in my next PR that implements dragging support. Trying to drag into a group currently results in undefined behavior.
|
Note: Follow-up PR coming to move the bulk of the tab grouping logic out of workspace/view.rs and into a new file called tab_grouping.rs — this file is getting large and the grouping changes warrant their own module. |
| }) | ||
| } | ||
|
|
||
| // Render each slot in the tab bar, either an indiovidual tab or tab group. |
peicodes
left a comment
peicodes
left a comment
There was a problem hiding this comment.
Two thoughts:
- We should be very confident existing horizontal tab dragging still works, incl dragging out into a new window, please make sure that's being tested
- The descriptions for PRs should be formatted to better suit a human reader who doesn't have knowledge of how tab dragging and grouping works. No need to enumerate code changes; it's much more helpful to know (in a few pragraphs) how it roughly works, what deviations you had to make with existing code, what things you did that might be sus and you want an extra pair of eyes on
|
|
||
| /// A unit the horizontal tab bar renders: either a single ungrouped tab or a | ||
| /// contiguous run of same-group tabs collapsed into one group container. | ||
| enum TabBarSlot { |
There was a problem hiding this comment.
We should leave a note on Vertical tabs (where that loop is) that we should use this instead
| header.finish() | ||
| } | ||
|
|
||
| /// Up to 4 distinct pane icons for the group's collage. Each member tab |
There was a problem hiding this comment.
nit: I think this description should focus on what this function does and why instead of how it's done. It would be helpful to illustrate with an example
| { | ||
| if *last_gid == group_id { | ||
| *run_len += 1; | ||
| continue; |
There was a problem hiding this comment.
nit: whenever you have looping logic with early returns, it helps to explain what case that's covering
| } | ||
|
|
||
| // Render each slot in the tab bar, either an indiovidual tab or tab group. | ||
| for slot in &slots { |
There was a problem hiding this comment.
nit: if we extract this to a function, it'll be more obvious to a reviewer what the blast radius of this code is
| } | ||
| TabContextMenuAnchor::Pointer(position) => { | ||
| let positioning = match (is_vertical, anchor) { | ||
| (true, TabContextMenuAnchor::VerticalTabsKebab) => { |
There was a problem hiding this comment.
nit: I think a match on multiple vars is an anti-pattern because it makes it harder to tell at a glance if all cases are covered
| .with_height(GROUP_ICON_COLLAGE_SIZE) | ||
| .finish(), | ||
| ); | ||
| for (idx, kind) in kinds.iter().take(count).enumerate() { |
There was a problem hiding this comment.
another example of where filter/map is probably possible
| appearance, | ||
| ); | ||
| let offset = match (count, idx) { | ||
| (3, 0) => vec2f(-diag, -diag), |
There was a problem hiding this comment.
// 3 items in group, 1st item
johnturcoo
deleted the
johnturco/app-4641-horizontal-tab-grouping-basic-rendering
branch
2 participants
Description
Adds basic rendering for horizontal tab groups in the top tab bar, on top of the existing vertical tab grouping data model. A group renders as a single tab-bar slot containing a header (icon collage + name) followed by its member tabs.
Dragging is out of scope for this PR — neither groups nor members can be dragged yet.
All new behavior is gated behind
FeatureFlag::GroupedTabs; when the flag is off, the horizontal tab bar renders exactly as before.Changes
workspace/view.rsTabBarSlot { Single | Group }enum and a preprocessing pass inrender_tab_bar_contentsthat turns the flat tab list into a list of slots (ungrouped tabs becomeSingle, contiguous same-group runs collapse intoGroup).render_horizontal_tab_group(container + member tabs) andrender_horizontal_tab_group_header(icon collage + name + click/double-click/right-click handlers).compute_group_member_kinds— picks up to 4 distinct pane icons for a group by reusing each member tab's "Summary pair" selection, so the group icons are always a subset of what the member tabs would show in vertical Summary.render_group_member_icon_collage— 1 or 2 icons reuse vertical Summary'sSingle/Pairlayout for visual parity; 3 or 4 fall back to a corner collage. Designs for reference:select_unique_pane_kinds— shared selection helper used by both vertical Summary (select_summary_pane_kind_icons) and the horizontal collage.horizontal_tab_group_mouse_states) onWorkspace.tab_group_menu_itemsnow takesis_vertical: boolso labels adapt to layout.tab.rsTabComponentgained agrouped_memberflag and.for_grouped_member()builder. In that mode the tab skips side dividers, paints an inset rounded highlight, and skips theDraggablewrapper (member dragging is a follow-up).workspace/view/vertical_tabs.rsrender_summary_pane_kind_iconsandSummaryPaneKindIconsare nowpub(super)and accept atotal_sizeparameter so the horizontal collage can reuse them at its own sizing.select_summary_pane_kind_iconsnow delegates to the sharedselect_unique_pane_kindshelper.Linked Issue
https://linear.app/warpdotdev/issue/APP-4641/horizontal-tab-grouping-basic-rendering
Testing
./script/runScreenshots / Videos
Demo Video