From 314ec61efef413ddb8feab7cc5d3a636ec9a5486 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:55:10 +0800 Subject: [PATCH] fix(pane): return to the pane a directional move left (#781) Refs #738. Keys the origin memory by the pane a move lands on as well as the direction: the per-direction array meant a two-step walk clobbered the first step, so Left, Left, Right, Right ended in the wrong pane. Entries are pruned on write when either side names a pane the tab no longer holds. --- src/ui/app.rs | 107 ++++++++++++++++++++-- src/ui/pane.rs | 241 ++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 326 insertions(+), 22 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index 08365c3d..8c73c6fb 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -440,6 +440,24 @@ pub struct Tab { /// Monotonic stamp of when this tab was last activated, used to order the /// switcher's tab column most-recently-used first. Zero means never. pub(crate) last_used: std::cell::Cell, + /// Where a directional focus move started, keyed by the pane it landed on + /// and the direction that undoes it, so reversing a move comes back here + /// instead of wherever geometry ranks first (#738). Per tab because the + /// panes are. + /// + /// Keyed by pane and not by direction alone: one slot per direction is + /// overwritten by the next move the same way, so a walk of two steps left + /// and two back right ends somewhere other than it started — the very drift + /// this is here to stop. A pane remembering its own way in retraces the + /// whole walk. + /// + /// A recorded pane only ever breaks a tie between the panes already next to + /// the one focus is leaving, so an entry left over from an older layout — or + /// from before a click moved focus somewhere else entirely — can at worst + /// pick a different neighbour, never a distant one. Entries naming a pane + /// the tab no longer holds are dropped as the next one is written, so a + /// closed pane leaves nothing behind either. + focus_origin: std::collections::HashMap<(gpui::EntityId, Dir), gpui::EntityId>, } #[derive(Clone, Copy, PartialEq, Eq, Debug, Default)] @@ -463,6 +481,7 @@ impl Tab { sidebar_group: std::cell::RefCell::new(None), tree_id: std::cell::Cell::new(tty7_core::core::machine::TabId::new()), last_used: std::cell::Cell::new(0), + focus_origin: Default::default(), } } @@ -481,6 +500,7 @@ impl Tab { ), tree_id: std::cell::Cell::new(tree.id), last_used: std::cell::Cell::new(0), + focus_origin: Default::default(), } } @@ -491,6 +511,28 @@ impl Tab { } } + /// The pane a move in `dir` out of `at` should return to, if it reverses + /// the move that brought focus to `at`. + fn focus_origin(&self, at: gpui::EntityId, dir: Dir) -> Option { + self.focus_origin.get(&(at, dir)).copied() + } + + /// Remember that a move in `dir` carried focus from `from` to `to`, so the + /// move back out of `to` returns. `live` names the panes the tab holds now: + /// anything the layout has moved on from is forgotten here rather than + /// accumulating for the life of the window. + fn remember_focus_origin( + &mut self, + from: gpui::EntityId, + to: gpui::EntityId, + dir: Dir, + live: &[gpui::EntityId], + ) { + self.focus_origin + .retain(|(at, _), origin| live.contains(at) && live.contains(origin)); + self.focus_origin.insert((to, dir.opposite()), from); + } + pub(crate) fn detail_pane( &self, window: &Window, @@ -1742,6 +1784,7 @@ impl Tty7App { sidebar_group: std::cell::RefCell::new(st.sidebar_group), tree_id: std::cell::Cell::new(tty7_core::core::machine::TabId::new()), last_used: std::cell::Cell::new(0), + focus_origin: Default::default(), }, ); self.active = insert_at; @@ -3678,13 +3721,22 @@ impl Tty7App { } fn focus_pane_dir(&mut self, dir: Dir, window: &mut Window, cx: &mut Context) { - let Some(target) = self - .tabs - .get(self.active) - .and_then(|tab| tab.pane.neighbor_in_dir(dir, window, cx)) - else { + let Some(tab) = self.tabs.get(self.active) else { return; }; + let Some(from) = tab.pane.focused_leaf(window, cx) else { + return; + }; + let back = tab.focus_origin(from.entity_id(), dir); + let Some(target) = tab.pane.neighbor_in_dir(dir, back, window, cx) else { + return; + }; + let live: Vec = tab.pane.leaves().iter().map(|l| l.entity_id()).collect(); + let (from, to) = (from.entity_id(), target.entity_id()); + let active = self.active; + if let Some(tab) = self.tabs.get_mut(active) { + tab.remember_focus_origin(from, to, dir, &live); + } self.maximized = None; self.focus_leaf(&target, window, cx); cx.notify(); @@ -8035,6 +8087,7 @@ fn tabs_from_session( .unwrap_or_else(tty7_core::core::machine::TabId::new), ), last_used: std::cell::Cell::new(0), + focus_origin: Default::default(), }); } let active = session.active.min(tabs.len().saturating_sub(1)); @@ -8826,16 +8879,52 @@ mod window_drag_tests { #[cfg(test)] mod tests { use super::{ - CloseReason, DOCUMENT_MIN_W, TERMINAL_MIN_W, TITLE_BAR_HEIGHT, TabAgentSession, - clear_window_override_values, close_prompt, document_column_px, join_shell_args, - leaf_shares_the_window_daemon, mru_order, pane_free_for, parse_ssh_connect_input, - parse_ssh_option_words, side_panel_max, split_shell_args, strip_band, wd_path_saveable, + CloseReason, DOCUMENT_MIN_W, Dir, Pane, TERMINAL_MIN_W, TITLE_BAR_HEIGHT, Tab, + TabAgentSession, clear_window_override_values, close_prompt, document_column_px, + join_shell_args, leaf_shares_the_window_daemon, mru_order, pane_free_for, + parse_ssh_connect_input, parse_ssh_option_words, side_panel_max, split_shell_args, + strip_band, wd_path_saveable, }; use gpui::{Edges, point, px, size}; const SIDEBAR_MIN: f32 = crate::ui::tab_sidebar::MIN_SIDEBAR_WIDTH; const PANEL_MIN: f32 = crate::ui::right_panel::MIN_WIDTH; + /// #738: which pane a directional move came from is remembered against the + /// pane it landed on, not against the direction alone. + /// + /// One slot per direction is enough for a single move and back, but the + /// second step of a walk overwrites the first: left off `3` onto `2` and + /// left again onto `1` would leave only `1 -> 2`, and the second move back + /// right — the one out of `2` — would be handed no origin and fall to the + /// geometry that sent the user to the wrong pane in the first place. + #[test] + fn each_pane_remembers_the_move_that_landed_on_it() { + fn id(n: u64) -> gpui::EntityId { + gpui::EntityId::from(n) + } + let mut tab = Tab::new(Pane::Empty); + let live = [id(1), id(2), id(3)]; + + tab.remember_focus_origin(id(3), id(2), Dir::Left, &live); + tab.remember_focus_origin(id(2), id(1), Dir::Left, &live); + + // Walking back retraces both steps rather than only the last one. + assert_eq!(tab.focus_origin(id(1), Dir::Right), Some(id(2))); + assert_eq!(tab.focus_origin(id(2), Dir::Right), Some(id(3))); + // Nothing is claimed about a direction no move went in, or about a pane + // no move has landed on. + assert_eq!(tab.focus_origin(id(2), Dir::Left), None); + assert_eq!(tab.focus_origin(id(3), Dir::Right), None); + + // A pane the tab no longer holds takes its entries with it, on either + // side: with `3` closed, `2` no longer remembers having come from it, + // and the move back out of `2` is left to geometry. + tab.remember_focus_origin(id(1), id(2), Dir::Right, &[id(1), id(2)]); + assert_eq!(tab.focus_origin(id(2), Dir::Right), None); + assert_eq!(tab.focus_origin(id(2), Dir::Left), Some(id(1))); + } + /// #679: the band now starts at the frame padding rather than at the /// window's corner, and off Linux CSD there is no padding to start at — /// `window_paddings` answers `Edges::all(0)` under server-side decorations, diff --git a/src/ui/pane.rs b/src/ui/pane.rs index 34778762..145d08d4 100644 --- a/src/ui/pane.rs +++ b/src/ui/pane.rs @@ -82,7 +82,7 @@ pub enum Pane { Empty, } -#[derive(Clone, Copy, PartialEq, Eq, Debug)] +#[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] pub enum Dir { Left, Right, @@ -107,6 +107,16 @@ impl Dir { fn grows(self) -> bool { matches!(self, Dir::Right | Dir::Down) } + + /// The direction that undoes a move this way. + pub fn opposite(self) -> Dir { + match self { + Dir::Left => Dir::Right, + Dir::Right => Dir::Left, + Dir::Up => Dir::Down, + Dir::Down => Dir::Up, + } + } } /// What a tab wants drawn around its panes this frame. @@ -137,6 +147,22 @@ fn overlap_1d(a0: f32, alen: f32, b0: f32, blen: f32) -> f32 { ((a0 + alen).min(b0 + blen) - a0.max(b0)).max(0.0) } +/// Slack for the arithmetic behind `leaf_rects`: ratios multiply out down the +/// tree, so edges that meet exactly in the layout can differ in the last bits. +const ADJACENCY_EPS: f32 = 1e-4; + +/// How far `c` lies from `f` in `dir` and how much edge the two share, or +/// `None` when `c` is not on that side of `f` or only touches it at a corner. +fn adjacency(f: Rect, c: Rect, dir: Dir) -> Option<(f32, f32)> { + let (dist, overlap) = match dir { + Dir::Left => (f.x - (c.x + c.w), overlap_1d(f.y, f.h, c.y, c.h)), + Dir::Right => (c.x - (f.x + f.w), overlap_1d(f.y, f.h, c.y, c.h)), + Dir::Up => (f.y - (c.y + c.h), overlap_1d(f.x, f.w, c.x, c.w)), + Dir::Down => (c.y - (f.y + f.h), overlap_1d(f.x, f.w, c.x, c.w)), + }; + (dist >= -ADJACENCY_EPS && overlap > ADJACENCY_EPS).then_some((dist, overlap)) +} + pub enum CloseOutcome { NotFound, Collapsed, @@ -792,22 +818,22 @@ impl Pane { pub fn neighbor_in_direction(&self, from: usize, dir: Dir) -> Option { let rects = self.leaf_rects(); + Self::ranked_neighbor(&rects, from, dir).map(|(i, _)| i) + } + + /// The pane a move in `dir` lands on and how far off it sits: nearest wins, + /// and the widest shared edge breaks a tie. + fn ranked_neighbor(rects: &[(L, Rect)], from: usize, dir: Dir) -> Option<(usize, f32)> { let f = rects.get(from)?.1; - const EPS: f32 = 1e-4; + const EPS: f32 = ADJACENCY_EPS; let mut best: Option<(usize, f32, f32)> = None; for (i, (_, c)) in rects.iter().enumerate() { if i == from { continue; } - let (dist, overlap) = match dir { - Dir::Left => (f.x - (c.x + c.w), overlap_1d(f.y, f.h, c.y, c.h)), - Dir::Right => (c.x - (f.x + f.w), overlap_1d(f.y, f.h, c.y, c.h)), - Dir::Up => (f.y - (c.y + c.h), overlap_1d(f.x, f.w, c.x, c.w)), - Dir::Down => (c.y - (f.y + f.h), overlap_1d(f.x, f.w, c.x, c.w)), - }; - if dist < -EPS || overlap <= EPS { + let Some((dist, overlap)) = adjacency(f, *c, dir) else { continue; - } + }; let better = match best { None => true, Some((_, bd, bo)) => dist < bd - EPS || (dist <= bd + EPS && overlap > bo + EPS), @@ -816,7 +842,52 @@ impl Pane { best = Some((i, dist, overlap)); } } - best.map(|(i, _, _)| i) + best.map(|(i, dist, _)| (i, dist)) + } + + /// Whether a move in `dir` could land on `to` without stepping over + /// anything: `to` shares an edge with that side of `from`, and nothing in + /// that direction sits nearer. + /// + /// Lying on the right side is not enough on its own. In a row of three + /// columns the far one also sits to the right of the first with a full edge + /// in common, and treating that as adjacent would skip the column between + /// them. + pub fn is_adjacent_in_direction(&self, from: usize, to: usize, dir: Dir) -> bool { + if from == to { + return false; + } + let rects = self.leaf_rects(); + let (Some((_, f)), Some((_, c))) = (rects.get(from), rects.get(to)) else { + return false; + }; + let Some((dist, _)) = adjacency(*f, *c, dir) else { + return false; + }; + Self::ranked_neighbor(&rects, from, dir) + .is_some_and(|(_, nearest)| dist <= nearest + ADJACENCY_EPS) + } + + /// The pane a move in `dir` lands on, preferring `back` — where the last + /// move the other way started — as long as it is still adjacent. + /// + /// Among the panes actually next to `from`, geometry can only rank by + /// shared edge, so at a T-junction (one tall pane facing a stack) the + /// reverse move lands on the same member of the stack whichever one you + /// left, and going back and forth drifts (#738). Preferring where you came + /// from settles that tie the only way the user can mean it. + /// + /// It settles a tie and nothing more: `back` still has to be one of the + /// nearest panes that way, so a move can never step over the pane in + /// between, however out of date the caller's memory is. + pub fn focus_target_in_direction( + &self, + from: usize, + dir: Dir, + back: Option, + ) -> Option { + back.filter(|&back| self.is_adjacent_in_direction(from, back, dir)) + .or_else(|| self.neighbor_in_direction(from, dir)) } pub fn resize_focused(&self, is_focused: &impl Fn(&L) -> bool, dir: Dir, step: f32) -> bool { @@ -890,13 +961,24 @@ impl Pane { .collect() } - pub fn neighbor_in_dir(&self, dir: Dir, window: &Window, cx: &App) -> Option { + /// The pane focus moves to, `back` naming the pane the last move the other + /// way started from. A `back` that is no longer a leaf here — closed, or + /// left behind in another tab — is simply not found; one that is still here + /// but no longer next to `from` loses to the pane that is. + pub fn neighbor_in_dir( + &self, + dir: Dir, + back: Option, + window: &Window, + cx: &App, + ) -> Option { let focused = self.focused_leaf(window, cx)?; let leaves = self.leaves(); let from = leaves .iter() .position(|l| l.entity_id() == focused.entity_id())?; - let target = self.neighbor_in_direction(from, dir)?; + let back = back.and_then(|id| leaves.iter().position(|l| l.entity_id() == id)); + let target = self.focus_target_in_direction(from, dir, back)?; leaves.get(target).cloned() } @@ -1591,6 +1673,139 @@ mod tests { assert_eq!(pane.neighbor_in_direction(idx(0), Dir::Right), Some(idx(1))); } + /// The layout from #738: one full-height pane facing a stack of two. + fn t_junction() -> TestPane { + TestPane::split_node( + Axis::Horizontal, + 0.5, + Pane::Leaf(0), + TestPane::split_node(Axis::Vertical, 0.5, Pane::Leaf(4), Pane::Leaf(6)), + ) + } + + #[test] + fn reversing_a_move_at_a_t_junction_returns_to_where_it_started() { + let pane = t_junction(); + let idx = |id: u32| pane.leaves().iter().position(|v| *v == id).unwrap(); + assert_eq!( + pane.focus_target_in_direction(idx(6), Dir::Left, None), + Some(idx(0)) + ); + // Geometry alone ties on overlap here and hands back the top pane. + assert_eq!(pane.neighbor_in_direction(idx(0), Dir::Right), Some(idx(4))); + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, Some(idx(6))), + Some(idx(6)) + ); + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, Some(idx(4))), + Some(idx(4)) + ); + } + + #[test] + fn a_recorded_pane_the_layout_moved_on_from_falls_back_to_geometry() { + let pane = t_junction(); + let idx = |id: u32| pane.leaves().iter().position(|v| *v == id).unwrap(); + // Gone entirely: the caller maps a missing pane to no index at all, and + // an index the tree no longer has must not resolve to whoever took it. + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, None), + Some(idx(4)) + ); + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, Some(99)), + Some(idx(4)) + ); + // Still there, but no longer reachable that way: splitting the tall + // pane leaves its top half facing only the top of the stack. + let mut pane = t_junction(); + split(&mut pane, 0, Axis::Vertical, 1); + let idx = |id: u32| pane.leaves().iter().position(|v| *v == id).unwrap(); + assert!(!pane.is_adjacent_in_direction(idx(0), idx(6), Dir::Right)); + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, Some(idx(6))), + Some(idx(4)) + ); + // And the direction still has to match: the pane below is never the + // answer to a move right, however recently focus came from there. + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, Some(idx(1))), + Some(idx(4)) + ); + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, Some(idx(0))), + Some(idx(4)) + ); + } + + /// Two steps out and two back is the same walk the reverse of a single move + /// is, and it has to end where it started for the same reason. Replays the + /// bookkeeping `focus_pane_dir` does — each move recorded against the pane + /// it landed on — over the layout that breaks it: three columns whose last + /// one is a stack, so the second move back is the one geometry cannot call. + #[test] + fn a_walk_of_two_steps_comes_back_to_the_pane_it_started_from() { + // 0 | 1 | (2 over 3). + let pane = TestPane::split_node( + Axis::Horizontal, + 1.0 / 3.0, + Pane::Leaf(0), + TestPane::split_node( + Axis::Horizontal, + 0.5, + Pane::Leaf(1), + TestPane::split_node(Axis::Vertical, 0.5, Pane::Leaf(2), Pane::Leaf(3)), + ), + ); + let idx = |id: u32| pane.leaves().iter().position(|v| *v == id).unwrap(); + // Geometry alone ties on overlap out of the middle column and answers + // with the top of the stack, whichever member the walk set off from. + assert_eq!(pane.neighbor_in_direction(idx(1), Dir::Right), Some(idx(2))); + + let mut origin: std::collections::HashMap<(usize, Dir), usize> = + std::collections::HashMap::new(); + let mut at = idx(3); + for dir in [Dir::Left, Dir::Left, Dir::Right, Dir::Right] { + let back = origin.get(&(at, dir)).copied(); + let to = pane + .focus_target_in_direction(at, dir, back) + .expect("a neighbour that way"); + origin.insert((to, dir.opposite()), at); + at = to; + } + assert_eq!( + at, + idx(3), + "the second move back must return to the bottom of the stack too" + ); + } + + #[test] + fn a_remembered_pane_further_off_never_steps_over_the_one_between() { + // Three equal full-height columns, 0 | 1 | 2. + let pane = TestPane::split_node( + Axis::Horizontal, + 1.0 / 3.0, + Pane::Leaf(0), + TestPane::split_node(Axis::Horizontal, 0.5, Pane::Leaf(1), Pane::Leaf(2)), + ); + let idx = |id: u32| pane.leaves().iter().position(|v| *v == id).unwrap(); + // Focus reaches 1 by moving left off 2, then leaves for 0 by a click or + // a cycle — neither of which records anything, so the origin still + // names 2 when the move back to the right happens from 0. + assert!(pane.is_adjacent_in_direction(idx(1), idx(2), Dir::Right)); + assert!(!pane.is_adjacent_in_direction(idx(0), idx(2), Dir::Right)); + // 2 does lie to the right of 0 with a full edge in common; only being + // farther off than 1 disqualifies it. + assert!(adjacency(rect_of(&pane, 0), rect_of(&pane, 2), Dir::Right).is_some()); + assert_eq!( + pane.focus_target_in_direction(idx(0), Dir::Right, Some(idx(2))), + Some(idx(1)), + "a move right must land on the next column, not skip it" + ); + } + #[test] fn resize_grows_the_focused_pane_from_either_side() { let build = || TestPane::split_node(Axis::Horizontal, 0.5, Pane::Leaf(0), Pane::Leaf(1));