diff --git a/src/ui/app.rs b/src/ui/app.rs index e8449b40..8409a7c5 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -3489,6 +3489,16 @@ impl Tty7App { } } + /// Sample which pane holds focus right now and record it against the + /// active tab. + /// + /// A sample only ever writes the truth, but it can only write it when + /// there is one to read: with focus one handle off the panes it finds no + /// leaf and leaves the field alone. That is why it is no longer the only + /// writer (see [`Tty7App::remember_focused_leaf`]) — it stays because the + /// callers below want the answer settled at a named moment, before a pane + /// is detached or a tab is torn down and the layout stops being able to + /// answer at all. pub(crate) fn remember_active_pane(&mut self, window: &Window, cx: &App) { let active = self.active; if let Some(tab) = self.tabs.get_mut(active) { @@ -3498,6 +3508,24 @@ impl Tty7App { } } + /// Record `leaf` as the pane its tab comes back to, as focus arrives in it. + /// + /// Sampling at switch time asks which leaf holds focus *at that instant*, + /// and by then focus is routinely somewhere else: the switcher's own + /// search input, a palette that just closed, the tab strip, a pane + /// restored and never clicked. The sample then wrote nothing and the tab + /// kept a stale pane — or the `None` it was born with — and came back to + /// its first leaf instead of the one the reader was working in (#843). + /// + /// Focus-in is the one moment that knows the answer without having to + /// guess when to look, so it is the primary writer now. The tab is found + /// by the leaf rather than assumed to be the active one: a pane dragged + /// into another tab is focused after the move, and it is the tab holding + /// it now that has to remember it. + pub(crate) fn remember_focused_leaf(&mut self, leaf: gpui::EntityId) { + remember_leaf_in(&mut self.tabs, leaf); + } + fn focus_leaf(&self, leaf: &PaneSlot, window: &mut Window, cx: &mut App) { let handle = leaf.focus_handle(cx); window.focus(&handle, cx); @@ -3549,9 +3577,7 @@ impl Tty7App { view.read(cx).run_command_line(&cmd); } let slot = PaneSlot::Ready(view.clone()); - self.tabs - .iter_mut() - .any(|tab| tab.pane.replace_leaf(slot_id, slot.clone())); + replace_leaf_in(&mut self.tabs, slot_id, slot.clone()); if was_focused { self.focus_leaf(&slot, window, cx); } @@ -3755,14 +3781,11 @@ impl Tty7App { return; } }; - for tab in &mut self.tabs { - if tab - .pane - .replace_leaf(dead.entity_id(), PaneSlot::Ready(fresh.clone())) - { - break; - } - } + replace_leaf_in( + &mut self.tabs, + dead.entity_id(), + PaneSlot::Ready(fresh.clone()), + ); self.maximized = None; self.focus_leaf(&PaneSlot::Ready(fresh), window, cx); self.save_session(cx); @@ -8573,6 +8596,8 @@ pub(crate) fn new_terminal( }, ) .detach(); + let handle = pending.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, pending.entity_id(), window, cx); start_pane_spawn(pending.clone(), window, cx); Ok(PaneSlot::Connecting(pending)) } @@ -8638,7 +8663,8 @@ fn build_terminal_view( ) .detach(); watch_open_file_requests(&view, window, cx); - watch_pane_focus(&view, window, cx); + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); view } @@ -8666,13 +8692,66 @@ fn watch_open_file_requests( .detach(); } -fn watch_pane_focus(view: &Entity, window: &mut Window, cx: &mut Context) { - let handle = view.read(cx).focus_handle.clone(); +/// Record `leaf` as the pane the tab holding it comes back to. +/// +/// Which tab that is gets asked of the layout rather than assumed to be the +/// active one: a pane dragged into another tab takes focus with it, and it is +/// the tab holding it now whose memory the arrival should change. A leaf no +/// tab holds — one that has just closed, or arrived after its slot went away — +/// is recorded nowhere. +fn remember_leaf_in(tabs: &mut [Tab], leaf: gpui::EntityId) { + let held = tabs + .iter_mut() + .find(|tab| tab.pane.leaves().iter().any(|l| l.entity_id() == leaf)); + if let Some(tab) = held { + tab.last_focused = Some(leaf); + } +} + +/// Put `new` where the slot `old` named stood, carrying that tab's focus +/// memory across with it. +/// +/// The memory has to move because the id it holds does not survive the swap. +/// Focus arriving in a pane that is still coming up is recorded against the +/// *pending* slot — that is why connecting slots are watched at all — and that +/// slot's id dies the moment the pane lands. Left behind, the memory names an +/// entity no tab holds, `focus_target` falls through `leaf_matching_or_first`, +/// and the tab comes back to its first leaf: #843 again, one landing later. +/// +/// Nothing else writes the answer down in that case. `land_pane` re-focuses +/// the pane it built only when the pending slot still held focus, and with +/// focus off the panes the switch-away sample has nothing to read either. +fn replace_leaf_in(tabs: &mut [Tab], old: gpui::EntityId, new: PaneSlot) { + for tab in tabs.iter_mut() { + if tab.pane.replace_leaf(old, new.clone()) { + if tab.last_focused == Some(old) { + tab.last_focused = Some(new.entity_id()); + } + break; + } + } +} + +/// Repaint the chrome that marks the focused pane, and record the leaf as the +/// one its tab returns to (#843). +/// +/// Every leaf is watched, connecting slots included: a pane can be focused +/// while it is still coming up, and if the tab is left in that moment the +/// answer has to already be written down. +fn watch_pane_focus( + handle: &gpui::FocusHandle, + leaf: gpui::EntityId, + window: &mut Window, + cx: &mut Context, +) { let app = cx.weak_entity(); window - .on_focus_in(&handle, cx, move |_window, cx| { + .on_focus_in(handle, cx, move |_window, cx| { if let Some(app) = app.upgrade() { - app.update(cx, |_, cx| cx.notify()); + app.update(cx, |app, cx| { + app.remember_focused_leaf(leaf); + cx.notify(); + }); } }) .detach(); @@ -8704,7 +8783,8 @@ pub(crate) fn new_terminal_native( ) .detach(); watch_open_file_requests(&view, window, cx); - watch_pane_focus(&view, window, cx); + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); Ok(view) } @@ -11146,3 +11226,281 @@ mod close_window_action_tests { ); } } + +/// #843: which pane a tab comes back to. +#[cfg(test)] +mod tab_focus_memory_tests { + use super::{Pane, PaneSlot, Tab, remember_leaf_in, replace_leaf_in}; + use crate::ui::pending_pane::{PendingPane, PendingSpawn}; + use gpui::{ + AppContext as _, Axis, Context, Entity, IntoElement, Render, Styled as _, TestAppContext, + Window, div, + }; + + /// A window has to exist for focus to live in, but nothing this file asks + /// is about what a pane paints. + struct Blank; + impl Render for Blank { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div().size_full() + } + } + + /// A leaf that owns a real focus handle without owning a shell. Focus + /// tracking asks the slot, not the terminal behind it, so a connecting + /// pane answers every question here exactly as a running one would — and + /// connecting panes are watched for focus now too, so this is not a + /// stand-in for the case under test but one of its cases. + fn leaf(cx: &mut Context) -> Entity { + cx.new(|cx| { + PendingPane::new( + "test", + PendingSpawn { + workspace: None, + working_directory: None, + restore_pane: None, + shell: None, + agent: None, + agent_session_id: None, + agent_launch_argv: None, + owner: None, + font_size: 14., + }, + cx, + ) + }) + } + + fn two_pane_tab(a: &Entity, b: &Entity) -> Tab { + Tab::new(Pane::split_node( + Axis::Horizontal, + 0.5, + Pane::Leaf(PaneSlot::Connecting(a.clone())), + Pane::Leaf(PaneSlot::Connecting(b.clone())), + )) + } + + fn one_pane_tab(a: &Entity) -> Tab { + Tab::new(Pane::Leaf(PaneSlot::Connecting(a.clone()))) + } + + /// The mechanism behind the report: `remember_active_pane`'s sample asks + /// which leaf holds focus *at that instant*, and a switch begun with focus + /// one handle away — the switcher's own search input, a palette closing, + /// the tab strip — finds no leaf and has nothing to write. This is why the + /// sample cannot be the only writer, and it is pinned here so that a change + /// making `focused_leaf` tolerant would have to argue with a test rather + /// than silently make this fix look unnecessary. + #[gpui::test] + fn a_switch_begun_off_the_panes_samples_nothing(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, elsewhere) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), cx.focus_handle())) + .unwrap(); + let tab = two_pane_tab(&a, &b); + + window + .update(cx, |_, window, cx| { + let on_b = b.read(cx).focus_handle.clone(); + window.focus(&on_b, cx); + assert_eq!( + tab.pane.focused_leaf(window, cx).map(|l| l.entity_id()), + Some(b.entity_id()), + "with focus in the pane the sample would have found it" + ); + + window.focus(&elsewhere, cx); + assert!( + tab.pane.focused_leaf(window, cx).is_none(), + "one handle off the pane and the switch-away sample has \ + nothing to write" + ); + }) + .unwrap(); + } + + /// So focus-in writes instead, and the tab comes back to the pane focus + /// was last in even though it had wandered off the panes before the switch + /// ever started. + #[gpui::test] + fn a_tab_comes_back_to_the_pane_focus_was_last_in(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b) = window.update(cx, |_, _, cx| (leaf(cx), leaf(cx))).unwrap(); + let mut tabs = vec![two_pane_tab(&a, &b)]; + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(a.entity_id()), + "a tab nobody has worked in yet still opens on its first leaf" + ); + + // The reader clicks into the right-hand pane: focus arrives, and that + // is the moment the tab is told. + remember_leaf_in(&mut tabs, b.entity_id()); + + // Focus then leaves the panes — the switcher opens, a palette closes — + // and the tab is switched away from. `remember_active_pane` finds no + // focused leaf and writes nothing, which is now harmless. + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(b.entity_id()), + "#843: the tab has to come back to the pane the reader was in" + ); + } + + /// A pane dragged into another tab is focused where it lands, so the + /// arrival has to change that tab's memory and not the one it left — the + /// reason the tab is found by the leaf rather than taken to be the active + /// one. + #[gpui::test] + fn a_moved_pane_is_remembered_by_the_tab_that_holds_it_now(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, c) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + // Tab 0 is the active one and holds `a`; `b` and `c` live in tab 1. + let mut tabs = vec![one_pane_tab(&a), two_pane_tab(&b, &c)]; + + remember_leaf_in(&mut tabs, c.entity_id()); + + assert_eq!( + tabs[1].focus_target().map(|l| l.entity_id()), + Some(c.entity_id()), + "the tab holding the focused pane is the one that remembers it" + ); + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(a.entity_id()), + "and no other tab's memory is touched" + ); + } + + /// A pane that arrives after its slot has gone — a spawn landing on a + /// closed tab, a leaf killed mid-flight — belongs to no tab, and must not + /// leave a memory behind for the first tab that happens to be looked at. + #[gpui::test] + fn a_leaf_no_tab_holds_is_recorded_nowhere(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, gone) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + let mut tabs = vec![two_pane_tab(&a, &b)]; + + remember_leaf_in(&mut tabs, b.entity_id()); + remember_leaf_in(&mut tabs, gone.entity_id()); + + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(b.entity_id()), + "a stranger's arrival leaves the tab's own answer alone" + ); + } + + /// A pane focused while it was still coming up is remembered under its + /// *pending* slot, and that id dies the moment the pane lands in its + /// place. The memory has to come along with the swap, or the landing is + /// itself what puts the tab back on its first leaf. + /// + /// What lands here is another slot rather than a running pane: the swap has + /// to move an id from one slot to another, and which kind of slot arrived + /// is no part of the question. + #[gpui::test] + fn a_landing_pane_inherits_what_its_pending_slot_was_told(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, connecting, landed, other) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + let mut tabs = vec![two_pane_tab(&a, &connecting)]; + + // Focus arrives while the pane is still connecting, then the pane it + // was waiting for lands in that slot. + remember_leaf_in(&mut tabs, connecting.entity_id()); + replace_leaf_in( + &mut tabs, + connecting.entity_id(), + PaneSlot::Connecting(landed.clone()), + ); + + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(landed.entity_id()), + "#843: the memory follows the pane, not the slot it arrived in" + ); + + // A landing somewhere else in the tab is not an answer to this + // question and does not touch it. + replace_leaf_in(&mut tabs, a.entity_id(), PaneSlot::Connecting(other)); + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(landed.entity_id()), + "another pane landing leaves the tab's answer alone" + ); + } + + /// The wiring, end to end. `watch_pane_focus` is the subscription that + /// writes the record, and a real round trip through `activate` has to come + /// back to the pane focus last arrived in — with focus off the panes well + /// before the switch was made, which is the moment the switch-away sample + /// cannot see (#843). + #[gpui::test] + fn a_tab_switch_returns_to_the_pane_focus_arrived_in(cx: &mut TestAppContext) { + use super::{test_window::harness, watch_pane_focus}; + use crate::terminal::view::quiet_test_pane; + + let (app, mut vcx) = harness(cx); + let (left, right, elsewhere, _held) = app.update_in(&mut vcx, |app, window, cx| { + let (left, left_stream) = quiet_test_pane(1, window, cx); + let (right, right_stream) = quiet_test_pane(2, window, cx); + let (only, only_stream) = quiet_test_pane(3, window, cx); + app.tabs.push(Tab::new(Pane::split_node( + Axis::Horizontal, + 0.5, + Pane::leaf(PaneSlot::Ready(left.clone())), + Pane::leaf(PaneSlot::Ready(right.clone())), + ))); + app.tabs.push(Tab::new(Pane::leaf(PaneSlot::Ready(only)))); + app.active = 0; + // The subscription every spawn path registers for the pane it + // built. + for view in [&left, &right] { + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); + } + cx.notify(); + ( + left, + right, + cx.focus_handle(), + (left_stream, right_stream, only_stream), + ) + }); + vcx.background_executor.run_until_parked(); + + // The reader clicks into the right-hand pane. + app.update_in(&mut vcx, |_, window, cx| { + let handle = right.read(cx).focus_handle.clone(); + handle.focus(window, cx); + }); + vcx.background_executor.run_until_parked(); + + // Focus then leaves the panes altogether — a palette closing, the tab + // strip, the switcher's own search input — before the tab is left. + app.update_in(&mut vcx, |_, window, cx| elsewhere.focus(window, cx)); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |app, window, cx| app.activate(1, window, cx)); + vcx.background_executor.run_until_parked(); + app.update_in(&mut vcx, |app, window, cx| app.activate(0, window, cx)); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |_, window, cx| { + assert!( + right.read(cx).focus_handle.is_focused(window), + "#843: the tab has to come back to the pane the reader was in" + ); + assert!( + !left.read(cx).focus_handle.is_focused(window), + "and not to the first leaf" + ); + }); + } +}