Support dragging actions for horizontal tab groups#12110
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
Reviewed the horizontal tab group dragging changes across TabComponent, horizontal group rendering, shared drag hit-testing helpers, and the new horizontal group save-position id. The PR includes visual evidence, and the attached spec context states that no approved or repository spec context was found.
Concerns
- No blocking correctness, security, or material spec-alignment concerns were found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Visual feedback: I think the tab group looks a little small - it has a different size than standard tabs, I feel like they should match? |
| /// Save-position id for a horizontal tab group's container rect, used for | ||
| /// drop hit-testing and as the collapsed-group fallback in horizontal-axis | ||
| /// drag math. | ||
| pub(crate) fn htab_group_position_id(group_id: TabGroupId) -> String { |
There was a problem hiding this comment.
does something like this exist for vtabs? Or is it taking a different approach?
There was a problem hiding this comment.
Yes, the equivalent for vtabs is found directly above this (vtab_group_position_id). The reason for its existence is to handle two edge cases when dragging tabs into/out of groups:
- The group is expanded, can we drag into the first or last position of the group without dragging past the current first/last tab in that group (hit testing for group bounds)
- The group is collapsed, we must drag the tab past the entire group and not compare with a neighboring tab (which would be inside the group)
| ) | ||
| .finish(); | ||
|
|
||
| // Skip the group `Draggable` while another drag (a member tab via the |
There was a problem hiding this comment.
How does this happen? I would expect there to be some kind of mutex on drag or that the drag is initiated by clicking and holding something (so it wouldn't be possible to initiate twice without release)
There was a problem hiding this comment.
The DraggableState mutex is enough here. This is redundant. Removing it
| tab_bar_state.is_any_tab_dragging && !is_this_group_dragging; | ||
| let group_id = group.id; | ||
| let group_draggable_state = group.draggable_state.clone(); | ||
| let positioned_container: Box<dyn Element> = if skip_group_draggable { |
There was a problem hiding this comment.
nit: A guard clause makes this more readable. E.g.
if tab_bar_state.is_any_tab_dragging && !is_this_group_dragging {
return container;
}
...
peicodes
left a comment
peicodes
left a comment
There was a problem hiding this comment.
Just questions to help my understanding - happy to stamp once I do
2 participants
Description
Adds dragging support for horizontal tab groups, building on the basic rendering from
johnturco/app-4641-horizontal-tab-grouping-basic-rendering. Horizontal tab bars now support:The implementation re-uses the existing vertical-tab group-drag plumbing (
StartGroupDrag/DragGroup/DropGroup,move_group_block,assign_tab_to_group,hop_tab_to_index) and generalizes the axis-specific helpers (neighbor_drag_rect,target_group_at_axis,on_group_drag) so both layouts share one code path.Changes
workspace/view.rsWired up a group-level
Draggableinrender_horizontal_tab_groupthat dispatches the existingStartGroupDrag/DragGroup/DropGroupactions, withDragAxis::HorizontalOnlyand child-mouse-down deferral so member tabs still drag individually. Generalized the axis-specific helpers used by both layouts:target_group_at_axis— replaces the priortarget_group_at_y/target_group_at_xpair; oneis_vertical-parameterized hit-test.neighbor_drag_rect— now takesis_verticaland falls back to the visible group container rect when the neighbor is a hidden collapsed-group member.on_group_drag— generalized over both axes; uses neighbor-midpoint thresholds for vertical and neighbor-edge thresholds for horizontal so each layout matches its per-tab drag feel.tab.rsTabComponentgainsgrouped_member/sole_grouped_memberflags, set via a newfor_grouped_member(is_sole_member)builder. Grouped members get the regular per-tabDraggable; the sole-member case suppresses it so the parent group'sDraggablepicks up the drag.workspace/view/vertical_tabs.rsAdded
htab_group_position_id, the horizontal counterpart to the existingvtab_group_position_id, used by the new hit-test and drag-rect helpers.Linked Issue
https://linear.app/warpdotdev/issue/APP-4649/horizontal-tab-group-dragging
Testing
./script/runScreenshots / Videos
Demo video