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;