From 6f7712a5ee03d03a2912402416245e876727759e Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:50:55 +0800 Subject: [PATCH 1/3] fix(terminal): keep a ligated run standing over its own cells (#785) Refs #751. The PR body's rationale is stale as of #788: build_font now emits calt:0 liga:0 clig:0 and gpui's DirectWrite backend zeroes all three, so ligatures are off by default on Windows and the Calibri office/waffle repro no longer fires with default settings. This is a fix for users who opt ligatures back on, and for any face on any platform that collapses glyph count. Reconciled onto #783: the fit flag is gone, so seg_budget now takes ink_covers_segment and the shaping moved above the budget. --- src/terminal/element.rs | 315 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 300 insertions(+), 15 deletions(-) diff --git a/src/terminal/element.rs b/src/terminal/element.rs index 55618483..e37ba9a9 100644 --- a/src/terminal/element.rs +++ b/src/terminal/element.rs @@ -1079,6 +1079,103 @@ fn ink_extent( }) } +/// Where a shaped run stops standing over the cells it came from. +/// +/// `force_width` puts the *n*th glyph at *n × cell_width*, which is the whole +/// grid only while the shaper hands back one glyph per character. A ligature +/// substitution collapses several characters into one glyph, and from there on +/// every glyph is pulled left by the cells the substitution swallowed: the +/// text after it overlaps the ligature's tail, and the caret — drawn at the +/// grid column, not at the ink — stands past the end of the run with a gap. +/// +/// Takes the glyphs' byte indices, which in a run are their columns, and +/// answers with the first column that has drifted. Only a glyph that arrives +/// later than its ordinal counts: a face that *adds* glyphs cannot be helped +/// by cutting the run, and treating it as drift would cut at column zero +/// forever. +fn run_drift(indices: impl IntoIterator) -> Option { + indices + .into_iter() + .enumerate() + .find_map(|(ordinal, index)| (index > ordinal).then_some(index)) +} + +/// One thing [`paint_run`] asks of the caller, in bytes into the run's text. +/// +/// A run is ASCII, one byte per cell, so `at` is also a column offset and +/// `len` is also a width in cells. +#[derive(Debug, PartialEq)] +enum RunStep { + /// Shape `text[at..at + len]` and answer [`run_drift`] for it. + Drift { at: usize, len: usize }, + /// Shape and paint `text[at..at + len]` starting at column `col`. + Paint { col: usize, at: usize, len: usize }, +} + +/// Walk a run in the pieces that stand over their own cells. +/// +/// A run that does not drift is one `Drift` and one `Paint` of the whole +/// thing, which is every run in a monospace face. Where it drifts, the part +/// in front of the drift is painted as a piece of its own — shaped on its +/// own, which is the point: clipping the tail away would not help, because +/// the tail drifted *left*, into the columns this piece is keeping. The +/// remainder then starts over in the column the grid puts it in. +/// +/// The loop is separated from the shaping so it can be tested at all: gpui's +/// test text system hands back one glyph per character, so nothing shaped +/// through it ever drifts and the interesting half would never run. +fn paint_run(start: usize, len: usize, mut step: impl FnMut(RunStep) -> Option) { + let mut col = start; + let mut at = 0; + loop { + match step(RunStep::Drift { at, len: len - at }) { + None => { + step(RunStep::Paint { + col, + at, + len: len - at, + }); + return; + } + Some(drift) => { + step(RunStep::Paint { + col, + at, + len: drift, + }); + col += drift; + at += drift; + } + } + } +} + +/// Shape one piece of a segment's text. +/// +/// The whole of it keeps the string it arrived in; only a piece cut out of a +/// drifted run has to be copied, and that happens where a ligature forced the +/// cut. Repeat calls for the same piece within a frame are answered from +/// gpui's line-layout cache rather than shaped again. +fn shape_piece( + window: &mut Window, + run_buf: &mut [TextRun; 1], + text: &SharedString, + at: usize, + len: usize, + font_size: Pixels, + force_width: Option, +) -> gpui::ShapedLine { + let piece = if at == 0 && len == text.len() { + text.clone() + } else { + SharedString::from(text[at..at + len].to_string()) + }; + run_buf[0].len = piece.len(); + window + .text_system() + .shape_line(piece, font_size, run_buf, force_width) +} + /// Whether [`ink_extent`]'s answer speaks for the whole segment. /// /// It measures the segment's first character in the run's first face. That is @@ -1148,7 +1245,12 @@ fn paint_glyphs( // A `Run` is one glyph per cell in the main font, which by // definition already fits; the rest can carry a glyph from a // fallback face that is wider than the cells it was handed. - let fit = !matches!(seg, RowSeg::Run { .. }); + // + // A run is also the only segment `segment_row` builds out of more + // than one cell of plain text, and it only batches ASCII: one + // byte, one character, one column. That is what lets `run_drift` + // read a glyph's byte index as the column it came from. + let run = matches!(seg, RowSeg::Run { .. }); let (start, cells, text, force_width, solo) = match seg { RowSeg::Run { start, cells, text } => ( start, @@ -1231,26 +1333,58 @@ fn paint_glyphs( let x = geom.origin.x + geom.cell_width * (start as f32); + // A run is walked in the pieces that stand over their own cells; + // every other segment is one shaping and one paint. + if run { + paint_run(start, text.len(), |step| match step { + RunStep::Drift { at, len } => { + let shaped = + shape_piece(window, run_buf, &text, at, len, font_size, force_width); + run_drift( + shaped + .runs + .iter() + .flat_map(|r| r.glyphs.iter().map(|g| g.index)), + ) + } + RunStep::Paint { col, at, len } => { + let shaped = + shape_piece(window, run_buf, &text, at, len, font_size, force_width); + let x = geom.origin.x + geom.cell_width * (col as f32); + let clip = Bounds::new( + point(x, y), + size(geom.cell_width * len as f32, geom.line_height), + ); + window.with_content_mask(Some(ContentMask { bounds: clip }), |window| { + _ = shaped.paint( + point(x, y), + geom.line_height, + TextAlign::Left, + None, + window, + cx, + ); + }); + None + } + }); + continue; + } + let mut shaped = window .text_system() .shape_line(text.clone(), font_size, run_buf, force_width); // Measured before the budget is set, because how far a solo glyph // may reach turns on whether its ink is known at all. - let ink = fit - .then(|| ink_extent(cx, &shaped, &text, font_size)) - .flatten(); - let budget = if fit { - seg_budget( - solo, - ink_covers_segment(ink, &text), - cells, - has_room_after(row_cells, start, cells), - geom.cell_width, - ) - } else { - geom.cell_width * cells as f32 - }; + let ink = ink_extent(cx, &shaped, &text, font_size); + let budget = seg_budget( + solo, + ink_covers_segment(ink, &text), + cells, + has_room_after(row_cells, start, cells), + geom.cell_width, + ); if let Some(ink) = ink { let scale = fit_scale(ink, budget); if scale < 1. { @@ -2432,6 +2566,130 @@ mod tests { assert_eq!(scale(0.5, true), 1.); } + #[test] + fn a_run_that_keeps_one_glyph_per_cell_is_painted_whole() { + assert_eq!(run_drift([0, 1, 2, 3]), None); + assert_eq!(run_drift([0]), None); + assert_eq!(run_drift([0usize; 0]), None); + } + + #[test] + fn a_ligature_cuts_the_run_at_the_column_that_drifted() { + // "office" through a face that ligates "ffi": the shaper answers with + // o, ffi, c, e, and `force_width` sits them on columns 0..4 — so 'c' + // lands two columns early, over the ligature's tail, and the row ends + // two columns short of where the caret is drawn. + assert_eq!(run_drift([0, 1, 4, 5]), Some(4)); + // The remainder, restarted at column 4, lines up on its own. + assert_eq!(run_drift([0, 1]), None); + // A two-character ligature drifts by one: "afib" -> a, fi, b. + assert_eq!(run_drift([0, 1, 3]), Some(3)); + // And a run can drift on its very first pair: "fib" -> fi, b. + assert_eq!(run_drift([0, 2]), Some(2)); + } + + /// Drives the loop `paint_glyphs` runs a `RowSeg::Run` through, with the + /// shaping stubbed out. `drifts` is what the shaper is pretending to + /// answer for each piece it is handed, in order. + fn run_steps(start: usize, text: &str, drifts: &[Option]) -> Vec { + let mut seen = Vec::new(); + let mut answers = drifts.iter().copied(); + paint_run(start, text.len(), |step| { + let drift = matches!(step, RunStep::Drift { .. }) + .then(|| { + answers + .next() + .expect("asked to shape more pieces than scripted") + }) + .flatten(); + seen.push(step); + drift + }); + seen + } + + /// What the caller is told to paint, as `(column, text)`. + fn painted<'a>(steps: &[RunStep], text: &'a str) -> Vec<(usize, &'a str)> { + steps + .iter() + .filter_map(|step| match *step { + RunStep::Paint { col, at, len } => Some((col, &text[at..at + len])), + RunStep::Drift { .. } => None, + }) + .collect() + } + + #[test] + fn a_run_that_does_not_drift_is_shaped_once_and_painted_whole() { + let steps = run_steps(7, "hello", &[None]); + assert_eq!( + steps, + [ + RunStep::Drift { at: 0, len: 5 }, + RunStep::Paint { + col: 7, + at: 0, + len: 5 + } + ], + "no drift is one shaping and one paint, at the column it started in" + ); + } + + #[test] + fn a_drifted_run_paints_the_head_alone_and_restarts_at_its_own_column() { + // "office" through a face that ligates "ffi": the shaper answers with + // o, ffi, c, e, so 'c' arrives at byte 4 as the third glyph and the + // run has to be cut there. + let steps = run_steps(0, "office", &[Some(4), None]); + assert_eq!( + painted(&steps, "office"), + [(0, "offi"), (4, "ce")], + "the head is a piece of its own, and the rest starts at column 4" + ); + // The head must be *shaped* as "offi", not painted as the whole run + // under a four-cell clip: 'c' and 'e' drifted left, into those very + // cells, so a clip would leave them on top of the ligature. + assert_eq!( + steps[1], + RunStep::Paint { + col: 0, + at: 0, + len: 4 + } + ); + // And the remainder is re-shaped on its own before it is painted, so + // its own drift is measured from its own column. + assert_eq!(steps[2], RunStep::Drift { at: 4, len: 2 }); + } + + #[test] + fn a_run_that_drifts_twice_keeps_advancing_and_finishes() { + // "affib" -> a, ffi, b cuts once; the tail "b" then stands on its own. + assert_eq!( + painted(&run_steps(3, "affib", &[Some(4), None]), "affib"), + [(3, "affi"), (7, "b")] + ); + // Two cuts in a row: every piece moves the column on by its own width + // and the walk ends on the piece that does not drift. + assert_eq!( + painted( + &run_steps(0, "offifie", &[Some(4), Some(2), None]), + "offifie" + ), + [(0, "offi"), (4, "fi"), (6, "e")] + ); + } + + #[test] + fn a_face_that_adds_glyphs_is_left_alone() { + // Two glyphs for one character walk ahead of their columns, not + // behind them. Cutting there would restart the run where it already + // is and never finish. + assert_eq!(run_drift([0, 0, 1, 2]), None); + assert_eq!(run_drift([0, 1, 1, 2]), None); + } + #[test] fn only_a_plain_blank_cell_counts_as_room() { let mut row: Vec<_> = "ab".chars().map(cell).collect(); @@ -2528,6 +2786,33 @@ mod tests { assert_eq!(segment_row(&row), [run(0, 5, "ab cd")]); } + /// `paint_glyphs` reads a glyph's byte index as the column it came from + /// and cuts a drifted run there, which only holds while a run is ASCII and + /// one byte wide per cell. Pin that here rather than in the paint code. + #[test] + fn a_run_is_as_long_in_bytes_as_it_is_wide_in_cells() { + let rows: [Vec; 4] = [ + " ab cd ".chars().map(cell).collect(), + "https://例/a".chars().map(cell).collect(), + { + let mut row: Vec<_> = "ab cd".chars().map(cell).collect(); + for c in &mut row { + c.underline = UnderlineKind::Single; + } + row + }, + "x->y a//b".chars().map(cell).collect(), + ]; + for row in rows { + for seg in segment_row(&row) { + if let RowSeg::Run { cells, text, .. } = seg { + assert!(text.is_ascii(), "{text:?} is not ASCII"); + assert_eq!(text.len(), cells, "{text:?} does not span {cells} cells"); + } + } + } + } + #[test] fn segment_row_ends_underlined_runs_at_blanks() { let mut row: Vec<_> = "ab cd".chars().map(cell).collect(); 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 2/3] 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)); From cae2aeb74fc09db6a304f4191b597c928853d978 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Mon, 7 Sep 2026 23:55:21 +0800 Subject: [PATCH 3/3] test(ui): run the window and pane gpui tests on Windows too (#791) Windows tty7-app tests go 1466 -> 1638 with no assertion weakened. Also holds the SCM graph idle test's daemon end open: the moved handle closed the socket right after writing Cwd, which on Windows (loopback TcpStream with unread data) is an abortive close, so settle_graph would time out and the test would silently skip every assertion. --- src/terminal/view.rs | 83 ++++++++++++++++++++++++++++++------------ src/ui/app.rs | 19 ++++------ src/ui/diff_overlay.rs | 20 ++++++++-- src/ui/file_tree.rs | 6 +-- src/ui/scm/detail.rs | 16 +++++++- src/ui/scm/graph.rs | 17 +++++---- src/ui/scm/panel.rs | 15 +++++++- src/ui/switcher.rs | 2 +- src/ui/tree_sync.rs | 2 - 9 files changed, 125 insertions(+), 55 deletions(-) diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 800f1713..bda9a686 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -98,7 +98,7 @@ pub fn declare_displayed(cx: &App, panes: impl IntoIterator Option { cx.try_global::()? .0 @@ -9132,22 +9132,42 @@ mod tests { } } +/// A connected pair of [`crate::daemon::transport::Stream`]s, one for each end +/// of a pane's link to its daemon. +/// +/// The client half is what a pane really reads and writes; the daemon half is +/// the test's, to speak protocol into. +/// +/// This is the one thing a pane harness needs that Unix and Windows spell +/// differently — `socketpair` there, a loopback connect here — and every gpui +/// test in this crate is portable once it goes through this instead of naming +/// `UnixStream` itself. +#[cfg(test)] +pub(crate) fn test_stream_pair() -> ( + crate::daemon::transport::Stream, + crate::daemon::transport::Stream, +) { + #[cfg(unix)] + { + std::os::unix::net::UnixStream::pair().unwrap() + } + #[cfg(windows)] + { + let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); + let addr = listener.local_addr().unwrap(); + let client_side = std::net::TcpStream::connect(addr).unwrap(); + let (daemon_side, _) = listener.accept().unwrap(); + (client_side, daemon_side) + } +} + #[cfg(test)] pub(crate) fn quiet_test_pane( pane_id: u64, window: &mut Window, cx: &mut gpui::App, ) -> (gpui::Entity, crate::daemon::transport::Stream) { - #[cfg(unix)] - let (client_side, daemon_side) = std::os::unix::net::UnixStream::pair().unwrap(); - #[cfg(windows)] - let (client_side, daemon_side) = { - let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap(); - let addr = listener.local_addr().unwrap(); - let client_side = std::net::TcpStream::connect(addr).unwrap(); - let (daemon_side, _) = listener.accept().unwrap(); - (client_side, daemon_side) - }; + let (client_side, daemon_side) = test_stream_pair(); let terminal = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24)) .expect("quiet test terminal"); let view = cx.new(|cx| TerminalView::with_terminal(terminal, pane_id, window, cx)); @@ -9200,20 +9220,20 @@ pub(crate) fn quiet_test_ssh_pane_with( (view, stream) } -#[cfg(all(test, unix))] +#[cfg(test)] mod gpui_tests { use super::*; use crate::daemon::protocol::{ClientMsg, DaemonMsg}; + use crate::daemon::transport::Stream; use gpui::{Entity, TestAppContext, point}; - use std::os::unix::net::UnixStream; - fn harness(cx: &mut TestAppContext) -> (gpui::WindowHandle, UnixStream) { + fn harness(cx: &mut TestAppContext) -> (gpui::WindowHandle, Stream) { // Building a view reads the config. Whether that hit the real user // directory used to come down to which test happened to pin the // scratch dir first. crate::core::config::pin_test_config_dir(); cx.executor().allow_parking(); - let (client_side, daemon_side) = UnixStream::pair().unwrap(); + let (client_side, daemon_side) = super::test_stream_pair(); cx.update(|cx| { gpui_component::init(cx); cx.set_global(Config::default()); @@ -9238,11 +9258,11 @@ mod gpui_tests { ) -> ( gpui::WindowHandle, Entity, - UnixStream, + Stream, ) { crate::core::config::pin_test_config_dir(); cx.executor().allow_parking(); - let (client_side, daemon_side) = UnixStream::pair().unwrap(); + let (client_side, daemon_side) = super::test_stream_pair(); cx.update(|cx| { gpui_component::init(cx); cx.set_global(Config::default()); @@ -9268,7 +9288,7 @@ mod gpui_tests { fn prompt_ready( window: &gpui::WindowHandle, cx: &mut TestAppContext, - daemon: &mut UnixStream, + daemon: &mut Stream, ) { DaemonMsg::Prompt { active: true, @@ -9292,7 +9312,7 @@ mod gpui_tests { fn alt_screen_ready( window: &gpui::WindowHandle, cx: &mut TestAppContext, - daemon: &mut UnixStream, + daemon: &mut Stream, ) { DaemonMsg::Output(b"\x1b[?1049h".to_vec()) .encode(daemon) @@ -9320,7 +9340,7 @@ mod gpui_tests { .encode(&mut daemon) .unwrap(); - let report = |status: AgentStatus, daemon: &mut UnixStream| { + let report = |status: AgentStatus, daemon: &mut Stream| { DaemonMsg::AgentStatus(Some(AgentSessionState { status, message: None, @@ -9427,7 +9447,7 @@ mod gpui_tests { status: crate::core::cli_agent::AgentStatus, pane: &gpui::Entity, cx: &mut TestAppContext, - daemon: &mut UnixStream, + daemon: &mut Stream, ) { use crate::core::cli_agent::AgentSessionState; @@ -9877,6 +9897,21 @@ mod gpui_tests { /// must not have made the promise. Otherwise those paths sit "not answered /// yet" for the life of the pane — no underline, and a click that says /// nothing, which is the silence this whole path exists to remove. + /// + /// Was unix-only because the path it prints is: `Path::new("/etc/hosts")` + /// is not absolute on Windows, so `FileCandidate::paths` measured it from + /// the roots rather than letting it stand alone — and a workspace that + /// never connected has no roots, so nothing was ever wanted. Which was + /// itself the divergence: a Windows tty7 looking at a *remote* Linux pane + /// never probed the POSIX paths that pane printed. + /// + /// #795 settled that. `paths` now asks the pane's own + /// [`super::search::PathStyle`] rather than this machine's, and a remote + /// pane that has not reported a cwd is read as `Posix`, so `/etc/hosts` + /// stands alone on every client. The gate is only still here because + /// nothing has run this test on Windows yet; lifting it belongs in a + /// change that can show it green, not in a merge. + #[cfg(unix)] #[gpui::test] fn a_probe_with_no_host_to_ask_stays_wanted(cx: &mut TestAppContext) { let (window, mut daemon) = harness(cx); @@ -10068,7 +10103,7 @@ mod gpui_tests { .unwrap(); } - fn next_input(daemon: &mut UnixStream) -> Vec { + fn next_input(daemon: &mut Stream) -> Vec { loop { match ClientMsg::read(daemon).expect("client socket stays open") { ClientMsg::Input(bytes) => return bytes, @@ -10100,7 +10135,7 @@ mod gpui_tests { } } - fn next_input_until_timeout(daemon: &mut UnixStream) -> Option> { + fn next_input_until_timeout(daemon: &mut Stream) -> Option> { use std::io::ErrorKind; daemon @@ -12845,7 +12880,7 @@ mod gpui_tests { } assert_eq!(seen, "before", "the pre-drop screen is what we relink over"); - let (new_client, mut new_daemon) = UnixStream::pair().unwrap(); + let (new_client, mut new_daemon) = super::test_stream_pair(); window .update(cx, |view, _, cx| { view.adopt_relink( diff --git a/src/ui/app.rs b/src/ui/app.rs index 8c73c6fb..6a76284b 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -9491,14 +9491,13 @@ pub(crate) mod test_window { } /// A window carrying `n` quiet tabs, active on the first. - #[cfg(unix)] pub(crate) fn harness_with_tabs( cx: &mut TestAppContext, n: usize, ) -> ( Entity, VisualTestContext, - Vec, + Vec, ) { use crate::terminal::view::quiet_test_pane; use crate::ui::pane::{Pane, PaneSlot}; @@ -9525,13 +9524,12 @@ pub(crate) mod test_window { (app, vcx, streams) } - #[cfg(unix)] pub(crate) fn harness_with_pane( cx: &mut TestAppContext, ) -> ( Entity, VisualTestContext, - std::os::unix::net::UnixStream, + crate::daemon::transport::Stream, ) { use crate::terminal::view::quiet_test_pane; use crate::ui::pane::{Pane, PaneSlot}; @@ -9580,7 +9578,6 @@ pub(crate) mod test_window { /// another frame 250ms later. So the sleep below is load-bearing too, and /// a round that drew nothing is not on its own enough to stop on — a burst /// still open is a frame already owed. - #[cfg(unix)] pub(crate) fn quiesce(vcx: &mut VisualTestContext, cwd: Option<&std::path::Path>) { use crate::terminal::git_data::ScmData; use crate::terminal::git_status::GitStatusCache; @@ -9692,7 +9689,7 @@ mod cursor_blink_gpui_tests { } } -#[cfg(all(test, unix))] +#[cfg(test)] mod ssh_rebuild_gpui_tests { use super::test_window::harness_with_pane; use crate::core::session::{ @@ -10129,9 +10126,7 @@ mod shell_menu_gpui_tests { } } -// `harness_with_tabs` hands back the panes' `UnixStream`s, so it exists only -// on unix — same as `ssh_rebuild_gpui_tests` below it. -#[cfg(all(test, unix))] +#[cfg(test)] mod rename_gpui_tests { use gpui::TestAppContext; @@ -10215,7 +10210,7 @@ mod rename_gpui_tests { // everything else hidden; a pane nobody has declared — or whose id nobody // registered — must err toward displayed, because the failure direction that // matters is a visible pane that stops repainting. -#[cfg(all(test, unix))] +#[cfg(test)] mod displayed_gpui_tests { use gpui::TestAppContext; @@ -10316,7 +10311,7 @@ mod displayed_gpui_tests { // Zoom is a tab's view state: it rides with the tab across a switch, while a // layout change (drag, split, close) still clears it. -#[cfg(all(test, unix))] +#[cfg(test)] mod zoom_gpui_tests { use gpui::TestAppContext; @@ -10444,7 +10439,7 @@ mod zoom_gpui_tests { // test config dir and nothing is listening on it — so every forward request // fails. That is exactly the case these are about: what the panel and the form // are left holding when the far side does not answer. -#[cfg(all(test, unix))] +#[cfg(test)] mod managed_forward_gpui_tests { use gpui::TestAppContext; use gpui_component::input::InputState; diff --git a/src/ui/diff_overlay.rs b/src/ui/diff_overlay.rs index 8d5eb30c..4a3e595c 100644 --- a/src/ui/diff_overlay.rs +++ b/src/ui/diff_overlay.rs @@ -2737,7 +2737,7 @@ mod tests { } } -#[cfg(all(test, unix))] +#[cfg(test)] mod overlay_gpui_tests { use super::*; use crate::ui::app::test_window; @@ -3015,8 +3015,22 @@ mod overlay_gpui_tests { /// terminal, an editor, a worktree command — and the cached branch is a branch /// the repository has left. That is what the stale entry below stands for. /// -/// Unix-gated like every other window harness in this tree: `harness_with_tabs` -/// hands back a `std::os::unix::net::UnixStream` for the pane. +/// Unix-only, and not for the harness: on Windows the root this test seeds +/// the cache with is not the root the probe lands with, so `scm_epoch` never +/// agrees with the landing snapshot and the overlay re-probes on every frame +/// — `load` reaches `Ready` and `loading` goes straight back to `true`, which +/// is the exact spin this test exists to catch. +/// +/// Not the slash direction — `Path` compares by component, so `C:/x` and +/// `C:\x` are already equal. It is the prefix, and since #796 it is this +/// test's own: the product keys a repository by one spelling now +/// (`Host::canonicalize` drops the `\\?\` extended-length prefix and +/// `core::git::git_path` re-spells what git prints), while the seed below +/// still comes straight from `std::fs::canonicalize` and so carries +/// `\\?\C:\Users\—` — a `VerbatimDisk` prefix where everything it is +/// compared against is now `Disk`. Seeding through +/// `tty7_core::core::path_spelling` should lift this, as a change that can +/// show it green rather than a drive-by. #[cfg(all(test, unix))] mod render_idle_gpui_tests { use super::*; diff --git a/src/ui/file_tree.rs b/src/ui/file_tree.rs index f5408ec8..0d379823 100644 --- a/src/ui/file_tree.rs +++ b/src/ui/file_tree.rs @@ -2931,7 +2931,7 @@ mod tests { } } -#[cfg(all(test, unix))] +#[cfg(test)] mod render_idle_gpui_tests { use super::*; use crate::daemon::protocol::DaemonMsg; @@ -2959,7 +2959,7 @@ mod render_idle_gpui_tests { ) -> ( Entity, VisualTestContext, - std::os::unix::net::UnixStream, + crate::daemon::transport::Stream, ) { let (app, mut vcx, mut pane) = test_window::harness_with_pane(cx); DaemonMsg::Cwd(root.to_path_buf()) @@ -3558,7 +3558,7 @@ mod render_idle_gpui_tests { /// What these cannot reach is the hit test — whether the row under the cursor /// is the one that gets the drop is decided by gpui's hitbox stack, and there /// is no headless way to put a cursor over a row. -#[cfg(all(test, unix))] +#[cfg(test)] mod drop_gpui_tests { use super::render_idle_gpui_tests::{files_panel_on, rows, scratch, serial, settle}; use super::*; diff --git a/src/ui/scm/detail.rs b/src/ui/scm/detail.rs index d1ffc073..043e2747 100644 --- a/src/ui/scm/detail.rs +++ b/src/ui/scm/detail.rs @@ -974,6 +974,20 @@ mod tests { /// here — a missing global, a theme token, a slice through the middle of a /// character — goes wrong during layout and paint, so these arm the render /// probe and insist something was actually drawn. +/// +/// Still unix-only, and for a reason worth naming rather than a harness one: +/// on Windows the root the panel settles on is not the root this module hands +/// it, so the panel never settles on the directory it is already showing. +/// +/// The forward slashes `git rev-parse --show-toplevel` prints are not what +/// breaks it — `Path` compares by component, so `C:/x` and `C:\x` are equal. +/// The prefix is, and since #796 it is this module's own: the product keys a +/// repository by one spelling now, while `scratch` below still hands the pane +/// `std::fs::canonicalize`'s `\\?\C:\Users\—`, a `VerbatimDisk` prefix where +/// every root it is compared against is `Disk`. Taking the gate off needs +/// that helper to spell its answer the way +/// `tty7_core::core::path_spelling` does, in a change that can show these +/// green rather than a drive-by. #[cfg(all(test, unix))] mod detail_gpui_tests { use super::*; @@ -1043,7 +1057,7 @@ mod detail_gpui_tests { ) -> ( Entity, VisualTestContext, - std::os::unix::net::UnixStream, + crate::daemon::transport::Stream, ) { let (app, mut vcx, mut pane) = test_window::harness_with_pane(cx); DaemonMsg::Cwd(root.to_path_buf()) diff --git a/src/ui/scm/graph.rs b/src/ui/scm/graph.rs index 553c9a61..c0e4ea87 100644 --- a/src/ui/scm/graph.rs +++ b/src/ui/scm/graph.rs @@ -2186,11 +2186,7 @@ mod tests { /// way to know is to settle the window and count frames. Same shape as the file /// tree's own idle tests, including the serial lock: the render probe is /// thread-local and two of these at once would count each other's frames. -/// -/// `unix` for the same reason `panel.rs`, `detail.rs` and `file_tree.rs` gate -/// theirs: a real pane means `test_window::harness_with_pane`, and that harness -/// hands back a `std::os::unix::net::UnixStream`. -#[cfg(all(test, unix))] +#[cfg(test)] mod render_idle_gpui_tests { use super::*; use crate::ui::app::{render_probe, test_window}; @@ -2290,9 +2286,16 @@ mod render_idle_gpui_tests { )); } - let (app, mut vcx, _pane) = test_window::harness_with_pane(cx); + // The daemon end is held for the life of the test, the way every other + // panel harness holds it. `&mut { _pane }` dropped it on the spot: on + // Unix a closed `socketpair` half still delivers the `Cwd` written a + // moment earlier, but on Windows the link is a loopback `TcpStream`, + // and closing one with the pane's own `Resize` sitting unread on it is + // an abortive close — the `Cwd` goes with the connection, and the test + // spends its 30s deadline waiting for a cwd that was thrown away. + let (app, mut vcx, mut pane) = test_window::harness_with_pane(cx); crate::daemon::protocol::DaemonMsg::Cwd(root.clone()) - .encode(&mut { _pane }) + .encode(&mut pane) .expect("the pane's socket takes the cwd"); app.update_in(&mut vcx, |app, _, cx| { app.right_panel_visible = true; diff --git a/src/ui/scm/panel.rs b/src/ui/scm/panel.rs index fc591cdf..f828597d 100644 --- a/src/ui/scm/panel.rs +++ b/src/ui/scm/panel.rs @@ -2894,7 +2894,7 @@ mod tests { /// `default_global`, which fires the global observers whether or not anything /// changed, and it is called every frame. A watcher that notified on every one /// of those would request a frame from inside a frame and never stop. -#[cfg(all(test, unix))] +#[cfg(test)] mod render_idle_gpui_tests { use super::*; use crate::daemon::protocol::DaemonMsg; @@ -2933,7 +2933,7 @@ mod render_idle_gpui_tests { ) -> ( Entity, VisualTestContext, - std::os::unix::net::UnixStream, + crate::daemon::transport::Stream, ) { let (app, mut vcx, mut pane) = test_window::harness_with_pane(cx); DaemonMsg::Cwd(root.to_path_buf()) @@ -2980,6 +2980,17 @@ mod render_idle_gpui_tests { render_probe::draws() } + /// The only test in this module that waits on `repo.root`, and so the only + /// one Windows cannot run: since #796 that root is keyed by one spelling + /// and carries a `Disk` prefix, while the pane's cwd came out of `scratch` + /// above — `std::fs::canonicalize`, so `\\?\C:\Users\—` and a + /// `VerbatimDisk` prefix — and the equality below never holds between the + /// two. The slashes are the red herring; `Path` compares by component, so + /// `C:/x` and `C:\x` are equal. Spelling `scratch`'s answer the way + /// `tty7_core::core::path_spelling` does should lift this, in a change + /// that can show it green. Its sibling keys off the pane's cwd instead, + /// and runs everywhere. + #[cfg(unix)] #[gpui::test] fn a_settled_source_control_panel_reaches_render_idle(cx: &mut TestAppContext) { let _serial = serial(); diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index e94f451d..9f318889 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -3720,7 +3720,7 @@ mod tests { } } -#[cfg(all(test, unix))] +#[cfg(test)] mod gpui_tests { use gpui::{Modifiers, TestAppContext}; diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index 5ea14edc..ba193b4e 100644 --- a/src/ui/tree_sync.rs +++ b/src/ui/tree_sync.rs @@ -3686,7 +3686,6 @@ mod tests { /// the window shows one, and the one it could not put up must not come out /// of a `Full` diff as `TabClose` — that op deleted from the machine exactly /// the tabs a restart had failed to bring back, panes and all (#672). - #[cfg(unix)] #[gpui::test] fn the_next_sync_leaves_a_tab_the_rebuild_could_not_put_up_on_the_machine( cx: &mut gpui::TestAppContext, @@ -3775,7 +3774,6 @@ mod tests { /// straight after clears the queue, so by the time a test can look the /// ops are gone either way — while `informed` outliving the arrival is /// both durable and the thing that made them possible. - #[cfg(unix)] #[gpui::test] fn arriving_at_a_workspace_does_not_prune_what_is_already_in_it(cx: &mut gpui::TestAppContext) { let (app, mut vcx, _pane_stream) = crate::ui::app::test_window::harness_with_pane(cx);