299 lines
13 KiB
Markdown
299 lines
13 KiB
Markdown
# Honoring `full_grid` — QoL Stage 1
|
||
|
||
**Status: revision 2 — proposed, awaiting approval.** 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 is the one decision that needs approval before implementation.**
|
||
|
||
---
|
||
|
||
## 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<Stdout>` (`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<u8>` and assert on the
|
||
exact escape sequences (`src/frontend.rs:587` says so in as many
|
||
words: *"Tested below by capturing into a `Vec<u8>`"*).
|
||
|
||
*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.2 PTY acceptance
|
||
|
||
`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.
|