diff --git a/AGENTS.md b/AGENTS.md index 69a3f01bc6..74e61b1b94 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -243,6 +243,17 @@ Text input system based on Rope data structure: else reads as a competing context. A callback receiving both takes the GPUI one as `cx` and names the other after what it holds (`trigger`, `state`). +## Pull Request Reviews + +Every PR review must present its conclusions in this order: + +1. Explain the PR's purpose: the problem it addresses and the scope of its changes. +2. Assess whether the requirement and proposed approach are reasonable. +3. Report concrete findings, followed by validation performed and its limitations. + +Do not start a review with findings alone; include the purpose and reasonableness +assessment even when no issues are found. + ## Code Style - Follow naming and organization patterns from existing code diff --git a/crates/base/src/dock/dock_area.rs b/crates/base/src/dock/dock_area.rs index 207facdade..f0718dcaa6 100644 --- a/crates/base/src/dock/dock_area.rs +++ b/crates/base/src/dock/dock_area.rs @@ -623,11 +623,89 @@ impl DockArea { let Some(region) = self.placement_of_panel(panel) else { return; }; + let focused = window.focused(cx); + let had_focus = self + .panel(panel) + .is_some_and(|view| view.focus_handle(cx).contains_focused(window, cx)); + // Prefer the same group, then the groups following it in layout order, + // then preceding groups from nearest to farthest. Preserve this order + // before normalization removes an emptied group or collapses its split. + let mut candidates = Vec::new(); + if had_focus && let Some(tree) = self.layout(region) { + tree.root().walk(&mut |node| { + if matches!(node.kind(), PaneRef::Tabs { .. }) { + candidates.push(node.id()); + } + }); + if let Some(ix) = tree + .find_panel_node(panel) + .and_then(|node| candidates.iter().position(|candidate| *candidate == node)) + { + candidates = candidates[ix..] + .iter() + .chain(candidates[..ix].iter().rev()) + .copied() + .collect(); + } + } let Some(tree) = self.tree_mut(region) else { return; }; let result = tree.remove_panel(panel); self.commit(result, window, cx); + + // on_removed may deliberately focus another control (or clear focus). + // Automatic handoff is only the fallback when the callback left it alone. + if !had_focus || window.focused(cx) != focused { + return; + } + let next = candidates + .into_iter() + .find_map(|node| self.focus_target_in_group(node, cx)) + .or_else(|| { + [ + DockPlacement::Center, + DockPlacement::Left, + DockPlacement::Right, + DockPlacement::Bottom, + ] + .into_iter() + .filter(|placement| *placement != region) + .filter(|placement| { + *placement == DockPlacement::Center + || self + .docks + .get(placement) + .is_some_and(|pane| pane.dock.is_open()) + }) + .find_map(|placement| { + self.layout(placement) + .and_then(|tree| self.focus_target_in_node(tree.root(), cx)) + }) + }); + if let Some(next) = next { + next.focus_handle(cx).focus(window, cx); + } + } + + fn focus_target_in_group(&self, node: NodeId, cx: &App) -> Option> { + if self.zoomed.is_some_and(|zoom| zoom != Zoomed::Group(node)) { + return None; + } + let group = self.groups.get(&node)?.entity.read(cx); + if group.is_collapsed() { + return None; + } + group.active_panel(cx) + } + + fn focus_target_in_node(&self, node: &PaneNode, cx: &App) -> Option> { + match node.kind() { + PaneRef::Tabs { .. } => self.focus_target_in_group(node.id(), cx), + PaneRef::Split { children, .. } => children + .iter() + .find_map(|child| self.focus_target_in_node(child, cx)), + } } } diff --git a/crates/kit/tests/dock.rs b/crates/kit/tests/dock.rs index 3fa79b7abe..85a34b9325 100644 --- a/crates/kit/tests/dock.rs +++ b/crates/kit/tests/dock.rs @@ -3,6 +3,7 @@ use gpui_kit::component::dock::{ BasePanel, DockArea, DockLayout, DockSkin, Panel, PanelControl, PanelEvent, PanelStyle, panel_handle, }; +use gpui_kit::component::input::{Input, InputState}; use gpui_kit::test::{TestAppContextExt, TestSupportExt, TestWindowExt}; use gpui_kit::{ App, AppContext, Context, Entity, EventEmitter, FocusHandle, Focusable, TestAppContext, Window, @@ -12,11 +13,33 @@ use std::time::Duration; struct Document { name: &'static str, focus: FocusHandle, + input: Entity, + removed_focus: Option, + visible: bool, +} +impl Document { + fn new(name: &'static str, window: &mut Window, cx: &mut Context) -> Self { + Self { + name, + focus: cx.focus_handle(), + input: cx.new(|cx| InputState::new(window, cx)), + removed_focus: None, + visible: true, + } + } } impl BasePanel for Document { fn panel_name(&self) -> &'static str { self.name } + fn visible(&self, _: &App) -> bool { + self.visible + } + fn on_removed(&mut self, window: &mut Window, cx: &mut Context) { + if let Some(focus) = &self.removed_focus { + focus.focus(window, cx); + } + } } impl Panel for Document { fn title(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { @@ -40,6 +63,7 @@ impl Render for Document { .track_focus(&self.focus) .size_full() .child(self.name) + .child(Input::new(&self.input).id("editor")) } } struct Editor { @@ -56,14 +80,8 @@ async fn dock_switches_and_reorders_real_tabs(cx: &mut TestAppContext) { let (handle, _) = common::open_window(cx, Some(size(px(900.), px(600.))), |window, cx| { let (area, skin) = DockSkin::dock_area("editor", None, window, cx); skin.set_panel_style(PanelStyle::TabBar, cx); - let a = cx.new(|cx| Document { - name: "alpha", - focus: cx.focus_handle(), - }); - let b = cx.new(|cx| Document { - name: "beta", - focus: cx.focus_handle(), - }); + let a = cx.new(|cx| Document::new("alpha", window, cx)); + let b = cx.new(|cx| Document::new("beta", window, cx)); let layout = DockLayout::tabs() .panel_view(panel_handle(a), cx) .panel_view(panel_handle(b), cx); @@ -101,18 +119,9 @@ async fn dock_moves_a_tab_between_groups_and_zooms_the_result(cx: &mut TestAppCo cx.update(gpui_kit::init); let (handle, _) = common::open_window(cx, Some(size(px(900.), px(600.))), |window, cx| { let (area, _) = DockSkin::dock_area("editor", None, window, cx); - let a = cx.new(|cx| Document { - name: "alpha", - focus: cx.focus_handle(), - }); - let b = cx.new(|cx| Document { - name: "beta", - focus: cx.focus_handle(), - }); - let c = cx.new(|cx| Document { - name: "gamma", - focus: cx.focus_handle(), - }); + let a = cx.new(|cx| Document::new("alpha", window, cx)); + let b = cx.new(|cx| Document::new("beta", window, cx)); + let c = cx.new(|cx| Document::new("gamma", window, cx)); let layout = DockLayout::h_split() .child( DockLayout::tabs() @@ -172,3 +181,236 @@ async fn dock_moves_a_tab_between_groups_and_zooms_the_result(cx: &mut TestAppCo }) .await; } + +struct TwoTabs { + area: Entity, + alpha: Entity, + beta: Entity, + elsewhere: FocusHandle, +} +impl Render for TwoTabs { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + .size_full() + .child(div().track_focus(&self.elsewhere).h(px(20.))) + .child(self.area.clone()) + } +} + +fn two_tabs(cx: &mut TestAppContext) -> (gpui_kit::AnyWindowHandle, Entity) { + cx.update(gpui_kit::init); + let (handle, view) = common::open_window(cx, Some(size(px(900.), px(600.))), |window, cx| { + let (area, skin) = DockSkin::dock_area("editor", None, window, cx); + skin.set_panel_style(PanelStyle::TabBar, cx); + skin.set_close_button_visible(true, cx); + let alpha = cx.new(|cx| Document::new("alpha", window, cx)); + let beta = cx.new(|cx| Document::new("beta", window, cx)); + let layout = DockLayout::tabs() + .panel_view(panel_handle(alpha.clone()), cx) + .panel_view(panel_handle(beta.clone()), cx); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + cx.new(|cx| TwoTabs { + area, + alpha, + beta, + elsewhere: cx.focus_handle(), + }) + }); + (handle.into(), view) +} + +#[gpui_kit::test] +fn dock_hands_the_focus_to_the_tab_that_replaces_a_closed_one(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + cx.update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, beta) = (view.area.clone(), view.alpha.clone(), view.beta.clone()); + window.render_frame(cx); + let alpha_focus = alpha.read(cx).focus.clone(); + alpha_focus.focus(window, cx); + window.render_frame(cx); + assert!(alpha.read(cx).focus.is_focused(window)); + + area.update(cx, |area, cx| area.remove_panel(alpha, window, cx)); + window.render_frame(cx); + assert!(window.find("beta").visible()); + assert!( + beta.read(cx).focus.is_focused(window), + "closing the focused tab must leave the keyboard on the one shown in its place" + ); + }) + .unwrap(); +} + +#[gpui_kit::test] +fn dock_leaves_the_focus_alone_when_a_tab_without_it_closes(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + cx.update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, elsewhere) = ( + view.area.clone(), + view.alpha.clone(), + view.elsewhere.clone(), + ); + window.render_frame(cx); + elsewhere.focus(window, cx); + window.render_frame(cx); + + area.update(cx, |area, cx| area.remove_panel(alpha, window, cx)); + window.render_frame(cx); + assert!(elsewhere.is_focused(window)); + }) + .unwrap(); +} + +fn close_visible_tab(window: &mut Window, name: &str, ix: usize, cx: &mut App) { + let panel = window.find(name.to_owned()); + let group_ix = panel + .path() + .iter() + .position(|id| *id == "tab-panel".into()) + .unwrap(); + let group = panel.path()[group_ix - 1].clone(); + window.within(group).click(("close-tab", ix), cx); +} + +#[gpui_kit::test] +async fn dock_close_button_transfers_focus_from_a_child_input(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let beta = cx + .update_window(handle, |_, window, cx| { + let beta = view.read(cx).beta.clone(); + window.render_frame(cx); + window.within("alpha").click("editor", cx); + assert_eq!(window.within("alpha").find("editor").focused(), Some(true)); + close_visible_tab(window, "alpha", 0, cx); + beta + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + window.try_find("alpha").is_none() + && window.try_find("beta").is_some_and(|panel| panel.visible()) + && beta.read(cx).focus.is_focused(window) + }) + .await; +} + +#[gpui_kit::test] +async fn dock_close_button_preserves_focus_chosen_by_on_removed(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let elsewhere = cx + .update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (alpha, elsewhere) = (view.alpha.clone(), view.elsewhere.clone()); + alpha.update(cx, |panel, _| panel.removed_focus = Some(elsewhere.clone())); + window.render_frame(cx); + window.within("alpha").click("editor", cx); + close_visible_tab(window, "alpha", 0, cx); + elsewhere + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, _| { + window.try_find("alpha").is_none() && elsewhere.is_focused(window) + }) + .await; +} + +#[gpui_kit::test] +async fn dock_close_button_focuses_the_remaining_split_when_a_group_disappears( + cx: &mut TestAppContext, +) { + for close_alpha in [true, false] { + let (handle, view) = two_tabs(cx); + let next = cx + .update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, beta) = + (view.area.clone(), view.alpha.clone(), view.beta.clone()); + let layout = DockLayout::h_split() + .child( + DockLayout::tabs().panel_view(panel_handle(alpha.clone()), cx), + None, + ) + .child( + DockLayout::tabs().panel_view(panel_handle(beta.clone()), cx), + None, + ); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + window.render_frame(cx); + let (name, next) = if close_alpha { + ("alpha", beta) + } else { + ("beta", alpha) + }; + window.within(name).click("editor", cx); + close_visible_tab(window, name, 0, cx); + next + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + let closed = if close_alpha { "alpha" } else { "beta" }; + window.try_find(closed).is_none() + && next.read(cx).focus.is_focused(window) + && window.find(next.read(cx).name).visible() + }) + .await; + } +} + +#[gpui_kit::test] +async fn dock_close_button_preserves_focus_when_closing_an_inactive_tab(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let input = cx + .update_window(handle, |_, window, cx| { + let input = view.read(cx).alpha.read(cx).input.clone(); + window.render_frame(cx); + window.within("alpha").click("editor", cx); + close_visible_tab(window, "alpha", 1, cx); + input + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + let beta = view.read(cx).beta.clone(); + let area = view.read(cx).area.clone(); + area.read(cx).panel(beta.entity_id().into()).is_none() + && input.focus_handle(cx).is_focused(window) + }) + .await; +} + +#[gpui_kit::test] +async fn dock_close_button_skips_a_hidden_neighbor_when_handing_off_focus(cx: &mut TestAppContext) { + let (handle, view) = two_tabs(cx); + let beta = cx + .update_window(handle, |_, window, cx| { + let view = view.read(cx); + let (area, alpha, beta) = (view.area.clone(), view.alpha.clone(), view.beta.clone()); + let hidden = cx.new(|cx| Document { + visible: false, + ..Document::new("hidden", window, cx) + }); + let layout = DockLayout::h_split() + .child(DockLayout::tabs().panel_view(panel_handle(alpha), cx), None) + .child( + DockLayout::tabs().panel_view(panel_handle(hidden), cx), + None, + ) + .child( + DockLayout::tabs().panel_view(panel_handle(beta.clone()), cx), + None, + ); + area.update(cx, |area, cx| area.set_center(layout, window, cx)); + window.render_frame(cx); + assert!(window.try_find("hidden").is_none()); + window.within("alpha").click("editor", cx); + close_visible_tab(window, "alpha", 0, cx); + beta + }) + .unwrap(); + cx.wait_for(handle, Duration::from_secs(1), |window, cx| { + window.try_find("alpha").is_none() + && window.find("beta").visible() + && beta.read(cx).focus.is_focused(window) + }) + .await; +}