From 6943a7f1bac59c79137ccc8e2022e7bf3c9ecb10 Mon Sep 17 00:00:00 2001 From: Jon Kinney Date: Mon, 3 Aug 2026 22:50:28 -0500 Subject: [PATCH] fix(layout): return focus to the pane a split was opened from Closing a focused pane handed focus to the next pane in tree order. For a pane opened beside another one -- a plugin split, a file viewer, any transient tool pane -- that is rarely where the user was: it lands on some unrelated neighbour rather than the pane that opened it. Track the pane focus came from in TileLayout and prefer it when the focused pane closes, falling back to tree order when there is no history, when it points at the pane being closed, or when it points at a pane that has since gone away. The history lives in the layout, so it can only ever name a pane in the same tab. A one-slot history is only sound if internal focus excursions never write it, so the tree edits that used to bounce focus around now go through target-taking primitives instead. close_pane removes a background pane directly, so detach_pane and take_pane_for_move stop focus-close-refocusing. split_pane splits a target without moving focus: the runtime split path only focuses the new pane once the spawn succeeds, which makes a failed split a pure rollback, and the targeted and unfocused workspace split paths stop fabricating history. insert_pane_near now takes the focus intent, so an unfocused pane move leaves the target tab's history alone. The layout-level focused-split helpers become test-only; production splits all flow through the target-taking path. --- src/app/api/panes.rs | 10 +- src/layout.rs | 232 +++++++++++++++++++++++++++++++++++++++++-- src/workspace.rs | 100 ++++++++----------- src/workspace/tab.rs | 118 ++++++++++------------ 4 files changed, 314 insertions(+), 146 deletions(-) diff --git a/src/app/api/panes.rs b/src/app/api/panes.rs index a5f1cbcbf5..97fa0633b2 100644 --- a/src/app/api/panes.rs +++ b/src/app/api/panes.rs @@ -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 { diff --git a/src/layout.rs b/src/layout.rs index 8a16da9ce2..c46a0753ef 100644 --- a/src/layout.rs +++ b/src/layout.rs @@ -84,6 +84,11 @@ pub enum Node { pub struct TileLayout { root: Node, focus: PaneId, + /// Pane focused before `focus`, used by `close_focused`. Only a real focus + /// move writes it; tree edits go through the target-taking primitives + /// (`split_pane`, `close_pane`, unfocused `insert_pane_near`) so internal + /// focus excursions never corrupt it. + prev_focus: Option, } impl TileLayout { @@ -95,11 +100,20 @@ impl TileLayout { Self { root: Node::Pane(root_id), focus: root_id, + prev_focus: None, }, root_id, ) } + /// Move focus, recording the pane being left. No-op when focus is unchanged. + fn set_focus(&mut self, id: PaneId) { + if id != self.focus { + self.prev_focus = Some(self.focus); + self.focus = id; + } + } + pub fn focused(&self) -> PaneId { self.focus } @@ -122,29 +136,52 @@ impl TileLayout { result } - /// Split the focused pane. Returns the new pane's id. + /// Split the focused pane. Returns the new pane's id. Production splits + /// flow through `Tab` so a failed runtime spawn can roll back; this remains + /// as the user-split shape for tests. + #[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 { + let new_id = self + .split_pane(self.focus, direction, ratio) + .expect("focused pane is in the layout"); + self.set_focus(new_id); + new_id + } + + /// Split `target` without moving focus. Returns the new pane's id, or None + /// when `target` is not in the layout. + pub fn split_pane( + &mut self, + target: PaneId, + direction: Direction, + ratio: f32, + ) -> Option { + if !self.pane_ids().contains(&target) { + return None; + } 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; - new_id + self.root = split_at(old, target, direction, new_id, valid_split_ratio(ratio)); + Some(new_id) } /// Insert an existing pane id next to a target pane without allocating a new - /// pane or spawning a terminal runtime. + /// pane or spawning a terminal runtime. When `focus` is false, focus and its + /// history are left untouched. pub fn insert_pane_near( &mut self, target: PaneId, moved: PaneId, direction: Direction, ratio: f32, + focus: bool, ) -> bool { if target == moved { return false; @@ -157,11 +194,14 @@ 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.set_focus(moved); + } true } - /// Close the focused pane. Returns false if it's the last pane. + /// Close the focused pane, returning focus to the pane it came from when + /// that pane is still open. Returns false if it's the last pane. pub fn close_focused(&mut self) -> bool { if self.pane_count() <= 1 { return false; @@ -169,25 +209,51 @@ impl TileLayout { let target = self.focus; let ids = self.pane_ids(); let pos = ids.iter().position(|id| *id == target).unwrap(); - let new_focus = if pos + 1 < ids.len() { + let ordered = if pos + 1 < ids.len() { ids[pos + 1] } else { ids[pos - 1] }; + let new_focus = match self.prev_focus { + Some(prev) if prev != target && ids.contains(&prev) => prev, + _ => ordered, + }; let placeholder = PaneId::from_raw(0); 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.prev_focus = None; true } else { false } } + /// Close any pane. Focus and its history are left alone unless the closed + /// pane is the focused one. + pub fn close_pane(&mut self, id: PaneId) -> bool { + if self.focus == id { + return self.close_focused(); + } + if self.pane_count() <= 1 || !self.pane_ids().contains(&id) { + return false; + } + let placeholder = PaneId::from_raw(0); + let old = std::mem::replace(&mut self.root, Node::Pane(placeholder)); + let Some(new_root) = remove_pane(old, id) else { + return false; + }; + self.root = new_root; + if self.prev_focus == Some(id) { + self.prev_focus = None; + } + true + } + pub fn focus_pane(&mut self, id: PaneId) { if self.pane_ids().contains(&id) { - self.focus = id; + self.set_focus(id); } } @@ -270,7 +336,11 @@ impl TileLayout { /// 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 } + Self { + root, + focus, + prev_focus: None, + } } } @@ -746,7 +816,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]); @@ -954,4 +1024,144 @@ mod tests { Some(pane(3)) ); } + + #[test] + fn close_focused_returns_to_the_pane_focus_came_from() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + + assert!(layout.close_focused()); + + assert_eq!(layout.focused(), pane(2)); + } + + #[test] + fn close_focused_returns_to_the_pane_that_opened_a_split() { + // Allocated ids only: sample_layout() uses from_raw and shares the id + // space with the allocator. + let (mut layout, first) = TileLayout::new(); + let second = layout.split_focused(Direction::Horizontal); + let third = layout.split_focused(Direction::Vertical); + assert_eq!(layout.pane_ids().len(), 3); + + layout.focus_pane(first); + let opened = layout.split_focused(Direction::Horizontal); + assert_eq!(layout.focused(), opened); + + assert!(layout.close_focused()); + + assert_eq!(layout.focused(), first); + assert!(layout.pane_ids().contains(&second)); + assert!(layout.pane_ids().contains(&third)); + } + + #[test] + fn closing_a_background_pane_keeps_the_focused_pane_history() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + + assert!(layout.close_pane(pane(1))); + assert_eq!(layout.focused(), pane(4)); + + assert!(layout.close_focused()); + assert_eq!(layout.focused(), pane(2)); + } + + #[test] + fn closing_the_remembered_pane_drops_the_focus_history() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + + assert!(layout.close_pane(pane(2))); + + assert!(layout.close_focused()); + assert_eq!(layout.focused(), pane(3)); + } + + #[test] + fn close_focused_uses_tree_order_without_focus_history() { + let mut layout = sample_layout(); + + assert!(layout.close_focused()); + + assert_eq!(layout.focused(), pane(3)); + } + + #[test] + fn close_focused_does_not_reuse_history_after_it_is_consumed() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + + assert!(layout.close_focused()); + assert_eq!(layout.focused(), pane(2)); + + assert!(layout.close_focused()); + assert_eq!(layout.focused(), pane(3)); + } + + #[test] + fn resize_does_not_disturb_the_close_focus_target() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + layout.resize_pane(pane(1), NavDirection::Right, 0.05, Rect::new(0, 0, 100, 40)); + + assert!(layout.close_focused()); + + assert_eq!(layout.focused(), pane(2)); + } + + #[test] + fn split_pane_leaves_focus_and_history_untouched() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + + let new_id = layout + .split_pane(pane(1), Direction::Horizontal, 0.5) + .expect("target exists"); + + assert!(layout.pane_ids().contains(&new_id)); + assert_eq!(layout.focused(), pane(4)); + assert!(layout.close_focused()); + assert_eq!(layout.focused(), pane(2)); + } + + #[test] + fn split_pane_missing_target_changes_nothing() { + let mut layout = sample_layout(); + let ids = layout.pane_ids(); + + assert_eq!( + layout.split_pane(pane(99), Direction::Horizontal, 0.5), + None + ); + + assert_eq!(layout.pane_ids(), ids); + } + + #[test] + fn insert_pane_near_unfocused_keeps_focus_and_history() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + + assert!(layout.insert_pane_near(pane(1), pane(9), Direction::Horizontal, 0.5, false)); + + assert_eq!(layout.focused(), pane(4)); + assert!(layout.close_focused()); + assert_eq!(layout.focused(), pane(2)); + } + + #[test] + fn failed_split_rollback_preserves_focus_history() { + let mut layout = sample_layout(); + layout.focus_pane(pane(4)); + + let new_id = layout + .split_pane(layout.focused(), Direction::Horizontal, 0.5) + .expect("target exists"); + assert!(layout.close_pane(new_id)); + + assert_eq!(layout.focused(), pane(4)); + assert!(layout.close_focused()); + assert_eq!(layout.focused(), pane(2)); + } } diff --git a/src/workspace.rs b/src/workspace.rs index 0d0e35bd60..bfda45f650 100644 --- a/src/workspace.rs +++ b/src/workspace.rs @@ -891,70 +891,40 @@ 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( - direction, - ratio, - rows, - cols, - cwd, - argv, - &launch_env, - scrollback_limit_bytes, - host_terminal_theme, - host_terminal_appearance, - ), - None => tab.split_focused_argv_command( - direction, - rows, - cols, - cwd, - argv, - &launch_env, - scrollback_limit_bytes, - host_terminal_theme, - host_terminal_appearance, - ), - } + tab.split_pane_argv( + pane_id, + focus_new_pane, + direction, + ratio, + rows, + cols, + cwd, + argv, + &launch_env, + scrollback_limit_bytes, + host_terminal_theme, + host_terminal_appearance, + ) } else { - match ratio { - Some(ratio) => tab.split_focused_with_ratio( - direction, - ratio, - rows, - cols, - cwd, - scrollback_limit_bytes, - host_terminal_theme, - host_terminal_appearance, - shell_config, - &launch_env, - ), - None => tab.split_focused( - direction, - rows, - cols, - cwd, - scrollback_limit_bytes, - host_terminal_theme, - host_terminal_appearance, - shell_config, - &launch_env, - ), - } + tab.split_pane_shell( + pane_id, + focus_new_pane, + direction, + ratio, + rows, + cols, + cwd, + scrollback_limit_bytes, + host_terminal_theme, + host_terminal_appearance, + shell_config, + &launch_env, + ) } { 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 +1004,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); } @@ -1665,7 +1636,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..6f0fa75e17 100644 --- a/src/workspace/tab.rs +++ b/src/workspace/tab.rs @@ -205,6 +205,7 @@ impl Tab { self.custom_name = Some(name); } + #[cfg(test)] pub fn split_focused( &mut self, direction: Direction, @@ -217,7 +218,9 @@ impl Tab { shell_config: crate::pane::PaneShellConfig<'_>, launch_env: &PaneLaunchEnv, ) -> std::io::Result { - self.split_focused_with_runtime( + self.split_pane_with_runtime( + self.layout.focused(), + true, direction, None, rows, @@ -232,34 +235,6 @@ impl Tab { ) } - pub fn split_focused_with_ratio( - &mut self, - direction: Direction, - ratio: f32, - rows: u16, - cols: u16, - cwd: Option, - scrollback_limit_bytes: usize, - host_terminal_theme: crate::terminal_theme::TerminalTheme, - host_terminal_appearance: Option, - shell_config: crate::pane::PaneShellConfig<'_>, - launch_env: &PaneLaunchEnv, - ) -> std::io::Result { - self.split_focused_with_runtime( - direction, - Some(ratio), - rows, - cols, - cwd, - scrollback_limit_bytes, - host_terminal_theme, - host_terminal_appearance, - shell_config, - launch_env, - None, - ) - } - pub fn split_focused_command( &mut self, direction: Direction, @@ -272,7 +247,9 @@ impl Tab { host_terminal_theme: crate::terminal_theme::TerminalTheme, host_terminal_appearance: Option, ) -> std::io::Result { - self.split_focused_with_runtime( + self.split_pane_with_runtime( + self.layout.focused(), + true, direction, None, rows, @@ -290,37 +267,51 @@ impl Tab { ) } - pub fn split_focused_argv_command( + /// Split `target` with a shell pane. Focus moves to the new pane only when + /// `focus_new_pane` is set; a spawn failure rolls the layout back without + /// touching focus or its history. + #[allow(clippy::too_many_arguments)] + pub(crate) fn split_pane_shell( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, + ratio: Option, rows: u16, cols: u16, cwd: Option, - argv: &[String], - launch_env: &PaneLaunchEnv, scrollback_limit_bytes: usize, host_terminal_theme: crate::terminal_theme::TerminalTheme, host_terminal_appearance: Option, + shell_config: crate::pane::PaneShellConfig<'_>, + launch_env: &PaneLaunchEnv, ) -> std::io::Result { - self.split_focused_with_runtime( + self.split_pane_with_runtime( + target, + focus_new_pane, direction, - None, + ratio, rows, cols, cwd, scrollback_limit_bytes, host_terminal_theme, host_terminal_appearance, - crate::pane::PaneShellConfig::new("", crate::config::ShellModeConfig::NonLogin), + shell_config, launch_env, - Some(SplitCommand::Argv { argv, launch_env }), + None, ) } - pub fn split_focused_argv_command_with_ratio( + /// Split `target` with an argv-command pane. Same focus contract as + /// `split_pane_shell`. + #[allow(clippy::too_many_arguments)] + pub(crate) fn split_pane_argv( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, - ratio: f32, + ratio: Option, rows: u16, cols: u16, cwd: Option, @@ -330,9 +321,11 @@ impl Tab { host_terminal_theme: crate::terminal_theme::TerminalTheme, host_terminal_appearance: Option, ) -> std::io::Result { - self.split_focused_with_runtime( + self.split_pane_with_runtime( + target, + focus_new_pane, direction, - Some(ratio), + ratio, rows, cols, cwd, @@ -347,8 +340,10 @@ impl Tab { // Split construction threads geometry, host context, launch policy, and command state. #[allow(clippy::too_many_arguments)] - fn split_focused_with_runtime( + fn split_pane_with_runtime( &mut self, + target: PaneId, + focus_new_pane: bool, direction: Direction, ratio: Option, rows: u16, @@ -361,10 +356,14 @@ 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 Some(new_id) = self + .layout + .split_pane(target, direction, ratio.unwrap_or(0.5)) + else { + return Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + "split target pane is not in the layout", + )); }; let actual_cwd = cwd.unwrap_or_else(|| std::env::current_dir().unwrap_or_else(|_| "/".into())); @@ -425,8 +424,7 @@ impl Tab { let runtime = match runtime { Ok(runtime) => runtime, Err(err) => { - self.layout.close_focused(); - self.layout.focus_pane(previous_focus); + self.layout.close_pane(new_id); return Err(err); } }; @@ -437,6 +435,9 @@ impl Tab { } None => TerminalState::new(terminal_id.clone(), actual_cwd), }; + if focus_new_pane { + self.layout.focus_pane(new_id); + } self.panes.insert(new_id, PaneState::new(terminal_id)); self.zoomed = false; Ok(NewPane { @@ -493,14 +494,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 +514,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 +535,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;