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) <noreply@anthropic.com>
This commit is contained in:
parent
90fc7a913e
commit
7e1bfb6dc5
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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<u8> {
|
|||
}
|
||||
}
|
||||
|
||||
/// 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::<String>()
|
||||
.trim_end()
|
||||
.to_owned()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn acc34_only_the_controller_resizes_the_pty() {
|
||||
let mut s = editor();
|
||||
|
|
|
|||
Loading…
Reference in New Issue