diff --git a/docs/active-work.md b/docs/active-work.md index 8ee9881..38955a9 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -225,6 +225,77 @@ are the lanes this one creates, not work it does. Retiring the CI-CRDT, Distribution, or reap-ledger lanes: each still owns undone work and rule 4 does not apply to them. +## Honoring `full_grid` (QoL Stage 1) — PR #219 OPEN + +**PR #219** — https://github.com/levineuwirth/pmacs/pull/219. **This +block was written with the lane's first commit, before the PR +existed** — the standing correction from #171 and #215 — so the row +below was filled in rather than invented. + +- **Branch `full-grid-resync`**, base `githubsucks/main` @ `da56bec` + (the #218 merge). `githubsucks/full-grid-resync` is the + authoritative tip; any edit to this block advances past whatever SHA + it records. Recover with `git fetch githubsucks && git checkout + full-grid-resync`. +- **Framing `docs/full-grid-resync-framing.md` revision 2**, approved + with **Q#FG1 = A**: the sole grid consumer honors the flag by + resetting style, clearing, then applying spans. +- **First of three QoL stages**, from daily-driver use. Stage 2 is GUI + zoom (the machinery exists — `FontMetrics::scale` already derives + every dimension and is driven by an attach message in centi-pixels; + it is unbound and unpersisted). Stage 3 is long-line wrap/scroll, + which is a design round: **no horizontal viewport exists at all** + (`view_left` / `col_offset` / `hscroll` match nothing in `src/` or + `pmacs-gpu/src/`). Separate branches on purpose — Stage 1 is a + contained fix and must not wait behind Stage 3's design. + +### What it ships + +`FG-INV` moves onto the protocol type, where consumer authors read it: +a `full_grid: true` delta carries only the frame's **non-default** +cells, so a consumer MUST blank its surface first. The rule already +existed — in the doc comment of a **private field** on the producer's +struct (`src/instance_render.rs:36`), which is why the one consumer +never honored it. + +`emit_cell_delta` joins the existing pure escape-sequence helpers in +`src/frontend.rs` (`emit_span`, `emit_status_overlay`, …), and +`apply_message` routes through it. + +### Why a green suite missed it for so long + +Seven tests cover the flag. Every one asserts the **producer sets it**; +none asserted a **consumer acts on it**, and no runtime reader existed +anywhere in the workspace. "Add a test for the flag" had already been +done. This is handoff §5's *enforcement and documentation drift apart +silently* in a second register, and it is why the fix ships the +contract and the consumer together. + +### Verification + +- Three unit witnesses, all bitten: order (reset → clear → spans), + **empty spans still clear**, and a differential frame clears never. + The empty-spans case discriminates on its own — under the plausible + `spans.is_empty()` early return the order test still passes and only + that one fails. +- PTY acceptance (`tests/full_grid_resync_acceptance.rs`) drives a real + `SIGWINCH`. **The mark is anchored to content, not time**: a + time-based settle cannot work because a settled pmacs screen emits + per-frame bytes forever. Bitten against the original defect: 34,831 + bytes after the first painted frame with no `CSI 2 J`. +- **What it does not prove:** the suites assert on raw bytes; there is + no screen model and no `vt100`/`termwiz`/`vte` dependency. This shows + pmacs *emitted* a blank at the right moment, not that the screen + ended correct. A terminal emulator in test deps is a candidate, not + smuggled in here. + +### Not in scope + +Stages 2 and 3. The `pmacs.terminal` child-PTY `SIGWINCH` path. Any +change to *when* `needs_full_grid` is set — the producer's triggers +were verified correct, along with per-frame geometry sync and +`view_top` reconciliation on shrink. + ## Tree primitive (P5) — MERGED as #217; adoption is the open work **The lane is gone, not the work.** Rule 4 removes a lane after merge, diff --git a/docs/full-grid-resync-framing.md b/docs/full-grid-resync-framing.md new file mode 100644 index 0000000..22cd30b --- /dev/null +++ b/docs/full-grid-resync-framing.md @@ -0,0 +1,331 @@ +# Honoring `full_grid` — QoL Stage 1 + +**Status: revision 2 — APPROVED, IMPLEMENTED, in review as PR #219.** +Q#FG1 was decided **A**: the sole grid consumer honors the flag by +resetting style, clearing, then applying spans. Reported from +daily-driver use: +zooming a terminal with `Ctrl +/-` leaves the TUI visibly broken — +stale glyphs where content should be blank, and dead regions where +content should be. + +This is Stage 1 of three. Stage 2 is GUI zoom; Stage 3 is long-line +wrap/scroll. **They are deliberately separate branches** — this one is +a contained correctness fix with a mechanism already established, and +Stage 3 is a design round that touches `view_top`'s missing sibling +everywhere. Bundling them would hold a daily-driver fix behind a +design. + +**Revision 2** folds in five review corrections: the invariant belongs +on the protocol type and updating it is in scope (§1.1a, FG-INV); the +PTY witness must be suffix-scoped or the startup clear satisfies it +against a broken build (§5.2); the unit witnesses must include a +`full_grid` message with **empty spans** (§5.1); the standalone TUI +reaches `present_messages` directly and the earlier draft cited only +the attach route (§3); and the claim that the 19 frontend tests assert +on state rather than output was **wrong** — most assert on output, +which makes the proposed seam a continuation of the house idiom rather +than a new one (§5). §6 now cites the concern it serves, not only the +§20 checklist. + +--- + +## 1. The mechanism, established rather than suspected + +`src/instance_render.rs:137` builds the full-grid resync by diffing +against a **blank** grid: + +```rust +let spans = if self.needs_full_grid { + let blank = vec![Cell::default(); self.next.len()]; + diff(&blank, &self.next, self.size.cols, self.size) +} else { + diff(&self.prev, &self.next, self.size.cols, self.size) +}; +``` + +A cell that *should be blank* is equal to its blank counterpart, so it +**produces no span**. The resync therefore carries only non-default +cells — which is exactly what its comment says it intends. + +`src/frontend.rs:326` then receives: + +```rust +InstanceMessage::CellDelta { spans, .. } => { + for span in spans { emit_span(&mut self.out, span)?; } +} +``` + +**`full_grid` is destructured away and discarded.** Nothing clears. + +So after a font zoom: the terminal reflows its own content, pmacs +repaints only its non-blank cells, and every position pmacs considers +blank keeps whatever the terminal left there. That is both reported +symptoms from one cause — leftovers where pmacs is blank, dead regions +where the terminal's own reflow already lost content. + +### 1.1 Why this survived a green suite + +The flag is **documented** (`pmacs-protocol/src/message.rs:651`: "the +initial sync sent on fresh attach *or after a resize where the previous +grid is no longer applicable*") and **tested** — +`first_frame_is_full_grid_sync`, `resize_reallocates_and_flags_full_grid`, +`instance_message_cell_delta_carries_full_grid_flag`, and four more. + +Every one of them asserts the **producer sets the flag**. Not one +asserts a **consumer acts on it**. A workspace-wide search finds no +runtime reader at all: the only other `CellDelta` match outside the +producer and its tests is `pmacs-gpu/src/main.rs:10486`, which uses it +to build a debug label. + +### 1.1a The rule exists — in the wrong place + +The binding invariant is already written down, in the doc comment of a +**private field** (`src/instance_render.rs:36`): + +> *"Remote frontends use this flag to know they must blank their local +> buffer before applying the deltas."* + +That is the real contract, and it is stronger than what the protocol +says. `pmacs-protocol/src/message.rs:651` describes `full_grid` only as +*"the initial sync sent on fresh attach … versus an incremental +frame"* — a **label**, from which no consumer obligation follows. A +consumer author reads the protocol type, not a private field on the +producer's struct. + +**So updating that protocol documentation is in scope**, and the +invariant is stated here as the thing the code must satisfy: + +> **FG-INV.** A `CellDelta` with `full_grid: true` carries **only +> non-default cells**. It is not a picture of the screen; it is a +> picture of the screen's *ink*. A consumer MUST blank its surface +> before applying those spans, or every cell the resync considers blank +> retains whatever was there before. + +Writing it on the protocol type is what stops the drift recurring: the +producer's field comment cannot reach whoever writes the next frontend. + +This is the handoff §5 lesson landed in #218 — *enforcement and +documentation drift apart silently; only the enforcement is real* — +recurring in a different register. Worth stating plainly in the lane, +because "add a test for the flag" was already done and did not help. + +### 1.2 Why it is specifically *zoom* + +`src/frontend.rs:166` clears **once**, at startup: + +```rust +queue!(me.out, EnterAlternateScreen, Clear(ClearType::All), cursor::Hide, …) +``` + +So "diff against blank" is correct exactly once — on the fresh-attach +frame, where the screen provably *is* blank — and wrong for every +resize after, where nothing has cleared. The original design is not +careless; it is correct for the case it was written for and was never +extended to the second case its own doc comment names. + +A font-size change is the worst version because the terminal reflows +content *in place* rather than dropping it, maximizing the stale glyphs +that survive. + +--- + +## 2. What was eliminated, so the lane does not re-investigate + +Three plausible causes were checked and are **not** it: + +| hypothesis | verdict | evidence | +|---|---|---| +| Panel/frame geometry goes stale | **no** | `sync_frame_geometry` runs per frame inside `paint_frame` (`src/editor.rs:4412`) against the live `term_size` | +| Render buffers are not reallocated | **no** | `RenderState::resize` reallocates `prev`/`next` and sets `needs_full_grid` | +| `view_top` is not reconciled on shrink | **no** | probed directly: cursor at line 150, painting at 40 rows gives `view_top=113` / cursor row 37; at 8 rows gives `view_top=145` / cursor row 5. Correct both times | + +The probe was scratch and is not kept — it proved a negative, and a +test that asserts working behaviour nobody is changing is upkeep +without a customer. + +--- + +## 3. Q#FG1 — consumer honors the flag, or producer emits everything? + +**Two designs, and the count of consumers decides it.** + +- **A — the consumer clears when `full_grid` is true**, then applies + spans. Protocol meaning: *the resync assumes a blank surface, and the + flag is the instruction to produce one.* +- **B — the producer emits every cell** on a resync, blanks included, + so no consumer needs to change. + +The instinct was that A is the trap this project has already hit twice +— the same predicate living in `wait_for_file`, then +`wait_for_published_file`, then a third copy in +`bottom_panel_stage1_acceptance` (R4, R6). Fixing N consumers +independently is how that happens. + +**But there is only one consumer.** Both TUI paths land on +`Frontend::present_messages`: the standalone run loop calls it +**directly** (`src/editor.rs:3911`), and the attached TUI reaches the +same method through its own `present_messages` impl +(`src/attach.rs:397`). One implementation, two routes — the earlier +draft cited only the second, which understated how direct the +standalone path is. The GPU frontend is a +**semantic** frontend and never consumes these spans at all — which is +independently consistent with the report that the GUI does not +mis-render on zoom, it simply ignores zoom. + +**Recommendation: A.** With one consumer the trap does not apply, and A +is the better answer on the merits: + +- The flag **exists solely to be acted on**. Under B it stays unread — + and a flag nobody reads is the defect, not the fix. B would leave + the next reader with the same puzzle plus more bandwidth. +- B ships a full grid of mostly-blank spans over the daemon socket on + every resize, to avoid a one-line clear. +- A makes the protocol's documented sentence true, rather than working + around it. + +**Q#FG1 — DECIDED: A**, on approval. The consumer honors the flag; +`full_grid` keeps its sparse-resync meaning and the producer is +unchanged. + +*Implementation note, recorded because it confirmed the reasoning +rather than merely following it:* the fix is `emit_cell_delta` beside +`emit_span` and `emit_status_overlay` — no struct change, no generic +parameter, no new pattern. B would have touched the producer, changed +what every resync costs on the wire, and left the flag unread. + +--- + +## 4. Q#FG2 — what "clear" must include + +Not just `Clear(ClearType::All)`. The clear paints with the *current* +background, so it must be preceded by `ResetColor` and +`SetAttribute(Attribute::Reset)`, or a resync taken while a styled span +was last emitted will wash the screen in that style. + +`present_messages` already brackets its output in +`BeginSynchronizedUpdate` / `EndSynchronizedUpdate` (`src/frontend.rs:223`, +`483`, `502`), so clear-then-repaint lands atomically and does not +flicker. **No new synchronization is needed** — this is why the fix is +small. + +--- + +## 5. Verification + +`out` is a concrete `BufWriter` (`src/frontend.rs:126`), so +`Frontend::apply_message` cannot be observed directly. **The seam is +already the house idiom in this file**, though — `emit_span` is a free +function over a writer, and the tests around +`src/frontend.rs:870`–`1037` capture into `Vec` and assert on the +exact escape sequences (`src/frontend.rs:587` says so in as many +words: *"Tested below by capturing into a `Vec`"*). + +*An earlier draft said those 19 tests "assert on state rather than +output". That was wrong — most of them assert on output, which makes +the seam a continuation rather than an introduction.* + +So: **add `emit_cell_delta` beside the existing helpers** and route +`apply_message` through it. No struct change, no generic parameter, no +new pattern. + +### 5.1 Unit witnesses + +1. **`full_grid: true` emits reset + clear before any span.** Order + matters and is asserted as order, not membership: a clear *after* a + span erases the frame it was meant to precede. +2. **`full_grid: true` WITH EMPTY SPANS still clears.** This is the + exact stale-blank case and the one a careless implementation + misses — an early return on `spans.is_empty()` is the obvious + "optimization", and it reintroduces the entire bug for the frame + that needs the clear most: a resync to a screen that should be + blank. +3. **`full_grid: false` emits neither**, whatever the spans are. + +### 5.1a What implementation changed about §5.2 + +**The time-based settle described below does not work**, and the +framing said to build it. A settled pmacs screen emits per-frame bytes +indefinitely, so "output stopped growing" never becomes true — the wait +simply runs to its deadline. The vterm suite already recorded the same +behaviour ("a settled screen emits empty diffs forever"); this framing +did not connect it. + +The shipped witness anchors its mark to **content** instead: just past +the first painted byte of the fixture. That excludes both of startup's +clears *by construction* rather than by timing — `Frontend::new` clears +before any frame exists, and the first frame is itself a resync whose +clear precedes its own spans — so no timing assumption survives in the +test at all. It is strictly stronger than what §5.2 asked for. + +One further correction found by running it: the repaint-ordering +assertion must be scoped to **after the new clear**. The fixture +repeats its marker, so the suffix opens with the tail of startup's own +frame, and an unscoped comparison measures the clear against paints it +was never meant to precede. + +### 5.2 PTY acceptance — as originally framed + +`tests/common/pty.rs:42` exposes `resize(rows, cols)`, so the real +scenario is reachable: spawn pmacs, put distinctive content on screen, +resize the PTY (a real `SIGWINCH`), and assert on what follows. + +**The assertion must be suffix-scoped, or it proves nothing.** +`src/frontend.rs:166` emits `Clear(ClearType::All)` at startup, so a +test that searches the whole output finds a clear **whether or not +resize handling works** — it would pass against the current broken +build. So: + +- capture `output().len()` as a mark **only after startup is known + complete** (a startup still in flight would put its own clear into + the suffix and recreate the same false pass, one step later); +- resize; +- slice **strictly after the mark**, and assert that within that + suffix, reset-color + SGR-reset + clear all appear **before the + first repaint output**. + +**A limit, stated rather than discovered in review:** the vterm suites +assert on **raw output bytes**; there is no screen model and no +`vt100` / `termwiz` / `vte` dependency in the workspace. So this proves +*pmacs emitted a clear at the right moment*, not *the screen ended up +correct*. Closing that gap means introducing a terminal emulator into +the test dependencies — **out of scope here**, and recorded as a +candidate rather than smuggled in. + +--- + +## 6. Coherence impact (§20 requirement) + +**Concern served: `COHERENCE.md` §16, "Productize the Semantic +Frontend Architecture."** §16 names *efficient incremental updates* and +*stable remote attachment* among the properties the semantic protocol +must actually deliver, and FG-INV is a rule the incremental-update +mechanism depends on. A resync that silently fails to resync is that +concern's failure mode, not a cosmetic one. + +**No scorecard change.** Row 16 reads **Strong** (`COHERENCE.md:112`), +and this lane neither earns nor forfeits that: it repairs one unhonored +invariant inside a mechanism the row already credits. Per §25 an +audited claim moves with the PR that changes it — nothing here changes +what the row asserts, so it stays put and this sentence records that +the question was asked. + +- **Journey steps touched:** none. This is rendering correctness, not + a new capability or step. +- **Interaction islands added:** none. No new keybinding, mode, or + surface. +- **Config registry:** no new settings. Stage 2's persisted zoom level + and Stage 3's wrap/scroll toggle enter it; Stage 1 has no option to + register. +- **Background-work attribution:** none. No async work. + +--- + +## 7. Not in scope + +GUI zoom (Stage 2) and long-line wrap/scroll (Stage 3). A terminal +emulator in the test dependencies. Changing what `full_grid` *means* — +FG-INV documents the existing contract on the protocol type, it does +not redefine it, and the sparse-resync design stays as is. The `pmacs.terminal` child-PTY +`SIGWINCH` path, which is a different question about a different +process. Any change to *when* `needs_full_grid` is set — the producer's +triggers are correct and verified in §2. diff --git a/pmacs-protocol/src/message.rs b/pmacs-protocol/src/message.rs index 5fd7c87..73ba142 100644 --- a/pmacs-protocol/src/message.rs +++ b/pmacs-protocol/src/message.rs @@ -648,15 +648,42 @@ pub enum GoodbyeReason { /// Rendering and signals from instance to frontend. #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize)] pub enum InstanceMessage { - /// Cell deltas. `full_grid = true` is the initial sync sent on - /// fresh attach (or after a resize where the previous grid is no - /// longer applicable); `full_grid = false` is a differential - /// frame. + /// Cell deltas. `full_grid = true` is a resync, sent on fresh + /// attach and after a resize; `full_grid = false` is a + /// differential frame. + /// + /// # FG-INV — a resync carries only the frame's *ink* + /// + /// **A consumer MUST blank its surface before applying the spans of + /// a `full_grid = true` delta.** This is an obligation, not a + /// label. + /// + /// The producer builds a resync by diffing the frame against a + /// **blank** grid, so a cell that should be blank equals its blank + /// counterpart and **produces no span**. A resync is therefore a + /// picture of the screen's *ink*, not a picture of the screen: it + /// says where the non-default cells are and says nothing whatever + /// about the rest. Apply it to a surface that was not blanked and + /// every cell the resync considers empty keeps what was underneath. + /// + /// A resync whose frame is entirely blank carries **no spans at + /// all**, and it is exactly the delta that most needs the surface + /// blanked — so "no spans, nothing to do" is wrong. + /// + /// This rule previously lived only in a doc comment on a private + /// field of the producer's struct, where no consumer author would + /// read it. The grid TUI consequently ignored the flag: correct for + /// the fresh-attach frame, which follows a `Clear`, and wrong for + /// every resize after, which follows nothing. Zooming a terminal + /// font left the previous frame showing through. CellDelta { - /// One run of changed cells per `DiffSpan`. + /// One run of changed cells per `DiffSpan`. On a resync these + /// are the frame's non-default cells **only** — see FG-INV + /// above. spans: Vec, - /// Whether `spans` represents a full-grid resync (true on - /// fresh attach or post-resize) versus an incremental frame. + /// Whether `spans` is a resync (fresh attach or post-resize) + /// versus an incremental frame. On `true` the consumer owes + /// the blanking FG-INV describes. full_grid: bool, }, /// Cursor position and visibility update. diff --git a/src/frontend.rs b/src/frontend.rs index 6b8755f..275f349 100644 --- a/src/frontend.rs +++ b/src/frontend.rs @@ -323,10 +323,8 @@ impl Frontend { /// are reserved for v0.3 (GUI / multi-frontend) and ignored here. pub fn apply_message(&mut self, msg: &InstanceMessage) -> io::Result<()> { match msg { - InstanceMessage::CellDelta { spans, .. } => { - for span in spans { - emit_span(&mut self.out, span)?; - } + InstanceMessage::CellDelta { spans, full_grid } => { + emit_cell_delta(&mut self.out, spans, *full_grid)?; } InstanceMessage::Cursor(state) => match state { Some(cs) if cs.visible => { @@ -581,6 +579,49 @@ impl Drop for Frontend { // Span emission (pure; testable) // --------------------------------------------------------------------------- +/// Emit one `CellDelta` — the resync blank, then every span. +/// +/// **FG-INV.** A `CellDelta` with `full_grid: true` carries only the +/// **non-default** cells of the frame: the producer diffs against a +/// blank grid (`crate::instance_render::RenderState::render_frame`), so +/// a cell that should be blank produces no span at all. The resync is a +/// picture of the screen's *ink*, not of the screen. A consumer that +/// applies those spans to a surface it has not blanked keeps whatever +/// was underneath every blank cell. +/// +/// That is not hypothetical. The startup path clears once +/// ([`Frontend::new`]), which made the sparse resync correct for +/// exactly one frame — the fresh-attach frame, where the screen +/// provably is blank — and wrong for every resize after it. Zooming a +/// terminal font left the reflowed previous frame showing through. +/// +/// The clear is preceded by `ResetColor` + `SetAttribute(Reset)` +/// because `Clear` paints with the *current* background: a resync taken +/// while a styled span was last emitted would otherwise wash the screen +/// in that style. +/// +/// **Empty spans still clear.** A resync whose frame is entirely blank +/// carries no spans at all, and that is precisely the frame that most +/// needs the surface blanked. Returning early on `spans.is_empty()` +/// would look like an optimization and reintroduce the whole defect. +/// +/// Pure and tested below by capturing into a `Vec`, like its +/// neighbours. +fn emit_cell_delta(w: &mut W, spans: &[DiffSpan], full_grid: bool) -> io::Result<()> { + if full_grid { + queue!( + w, + ResetColor, + SetAttribute(Attribute::Reset), + Clear(ClearType::All) + )?; + } + for span in spans { + emit_span(w, span)?; + } + Ok(()) +} + /// Emit a diff span as escape sequences to `w`. /// /// Pure: the same span produces the same byte output, regardless of any @@ -861,6 +902,74 @@ mod tests { .expect("the grid frontend must drop StatuslineSegments silently"); } + /// FG-INV, witness 1: a resync blanks the surface before it paints, + /// and the ORDER is the claim — a clear *after* a span erases the + /// frame it was meant to precede. + #[test] + fn a_resync_resets_style_and_clears_before_any_span() { + let span = DiffSpan { + start: CellCoord::new(0, 0), + cells: vec![ch('x')], + }; + let mut out = Vec::new(); + emit_cell_delta(&mut out, std::slice::from_ref(&span), true).unwrap(); + let s = String::from_utf8_lossy(&out); + + let clear = s.find("\x1b[2J").expect("resync must clear: {s:?}"); + let reset_color = s.find("\x1b[0m").expect("resync must reset: {s:?}"); + let glyph = s.find('x').expect("the span is still painted: {s:?}"); + + assert!( + reset_color < clear, + "reset must precede the clear — `Clear` paints with the CURRENT \ + background, so a resync taken mid-style would wash the screen \ + in it: {s:?}" + ); + assert!( + clear < glyph, + "the clear must precede the paint, or it erases the frame it \ + was meant to precede: {s:?}" + ); + } + + /// FG-INV, witness 2: the frame that most needs blanking carries no + /// spans at all. + /// + /// A resync of an entirely blank frame produces zero spans, because + /// the producer diffs against a blank grid. `spans.is_empty()` looks + /// exactly like "nothing to do" — and an early return there + /// reintroduces the whole defect for the one frame whose entire + /// content IS the blanking. + #[test] + fn a_resync_with_no_spans_still_clears() { + let mut out = Vec::new(); + emit_cell_delta(&mut out, &[], true).unwrap(); + let s = String::from_utf8_lossy(&out); + assert!( + s.contains("\x1b[2J"), + "an empty resync is the stale-blank case, not a no-op: {s:?}" + ); + } + + /// FG-INV, witness 3: the obligation is the flag's, not the + /// message's. A differential frame that clears would erase + /// everything it does not repaint. + #[test] + fn a_differential_frame_never_clears() { + let span = DiffSpan { + start: CellCoord::new(1, 1), + cells: vec![ch('y')], + }; + let mut out = Vec::new(); + emit_cell_delta(&mut out, std::slice::from_ref(&span), false).unwrap(); + let s = String::from_utf8_lossy(&out); + assert!( + !s.contains("\x1b[2J"), + "a differential frame must not clear: {s:?}" + ); + assert!(s.contains('y'), "but it still paints: {s:?}"); + } + #[test] fn emit_span_writes_cursor_move_then_chars() { let span = DiffSpan { diff --git a/tests/full_grid_resync_acceptance.rs b/tests/full_grid_resync_acceptance.rs new file mode 100644 index 0000000..bcfe276 --- /dev/null +++ b/tests/full_grid_resync_acceptance.rs @@ -0,0 +1,191 @@ +//! Full-grid resync acceptance — FG-INV at the real PTY boundary. +//! +//! A `CellDelta` with `full_grid: true` carries only the frame's +//! **non-default** cells: the producer diffs against a blank grid, so a +//! cell that should be blank produces no span. A consumer that applies +//! those spans to a surface it has not blanked keeps whatever was +//! underneath every blank cell. +//! +//! The grid TUI ignored the flag entirely. That was correct for exactly +//! one frame — the fresh-attach frame, which follows `Frontend::new`'s +//! `Clear` — and wrong for every resize after it, which follows +//! nothing. Zooming a terminal font left the reflowed previous frame +//! showing through. +//! +//! # Why this test is suffix-scoped, and why it settles first +//! +//! Startup emits its own `Clear(ClearType::All)`. A test that searches +//! the whole output therefore finds a clear **whether or not resize +//! handling works** — it passes against the broken build and proves +//! nothing. So the assertion is confined to bytes emitted strictly +//! after a mark. +//! +//! Taking that mark is the subtle half. Marking while startup is still +//! in flight puts startup's own clear *into the suffix* and recreates +//! the same false pass one step later. The mark is therefore anchored +//! to CONTENT, not to time: it sits just past the first painted byte of +//! the fixture, and both of startup's clears provably precede that — +//! `Frontend::new` clears before any frame exists, and the first frame +//! is itself a resync whose clear precedes its own spans. +//! +//! A time-based settle was tried first and does not work: a settled +//! pmacs screen emits per-frame bytes indefinitely, so "output stopped +//! growing" never becomes true. +//! +//! # What this proves, and what it does not +//! +//! The vterm suites assert on raw output bytes; there is no screen +//! model and no `vt100` / `termwiz` / `vte` dependency in the +//! workspace. This proves **pmacs emitted a blanking sequence at the +//! right moment**, not that the screen ended up correct. Closing that +//! gap needs a terminal emulator in the test dependencies, which is +//! deliberately out of this lane's scope. + +use std::time::{Duration, Instant}; + +#[path = "common/mod.rs"] +mod common; + +use common::pty::{PmacsPty, spawn_pmacs_in_pty}; + +/// CSI 2 J — erase the whole display. +const CLEAR_ALL: &[u8] = b"\x1b[2J"; +/// SGR 0 — reset colors and attributes. `Clear` paints with the +/// *current* background, so a resync taken mid-style would otherwise +/// wash the screen in it. +const SGR_RESET: &[u8] = b"\x1b[0m"; + +fn contains(haystack: &[u8], needle: &[u8]) -> bool { + haystack.windows(needle.len()).any(|w| w == needle) +} + +fn find(haystack: &[u8], needle: &[u8]) -> Option { + haystack.windows(needle.len()).position(|w| w == needle) +} + +/// Wait until `needle` has been painted, and return the index just past +/// its first occurrence. +/// +/// **That index is the mark, and it is anchored to CONTENT rather than +/// to time.** Startup's blanking provably precedes it: `Frontend::new` +/// clears before any frame exists, and the first frame is itself a +/// resync whose own clear precedes its own spans — so both clears are +/// behind the first byte of painted content, always, with no timing +/// assumption at all. +/// +/// A time-based settle was tried first and is wrong here: a settled +/// pmacs screen keeps emitting per-frame bytes indefinitely (the vterm +/// suite records the same thing — "a settled screen emits empty diffs +/// forever"), so "output stopped growing" never becomes true and the +/// wait cannot distinguish a finished startup from a live one. +fn mark_after_first_paint(pty: &PmacsPty, needle: &[u8], timeout: Duration) -> usize { + let deadline = Instant::now() + timeout; + loop { + let out = pty.output(); + if let Some(at) = find(&out, needle) { + let mark = at + needle.len(); + assert!( + contains(&out[..mark], CLEAR_ALL), + "premise: startup DOES blank the host before painting — \ + which is exactly why the assertion below must not be \ + allowed to see startup's bytes" + ); + return mark; + } + assert!( + Instant::now() < deadline, + "pmacs never painted {:?} within {timeout:?}; emitted {} bytes", + String::from_utf8_lossy(needle), + pty.output().len() + ); + std::thread::sleep(Duration::from_millis(20)); + } +} + +/// Wait for a blanking sequence to appear strictly after `mark`, and +/// return the suffix containing it. +/// +/// Waiting for the *clear specifically* rather than for "any new +/// output" is what makes the timeout meaningful: pmacs emits per-frame +/// bytes regardless, so "output grew" would be satisfied instantly by +/// noise and the assertion would race the resize it is meant to +/// observe. +fn suffix_with_blank_after(pty: &PmacsPty, mark: usize, timeout: Duration) -> Vec { + let deadline = Instant::now() + timeout; + loop { + let out = pty.output(); + if out.len() > mark && contains(&out[mark..], CLEAR_ALL) { + // Let the rest of the frame land so the ordering assertions + // see the whole repaint, not its first fragment. + std::thread::sleep(Duration::from_millis(200)); + return pty.output()[mark..].to_vec(); + } + if Instant::now() >= deadline { + let out = pty.output(); + let suffix = &out[mark.min(out.len())..]; + panic!( + "FG-INV: the post-resize resync must blank the host, and \ + no CSI 2 J appeared in the {} bytes emitted after the \ + first painted frame. Suffix head: {:?}", + suffix.len(), + String::from_utf8_lossy(&suffix[..suffix.len().min(400)]) + ); + } + std::thread::sleep(Duration::from_millis(20)); + } +} + +/// Resizing a real PTY makes pmacs blank the host surface before it +/// repaints. +/// +/// This is the user-visible bug: `Ctrl +/-` in a terminal changes the +/// font, the terminal reflows and emits `SIGWINCH`, and pmacs repaints +/// only its non-blank cells over content it never cleared. +#[test] +fn a_pty_resize_blanks_the_host_before_repainting() { + let dir = tempfile::tempdir().expect("tempdir"); + let file = dir.path().join("marker.txt"); + // Distinctive, and long enough that the shrink below genuinely + // changes what fits. + std::fs::write(&file, "ZQXMARKERQZ\n".repeat(40)).expect("write fixture"); + + let mut pty = spawn_pmacs_in_pty( + &[file.to_str().expect("utf-8 path")], + &[("HOME", dir.path()), ("XDG_CONFIG_HOME", dir.path())], + 24, + 80, + ); + + let mark = mark_after_first_paint(&pty, b"ZQXMARKERQZ", Duration::from_secs(20)); + + // The zoom: same window, different cell geometry. + pty.resize(12, 40).expect("resize the host PTY"); + + let suffix = suffix_with_blank_after(&pty, mark, Duration::from_secs(20)); + + let clear = find(&suffix, CLEAR_ALL).expect("waited for it above"); + let reset = find(&suffix, SGR_RESET).expect("the blank is style-reset first"); + assert!( + reset < clear, + "the reset must precede the clear — `Clear` paints with the \ + CURRENT background, so a resync taken mid-style washes the \ + screen in it" + ); + + // …and the blank is followed by the repaint it exists to precede. + // + // Scoped to AFTER the clear on purpose. The mark sits just past the + // *first* painted marker line and the fixture repeats it, so the + // suffix still opens with the tail of startup's own frame — + // comparing the clear against those would compare it against paints + // it was never supposed to precede. + assert!( + contains(&suffix[clear..], b"ZQXMARKERQZ"), + "the resync must repaint after blanking. Without this the test \ + would pass on a resize that cleared the screen and painted \ + nothing — which is the other way to have a broken frame" + ); + + let _ = pty.write_input(b"\x18\x03"); // C-x C-c + let _ = pty.wait_for_exit(Duration::from_secs(5)); +}