From 1c9ff6a033655f8812b6aac44cdbfe335409a1d8 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 6 Aug 2026 22:38:51 +0200 Subject: [PATCH] docs: the GPU had no wire, and "everything is local" was false Two holes in revision 14, both found by review of this lane's own first commit, and both would have been discovered during implementation at much greater cost. Q#LL7 --- the mode never reached the GPU. Section 4 resolves ui.line-wrap into Viewport, which reaches the GRID renderer. The GPU is not a grid consumer: it lays out locally, ignores CellDelta, BufferSnapshot carries only CRDT bytes, and no InstanceMessage variant expresses a wrap mode. So truncate would have changed the TUI and left the GPU wrapping --- the two frontends still disagreeing, which is the one thing this lane exists to fix. Q#LL5's "character wrap in both" was equally unreachable: setting Wrap::Glyph at startup is not honoring a mode that can change. Specified as an additive variant at v22, appended after the current final variant with the advertised baseline left at 20 --- the path FontFacts took at v17 and the panel shapes at v21, and the baseline constant's own doc reserves moving it for changes that cannot be expressed additively. This one can. It carries buffer_id, and is resent on attach, on config change, AND on buffer switch. The third trigger is the one a FontFacts-shaped design misses: font size is global, wrap mode is per buffer, so switching from a truncate buffer to a wrap buffer changes the effective mode with no config event at all. A design listening only to on_change is silently wrong and passes every single-buffer test. Q#LL8 --- the scroll indicator falsifies the locality claim. Revision 14 asserted every vertical consumer is local. format_scroll_indicator is not: a one-line buffer wrapping to fifty screen rows has total_lines == 1, so the first branch returns "All" while forty-nine rows sit below the viewport. The indicator claims the whole buffer is visible when almost none of it is. The claim is narrowed rather than abandoned, because the distinction that bounds the cost survives: a TOTAL is one number, lazily computed and cached; a PREFIX-SUM INDEX is O(N) resident storage. Stage 3 needs the first and still does not need the second. All, Top and Bot need no aggregate at all --- each is a local predicate falling out of the render walk --- so only NN% pays, which matters because the M1 gate measures open time on a 100MB file. Also fixes a notation hazard. Section 7 said pos_to_display returns "the visual row", which reads as redefining row --- the exact thing 5b.5 forbids, stated two sections apart. Every wrap-point example is now the explicit triple {row, sub_row, col}. Writing them out makes the point visible: both coordinates at a soft break share the same row, because a wrap does not cross a source line. That is the information a redefinition would have destroyed, and the pair notation hid it. Status returns to not-approved. Both questions change what gets built, not how, so implementation waits on them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 24 +++- docs/long-lines-framing.md | 234 ++++++++++++++++++++++++++++++++++--- 2 files changed, 237 insertions(+), 21 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 7346389..206d1fb 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -306,8 +306,10 @@ records. Recover: `git fetch githubsucks && git checkout long-lines`. **This block was written with the lane's first commit, before any PR exists** — the standing correction from #171, #215 and #220. -- **Framing `docs/long-lines-framing.md` revision 14, APPROVED**, after - **eleven** revisions of review. All six questions answered. +- **Framing `docs/long-lines-framing.md` revision 15.** Q#LL1-LL6 + answered; **Q#LL7 and Q#LL8 raised by review of this lane's first + commit and not yet approved** --- implementation is blocked on them, + because both change what gets built rather than how. - **Independent of #220.** Stage 2 (GUI zoom) and Stage 3 share no code, so this branch does not wait on that merge — which matters, because #220 is blocked on a GitHub Actions outage, not on itself. @@ -339,6 +341,24 @@ for the user to express a preference in either. layout is per-line); `view_top`'s sub-line component is a **byte**; `DisplayCoord` gains `sub_row` rather than redefining `row`. +### The two holes review found in the first commit + +- **Q#LL7 --- the GPU had no wire.** The mode resolved into `Viewport`, + which reaches only the *grid* renderer; the GPU lays out locally and + no message expresses a wrap mode. `truncate` would have changed the + TUI and left the GPU wrapping --- the exact disagreement this lane + exists to close. Now specified as an additive v22 variant carrying + `buffer_id`, resent on attach, config change, **and buffer switch**. + That third trigger is the subtle one: font size is global, wrap mode + is per buffer, so a `FontFacts`-shaped design is silently wrong and + looks right in every single-buffer test. +- **Q#LL8 --- "every vertical consumer is local" was false.** The + scroll indicator needs a total. A one-line buffer wrapping to fifty + rows has `total_lines == 1`, so the indicator reports `All` while + forty-nine rows sit off-screen. The bounded distinction survives: a + *total* is one lazily-computed number; an *index* is `O(N)` resident + storage, still ruled out. + ### The two decisions most likely to be questioned later - **GUI users lose word wrap.** Character-wrap parity is cheap and diff --git a/docs/long-lines-framing.md b/docs/long-lines-framing.md index f36d5b1..a41fd68 100644 --- a/docs/long-lines-framing.md +++ b/docs/long-lines-framing.md @@ -1,7 +1,8 @@ # Long lines — QoL Stage 3 -**Status: revision 14 — APPROVED to branch (2026-08-06). All six -questions answered. Not yet implemented.** +**Status: revision 15 — branched as `long-lines`; Q#LL1–LL6 answered; +Q#LL7 and Q#LL8 raised by review of the lane's first commit and NOT yet +approved. Not yet implemented.** **Revision 2** corrected a load-bearing error in revision 1: it claimed both frontends render from the same `CellGrid`. They do not — the GPU @@ -177,7 +178,7 @@ anchor** — `scroll_top` plus `code_scroll_residual`, renormalized by `normalize_code_scroll` (framing Q#F6) when reflow pushes the residual across source lines. The shape is precedent, not invention. -**Revision 14 — the current one.** Answers the remaining four questions +**Revision 14** — answers the remaining four questions and moves the document to APPROVED. **Q#LL2** (§4): buffer-local, with `Viewport` carrying the *resolved* @@ -195,6 +196,35 @@ is worse than honest divergence. **Q#LL6 items 1-2** (§5b.4): breaking on the input side so the compiler enumerates the audit, additive on the output side so untouched consumers stay correct. +**Revision 15 — the current one.** Review of `bd752f2` found two holes +and one notation hazard. + +**Q#LL7 (§5c) — the GPU had no wire.** §4 resolves the mode into +`Viewport`, which reaches the *grid*. The GPU is not a grid consumer: +it lays out locally, `BufferSnapshot` carries only CRDT bytes, and no +message expresses a wrap mode. So `truncate` would have changed the TUI +and left the GPU wrapping — the exact disagreement this lane closes. +Specified as an additive variant at v22 (advertised baseline unmoved), +carrying `buffer_id` because the mode is buffer-local, resent on +attach, on config change, **and on buffer switch** — the third being +the one a `FontFacts`-shaped design misses, since font size is global +while wrap mode is per buffer. + +**Q#LL8 (§5d) — "every vertical consumer is local" was false.** The +scroll indicator needs a total: a one-line buffer wrapping to fifty +rows has `total_lines == 1`, so `format_scroll_indicator` returns +`All` while forty-nine rows sit off-screen. §5b.5 is narrowed +accordingly, keeping the distinction that bounds the cost — a *total* +is one lazily-computed number, an *index* is `O(N)` resident. `All`, +`Top` and `Bot` need no aggregate at all; only `NN%` does. + +**Notation (§7).** Revision 14 said `pos_to_display` returns "the +visual row", which reads as redefining `row` — the thing §5b.5 +forbids. Every wrap-point example is now the explicit triple +`{row, sub_row, col}`, and both coordinates at a soft break share the +same `row`, which is the information a redefinition would have +destroyed. + Drafted while GitHub Actions was in a major outage and #220 could not merge. Nothing here depends on #220 landing; the two lanes touch no common code. @@ -873,11 +903,20 @@ memory and an `O(N)` rebuild on every width change --- against an M1 gate that includes `open_100mb_under_200ms`. That is a real perf risk and §5b.7's "authoritative map" framing invited it. -It is not required, because every vertical consumer is **local**: -rendering walks forward from `view_top` bounded by viewport height; -`move_down`/`move_up` need one step; paging needs viewport-height rows; -the wheel needs *n* rows from `view_top`. Nothing asks for the absolute -visual row of line 40,000, and nothing indexes by one. +No **index** is required, because every *positioning* consumer is +local: rendering walks forward from `view_top` bounded by viewport +height; `move_down`/`move_up` need one step; paging needs +viewport-height rows; the wheel needs *n* rows from `view_top`. Nothing +asks for the absolute visual row of line 40,000, and nothing **indexes** +by one. + +**Revision 14 overstated this as "every vertical consumer is local", +and that is false.** Review of `bd752f2` found the counterexample: the +scroll indicator needs a **total**. See §5d --- and note the +distinction that survives, because it is what keeps the cost bounded: a +*total* is one number, computable lazily and cacheable; a *prefix-sum +index* is `O(N)` resident storage. Stage 3 needs the former and still +does not need the latter. So "the map" is two per-line functions --- how many rows this line occupies at this width, and which row a given byte falls on --- plus @@ -987,6 +1026,146 @@ its current behavior is provably unchanged. --- +## 5c. Q#LL7 --- the GPU needs a wire message, and revision 14 had none + +**Raised in review of `bd752f2`, and it is a hole in the lane's central +claim.** §4 resolves `ui.line-wrap` into `Viewport`, which reaches the +**grid** renderer. The GPU is not a grid consumer (§1.2): it lays out +locally and ignores `CellDelta`. `BufferSnapshot` carries only CRDT +bytes (`pmacs-protocol/src/message.rs:777`), and no `InstanceMessage` +variant expresses a wrap mode. + +So as framed through revision 14, `ui.line-wrap = "truncate"` would +change the TUI and **leave the GPU wrapping** --- the two frontends +still disagreeing, which is the exact defect this lane exists to close. +Q#LL5's "character wrap in both" is likewise unreachable without a +wire: setting `Wrap::Glyph` at GPU startup is not the same as honoring +a mode that can change. + +### 5c.1 The message + +**Additive variant, appended after the current final `InstanceMessage` +variant; `PROTOCOL_VERSION` 21 -> 22; `ADVERTISED_PROTOCOL_VERSION` +stays 20.** This is the path `FontFacts` took at v17 and the panel +shapes took at v21, and the constant's own doc reserves moving the +advertised baseline for changes "that cannot be expressed additively" +--- this one can. + +It carries `buffer_id` alongside the mode. **Not optional: the mode is +buffer-local (§4)**, so "the current mode" is meaningless without +naming the buffer it belongs to, and the GPU tracks +`current_buffer_id` already. + +### 5c.2 Resend semantics --- the part most likely to be got wrong + +The mode must reach the GPU on **all three** of: + +1. **Attach**, for the initially-shown buffer, as part of the same + initial-state burst that establishes font facts. A frontend that + attaches to an existing session must not have to wait for a change + to learn the current mode. +2. **Config change**, via the registry's `on_change` --- for every + attached frontend showing that buffer. +3. **Buffer switch.** This is the one a `FontFacts`-shaped design + misses. Font size is global; **wrap mode is per buffer**, so + switching from a buffer set to `truncate` to one left at `wrap` + changes the effective mode with **no config event at all**. A + design that only listens to `on_change` is silently wrong here, and + would look correct in every single-buffer test. + +### 5c.3 GPU behavior on receipt + +Set `Wrap::Glyph` (mode `wrap`) or `Wrap::None` (mode `truncate`) on +the **document** buffer --- its first explicit `set_wrap` either way +(§1.2) --- then reshape and **renormalize the scroll anchor** through +`normalize_code_scroll` (`pmacs-gpu/src/main.rs:7955`). That path +already exists for exactly this situation: reflow moving the retained +residual across source lines. Changing wrap mode reflows the whole +document, so it is the same event class as a font-size change, and must +reuse that repair rather than reimplement it. + +An out-of-range or unknown mode value is **rejected as a whole +message**, matching `apply_font_facts` rather than clamping --- the +convention Stage 2 followed (`docs/gui-zoom-framing.md`). + +### 5c.4 Older frontends + +A v21-or-older frontend never receives the variant and keeps wrapping. +That is a **documented divergence**, not a silent one: the guarantee +"both frontends agree" holds for peers that negotiated v22, and the +release notes must say so alongside the word-wrap regression (§5a). + +--- + +## 5d. Q#LL8 --- the scroll indicator, which falsifies "everything is local" + +**Raised in review of `bd752f2`.** `format_scroll_indicator` +(`src/editor.rs:5509`) reckons `All`/`Top`/`Bot`/`NN%` from +`total_lines`, fed in visible-line space (`src/editor.rs:4336`, Arc 6 +Q#FD18) so a collapsed remainder correctly reads `All`. + +Under `wrap` that is wrong in a way a user sees immediately. **A +one-line buffer wrapping to fifty screen rows has `total_lines == 1`, +so the very first branch --- `if total_lines <= 1 { return "All" }` --- +reports `All` while forty-nine rows sit below the viewport.** The +indicator claims the whole buffer is on screen when almost none of it +is. + +### 5d.1 The contract + +The indicator is reckoned in **visual rows** whenever the mode is +`wrap`, and in visible lines under `truncate` --- where the two +coincide, so `truncate` remains exactly today's behavior, consistent +with §5b.5's identity-case strategy. + +- `All` --- every visual row of the buffer is on screen. +- `Top` --- the first visual row is on screen and `All` does not hold. +- `Bot` --- the last visual row is on screen and `All` does not hold. +- `NN%` --- the cursor's **visual row ordinal** as a percent of the + buffer's total visual rows. + +### 5d.2 What must be computed, and what must not + +**`All` / `Top` / `Bot` need no aggregate.** Each is a local predicate: +is the first visual row on screen (`view_top` byte == first visible +byte), and is the last one (does the forward walk from `view_top` reach +the buffer end within the viewport)? Both fall out of the render walk +that already happens. **Only `NN%` needs a total**, which matters +because `All`/`Top`/`Bot` are the states a user reads most and the +common cases stay `O(viewport)`. + +The total may be computed **lazily and cached**, keyed by buffer +generation, width and mode, and invalidated by any of the three. It +composes with folds by counting rows only for lines the fold map +vouches as visible (§5b.3). + +**It must not become a resident prefix-sum index** --- that is the +`O(N)` storage §5b.5 rules out, and the distinction is exactly one +number versus one number per line. + +**The `open_100mb_under_200ms` gate (M1) constrains this.** Computing +total visual rows means laying out every line, so it must not happen on +open, on every frame, or on any path the gate measures --- only on +first `NN%` paint after an invalidation. If that proves too slow on +large buffers, the fallback is to report a **byte-based** percentage +under `wrap` and say so; what is not acceptable is today's silent +`All`. + +### 5d.3 Verification + +- **The reported case, as a direct witness:** one source line, viewport + shorter than its wrapped height, mode `wrap` --- the indicator must + **not** be `All`. This fails against revision 14's design, which is + what makes it worth writing first. +- `Top` at the buffer start, `Bot` at the end, `All` only when every + visual row fits --- each with a wrapped line present. +- **A `truncate` control** asserting the indicator is byte-identical to + today's output for the same buffer and viewport. +- A **folded + wrapped** case, since §5b.3's composition applies here + too and the Q#FD18 contract must survive. + +--- + ## 6. Q#LL4 --- `editing.fill-column` **ANSWERED** > **Answered 2026-08-06: do not adopt it** --- but the reason is @@ -1034,9 +1213,17 @@ Not final --- it depends on Q#LL1. quietly re-importing deferred scope. What `wrap` actually needs witnessed: - For a source line occupying N visual rows, `pos_to_display` returns - the **visual** row and the column *within* that row --- not the - source-line index and the whole prefix width, which is what it - returns today (`src/text_view.rs:184`). + `{ row: source_line, sub_row, col }` --- the **same `row` it + returns today**, plus which visual row *within* that line and the + column within *that* row, rather than the whole prefix width + (`src/text_view.rs:184`). + + **Notation matters here and revision 14 got it wrong.** It said + `pos_to_display` returns "the visual row", which reads as a + redefinition of `row` --- exactly what §5b.5 forbids. Every example + below is therefore written as the explicit triple + `{row, sub_row, col}`; a bare pair anywhere in this section is a + bug in the document, not a shorthand. - `display_to_pos` inverts it: a click on visual row *k* of a wrapped line lands in that row's byte range, not the source line's head. - Round trip is identity for **every valid cursor boundary** in a @@ -1075,9 +1262,13 @@ Not final --- it depends on Q#LL1. Take `abcdef` soft-wrapping after `abc`. Buffer positions are `0=a 1=b 2=c 3=d 4=e 5=f`. **Position 3 is a single source position - with two defensible display coordinates**: `(k, 3)` --- just past - the last glyph of row *k* --- and `(k+1, 0)` --- just before the - first glyph of row *k+1*. Revision 5 called these "the last + with two defensible display coordinates**: + `{row: L, sub_row: k, col: 3}` --- just past the last glyph of + visual row *k* of line *L* --- and `{row: L, sub_row: k+1, col: 0}` + --- just before the first glyph of visual row *k+1* of the **same + source line**. Note both share `row: L`: the wrap point does not + cross a source line, which is precisely why redefining `row` would + have destroyed the information this case turns on. Revision 5 called these "the last position on row *k* and the first on row *k+1*" and demanded they "not collide". They are the same position. Nothing can be asserted about their collision. @@ -1104,12 +1295,14 @@ Not final --- it depends on Q#LL1. > one exists on some row; otherwise to the column just past the > last glyph.** - - Soft wrap: position 3 is followed by `d` at `(k+1, 0)`. A + - Soft wrap: position 3 is followed by `d` at + `{row: L, sub_row: k+1, col: 0}`. A following glyph exists, so that is the answer. There is a genuine choice here, and this resolves it. - Hard line end: no glyph follows on any row, so the coordinate is the column just past the last glyph --- `(k, width)`, **including - `(k, max_cols)` when the line fills the row exactly.** No choice + `{row: L, sub_row: last, col: max_cols}` when the line fills its + final visual row exactly.** No choice exists, and **this preserves current behavior unchanged**, which is the point: the wrap work must not quietly move hard-end coordinates. @@ -1127,9 +1320,11 @@ Not final --- it depends on Q#LL1. source-line space. Read §5b before costing this. Consequences to witness, and they are the discriminating ones: - - `pos_to_display(3)` is `(k+1, 0)`, never `(k, 3)`. + - `pos_to_display(3)` is `{row: L, sub_row: k+1, col: 0}`, never + `{row: L, sub_row: k, col: 3}`. - **A hard line end that exactly fills the row still maps to - `(k, max_cols)`** --- a control asserting the wrap work left the + `{row: L, sub_row: 0, col: max_cols}` with `sub_row` still 0** --- + a control asserting the wrap work left the existing hard-end coordinate alone, since the soft-wrap rule superficially resembles a rule that would have moved it. - `display_to_pos` on the trailing cells of row *k* --- which exist @@ -1148,7 +1343,8 @@ Not final --- it depends on Q#LL1. distinctly**, which is the requirement revision 5 was reaching for. The start of the last codepoint on row *k* (position 2, `c`) and the start of the first on row *k+1* (position 3, `d`) are two - different positions; they must give `(k, 2)` and `(k+1, 0)`, and + different positions; they must give `{row: L, sub_row: k, col: 2}` + and `{row: L, sub_row: k+1, col: 0}`, and the round trip must return each unchanged. - A `truncate` **control** asserting the mapping is unchanged from today, so the wrap work cannot silently alter the non-wrapped path.