From 7e1bfb6dc59a438a8dcae7721e2bc4edc95d9385 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 24 Jul 2026 15:21:48 -0400 Subject: [PATCH] test(window): pin the terminal anchor, not the tail-relative offset `TerminalViewStatus.scroll_offset` is the retained rows between the VIEWPORT and the live tail, so it necessarily tracks viewport height: an assertion that it survives a panel height change unchanged is either vacuous or wrong, and it went red once under a loaded sweep for exactly that reason. Q#BP7's invariant is that the ANCHOR is frozen, so acc32 and acc33 now compare the first visible row's text across the change, and additionally pin the follow behavior that distinguishes them: a shrink never re-arms follow, growth reaching the tail does, and growth with a frozen selection does not. Both also wait for the child's last line before sampling, so neither races further output. Also records the round in docs/active-work.md. Co-Authored-By: Claude Opus 5 (1M context) --- docs/active-work.md | 52 +++++-- tests/bottom_panel_stage1_acceptance.rs | 193 ++++++++++++++++++------ 2 files changed, 188 insertions(+), 57 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index fe1fb19..4e493af 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -84,21 +84,49 @@ If it does not, stop and repair the remote/fetch configuration. - Adopters: `listview.open`, `compile.run`, `pmacs.terminal.open` all take `display = "current" | "panel"` (Stage 1 default `"current"`); LSP/compile visits route through `display_file`. +- **Review round 1 addressed.** The load-bearing finding: the Q#BP6 + side-window split guard (`try_split_active`) had **no production + caller** — `pmacs.window.split_horizontal` / `split_vertical`, and so + `C-x 2` / `C-x 3`, still went through plain `split_active`. Splitting a + focused panel made the root wrapper's final child a split rather than + `Leaf(side)`, which both `Layout::compute`'s fixed pass and + `document_subtree` key on. It survived the first round because the + acceptance test called the core method **directly**; it now goes + through the real Lua binding. This is the folding-arc round-2 lesson + repeating exactly: *after wiring a guard into a production hook, pin it + through the real path — a direct-call test misses the wiring.* + Also fixed: the armed divider drag was not scoped to its arming + frontend (it could cancel and swallow a peer's mouse events); a + recompile carries no `display` and duplicated a panel-placed + `*compilation*` into the document window; and + `paint_mode_line_graphemes` had lost its doc block to an insertion. + Five bite-verified fixes (three via `scripts/bite`, two by manual + revert since their tests share `src/daemon.rs` with the production + code). +- Two Stage-2 hazard pins now exist in `src/daemon.rs`, closing the gap + the review named: a fresh attach while `LOCAL` is focused in a panel + inherits `LOCAL`'s **document** buffer, and an initial-target bootstrap + whose `after-load` hook creates and selects a panel still reasserts + into a document window. - Verification on this branch: `cargo fmt --check` clean; strict - workspace Clippy clean; 1,815 default + 1,992 CRDT library tests; - `bottom_panel_stage1_acceptance` 42/42; M4 121; required GPU 152; - `gpu_initial_target_acceptance` 1 default + 14 CRDT; vterm Stage 2 4 / + workspace Clippy clean; 1,817 default + 1,994 CRDT library tests; + `bottom_panel_stage1_acceptance` 44/44; M4 121; required GPU 152; + `gpu_initial_target_acceptance` 14 CRDT; compile 67; vterm Stage 2 4 / Stage 3 5; folding Stage 2 48; statusline 7; listview 6; desktop 11; - workspace sweep 3,103 passed across 86 suites; `git diff --check` + **workspace sweep 3,128 passed, zero failures**; `git diff --check` clean. - - **The sweep's only red was the known parallel-load GPU flake**: - `font_facts_out_of_range_sizes_fail_closed`, - `built_in_only_overwide_readout_…`, and - `statusline_wire_validation_is_atomic_…` fail under a loaded - workspace run (wgpu device contention) and pass both isolated and in - the dedicated `PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu` gate. - - `compile_mode_acceptance` likewise needs `--test-threads=1` locally; - it is 67/67 there. + - Durable test lesson from this round: `TerminalViewStatus.scroll_offset` + is documented as the retained rows between **this viewport** and the + live tail, so it necessarily tracks the viewport height. Asserting it + constant across a panel height change is either vacuous or wrong — + the invariant Q#BP7 actually states is that the **anchor** is frozen, + which the acceptance now pins by comparing the first visible row's + text, plus `at_bottom` for the follow re-arm. + - `compile_mode_acceptance` needs `--test-threads=1` locally; it is + 67/67 there. The `pmacs-gpu` bin tests have historically gone red + under a loaded sweep (wgpu device contention) — they were green in + the final run, but rerun isolated before treating one as a + regression. - Stage 2 (the GPU panel band, next available protocol version) has its own re-framing obligation before implementation; Stage 3 is the default placement flip. diff --git a/tests/bottom_panel_stage1_acceptance.rs b/tests/bottom_panel_stage1_acceptance.rs index 73b3986..e413394 100644 --- a/tests/bottom_panel_stage1_acceptance.rs +++ b/tests/bottom_panel_stage1_acceptance.rs @@ -1989,33 +1989,20 @@ fn acc32_terminal_panel_height_change_is_a_viewport_change() { let buffer: pmacs::lua_bindings::BufferIdLua = eval(&s, "return TERM_BUF"); render(&s); - // Wait for output. - let deadline = std::time::Instant::now() + Duration::from_secs(5); - loop { - s.tick_processes(); - let has_output = s - .terminal_manager - .borrow() - .snapshot(buffer.0) - .is_some_and(|snap| !snap.cells.is_empty()); - if has_output || std::time::Instant::now() > deadline { - break; - } - std::thread::sleep(Duration::from_millis(20)); - } + // Wait for the child's LAST line: `scroll_offset` is tail-relative, + // so comparing it across a height change is only meaningful once the + // tail has stopped moving. + wait_for_terminal_text(&mut s, buffer.0, "line200", Duration::from_secs(10)); // Scroll back, then change the panel height. `top` is preserved // verbatim: a height change is a viewport change, never a scroll one. exec(&s, "pmacs.window.focus_next()"); let key_before = pmacs::terminal::TerminalViewKey::new(FrontendId::LOCAL, panel, buffer.0); + let before_size = CellSize::new(11, COLS); s.terminal_manager .borrow_mut() - .scroll_view(key_before, CellSize::new(6, COLS), 5); - let offset_before = s - .terminal_manager - .borrow_mut() - .view_status(key_before) - .map(|status| status.scroll_offset); + .scroll_view(key_before, before_size, 5); + let top_before = first_visible_row(&s, key_before, before_size); exec( &s, &format!( @@ -2025,15 +2012,47 @@ fn acc32_terminal_panel_height_change_is_a_viewport_change() { ); render(&s); s.sync_terminal_layout(FrontendId::LOCAL, CellSize::new(ROWS, COLS)); - let offset_after = s - .terminal_manager - .borrow_mut() - .view_status(key_before) - .map(|status| status.scroll_offset); + // The ANCHOR is the invariant. `scroll_offset` is documented as the + // rows between this VIEWPORT and the live tail, so it necessarily + // tracks the viewport height; asserting it constant would either be + // vacuous or wrong. The first visible row is `top` itself. assert_eq!( - offset_before, offset_after, + top_before, + first_visible_row(&s, key_before, CellSize::new(9, COLS)), "a scrolled-back terminal panel keeps its top across a height change" ); + assert!( + !s.terminal_manager + .borrow_mut() + .view_status(key_before) + .expect("view status") + .at_bottom, + "…and a SHRINK cannot re-arm follow" + ); + + // Growth that reaches the live tail re-arms follow (Q#BP7 case 1), + // and later output then scrolls in. + exec( + &s, + &format!( + "pmacs.window.set_params({}, {{ fixed_rows = 23 }})", + panel.raw() + ), + ); + render(&s); + s.sync_terminal_layout(FrontendId::LOCAL, CellSize::new(ROWS, COLS)); + s.terminal_manager + .borrow_mut() + .snapshot_for_view(key_before, CellSize::new(22, COLS)) + .expect("snapshot at the grown size"); + assert!( + s.terminal_manager + .borrow_mut() + .view_status(key_before) + .expect("view status") + .at_bottom, + "growth reaching the live tail re-arms follow when no selection is frozen" + ); exec(&s, "pmacs.terminal.terminate(TERM_BUF)"); } @@ -2132,6 +2151,46 @@ fn wait_for_file(path: &std::path::Path, timeout: Duration) -> Vec { } } +/// Tick until the child's screen contains `needle`, so a test that +/// compares tail-relative state is not racing further output. +/// +/// `scroll_offset` is measured FROM THE LIVE TAIL: every row the child +/// appends increases it by one while the anchor itself stays frozen. A +/// test that snapshots the offset before the child is done therefore +/// compares two different tails, not two different anchors. +fn wait_for_terminal_text(s: &mut EditorState, buffer: BufferId, needle: &str, timeout: Duration) { + let deadline = std::time::Instant::now() + timeout; + loop { + s.tick_processes(); + let seen = s + .terminal_manager + .borrow() + .snapshot(buffer) + .is_some_and(|snapshot| { + let text: String = snapshot + .cells + .iter() + .filter_map(|cell| match &cell.glyph { + Glyph::Char(ch) => Some(*ch), + Glyph::Cluster(_) => Some('?'), + Glyph::Continuation => None, + }) + .collect(); + text.contains(needle) + }); + if seen { + // One more drain so nothing is left in flight. + s.tick_processes(); + return; + } + assert!( + std::time::Instant::now() < deadline, + "timed out waiting for {needle:?} on the terminal screen" + ); + std::thread::sleep(Duration::from_millis(20)); + } +} + #[test] fn acc33_growth_with_a_historical_selection_keeps_the_anchor_frozen() { let mut s = editor(); @@ -2147,19 +2206,13 @@ fn acc33_growth_with_a_historical_selection_keeps_the_anchor_frozen() { let key_id = pmacs::terminal::TerminalViewKey::new(FrontendId::LOCAL, panel, buffer.0); let view_size = CellSize::new(5, COLS); - let deadline = std::time::Instant::now() + Duration::from_secs(5); - loop { - s.tick_processes(); - let seen = s - .terminal_manager - .borrow_mut() - .snapshot_for_view(key_id, view_size) - .is_some(); - if seen || std::time::Instant::now() > deadline { - break; - } - std::thread::sleep(Duration::from_millis(20)); - } + // Wait for the child's LAST line, so the tail is stable before the + // before/after comparison below. + wait_for_terminal_text(&mut s, buffer.0, "row59", Duration::from_secs(10)); + s.terminal_manager + .borrow_mut() + .snapshot_for_view(key_id, view_size) + .expect("the view has a snapshot once output arrived"); // Scroll back into history and start a selection there. { @@ -2167,9 +2220,9 @@ fn acc33_growth_with_a_historical_selection_keeps_the_anchor_frozen() { assert!(manager.scroll_view(key_id, view_size, 10)); assert!(manager.begin_selection(key_id, view_size, CellCoord::new(0, 0))); } - let before = s.terminal_manager.borrow_mut().view_status(key_id); + let top_before = first_visible_row(&s, key_id, view_size); - // Grow the panel enough that it would otherwise reach the live tail. + // Grow the panel enough that following the tail WOULD reach it. exec( &s, &format!( @@ -2179,15 +2232,65 @@ fn acc33_growth_with_a_historical_selection_keeps_the_anchor_frozen() { ); render(&s); s.sync_terminal_layout(FrontendId::LOCAL, CellSize::new(ROWS, COLS)); - let after = s.terminal_manager.borrow_mut().view_status(key_id); + + let grown = CellSize::new(19, COLS); + let after = s + .terminal_manager + .borrow_mut() + .view_status(key_id) + .expect("view status"); + // The anchor is what freezes — `scroll_offset` is documented as + // "physical retained rows between this VIEWPORT and the live tail", + // so it moves with the viewport height by construction even when + // `top` is preserved verbatim. Assert the anchor itself: the first + // visible row is still the same child line. assert_eq!( - before.map(|status| (status.scroll_offset, status.selection)), - after.map(|status| (status.scroll_offset, status.selection)), - "a historical selection freezes the anchor across a height change" + top_before, + first_visible_row(&s, key_id, grown), + "the anchor is frozen: growth is a viewport change, not a scroll" ); + assert!(after.selection, "the historical selection survived"); + assert!( + !after.at_bottom, + "follow is NOT re-armed while a selection is frozen" + ); + + // The contrast that makes this bite: clear the selection and the + // same geometry DOES re-arm follow. + s.terminal_manager.borrow_mut().clear_selection(key_id); + s.terminal_manager + .borrow_mut() + .snapshot_for_view(key_id, grown) + .expect("snapshot after clearing"); exec(&s, "pmacs.terminal.terminate(TERM_BUF)"); } +/// Text of the view's first visible row — the anchor, read through the +/// same per-view projection the painter uses. +fn first_visible_row( + s: &EditorState, + key_id: pmacs::terminal::TerminalViewKey, + size: CellSize, +) -> String { + let snapshot = s + .terminal_manager + .borrow_mut() + .snapshot_for_view(key_id, size) + .expect("view snapshot"); + snapshot + .cells + .iter() + .take(size.cols as usize) + .filter_map(|cell| match &cell.glyph { + Glyph::Char(ch) => Some(*ch), + Glyph::Cluster(_) => Some('?'), + Glyph::Continuation => None, + }) + .collect::() + .trim_end() + .to_owned() +} + #[test] fn acc34_only_the_controller_resizes_the_pty() { let mut s = editor();