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

@@ -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<u64>,
+    /// 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<gpui::EntityId> {
+        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<Self>) {
-        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<gpui::EntityId> = 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,

src/ui/pane.rs

@@ -82,7 +82,7 @@ pub enum Pane<L = PaneSlot> {
     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<L: Clone> Pane<L> {
 
     pub fn neighbor_in_direction(&self, from: usize, dir: Dir) -> Option<usize> {
         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<L: Clone> Pane<L> {
                 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<usize>,
+    ) -> Option<usize> {
+        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<PaneSlot> {
             .collect()
     }
 
-    pub fn neighbor_in_dir(&self, dir: Dir, window: &Window, cx: &App) -> Option<PaneSlot> {
+    /// 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<gpui::EntityId>,
+        window: &Window,
+        cx: &App,
+    ) -> Option<PaneSlot> {
         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));