diff --git a/src/app/actions.rs b/src/app/actions.rs index 51f7187791..61a6359d57 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -1800,7 +1800,9 @@ impl AppState { }; if let Some(focused) = panes.iter().find(|p| p.is_focused) { - if let Some(target) = find_in_direction(focused, direction, &panes) { + if let Some(target) = + find_in_direction(focused, direction, &panes, tab.layout.focus_history()) + { self.focus_pane_in_workspace(ws_idx, target); } } @@ -1823,7 +1825,12 @@ impl AppState { let Some(focused) = panes.iter().find(|p| p.is_focused) else { return false; }; - let Some(target) = find_in_direction(focused, direction, &panes) else { + let Some(target) = find_in_direction( + focused, + direction, + &panes, + &crate::layout::FocusHistory::default(), + ) else { return false; }; let source = focused.id; diff --git a/src/app/api/panes.rs b/src/app/api/panes.rs index a5f1cbcbf5..27c90a8da7 100644 --- a/src/app/api/panes.rs +++ b/src/app/api/panes.rs @@ -17,7 +17,7 @@ use crate::app::actions::{PaneZoomCommand, PaneZoomNoopReason}; use crate::app::App; #[cfg(test)] use crate::app::Mode; -use crate::layout::{find_in_direction, NavDirection, PaneId}; +use crate::layout::{find_in_direction, FocusHistory, NavDirection, PaneId}; use super::super::api_helpers::{ detect_state_from_api, encode_api_keys, normalize_metadata_source, normalize_metadata_tokens, @@ -470,7 +470,7 @@ impl App { .unwrap_or_default(), ); }; - let target = self.directional_pane_target(ws_idx, tab_idx, source_pane_id, direction); + let target = self.directional_swap_target(ws_idx, tab_idx, source_pane_id, direction); match target { Some(target_pane_id) => { (ws_idx, tab_idx, source_pane_id, Some(target_pane_id), None) @@ -871,10 +871,6 @@ impl App { self.recover_failed_pane_move(recovery_context, moved); return encode_error(id, "pane_move_failed", "target tab disappeared"); }; - let previous_target_focus = self.state.workspaces[target_ws_idx].tabs - [target_tab_idx] - .layout - .focused(); let direction = split_direction_to_layout(split); let moved_pane_id = match self.state.workspaces[target_ws_idx] .insert_moved_pane_into_tab( @@ -883,6 +879,7 @@ impl App { moved, direction, ratio, + focus, ) { Ok(pane_id) => pane_id, Err(moved) => { @@ -894,11 +891,6 @@ impl App { ); } }; - if !focus { - self.state.workspaces[target_ws_idx].tabs[target_tab_idx] - .layout - .focus_pane(previous_target_focus); - } (target_ws_idx, target_tab_idx, moved_pane_id) } ResolvedPaneMoveDestination::NewTab { @@ -1676,7 +1668,28 @@ impl App { let tab = self.state.workspaces.get(ws_idx)?.tabs.get(tab_idx)?; let panes = tab.layout.panes(self.state.view.terminal_area); let source = panes.iter().find(|pane| pane.id == source_pane_id)?; - find_in_direction(source, direction.into(), &panes) + find_in_direction(source, direction.into(), &panes, tab.layout.focus_history()) + } + + /// Resolve a directional swap target using geometry only. + /// + /// Unlike [`Self::directional_pane_target`], swaps deliberately ignore focus + /// history: a directional swap exchanges the source with its geometric + /// neighbor, matching the TUI swap path (`directional_pane_swap_from_view`). + /// Biasing swaps toward the most-recently-focused neighbor would make the + /// API diverge from the keyboard swap and make swap targets depend on + /// unrelated focus movements. + fn directional_swap_target( + &self, + ws_idx: usize, + tab_idx: usize, + source_pane_id: PaneId, + direction: PaneDirection, + ) -> Option { + let tab = self.state.workspaces.get(ws_idx)?.tabs.get(tab_idx)?; + let panes = tab.layout.panes(self.state.view.terminal_area); + let source = panes.iter().find(|pane| pane.id == source_pane_id)?; + find_in_direction(source, direction.into(), &panes, &FocusHistory::default()) } pub(super) fn pane_layout_snapshot( @@ -2452,6 +2465,46 @@ mod tests { assert!(app.event_hub.events_after(0).is_empty()); } + #[test] + fn api_pane_swap_direction_ignores_focus_history() { + // Layout: H(L, V(RT, RB)). RB is focused most recently among the right + // column, then focus returns to L. A directional swap must exchange L + // with its geometric neighbor (RT, the top pane), not the most-recently + // focused one (RB); swaps are geometry-only, unlike focus navigation. + let mut app = app_with_linked_worktree(); + let left = app.state.workspaces[0].tabs[0].root_pane; + let right_top = app.state.workspaces[0].test_split(ratatui::layout::Direction::Horizontal); + let right_bottom = app.state.workspaces[0].test_split(ratatui::layout::Direction::Vertical); + // Make RB the most-recently-focused right pane, then return focus to L. + app.state.workspaces[0].tabs[0] + .layout + .focus_pane(right_bottom); + app.state.workspaces[0].tabs[0].layout.focus_pane(left); + crate::ui::compute_view(&mut app.state, ratatui::layout::Rect::new(0, 0, 100, 20)); + let left_public = app.public_pane_id(0, left).unwrap(); + let right_top_public = app.public_pane_id(0, right_top).unwrap(); + let right_bottom_public = app.public_pane_id(0, right_bottom).unwrap(); + + let response = app.handle_pane_swap( + "req".into(), + PaneSwapParams { + pane_id: Some(left_public.clone()), + direction: Some(PaneDirection::Right), + ..PaneSwapParams::default() + }, + ); + + let success: SuccessResponse = serde_json::from_str(&response).unwrap(); + let ResponseResult::PaneSwap { swap } = success.result else { + panic!("expected pane swap response"); + }; + assert!(swap.changed); + assert_eq!(swap.reason, None); + assert_eq!(swap.source_pane_id, left_public); + assert_eq!(swap.target_pane_id, Some(right_top_public)); + assert_ne!(swap.target_pane_id, Some(right_bottom_public)); + } + #[test] fn api_pane_swap_explicit_missing_target_returns_not_found_noop() { let mut app = app_with_linked_worktree(); @@ -3135,6 +3188,60 @@ mod tests { ); } + #[test] + fn api_pane_move_no_focus_pane_is_not_a_directional_navigation_target() { + // Target tab: H(A, V(RT, RB)), focus A. Move `source` next to RT with a + // downward split and focus: false, producing H(A, V(V(RT, source), RB)). + // Directional navigation from A must not select the never-focused moved + // pane; it has no focus history and loses to the geometric winner (RB). + let mut app = app_with_linked_worktree(); + let source = app.state.workspaces[0].tabs[0].root_pane; + let target_tab = app.state.workspaces[0].test_add_tab(Some("target")); + app.state.workspaces[0].active_tab = target_tab; + let a = app.state.workspaces[0].tabs[target_tab].root_pane; + let rt = app.state.workspaces[0].test_split(ratatui::layout::Direction::Horizontal); + let rb = app.state.workspaces[0].test_split(ratatui::layout::Direction::Vertical); + app.state.workspaces[0].tabs[target_tab] + .layout + .focus_pane(a); + seed_terminal_states(&mut app); + let source_public = app.public_pane_id(0, source).unwrap(); + let target_tab_public = app.public_tab_id(0, target_tab).unwrap(); + let rt_public = app.public_pane_id(0, rt).unwrap(); + + let response = app.handle_pane_move( + "req".into(), + PaneMoveParams { + pane_id: source_public, + destination: PaneMoveDestination::Tab { + tab_id: target_tab_public, + target_pane_id: Some(rt_public), + split: SplitDirection::Down, + ratio: None, + }, + focus: false, + }, + ); + + let success: SuccessResponse = serde_json::from_str(&response).unwrap(); + let ResponseResult::PaneMove { move_result } = success.result else { + panic!("expected pane move response"); + }; + assert!(move_result.changed); + // Focus stayed on A; the moved pane was never focused. The source's + // emptied tab is removed by the move, so resolve the target tab by pane. + let target_tab_after = app.state.workspaces[0].find_tab_index_for_pane(a).unwrap(); + let layout = &app.state.workspaces[0].tabs[target_tab_after].layout; + assert_eq!(layout.focused(), a); + + let panes = layout.panes(ratatui::layout::Rect::new(0, 0, 100, 40)); + let focused = panes.iter().find(|pane| pane.id == a).unwrap(); + let nav_target = + find_in_direction(focused, NavDirection::Right, &panes, layout.focus_history()); + assert_ne!(nav_target, Some(source)); + assert_eq!(nav_target, Some(rb)); + } + #[test] fn api_pane_move_recovery_restores_removed_source_workspace() { let mut app = app_with_linked_worktree(); diff --git a/src/app/input/navigate.rs b/src/app/input/navigate.rs index 68941e663f..12d25ec3c9 100644 --- a/src/app/input/navigate.rs +++ b/src/app/input/navigate.rs @@ -689,8 +689,19 @@ impl App { .pane_infos .iter() .find(|pane| pane.is_focused)?; - let target = - crate::layout::find_in_direction(focused, direction, &self.state.view.pane_infos)?; + let history = self + .state + .workspaces + .get(ws_idx)? + .active_tab()? + .layout + .focus_history(); + let target = crate::layout::find_in_direction( + focused, + direction, + &self.state.view.pane_infos, + history, + )?; Some((ws_idx, target)) } @@ -705,8 +716,13 @@ impl App { .pane_infos .iter() .find(|pane| pane.is_focused)?; - let target = - crate::layout::find_in_direction(focused, direction, &self.state.view.pane_infos)?; + let empty = crate::layout::FocusHistory::default(); + let target = crate::layout::find_in_direction( + focused, + direction, + &self.state.view.pane_infos, + &empty, + )?; Some((ws_idx, focused.id, target)) } diff --git a/src/layout.rs b/src/layout.rs index 8a16da9ce2..81c181599b 100644 --- a/src/layout.rs +++ b/src/layout.rs @@ -1,6 +1,7 @@ //! BSP tree layout for tiling panes within a workspace. use std::cmp::Reverse; +use std::collections::HashMap; use ratatui::{ layout::{Direction, Rect}, @@ -80,10 +81,32 @@ pub enum Node { }, } +/// Per-pane "last focused" timestamps for MRU directional navigation. +#[derive(Debug, Default, Clone)] +pub struct FocusHistory { + stamps: HashMap, + clock: u64, +} + +impl FocusHistory { + fn record(&mut self, id: PaneId) { + self.clock += 1; + self.stamps.insert(id, self.clock); + } + fn forget(&mut self, id: PaneId) { + self.stamps.remove(&id); + } + /// 0 = never focused; larger = more recent. + fn get(&self, id: PaneId) -> u64 { + self.stamps.get(&id).copied().unwrap_or(0) + } +} + /// BSP tiling layout. Tracks a tree of splits and a focused pane. pub struct TileLayout { root: Node, focus: PaneId, + focus_history: FocusHistory, } impl TileLayout { @@ -91,19 +114,24 @@ impl TileLayout { /// Returns (layout, root_pane_id) so the caller can create the pane. pub fn new() -> (Self, PaneId) { let root_id = PaneId::alloc(); - ( - Self { - root: Node::Pane(root_id), - focus: root_id, - }, - root_id, - ) + let mut layout = Self { + root: Node::Pane(root_id), + focus: root_id, + focus_history: FocusHistory::default(), + }; + layout.focus_history.record(root_id); + (layout, root_id) } pub fn focused(&self) -> PaneId { self.focus } + /// Access the per-pane focus history for MRU directional navigation. + pub fn focus_history(&self) -> &FocusHistory { + &self.focus_history + } + pub fn pane_count(&self) -> usize { count_panes(&self.root) } @@ -123,28 +151,71 @@ impl TileLayout { } /// Split the focused pane. Returns the new pane's id. + #[cfg(test)] pub fn split_focused(&mut self, direction: Direction) -> PaneId { self.split_focused_with_ratio(direction, 0.5) } /// Split the focused pane with a custom first-child ratio. + #[cfg(test)] pub fn split_focused_with_ratio(&mut self, direction: Direction, ratio: f32) -> PaneId { + self.split_target(self.focus, direction, ratio, true) + } + + /// Split a specific pane, inserting a newly allocated sibling. + /// + /// When `focus` is true the new pane becomes focused and is recorded in the + /// focus history. When false the current focus and history are left + /// untouched, so a pane created without focus (for example `pane.split` with + /// `focus: false`) does not become an MRU directional navigation target. + /// Returns the new pane id, or None when `target` is not in the layout. + pub fn split_pane( + &mut self, + target: PaneId, + direction: Direction, + ratio: f32, + focus: bool, + ) -> Option { + if !self.pane_ids().contains(&target) { + return None; + } + Some(self.split_target(target, direction, ratio, focus)) + } + + /// Structural split of `target` (assumed present). Focuses and records the + /// new pane only when `focus` is true. + fn split_target( + &mut self, + target: PaneId, + direction: Direction, + ratio: f32, + focus: bool, + ) -> PaneId { let new_id = PaneId::alloc(); let placeholder = PaneId::from_raw(0); let old = std::mem::replace(&mut self.root, Node::Pane(placeholder)); - self.root = split_at(old, self.focus, direction, new_id, valid_split_ratio(ratio)); - self.focus = new_id; + self.root = split_at(old, target, direction, new_id, valid_split_ratio(ratio)); + if focus { + self.focus = new_id; + self.focus_history.record(new_id); + } new_id } /// Insert an existing pane id next to a target pane without allocating a new /// pane or spawning a terminal runtime. + /// + /// When `focus` is true the moved pane becomes focused and is recorded in the + /// focus history. When false the current focus and history are left untouched, + /// so a pane relocated without focus does not become an MRU directional + /// navigation target. pub fn insert_pane_near( &mut self, target: PaneId, moved: PaneId, direction: Direction, ratio: f32, + focus: bool, ) -> bool { if target == moved { return false; @@ -157,18 +228,27 @@ impl TileLayout { let placeholder = PaneId::from_raw(0); let old = std::mem::replace(&mut self.root, Node::Pane(placeholder)); self.root = split_at(old, target, direction, moved, valid_split_ratio(ratio)); - self.focus = moved; + if focus { + self.focus = moved; + self.focus_history.record(moved); + } true } - /// Close the focused pane. Returns false if it's the last pane. - pub fn close_focused(&mut self) -> bool { + /// Close a specific pane by id. When the removed pane is the focused one, + /// focus moves to a neighbor and that neighbor is recorded as + /// most-recently-focused; otherwise the current focus and all other history + /// are left untouched. The removed pane's history stamp is always pruned. + /// Returns false if the pane is missing or it is the last remaining pane. + pub fn close_pane(&mut self, target: PaneId) -> bool { if self.pane_count() <= 1 { return false; } - let target = self.focus; let ids = self.pane_ids(); - let pos = ids.iter().position(|id| *id == target).unwrap(); + let Some(pos) = ids.iter().position(|id| *id == target) else { + return false; + }; + let removing_focused = self.focus == target; let new_focus = if pos + 1 < ids.len() { ids[pos + 1] } else { @@ -178,7 +258,11 @@ impl TileLayout { let old = std::mem::replace(&mut self.root, Node::Pane(placeholder)); if let Some(new_root) = remove_pane(old, target) { self.root = new_root; - self.focus = new_focus; + self.focus_history.forget(target); + if removing_focused { + self.focus = new_focus; + self.focus_history.record(new_focus); + } true } else { false @@ -188,6 +272,7 @@ impl TileLayout { pub fn focus_pane(&mut self, id: PaneId) { if self.pane_ids().contains(&id) { self.focus = id; + self.focus_history.record(id); } } @@ -267,10 +352,15 @@ impl TileLayout { &self.root } - /// Reconstruct a layout from a saved tree. /// Reconstruct a layout from a saved tree. pub fn from_saved(root: Node, focus: PaneId) -> Self { - Self { root, focus } + let mut layout = Self { + root, + focus, + focus_history: FocusHistory::default(), + }; + layout.focus_history.record(focus); + layout } } @@ -281,10 +371,12 @@ pub fn find_in_direction( focused: &PaneInfo, direction: NavDirection, panes: &[PaneInfo], + history: &FocusHistory, ) -> Option { let fr = focused.rect; - panes + // Candidates strictly in `direction` with cross-axis overlap. + let candidates: Vec<(usize, &PaneInfo)> = panes .iter() .enumerate() .filter(|(_, p)| p.id != focused.id) @@ -305,14 +397,35 @@ pub fn find_in_direction( } } }) + .collect(); + + // Nearest boundary: the adjacent sibling subtree tiles the shared edge at the + // smallest edge distance; deeper panes sit farther in. This is the structural + // "near-edge" filter expressed geometrically (gapless BSP => equal for all + // near-edge panes). + let min_edge = candidates + .iter() + .map(|(_, p)| pane_edge_distance(p.rect, fr, direction)) + .min()?; + let nearest: Vec<(usize, &PaneInfo)> = candidates + .into_iter() + .filter(|(_, p)| pane_edge_distance(p.rect, fr, direction) == min_edge) + .collect(); + + // History path: most-recently-focused adjacent pane (clock monotonic => unique max). + if let Some((_, p)) = nearest + .iter() + .filter(|(_, p)| history.get(p.id) > 0) + .max_by_key(|(_, p)| history.get(p.id)) + { + return Some(p.id); + } + + // Geometric fallback (unchanged tiebreak), restricted to the nearest column/row. + nearest + .into_iter() .min_by_key(|(index, p)| { let r = p.rect; - let edge_distance = match direction { - NavDirection::Left => fr.x.saturating_sub(r.x + r.width), - NavDirection::Right => r.x.saturating_sub(fr.x + fr.width), - NavDirection::Up => fr.y.saturating_sub(r.y + r.height), - NavDirection::Down => r.y.saturating_sub(fr.y + fr.height), - }; let overlap = match direction { NavDirection::Left | NavDirection::Right => { range_overlap_amount(r.y, r.height, fr.y, fr.height) @@ -329,11 +442,20 @@ pub fn find_in_direction( range_center_distance(r.x, r.width, fr.x, fr.width) } }; - (edge_distance, Reverse(overlap), center_distance, *index) + (Reverse(overlap), center_distance, *index) }) .map(|(_, p)| p.id) } +fn pane_edge_distance(candidate: Rect, focused: Rect, direction: NavDirection) -> u16 { + match direction { + NavDirection::Left => focused.x.saturating_sub(candidate.x + candidate.width), + NavDirection::Right => candidate.x.saturating_sub(focused.x + focused.width), + NavDirection::Up => focused.y.saturating_sub(candidate.y + candidate.height), + NavDirection::Down => candidate.y.saturating_sub(focused.y + focused.height), + } +} + fn ranges_overlap(a_start: u16, a_len: u16, b_start: u16, b_len: u16) -> bool { a_start < b_start + b_len && a_start + a_len > b_start } @@ -746,7 +868,7 @@ mod tests { let (mut layout, root) = TileLayout::new(); let moved = pane(99); - assert!(layout.insert_pane_near(root, moved, Direction::Horizontal, 0.25)); + assert!(layout.insert_pane_near(root, moved, Direction::Horizontal, 0.25, true)); assert_eq!(layout.pane_count(), 2); assert_eq!(layout.pane_ids(), vec![root, moved]); @@ -757,6 +879,71 @@ mod tests { assert_eq!(pane_rect(&layout, moved), Rect::new(25, 0, 75, 40)); } + #[test] + fn insert_pane_near_without_focus_leaves_focus_and_history_untouched() { + // Tab: H(A, V(RT, RB)), focus A. from_saved stamps only A. + let mut layout = TileLayout::from_saved( + Node::Split { + direction: Direction::Horizontal, + ratio: 0.5, + first: Box::new(Node::Pane(pane(1))), // A + second: Box::new(Node::Split { + direction: Direction::Vertical, + ratio: 0.5, + first: Box::new(Node::Pane(pane(2))), // RT + second: Box::new(Node::Pane(pane(3))), // RB + }), + }, + pane(1), + ); + let moved = pane(99); + + // Relocate `moved` next to RT without focusing it (a focus: false move). + assert!(layout.insert_pane_near(pane(2), moved, Direction::Vertical, 0.5, false)); + // Focus is unchanged; the moved pane was never focused. + assert_eq!(layout.focused(), pane(1)); + + // The never-focused moved pane must not win directional selection: it has + // no focus history and loses to the geometric winner (RB, the largest + // overlap). With a phantom MRU stamp it would incorrectly capture focus. + let target = find_from(&layout, pane(1), NavDirection::Right); + assert_ne!(target, Some(moved)); + assert_eq!(target, Some(pane(3))); + } + + #[test] + fn split_pane_without_focus_is_not_a_navigation_target() { + // Tab: H(A, V(RT, RB)), focus A. from_saved stamps only A. + let mut layout = TileLayout::from_saved( + Node::Split { + direction: Direction::Horizontal, + ratio: 0.5, + first: Box::new(Node::Pane(pane(1))), // A + second: Box::new(Node::Split { + direction: Direction::Vertical, + ratio: 0.5, + first: Box::new(Node::Pane(pane(2))), // RT + second: Box::new(Node::Pane(pane(3))), // RB + }), + }, + pane(1), + ); + + // Split RT downward without focusing the new pane (pane.split focus: false). + let new_id = layout + .split_pane(pane(2), Direction::Vertical, 0.5, false) + .expect("target pane exists"); + // Focus and the target pane's history are untouched. + assert_eq!(layout.focused(), pane(1)); + + // The unfocused new pane must not win directional selection: it has no + // focus history and loses to the geometric winner (RB). A phantom MRU + // stamp would incorrectly steer navigation onto it. + let target = find_from(&layout, pane(1), NavDirection::Right); + assert_ne!(target, Some(new_id)); + assert_eq!(target, Some(pane(3))); + } + #[test] fn split_focused_with_ratio_sets_new_split_ratio() { let (mut layout, root) = TileLayout::new(); @@ -950,8 +1137,128 @@ mod tests { let panes = vec![focused.clone(), small_overlap_first, larger_overlap_second]; assert_eq!( - find_in_direction(&focused, NavDirection::Left, &panes), + find_in_direction( + &focused, + NavDirection::Left, + &panes, + &FocusHistory::default() + ), Some(pane(3)) ); } + + fn find_from(layout: &TileLayout, from: PaneId, direction: NavDirection) -> Option { + let panes = layout.panes(Rect::new(0, 0, 100, 40)); + let focused = panes.iter().find(|p| p.id == from).expect("pane exists"); + find_in_direction(focused, direction, &panes, layout.focus_history()) + } + + // root = H(L, R=V(RT, RB)) + fn side_stacked_layout() -> TileLayout { + TileLayout::from_saved( + Node::Split { + direction: Direction::Horizontal, + ratio: 0.5, + first: Box::new(Node::Pane(pane(1))), // L + second: Box::new(Node::Split { + direction: Direction::Vertical, + ratio: 0.5, + first: Box::new(Node::Pane(pane(2))), // RT + second: Box::new(Node::Pane(pane(3))), // RB + }), + }, + pane(1), + ) + } + + #[test] + fn navigate_returns_to_last_focused_pane_in_subtree() { + let mut layout = side_stacked_layout(); + // Visit RB, then move back to L; moving Right must return to RB (MRU), + // not the geometric winner RT. + layout.focus_pane(pane(3)); // RB + layout.focus_pane(pane(1)); // L + assert_eq!( + find_from(&layout, pane(1), NavDirection::Right), + Some(pane(3)) + ); + // Reciprocity: from RB moving Left returns L. + assert_eq!( + find_from(&layout, pane(3), NavDirection::Left), + Some(pane(1)) + ); + } + + #[test] + fn navigate_adjacency_beats_recency() { + // root = H(L, R=H(RL, RR)); RR is more recent but sits in the far column. + let mut layout = TileLayout::from_saved( + Node::Split { + direction: Direction::Horizontal, + ratio: 0.5, + first: Box::new(Node::Pane(pane(1))), // L + second: Box::new(Node::Split { + direction: Direction::Horizontal, + ratio: 0.5, + first: Box::new(Node::Pane(pane(2))), // RL + second: Box::new(Node::Pane(pane(3))), // RR + }), + }, + pane(1), + ); + layout.focus_pane(pane(3)); // RR (recent, far column) + layout.focus_pane(pane(1)); // L + // Only the adjacent column (RL) is eligible; RR is excluded despite recency. + assert_eq!( + find_from(&layout, pane(1), NavDirection::Right), + Some(pane(2)) + ); + } + + #[test] + fn navigate_without_history_uses_geometry() { + // Only L (root focus) is stamped; RT/RB have no history, so geometry decides. + let layout = side_stacked_layout(); + assert_eq!( + find_from(&layout, pane(1), NavDirection::Right), + Some(pane(2)) + ); + } + + #[test] + fn closing_non_focused_pane_does_not_corrupt_navigation_history() { + // root = H(L, R=V(RT, RB=H(RBL, RBR))) + let mut layout = TileLayout::from_saved( + Node::Split { + direction: Direction::Horizontal, + ratio: 0.5, + first: Box::new(Node::Pane(pane(1))), // L + second: Box::new(Node::Split { + direction: Direction::Vertical, + ratio: 0.5, + first: Box::new(Node::Pane(pane(2))), // RT + second: Box::new(Node::Split { + direction: Direction::Horizontal, + ratio: 0.5, + first: Box::new(Node::Pane(pane(3))), // RBL + second: Box::new(Node::Pane(pane(4))), // RBR + }), + }), + }, + pane(1), + ); + layout.focus_pane(pane(2)); // RT becomes MRU on the right + layout.focus_pane(pane(1)); // focus returns to L + + // Close a background, non-focused pane, as handle_pane_died does. + assert!(layout.close_pane(pane(4))); + assert_eq!(layout.focused(), pane(1)); // focus is untouched + + // RT is still the most-recently-focused right pane; closing RBR's sibling + // must not have stamped a bystander (RBL) as MRU. + assert_eq!( + find_from(&layout, pane(1), NavDirection::Right), + Some(pane(2)) + ); + } } diff --git a/src/workspace.rs b/src/workspace.rs index 0d0e35bd60..54a8fc4df7 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -683,10 +683,16 @@ impl Workspace { .map(|tab| tab.number) .expect("workspace must always have at least one tab"); let launch_env = self.launch_env_for_new_pane(tab_number, pane_number, extra_env); + let target = self + .active_tab() + .map(|tab| tab.layout.focused()) + .expect("workspace must always have at least one tab"); let new_pane = self .active_tab_mut() .expect("workspace must always have at least one tab") .split_focused( + target, + true, direction, rows, cols, @@ -720,10 +726,16 @@ impl Workspace { .map(|tab| tab.number) .expect("workspace must always have at least one tab"); let launch_env = self.launch_env_for_new_pane(tab_number, pane_number, extra_env); + let target = self + .active_tab() + .map(|tab| tab.layout.focused()) + .expect("workspace must always have at least one tab"); let new_pane = self .active_tab_mut() .expect("workspace must always have at least one tab") .split_focused_command( + target, + true, direction, rows, cols, @@ -891,11 +903,11 @@ impl Workspace { let tab_number = self.tabs[tab_idx].number; let launch_env = self.launch_env_for_new_pane(tab_number, pane_number, extra_env); let tab = &mut self.tabs[tab_idx]; - let previous_focus = tab.layout.focused(); - tab.layout.focus_pane(pane_id); let new_pane = match if let Some(argv) = argv { match ratio { Some(ratio) => tab.split_focused_argv_command_with_ratio( + pane_id, + focus_new_pane, direction, ratio, rows, @@ -908,6 +920,8 @@ impl Workspace { host_terminal_appearance, ), None => tab.split_focused_argv_command( + pane_id, + focus_new_pane, direction, rows, cols, @@ -922,6 +936,8 @@ impl Workspace { } else { match ratio { Some(ratio) => tab.split_focused_with_ratio( + pane_id, + focus_new_pane, direction, ratio, rows, @@ -934,6 +950,8 @@ impl Workspace { &launch_env, ), None => tab.split_focused( + pane_id, + focus_new_pane, direction, rows, cols, @@ -947,14 +965,8 @@ impl Workspace { } } { Ok(new_pane) => new_pane, - Err(err) => { - tab.layout.focus_pane(previous_focus); - return Some(Err(err)); - } + Err(err) => return Some(Err(err)), }; - if !focus_new_pane { - tab.layout.focus_pane(previous_focus); - } self.register_new_pane_with_number(new_pane.pane_id, pane_number); Some(Ok((tab_idx, new_pane))) } @@ -1034,12 +1046,13 @@ impl Workspace { moved: MovedPane, direction: Direction, ratio: f32, + focus: bool, ) -> Result { let pane_id = moved.pane_id; let Some(tab) = self.tabs.get_mut(tab_idx) else { return Err(moved); }; - tab.insert_existing_pane(target_pane_id, moved, direction, ratio)?; + tab.insert_existing_pane(target_pane_id, moved, direction, ratio, focus)?; if !self.public_pane_numbers.contains_key(&pane_id) { self.register_new_pane_with_number(pane_id, self.next_public_pane_number); } @@ -1332,7 +1345,11 @@ impl Workspace { pub(crate) fn test_split(&mut self, direction: Direction) -> PaneId { let tab = self.active_tab_mut().expect("workspace must have tab"); - let new_id = tab.layout.split_focused(direction); + let target = tab.layout.focused(); + let new_id = tab + .layout + .split_pane(target, direction, 0.5, true) + .expect("focused pane is always present"); tab.panes .insert(new_id, PaneState::new(TerminalId::alloc())); self.register_new_pane(new_id); @@ -1665,7 +1682,14 @@ mod tests { let missing_target = PaneId::alloc(); let recovered = target - .insert_moved_pane_into_tab(0, missing_target, taken.moved, Direction::Horizontal, 0.5) + .insert_moved_pane_into_tab( + 0, + missing_target, + taken.moved, + Direction::Horizontal, + 0.5, + true, + ) .expect_err("invalid target should return the moved pane"); assert_eq!(recovered.pane_id, source_pane); diff --git a/src/workspace/tab.rs b/src/workspace/tab.rs index 5cc35b6851..e1847aab24 100644 --- a/src/workspace/tab.rs +++ b/src/workspace/tab.rs @@ -205,8 +205,12 @@ impl Tab { self.custom_name = Some(name); } + // Terminal-launch split builder; params mirror the runtime spawn API. + #[allow(clippy::too_many_arguments)] pub fn split_focused( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, rows: u16, cols: u16, @@ -218,6 +222,8 @@ impl Tab { launch_env: &PaneLaunchEnv, ) -> std::io::Result { self.split_focused_with_runtime( + target, + focus_new_pane, direction, None, rows, @@ -232,8 +238,12 @@ impl Tab { ) } + // Terminal-launch split builder; params mirror the runtime spawn API. + #[allow(clippy::too_many_arguments)] pub fn split_focused_with_ratio( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, ratio: f32, rows: u16, @@ -246,6 +256,8 @@ impl Tab { launch_env: &PaneLaunchEnv, ) -> std::io::Result { self.split_focused_with_runtime( + target, + focus_new_pane, direction, Some(ratio), rows, @@ -260,8 +272,12 @@ impl Tab { ) } + // Terminal-launch split builder; params mirror the runtime spawn API. + #[allow(clippy::too_many_arguments)] pub fn split_focused_command( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, rows: u16, cols: u16, @@ -273,6 +289,8 @@ impl Tab { host_terminal_appearance: Option, ) -> std::io::Result { self.split_focused_with_runtime( + target, + focus_new_pane, direction, None, rows, @@ -290,8 +308,12 @@ impl Tab { ) } + // Terminal-launch split builder; params mirror the runtime spawn API. + #[allow(clippy::too_many_arguments)] pub fn split_focused_argv_command( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, rows: u16, cols: u16, @@ -303,6 +325,8 @@ impl Tab { host_terminal_appearance: Option, ) -> std::io::Result { self.split_focused_with_runtime( + target, + focus_new_pane, direction, None, rows, @@ -317,8 +341,12 @@ impl Tab { ) } + // Terminal-launch split builder; params mirror the runtime spawn API. + #[allow(clippy::too_many_arguments)] pub fn split_focused_argv_command_with_ratio( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, ratio: f32, rows: u16, @@ -331,6 +359,8 @@ impl Tab { host_terminal_appearance: Option, ) -> std::io::Result { self.split_focused_with_runtime( + target, + focus_new_pane, direction, Some(ratio), rows, @@ -349,6 +379,8 @@ impl Tab { #[allow(clippy::too_many_arguments)] fn split_focused_with_runtime( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, ratio: Option, rows: u16, @@ -361,10 +393,15 @@ impl Tab { launch_env: &PaneLaunchEnv, command: Option>, ) -> std::io::Result { - let previous_focus = self.layout.focused(); - let new_id = match ratio { - Some(ratio) => self.layout.split_focused_with_ratio(direction, ratio), - None => self.layout.split_focused(direction), + let restore_focus = self.layout.focused(); + let Some(new_id) = + self.layout + .split_pane(target, direction, ratio.unwrap_or(0.5), focus_new_pane) + else { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + "split target pane not found", + )); }; let actual_cwd = cwd.unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| "/".into())); @@ -425,8 +462,10 @@ impl Tab { let runtime = match runtime { Ok(runtime) => runtime, Err(err) => { - self.layout.close_focused(); - self.layout.focus_pane(previous_focus); + if focus_new_pane { + self.layout.focus_pane(restore_focus); + } + self.layout.close_pane(new_id); return Err(err); } }; @@ -493,14 +532,7 @@ impl Tab { if self.layout.pane_count() > 1 { let next_root = self.promoted_root_if_needed(pane_id); - if self.layout.focused() == pane_id { - self.layout.close_focused(); - } else { - let prev_focus = self.layout.focused(); - self.layout.focus_pane(pane_id); - self.layout.close_focused(); - self.layout.focus_pane(prev_focus); - } + self.layout.close_pane(pane_id); if let Some(next_root) = next_root { self.root_pane = next_root; } @@ -520,10 +552,11 @@ impl Tab { moved: MovedPane, direction: Direction, ratio: f32, + focus: bool, ) -> Result { if !self .layout - .insert_pane_near(target_pane_id, moved.pane_id, direction, ratio) + .insert_pane_near(target_pane_id, moved.pane_id, direction, ratio, focus) { return Err(moved); } @@ -540,14 +573,7 @@ impl Tab { let next_root = self.promoted_root_if_needed(pane_id); - if self.layout.focused() == pane_id { - self.layout.close_focused(); - } else { - let prev_focus = self.layout.focused(); - self.layout.focus_pane(pane_id); - self.layout.close_focused(); - self.layout.focus_pane(prev_focus); - } + self.layout.close_pane(pane_id); let pane = self.panes.remove(&pane_id)?; let terminal_id = pane.attached_terminal_id;