diff --git a/docs/active-work.md b/docs/active-work.md index f95b5dd..0446ae9 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -435,21 +435,21 @@ Whether `docs/ci-red-signatures.md` should grow a short non-row section for this class is an open question for its owner, not something this lane decided. -## Long lines (QoL arc) — Stage 3 MERGED as #221; Stage 4 is PR #222 OPEN; Stage 5 ahead +## Long lines (QoL arc) — Stages 3 and 4 MERGED (#221, #222); Stage 5 closes it **Rewritten, not removed.** Rule 4 removes a lane when its ARC is done; -this one has Stage 4 ahead. Stage 3's durable facts are absorbed into -`docs/agent-handoff.md` §1 — rule 4's precondition, satisfied rather -than deferred — so what remains here is the Stage 4 plan and only the -Stage 3 residue that constrains it. +this one has **Stage 5** ahead. The durable facts of Stages 3 AND 4 are +both in `docs/agent-handoff.md` §1 — rule 4's precondition, satisfied +rather than deferred — so what remains here is the Stage 5 plan and +only the residue from earlier stages that constrains it. -> **RULE 4 DOES NOT APPLY AT STAGE 4's MERGE.** The arc closes at -> **Stage 5** (GPU horizontal scroll), not Stage 4. Q#HS1 split the GPU -> out deliberately and time-boxed it; retiring this lane when the TUI -> half merges would orphan exactly the half the time box exists to -> guarantee, and would do so while `truncate` is still a dead end in -> the GUI. **Do not remove this block until Stage 5 has merged**, and -> read Q#HS1's four time-box items before concluding otherwise. +> **RULE 4 APPLIES AT STAGE 5's MERGE, AND NOT BEFORE.** The arc closes +> at Stage 5 (GPU horizontal scroll). Q#HS1 split the GPU out +> deliberately and time-boxed it; retiring this lane at Stage 4's merge +> would have orphaned exactly the half the time box exists to +> guarantee, while `truncate` was still a dead end in the GUI. **Do not +> remove this block until Stage 5 has merged** — and when it does, the +> handoff bullets are what makes removal legitimate rather than lossy. **Branch `horizontal-scroll`**, based on `githubsucks/main` @ `02f3ec3` (the #221 merge). `githubsucks/horizontal-scroll` is the authoritative @@ -457,9 +457,85 @@ tip — the ref, not a SHA, since any edit to this block advances past whatever SHA it records. Recover: `git fetch githubsucks && git checkout horizontal-scroll`. -**Status: `docs/horizontal-scroll-framing.md` revision 4 — APPROVED -2026-08-07. Stage 4 implemented; **PR #222 OPEN**, awaiting review. Do -not merge unprompted.** +**Stage 4 MERGED as #222** (`2b56d16`). Long lines are now reachable +**in the TUI**; the GUI half is Stage 5. + +**Branch `gpu-horizontal-scroll`**, based on `githubsucks/main` @ +`2b56d16`. `githubsucks/gpu-horizontal-scroll` is the authoritative tip +— the ref, not a SHA. Recover: +`git fetch githubsucks && git checkout gpu-horizontal-scroll`. + +**Status: PR #223 OPEN, awaiting CI. DO NOT MERGE until the user says +so.** `https://github.com/levineuwirth/pmacs/pull/223`. Framing revision +4 approved 2026-08-07; review round 1 answered 2026-08-08 (the +completion point-predicate defect below). + +**The tip is the ref, `githubsucks/gpu-horizontal-scroll`, not a SHA +written here.** The first version of this line pinned `55faa45` — which +the very commit that wrote it invalidated, because recording the PR +moved the head. Verify CI against the PR's live `headRefOid`, never +against a SHA quoted in a document. + +G1 pixels (exact conversion via the +supported monospace advance); G2 automatic cursor-follow only, zeroing +on **both** wrap transition and `BufferSnapshot`; G3 monospace-only by +the existing font contract; G4 minimap unchanged; G5 accepted whole — +**all twelve witnesses written and mutation-tested** (see below). + +**Scope: local GPU viewport state.** No wire message, no protocol bump, +no command surface, no minimap movement. + +**ONE APPROVED EXCEPTION TO THAT SCOPE** (user, 2026-08-08; recorded in +the framing doc at §1.2a). Q#G5's +TUI-parity witness asks for agreement that is "checkable rather than +asserted". Two tests in two crates asserting the same literal is not +that — it is exactly the structural duplication +`pmacs-protocol::scroll`'s own module docs condemn, and that module +exists because **this arc already shipped that defect** (the scroll +indicator, fixed in one copy and left wrong in the other). So the follow +rule moved to `pmacs_protocol::scroll::follow_left`, beside `classify`, +and **both** frontends call it: `src/editor.rs::horizontal_follow` +delegates, and the GPU converts px ↔ columns around it (exact by Q#G3). + +What it costs: Stage 5 touches `src/editor.rs`, which the scope line +above does not cover. What it buys: the two frontends *cannot* choose +different edges. The approval turned on what it does **not** do — it +moves no viewport state, adds no wire message, and needs no +protocol-version bump. + +**Review round 1 (2026-08-08) — one non-zero-offset defect, fixed.** +`completion_anchor_px` reused `survives_code_clip_left` and passed +`line_height` as the horizontal extent: **a vertical dimension standing +in for a horizontal one**. An anchor up to a line-height left of the +gutter survived, and `completion_dropdown_rect` clamps `ax` against the +right margin only — so the popup painted over the line numbers. Now a +point predicate, `screen_x < code_clip_left()`. + +**The lesson is about the witness, not the predicate.** The existing +test placed the anchor 200px off-left, which fails a width-based +predicate too — it stayed green straight through the defect and the +mutation battery agreed with it, because the battery only ever asked +whether *removing* the check was caught. **A boundary this stage cares +about must be tested AT the boundary**: the replacement straddles the +edge by ±0.05px and additionally asserts the popup's own left edge stays +out of the gutter, which is what makes "`completion_dropdown_rect` needs +no left clamp" a checked claim rather than a comment. + +**Its first finding corrects Stage 4's framing.** §1.3 there said the +GPU "needs a mechanism that does not exist", and I endorsed the +Stage 4/5 split partly on that basis. Half of it was right — +`Scroll::horizontal` really is discarded, because glyphon 0.11 never +applies it — but the document `TextArea` already carries an explicit +`left` origin and a `TextBounds` clip, and shifting that origin is the +same "paint from 0, clip at the edge" shape the grid uses. The split +stays right (the three consumers below are real work), but it was +justified partly by an overstatement. + +The real work is applying **one** offset to the three consumers Stage +4's framing did name correctly: the caret (`code_byte_px`), decoration +geometry (`push_glyph_extent_rects`), and hit testing +(`gutter_aware_rel_x`). **No wire and no version bump** — the GPU owns +its viewport locally, exactly as it owns `scroll_top`. **What Stage 4 shipped**, beyond the `view_left` contract below: @@ -486,6 +562,32 @@ not merge unprompted.** incident this session with a complete signature, so a matchable row rather than a `U` note. +**What Stage 5 shipped, beyond the offset itself:** + +- **`crop_to_code_clip_left`, and `survives_code_clip_left` delegating + to it.** One boundary rule, so a caret the crop would discard is never + painted. The washes **crop** rather than drop — a selection running in + from off the left edge must paint the part that IS visible, which is + the same boundary Stage 4's review caught the TUI painter getting + wrong. +- **Twelve witnesses, each mutation-tested.** Eleven production + mutations (unshifted wash x, uncropped wash, unshifted math origin, + uncropped math rule, untested caret left edge, missing snapshot reset, + missing wrap reset, unhidden completion anchor, unscrolled glyphs, + inverted hit-test sign, pixel-instead-of-column snap) each fail the + intended witness as an **assertion** failure, not a compile error. + The minimap-stability witness was mutation-tested separately by + threading the offset into `minimap_vertex_bytes`. +- **The glyph-motion witness exists because the first pass lacked it.** + The gutter byte-identity test's "the code area must actually have + moved" assertion is satisfied by a decoration wash and the caret + alone: it **passed with `TextArea.left` pinned to `text_left`**. The + mutation battery caught that, not review. Its replacement isolates the + glyph layer — no decorations, and a source line carrying no caret. +- **`R8`** in `docs/ci-red-signatures.md` — a **deterministic**, + pre-existing `m4_acceptance` listview failure, confirmed on `main` by + merge-base control. Not this lane's, and deliberately not fixed here. + **Answered by the user 2026-08-07:** - **Q#HS1 — the GPU is Stage 5, not Stage 4.** A conscious, bounded @@ -536,7 +638,7 @@ and a source comment, neither of which a user sees if they set the mode in `init.lua`. **A real, small user-facing gap shipped in #221**; amending the description is now a Stage 4 deliverable (framing §6). -### What Stage 3 shipped that Stage 4 must live with +### What Stages 3 and 4 shipped that Stage 5 must live with - **`ui.line-wrap` is BUFFER-local** (`ConfigKind::Enum`, `wrap`/`truncate`, default `wrap`). Q#LL2 recorded the consequence diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 2029b9d..1b6d743 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -85,10 +85,10 @@ commands, read `docs/active-work.md` immediately after this file. ## 1. Where the project stands (2026-08-07) -- **QoL arc — Stages 1-3 merged (#219, #220, #221); Stages 4 AND 5 - remain, and the arc closes at Stage 5.** From one daily-driver - report: terminal zoom broke TUI rendering and did nothing in the GUI, - and a long line was unreadable past the edge. +- **QoL arc — Stages 1-4 merged (#219, #220, #221, #222); Stage 5 + remains, and the arc closes there.** From one daily-driver report: + terminal zoom broke TUI rendering and did nothing in the GUI, and a + long line was unreadable past the edge. - **#219** made the grid TUI honor `full_grid`, so a post-resize resync blanks the host before repainting. - **#220** gave the GUI native zoom over the font preference that @@ -125,24 +125,54 @@ commands, read `docs/active-work.md` immediately after this file. drawable clip. Neither `view_range` (it carries `SCROLL_OVERSCAN` past the window) nor `scroll_top` (it ignores `code_scroll_residual`) can answer it. - - **Stage 4 is horizontal scroll in the TUI; Stage 5 is the GPU**, a - split decided rather than inherited (framing Q#HS1) and time-boxed: - Stage 5 is the immediately-next QoL lane after Stage 4 merges, and - `wrap` stays the default until it lands — which is what keeps the - divergence invisible to anyone who has not opted in. + - **#222 made `truncate` navigable in the TUI**: moving the cursor + past the right edge scrolls the view. **Automatic only** — no + commands, no bindings (framing Q#HS2). Durable facts: + - **`view_left` is an UNSNAPPED per-window display column, and each + line derives its own effective edge.** A setter-time snap cannot + exist: one column can bisect a wide glyph on one line and be an + ordinary boundary on the next, so no single snapped value is + canonical for every visible line, and snapping per line would + break vertical alignment. Per-window (not buffer, unlike + `ui.line-wrap`) because two panes on one buffer scroll + independently. + - **The walk stays LINE-absolute; only the emit translates.** Tab + expansion depends on the absolute column from the line start, so + a walk beginning at the edge puts tab stops in the wrong place. + - **Every visible cell has a designated byte**: ordinary character → + its own start; bisected wide glyph → the **glyph's start** (its + trailing cell paints a styled blank, never a `Continuation`, which + would name an off-screen cell); tab expansion → the byte **after** + the tab (pre-existing, preserved). The wide glyph rounds backward + and the tab forward, on one principle: every cell maps to the byte + a user would mean by clicking it. + - **`Viewport::visible_cols` is the single clip rule, with five + adopters** — syntax/LSP styling, diagnostic underlines, search + washes, `BufferStyleOverlay`, and the selection painter. Review + found the first version had translated only the base glyph walk, + leaving decorations painting at absolute columns; a second round + found the selection painter keeping its own copy on a false + premise about needing a different width. + `StyleSpanOverlay`/`VirtualCellOverlay` are **excluded** by + contract: viewport-relative already. + - **`view_left` is reset to 0 on the wrap branch**, not merely + ignored — inertness would leave a stale offset that reappears on + the next toggle back to `truncate`. + - **Persisted per leaf at `DESKTOP_VERSION = 1`** via + `#[serde(default)]`. Nothing else in `src/desktop.rs` carries that + attribute, so removing it makes serde reject every desktop saved + before the field existed. A literal v1 JSON fixture guards it. + - **No wire.** `view_left` is per-window viewport state, so no + protocol message and no version bump. + - **Stage 5 is the GPU half** — a split decided rather than inherited + (framing Q#HS1), time-boxed: it is the immediately-next QoL lane, + and **`wrap` stays the default until it lands**, which keeps the + divergence invisible to anyone who has not opted in. Framing in + `docs/gpu-horizontal-scroll-framing.md`. - `truncate` is incomplete without scroll: text past the right edge is - currently *unreachable*. **That caveat is NOT in the setting's - description** — `builtin/runtime/linewrap.lua:23` says only - "truncate at the edge", and the word appears in - `ui.toggle-line-wrap`'s status message and a source comment, which - a user who sets the mode in `init.lua` never sees. A small - user-facing gap shipped in #221; amending the description is a - Stage 4 deliverable. - - **Rule 4 must not retire the long-lines lane when Stage 4 merges** — - the arc closes at Stage 5. Framing in - `docs/horizontal-scroll-framing.md`. + **Rule 4 removes the long-lines lane when Stage 5 merges** — the + arc closes there, and these bullets are the precondition it depends + on. - **`main` @ `db1bbe9`.** The **tree primitive #217** — `listview` rows take optional `depth`/`id`, collapse is primitive-owned, folding is diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 288b75b..2b7f268 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -226,6 +226,30 @@ readiness helpers exist, whether they can be one, and what each promises. Patching this call site alone would leave the same question open under a fourth selector. +### R8 — LSP listview row renders a path relative to a root the test does not expect + +The first row here that is **not intermittent**. It reproduces on every +run, and the merge-base control has already been done — so the one +question this registry exists to answer is settled for it, and settled +against the branch that found it. + +| field | value | +|---|---| +| **selector** | `--test m4_acceptance flat_listview_consumers_render_byte_identically_after_the_tree_extension` | +| **job / flavor** | local (Linux), any invocation — isolated single-test runs included. Not load-sensitive | +| **required fragments** | `the flat references row renders verbatim` **and** a `left` value that is the `right` value with a **leading directory removed** | +| **status** | **deterministic locally, NOT attributed to the long-lines arc** | +| **what IS established** | it fails identically on `main` and on `gpu-horizontal-scroll` with the working tree stashed — a merge-base control, not an inference from "my diff looks unrelated". The rendered row is the expected path with `/tmp/` stripped: `.tmpPZsycN/r.rs:12:3` vs `/tmp/.tmpPZsycN/r.rs:12:3` | +| **what is NOT** | the mechanism. **This is a prefix strip, not a width truncation** — the fixture declares `CellSize::new(40, 100)`, so 25 characters fit with room to spare, and the missing text is at the front. The likeliest reading is that the row is rendered relative to a workspace root that resolves to `/tmp` on this machine, which would make it depend on `TMPDIR`. **Not verified**, and the alternative — that the renderer relativizes against something else entirely — is not excluded | +| **why CI is green** | unestablished. If the `TMPDIR` reading is right, a runner whose temp dir is not directly under the relativization root would never see it. That is a hypothesis, not a finding | +| **retirement** | a diagnosis of what the row is rendered relative to, then either fixing the renderer or making the assertion state the relativization it expects. A green run on a machine with a different `TMPDIR` retires nothing | + +**Not this lane's to fix, and deliberately not fixed here.** Stage 5 +touches `pmacs-gpu`, `pmacs-protocol::scroll`, and `horizontal_follow` +in `src/editor.rs`; it has no path to the LSP listview renderer. Fixing +it inside this branch would put an unrelated, undiagnosed change in a +viewport PR. + ## Retired rows **These stay here on purpose.** A retirement is a claim that a mechanism @@ -460,6 +484,36 @@ be matched either. Recorded so a recurrence is recognisable. | **what is NOT** | any mechanism. Not reproduced in a later full `--tests --no-fail-fast` sweep (108 targets, exit 0) nor in 3 isolated `--lib` runs (1917/0 each) | | **rival explanation not excluded** | leaked `pmacs --daemon` processes, which the handoff names as a standing confound for any load-sensitive local red | +### U3 — the R7 selector again, fragments lost the same way U2's were + +`attach::tests::managed_retry_survives_transients_and_uses_the_successful_stream` +failed once during long-lines Stage 5's default-features sweep and +passed on the recaptured rerun. + +**This is not recorded as an R7 match, and the distinction is the +point.** R7's job/flavor is `--features crdt`; this was +default-features. More importantly its three required fragments +(`transient sequence must attach`, `Handshake(Io(`, `BrokenPipe` / +`code: 32`) are **unverified**, because the run's output was filtered to +the failing test names before it was read. By this file's own matching +rule that makes it a new incident, not a recurrence. + +| field | value | +|---|---| +| **selector** | `-p pmacs-gpu attach::tests::managed_retry_survives_transients_and_uses_the_successful_stream` | +| **job / flavor** | local (Linux), `cargo test --workspace --no-fail-fast` — **default features**, unlike R7 | +| **required fragments** | **none captured** | +| **status** | **new incident, not reproduced** | +| **what IS established** | one failure; a recaptured rerun of the same command was green for this test, and the `--features crdt` sweep was green for it too. Per the rerun rule: **intermittence only** | +| **what is NOT** | whether it is R7's mechanism. It may well be. Nothing in hand shows it | + +**The recurring mistake is mine, and it is now twice.** U2 records the +identical loss — "output was filtered to the `FAILED` line" — and I did +it again here by piping a sweep through `grep`. The fix is mechanical: +**redirect a full sweep to a file and grep the file**, never the live +stream. A signature that is cheap to capture and impossible to +reconstruct should never be traded for terminal brevity. + **The retirements are not occurrences and do not close the log.** R1 and R3 stay live, and each retired row keeps its signature so a later red matching one reopens it. diff --git a/docs/gpu-horizontal-scroll-framing.md b/docs/gpu-horizontal-scroll-framing.md new file mode 100644 index 0000000..042a81a --- /dev/null +++ b/docs/gpu-horizontal-scroll-framing.md @@ -0,0 +1,369 @@ +# GPU horizontal scroll — QoL Stage 5 + +**Status: revision 4 — APPROVED 2026-08-07. All five questions +resolved. Implementation may begin within this scope.** + +- **Q#G1** — the GPU-local offset is stored in **pixels**; parity + conversion is exact via the supported monospace advance. +- **Q#G2** — **automatic cursor-follow only**; reset on **both** the + wrap transition and `BufferSnapshot`, with the specified pre-motion + witnesses. +- **Q#G3** — monospace-only, by the font contract that already exists. +- **Q#G4** — the minimap does not move. +- **Q#G5** — the complete verification set is accepted, including the + snapshot-reset and minimap-stability witnesses. + +**Scope boundary, restated because it is what makes this lane small:** +local GPU viewport state only. **No wire message, no protocol bump, no +command surface, no minimap movement.** + +Revision 3 fixes four things review found in revision 2: Q#G1 still +carried two claims Q#G3 had already falsified; Q#G2 was missing the +**buffer-snapshot** reset; Q#G5's "nothing paints into the gutter" is +**impossible** as written, because the gutter legitimately holds line +numbers and diagnostic signs; and an off-left completion anchor +**hides** the popup rather than closing it, which is a protocol +distinction this lane must not blur. + +Revision 2 had answered two functional findings: §1.1's "three +consumers" was incomplete because the manual quad/squiggle renderers +have **no code-area scissor at all**, and Q#G2's "inert under wrap" was +too weak — the offset must be **reset to zero**, as the TUI already +does. + +**This closes the QoL arc.** Stage 1 (#219) made the TUI survive +terminal zoom; Stage 2 (#220) gave the GUI native zoom; Stage 3 (#221) +added `ui.line-wrap`; Stage 4 (#222) made `truncate` navigable **in the +TUI only**. Stage 5 is the GPU half, and it is the lane Q#HS1's time box +exists to guarantee: it is the immediately-next QoL work, and `wrap` +stays the default until it lands. + +--- + +## 1. Stage 4's framing was wrong about the hard part + +**§1.3 of `docs/horizontal-scroll-framing.md` said the GPU "needs a +mechanism that does not exist" and called it the fact most likely to +invert the cost estimate. It was half right and the half it got wrong +is the expensive half.** + +What it got right: `Scroll::horizontal` **is** discarded throughout, +because glyphon 0.11 never applies it when placing glyphs +(`pmacs-gpu/src/main.rs:1611`, `:6316`, `:8020`, asserted at `:16266`, +`:16337`, `:16737`). Scrolling via cosmic-text's own scroll is not +available. + +What it got wrong: that is not the only mechanism. **The document +`TextArea` already carries an explicit origin and a clip rectangle** +(`pmacs-gpu/src/main.rs:8712`): + +```rust +TextArea { + buffer: &self.buffer, + left: text_left, // paint origin + top: TEXT_TOP, + bounds: TextBounds { left: gutter_clip_left, right: text_bounds_right, .. }, + .. +} +``` + +Horizontal scroll is `left: text_left - offset_px` with `bounds.left` +unchanged. glyphon clips to `bounds`, so glyphs pushed left of the +gutter simply are not painted — which is the same "paint from column 0, +clip at the edge" shape the grid renderer uses, expressed in pixels. + +**This is a mechanism the file already relies on**, not a new one: +`gutter_clip_left` exists precisely so the gutter does not get painted +over. + +**Why the correction matters beyond the estimate.** Stage 4's framing +used §1.3 to justify splitting the GPU out, and I endorsed the split on +that basis. The split is still right — the *consumers* below are real +work, and shipping them inside Stage 4 would have made one reviewable +change into two unreviewable ones — but it was justified partly by a +claim that overstated the difficulty. Recorded here rather than quietly +dropped. + +### 1.1 The real work is a shared transform AND a shared clip + +**Revision 1 said "three consumers" and that was wrong in a way that +would have shipped a defect.** Shifting the `TextArea` clips *glyphon's* +text, because glyphon honors `TextBounds`. **The manual quad and +squiggle renderers have no code-area scissor at all** — nothing stops +them painting into the gutter, and today nothing needs to, because no +code-relative x can be negative. Scrolling makes that false. + +So Stage 5 needs **two** shared things, not one offset: + +1. **One screen↔code transform.** `code_x → screen_x` is + `text_left() - offset_px + code_x`, and hit testing is its exact + inverse. Applied once, in one place, or the consumers disagree. +2. **One code clip rectangle.** `[gutter_clip_left, text_bounds_right)` + — the same bounds the `TextArea` gets, expressed for the paths + glyphon does not clip. **Every code-relative painter must intersect + with it**, and that is a new obligation, not a threading exercise. + +The paths that need both: + +| path | site | what breaks without the clip | +|---|---|---| +| caret rect in clip | `:9698` | **asserts the caret cannot precede `text_left`** — its comment says so explicitly. False after scrolling; the caret paints over the gutter | +| caret-painted predicate | `:9734` | same missing left-edge test. **So revision 1's claim that the scroll indicator "inherits the fix" is FALSE** — `code_byte_painted` reuses this and would call an off-left byte painted | +| glyph extent rects | `:9766` | washes, squiggles and selection extents must be **cropped** at the gutter edge, not merely offset | +| inline math origins | `:9434` | math boxes derive from the code origin and would render into the gutter | +| completion anchor | `:7606` | an off-left anchor must **hide** the popup — it already returns `None` when scrolled out, and closure is the daemon's `CompletionPopup { anchor: None }`, not this lane's | + +The two caret sites are the sharpest: `:9698` does not merely lack a +check, it **documents the absence as safe** (*"right of the gutter isn't +needed: the caret x can't precede `text_left`"*). A comment asserting an +invariant this lane deletes is worse than silence, so it must be +rewritten rather than merely joined by a new test. + +### 1.2 No wire, and that is not an accident + +The GPU **owns its viewport locally** — `scroll_top` and +`code_scroll_residual` are local state, never sent. A horizontal offset +is the same kind of state, so Stage 5 adds **no protocol message and no +version bump**, exactly as Stage 4 added none. + +This is worth stating because the parallel with `ui.line-wrap` is +misleading: the *mode* is buffer state and needed `LineWrapFacts` at +v22, but the *offset* is viewport state and needs nothing. + +### 1.2a The one approved exception to "local GPU viewport state" + +**Approved by the user 2026-08-08**, after implementation raised it. +The scope line for this stage is *local GPU viewport state — no wire +message, protocol bump, command surface, or minimap movement*. One +change lands outside it, and only one: + +**`pmacs_protocol::scroll::follow_left`.** The follow rule — *scroll the +minimum distance that puts the cursor back inside* — now lives in the +protocol crate beside `classify`, and **both** frontends call it: +`src/editor.rs::horizontal_follow` delegates, and the GPU converts +px ↔ columns around it (exact, per Q#G1/Q#G3). + +*Why it is not scope creep.* Q#G5 requires a TUI-parity witness that is +"checkable rather than asserted". Two tests in two crates asserting the +same literal is not that — it is the structural duplication +`pmacs-protocol::scroll`'s own module docs condemn, and **that module +exists because this arc already shipped exactly that defect**: the +scroll indicator, fixed in one copy and left wrong in the other. Without +a shared rule there is no way to make the witness real. + +*What it does not do*, which is what keeps it narrow and is the basis of +the approval: it moves **no viewport state** (the TUI still owns +`view_left`, the GPU still owns `code_scroll_left`), adds **no wire +message**, and needs **no protocol-version bump**. `follow_left` is a +pure function over values each side already holds — the identical +argument `classify`'s module docs already make for living there. + +--- + +## 2. Open questions + +### Q#G1 — is the stored offset a column or a pixel count? + +The TUI stores a display **column** (`view_left`). The GPU paints in +**pixels**. + +**Revision 2 left two claims here that its own Q#G3 answer had already +falsified**: that the GPU's font "need not be monospace", and that Q#G3 +makes "column" ill-defined. Neither is true — the code font is +monospace by contract, so a column has one well-defined width, measured +by the existing `ADVANCE_PROBE` helper. + +*My vote is unchanged — store **pixels** — but the reasons narrow to +the ones that survive:* the GPU's other viewport state is already +pixel-flavoured (`code_scroll_residual` is a float), and a pixel offset +composes with the clip rectangle without rounding at every frame. + +**Conversion is therefore exact, not approximate.** `columns × the +supported monospace advance` is the definition, and it is what makes +the unconditional TUI-parity witness in Q#G5 checkable at all. A +proportional font would have made this a lossy conversion; the font +contract means it never is. + +### Q#G2 — how is the offset moved? + +Stage 4 chose **automatic only**: the cursor-visibility pass gains a +horizontal component. The GPU's analogous pass is +**`ensure_caret_painted`** — named rather than cited by line, since +revision 2 also invented a `follow_cursor` that does not exist. + +*My vote: mirror it exactly*, so the two frontends agree on when the +view moves. A GUI is also the place a horizontal **wheel/trackpad** +gesture exists — but that is an explicit-scroll surface, which Stage 4 +deliberately deferred, and adding it here would make the frontends +disagree again in the lane that exists to stop that. + +**And the wrap transition must RESET the offset to zero, not merely +ignore it.** Revision 1 said "inert under wrap", which is too weak: +inertness hides a stale value that reappears the moment the buffer +toggles back to `truncate`, before any cursor motion. The TUI does not +rely on inertness — `horizontal_follow` (`src/editor.rs`) assigns +`view_left = 0` on the wrap branch and returns. + +The GPU must specify the **identical lifecycle**, and `apply_line_wrap` +is where it belongs, beside the reflow it already performs. + +**And a second reset the TUI has no analogue for: the buffer +snapshot.** The GPU zeroes `scroll_top` and `code_scroll_residual` +whenever a snapshot installs a new buffer — the offset is viewport +state tied to the document being shown, and it must reset there for the +same reason they do. Without it a buffer switch **inherits the previous +document's leftward viewport**, showing the new buffer scrolled +sideways until a cursor motion repairs it. + +That is a worse symptom than the wrap case, because nothing about the +new buffer explains it. Both resets are lifecycle requirements with +their own witnesses (Q#G5), not properties that fall out of the +transform. + +### Q#G3 — proportional fonts **ANSWERED: they do not occur** + +> **Revision 1 asked the wrong question, from a false premise.** It said +> "the GPU can resolve a non-monospace family" and proposed accepting a +> new TUI/GPU divergence to accommodate it. +> +> **The GPU does not resolve proportional code fonts.** +> `family_is_monospace_everywhere` (`pmacs-gpu/src/main.rs:8199`) gates +> the family across all four weight/style combinations, `apply_font_facts` +> (`:8235`) falls back when it fails, and +> `unresolvable_and_proportional_families_fall_back` **requires** that +> fallback. A proportional family cannot become the code font. +> +> So the answer is **monospace-only, by the font contract that already +> exists** — not a new constraint this lane imposes, and not a +> divergence to negotiate. Revision 1 would have introduced a +> font-dependent behavior difference to solve a problem the codebase had +> already solved, in the lane whose entire purpose is removing +> unchosen divergence. +> +> **The consequence for Q#G5**: the monospace TUI-parity witness is +> **unconditional** for every font the GPU supports, rather than gated +> on a font check. That is a stronger test, and it exists only because +> the premise was corrected. + +Pixel storage (Q#G1) is unaffected and still preferred — but its +column↔pixel conversion is now defined against **the supported +monospace advance**, which is a well-defined quantity rather than an +approximation. + +### Q#G4 — does the minimap move? + +The minimap draws line-shape bands. Horizontal scroll does not change +which lines exist. + +*My vote: no.* The minimap is a whole-document overview; scrolling the +text sideways should not scroll it. + +### Q#G5 — what does the verification look like? + +The GPU suite is headless-render based (`headless_or_skip`), and +`PMACS_REQUIRE_GPU=1` makes a missing adapter a failure rather than a +skip. + +Sketch, pending Q#G1/G2/G4: + +- **The gutter is unchanged by scrolling.** Revision 2 proposed + asserting that *nothing* paints left of `gutter_clip_left`, which is + **impossible**: with line numbers on, the gutter deliberately holds + digit glyphs and diagnostic-sign quads. The assertion would fail on a + correct implementation. + + The checkable form of the same intent: **the gutter rectangle is + byte-identical before and after a horizontal scroll**, and separately, + only *code-relative* output geometry is inspected for the left-edge + rule. That still catches the failure a per-painter test would miss — + a code painter bleeding into the gutter changes those pixels — while + remaining true of a working build. +- **A left-clipped caret predicate.** `code_caret_rect_in_clip` and + `caret_painted_in_code_clip` must both report *not painted* for a + caret scrolled off-left. This is what makes the scroll indicator + correct; revision 1 wrongly assumed it came for free. +- **Math and rule clipping**: an inline math box and a decoration rule + whose origins are left of the edge are cropped or culled, not drawn. +- **An off-left completion anchor HIDES the popup; it does not close + it** — and the boundary is a **point**, tested at the edge. + + *Added after review found the first implementation wrong here.* It + reused `survives_code_clip_left` and passed `line_height` as the + horizontal extent: a vertical dimension standing in for a horizontal + one. An anchor up to a line-height left of the gutter therefore + survived, and `completion_dropdown_rect` bounds `ax` against the right + margin only — so the popup painted over the line numbers. An anchor is + a position between glyphs with no width of its own, so the predicate + is `screen_x < code_clip_left()`. + + **The far-off-left witness cannot catch this**, which is why it stayed + green: 200px off-left fails a width-based predicate too. The witness + must **straddle** the edge — the same anchor a fraction of a pixel + either side — and assert the popup's own left edge stays out of the + gutter on the visible side. + + The distinction is a protocol one and revision 2 got it wrong. + `completion_anchor_px` returns `None`, so nothing draws — but the + daemon-owned completion state and its key handling are retained. + Actual closure is `CompletionPopup { anchor: None }`, which is the + daemon's to send. So the witness is: **no completion paint while the + anchor is off-left, and the popup reappears when it scrolls back into + view** — session semantics unchanged. A lane about viewport geometry + must not quietly redefine when a completion ends. +- **The three consumers agree with the shifted origin** — caret, + decoration rect, and hit test at one non-zero offset, since a partial + fix shows up as disagreement between them. +- **Round-trip hit testing**: pixel → byte → pixel at a non-zero + offset. +- **The wrap transition resets to zero** (Q#G2): scroll right under + `truncate`, toggle to `wrap`, toggle back, and assert the offset is 0 + **before any cursor motion**. An inertness-only implementation passes + a "wrap looks right" test and fails this one. +- **The buffer snapshot resets to zero** (Q#G2, second lifecycle rule). + Revision 3 added the requirement and left it untested, which is how a + lifecycle rule quietly becomes a comment. Scroll to a non-zero offset + in buffer A, install a **buffer B snapshot**, and assert the offset is + 0 **and B renders at its code origin — before any `CursorByte` + arrives**. The pre-cursor scoping is the whole test: a later cursor + motion repairs the offset anyway, so a witness that waits for one + cannot distinguish "reset on snapshot" from "repaired on first + motion", which is precisely the bug. +- **The minimap does not move** (Q#G4). The implementation already + supports the vote — the minimap derives from the summary, the surface + dimensions and `scroll_top`, with no horizontal input — so this pins + an existing property rather than asking for new work, and that is the + reason to write it: an offset threaded one seam too far would break + it silently. Assert **equal minimap vertices (or pixels) before and + after a non-zero horizontal offset**, with vertical state unchanged. +- **A TUI-parity witness**, now **unconditional** (Q#G3): the same + buffer and the same column offset yield the same first visible + character in both frontends. This is what makes "the frontends agree" + checkable rather than asserted. + +--- + +## 3. Coherence impact (§20) + +- **§16 Semantic Frontend Architecture — the direct target.** This lane + *closes* the divergence Stage 4 opened deliberately. The release notes + should say the gap is closed, since Stage 4's said it existed. +- **Journey step 4 is NOT completed by this lane, and revision 1 + claimed it was.** Step 4 ("Understand interface") is scored on + **welcome / help / tutorial discoverability**, and `COHERENCE.md:395` + holds it **Partial** for reasons this lane does not touch: `C-h` + deletes a word (deliberately, §18) and there is no tutorial. + Long-line reachability is interface *comprehension*, which the step + benefits from without being scored on. + + So the honest claim is **preserving and improving interface + comprehension**, with no scorecard movement — and a scorecard edit + here would need new journey evidence, which this lane does not + produce. Recorded because writing an unearned ✔ into a scorecard is + how a coherence document stops being ground truth. +- **After this merges, Q#HS6 becomes live**: whether `wrap` remains the + default is revisitable on use evidence once parity exists. Not part + of this stage. +- **Rule 4 applies at this merge**: the arc is done, so the long-lines + lane in `docs/active-work.md` is removed — after its durable facts + reach `docs/agent-handoff.md`, which is the precondition, not a + formality. diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 0c71682..52a2a29 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -1614,6 +1614,23 @@ struct State { /// resets it (buffer-scoped view state), and every full reshape /// reapplies it instead of installing `Scroll::default`. code_scroll_residual: f32, + /// Horizontal scroll offset of the code area, in **pixels** (Stage + /// 5, framing Q#G1). + /// + /// Pixels rather than columns because the GPU's other viewport + /// state already is (`code_scroll_residual` above), and a pixel + /// offset composes with the clip rectangle without per-frame + /// rounding. Column parity with the TUI is still exact: the code + /// font is monospace by contract + /// (`family_is_monospace_everywhere`), so `columns × advance` is a + /// definition rather than an approximation. + /// + /// **Local viewport state, never sent.** Same category as + /// `scroll_top`: no wire message and no protocol bump (§1.2). + /// + /// Reset to 0 on BOTH the wrap transition and a buffer snapshot + /// (Q#G2) — inertness alone would let a stale offset reappear. + code_scroll_left: f32, swash_cache: SwashCache, viewport: Viewport, atlas: TextAtlas, @@ -4016,6 +4033,7 @@ impl State { resolved_family: DEFAULT_FONT_FAMILY.to_owned(), measured_mono_advance: None, code_scroll_residual: 0.0, + code_scroll_left: 0.0, swash_cache, viewport, atlas, @@ -4634,6 +4652,14 @@ impl State { // preference/metrics survive (framing Q#F6). self.scroll_top = 0; self.code_scroll_residual = 0.0; + // Stage 5 (Q#G2): the horizontal offset is viewport + // state tied to the document being shown, so it resets + // with the other two. Without this a buffer switch + // inherits the PREVIOUS document's leftward viewport + // and renders the new buffer scrolled sideways until a + // cursor motion repairs it — a symptom nothing about + // the new buffer explains. + self.code_scroll_left = 0.0; self.last_viewport_sent = None; // Vterm Stage 3 — a snapshot ALWAYS leaves terminal // mode, including a terminal→terminal switch. The prior @@ -6337,9 +6363,60 @@ impl State { .shape_until_cursor(&mut self.font_system, cursor, false); } self.normalize_code_scroll(); + self.horizontal_follow(byte); self.request_redraw(); } + /// Move `code_scroll_left` so the caret's column is on screen + /// (Stage 5, framing Q#G2 — automatic only). + /// + /// The horizontal mirror of `scroll_to_cursor`, and deliberately + /// the same shape as the TUI's `horizontal_follow`: scroll only far + /// enough to bring the caret back inside, so a caret already + /// visible never moves the view. With no explicit scroll commands, + /// every horizontal viewport move originates here. + /// + /// Runs AFTER `normalize_code_scroll` because it reads the caret's + /// laid-out x, which the vertical normalization can change. + /// + /// **The decision is `pmacs_protocol::scroll::follow_left`**, the + /// same function `src/editor.rs` calls, so the two frontends cannot + /// choose different edges for the same cursor. That rule is stated + /// in columns; this is where the conversion happens, and Q#G3 is + /// what makes it exact — a non-monospace code font is rejected + /// before it can reach layout, so every advance is the same width + /// and `px / advance` is a column count rather than an estimate. + /// + /// The result is re-multiplied rather than kept in pixels, which + /// **snaps the offset to the column grid**. That is the point: it + /// is what makes "the same first visible character in both + /// frontends" true rather than approximately true. + fn horizontal_follow(&mut self, byte: u64) { + // A wrapped buffer has nothing past the right edge; the offset + // is pinned to 0 by `apply_line_wrap` and must stay there. + if self.buffer.wrap() != Wrap::None { + self.code_scroll_left = 0.0; + return; + } + let Some((code_x, _top, _h)) = self.code_byte_px(byte) else { + return; + }; + let advance = self.mono_advance(); + let width = self.text_bounds_right() as f32 - self.text_left(); + if advance <= 0.0 || width <= 0.0 { + return; + } + // `floor` for the width — a half-visible trailing column is not + // a column you can read — and `round` for the two positions, + // which are exact multiples of the advance up to f32 + // accumulation error. + let cols = (width / advance).floor().max(0.0) as u32; + let cursor_col = (code_x / advance).round().max(0.0) as u32; + let left_col = (self.code_scroll_left / advance).round().max(0.0) as u32; + let next = pmacs_protocol::scroll::follow_left(left_col, cursor_col, cols); + self.code_scroll_left = next as f32 * advance; + } + /// Monospace glyph advance in px, used to size the line-number /// gutter (UX gutter arc). Once a `FontFacts` has been applied the /// measured NORMAL-face probe advance is authoritative (framing @@ -6388,6 +6465,65 @@ impl State { TEXT_LEFT + self.gutter_width_px() } + /// **The** screen↔code transform (Stage 5, framing §1.1). + /// + /// A code-relative x — what `code_byte_px`, the decoration geometry + /// and the math/completion origins all produce — becomes a screen x + /// here and nowhere else. Written once because the alternative is + /// five call sites that can disagree, and a disagreement between + /// the caret and the glyphs it sits among is invisible until + /// somebody scrolls. + fn code_x_to_screen(&self, code_x: f32) -> f32 { + self.text_left() - self.code_scroll_left + code_x + } + + /// The exact inverse, for hit testing. + fn screen_x_to_code(&self, screen_x: f32) -> f32 { + screen_x - self.text_left() + self.code_scroll_left + } + + /// **The** code clip rectangle's left edge (Stage 5, framing §1.1). + /// + /// glyphon honors `TextBounds`, so the document `TextArea` clips + /// itself. **The manual quad and squiggle renderers do not** — + /// nothing stopped them painting into the gutter, and nothing + /// needed to, because before this stage no code-relative x could be + /// negative. Every code-relative painter must now intersect with + /// this. + fn code_clip_left(&self) -> f32 { + if self.line_numbers.is_on() { + self.text_left().floor() + } else { + 0.0 + } + } + + /// Crop a code-relative rect `[x, x + w)` in SCREEN coordinates to + /// the code clip's left edge, returning the surviving `(x, w)`. + /// `None` when the rect lies wholly inside the gutter. + /// + /// **Cropping, not dropping**, because these rects are washes: a + /// selection or search band running in from off the left edge must + /// still paint the part that IS on screen. Dropping it whole is the + /// exact boundary defect the TUI painter had before Stage 4. + fn crop_to_code_clip_left(&self, screen_x: f32, w: f32) -> Option<(f32, f32)> { + let left = self.code_clip_left(); + let right = screen_x + w; + (right > left).then(|| { + let x = screen_x.max(left); + (x, right - x) + }) + } + + /// Whether a code-relative rect `[x, x + w)` in SCREEN coordinates + /// survives the code clip's left edge — the same boundary as + /// [`Self::crop_to_code_clip_left`], for the callers (the caret) + /// that want a yes/no rather than a cropped rect. Delegating keeps + /// one rule: a caret the crop would discard is never painted. + fn survives_code_clip_left(&self, screen_x: f32, w: f32) -> bool { + self.crop_to_code_clip_left(screen_x, w).is_some() + } + /// The GPU's own cursor's 0-based buffer line, or `0` when there's no /// own cursor in the displayed buffer (relative/hybrid then count from /// the top — a rare transient). Derived from the whole-buffer line @@ -6467,12 +6603,15 @@ impl State { /// seam a future gutter marker would branch on instead of relying on /// glyphon's negative-x edge behavior. fn gutter_aware_rel_x(&self, x: f64) -> f32 { - let raw_x = x as f32 - self.text_left(); - if self.line_numbers.is_on() && raw_x < 0.0 { - 0.0 - } else { - raw_x + // Stage 5: the EXACT inverse of `code_x_to_screen`, so a click + // lands on the glyph under the pointer at any offset. The + // gutter clamp stays in SCREEN space and is applied first — a + // click in the gutter band means "the first visible column", + // which after scrolling is the offset, not column 0. + if self.line_numbers.is_on() && (x as f32) < self.text_left() { + return self.code_scroll_left; } + self.screen_x_to_code(x as f32) } /// The shaped slice's math substitutions, read back from the @@ -7622,7 +7761,36 @@ impl State { if y >= bottom || y + line_height <= TEXT_TOP { return None; } - Some((self.text_left() + x, y, line_height)) + // Stage 5: an anchor scrolled off the LEFT is out of view the + // same way one below the band is. Returning `None` HIDES the + // popup — it does not close it. The daemon owns completion + // state and its key handling; closure is `CompletionPopup { + // anchor: None }`, which is the daemon's to send. Scrolling + // back brings the popup straight back, and a viewport lane must + // not quietly redefine when a completion ends. + // + // **A POINT predicate, not `survives_code_clip_left`.** An + // anchor is a position between glyphs — it has no horizontal + // extent of its own, and the popup it places is drawn to its + // RIGHT. The first version of this passed `line_height` as the + // width, which is a vertical dimension standing in for a + // horizontal one: an anchor up to a line-height left of the + // gutter then "survived", and `completion_dropdown_rect` has no + // left clamp (it bounds `ax` against the right margin only), so + // that x reached the popup's left edge and painted over the + // line numbers. + // + // That absent clamp stays absent deliberately. This predicate + // is what guarantees `ax >= code_clip_left()`, and a second + // clamp downstream would be a duplicate of the same rule — the + // failure mode this stage's whole shared-transform design + // exists to avoid. `completion_dropdown_rect` is witnessed + // against it instead. + let screen_x = self.code_x_to_screen(x); + if screen_x < self.code_clip_left() { + return None; + } + Some((screen_x, y, line_height)) } /// Layout of the completion dropdown: `(first_row, row_count, @@ -8078,6 +8246,13 @@ impl State { return; } self.buffer.set_wrap(&mut self.font_system, want); + // Stage 5 (Q#G2): RESET, not merely ignore. A wrapped buffer has + // nothing past the right edge, so an offset is meaningless here + // — but leaving it parked would surface a stale viewport the + // instant the buffer toggled back to `truncate`, before any + // cursor motion. The TUI's `horizontal_follow` zeroes it on the + // same branch for the same reason. + self.code_scroll_left = 0.0; self.reshape(); } @@ -8711,7 +8886,11 @@ impl State { } else { vec![TextArea { buffer: &self.buffer, - left: text_left, + // Stage 5: the whole of the glyph-side mechanism. + // glyphon clips to `bounds`, whose `left` stays at the + // gutter, so shifting the origin scrolls the text and + // the gutter keeps its own pixels. + left: text_left - self.code_scroll_left, top: TEXT_TOP, scale: 1.0, bounds: TextBounds { @@ -9437,7 +9616,6 @@ impl State { if self.line_math_cache.iter().all(|m| m.placed.is_empty()) { return (Vec::new(), rules); } - let text_left = self.text_left(); for run in self.buffer.layout_runs() { let Some(state) = self.line_math_cache.get(run.line_i) else { continue; @@ -9453,7 +9631,12 @@ impl State { else { continue; }; - let origin_x = text_left + anchor.x; + // Stage 5: the box rides the offset with the spacer + // glyph it is anchored to. The glyph mini-buffers are + // clipped by their layer's `TextBounds` (the same + // `gutter_clip_left` the code layer uses), so only the + // rule quads below need the manual crop. + let origin_x = self.code_x_to_screen(anchor.x); let baseline_px = TEXT_TOP + run.line_y; for item in &placed.boxed.items { match *item { @@ -9473,13 +9656,19 @@ impl State { width, thickness, } => { - rules.push(MinimapRect { - x: origin_x + x, - y: baseline_px - y - thickness / 2.0, - w: width, - h: thickness, - color: MATH_INK_RGBA, - }); + // The fraction bars are quads in the bg + // batch, so unlike the glyphs above they + // carry no scissor and must be cropped. + if let Some((sx, sw)) = self.crop_to_code_clip_left(origin_x + x, width) + { + rules.push(MinimapRect { + x: sx, + y: baseline_px - y - thickness / 2.0, + w: sw, + h: thickness, + color: MATH_INK_RGBA, + }); + } } } } @@ -9681,8 +9870,10 @@ impl State { } let (x, top, line_height) = self.code_byte_px(cursor)?; Some(MinimapRect { - // UX gutter: the caret sits in the code area, past the gutter. - x: self.text_left() + x, + // UX gutter: the caret sits in the code area, past the + // gutter — and past the horizontal offset (Stage 5), which + // is why this goes through the one transform. + x: self.code_x_to_screen(x), y: TEXT_TOP + top, w: CARET_WIDTH, h: line_height, @@ -9691,15 +9882,24 @@ impl State { } /// [`Self::caret_rect`] intersected with the drawable code clip — - /// the painter's own bounds (right of the gutter isn't needed: - /// the caret x can't precede `text_left`), NOT `view_range`, so - /// the two-line source overscan and wrapped runs clipped below - /// the band don't count as painted (framing Q#F6). + /// the painter's own bounds, NOT `view_range`, so the two-line + /// source overscan and wrapped runs clipped below the band don't + /// count as painted (framing Q#F6). + /// + /// **The left edge is now tested too.** This comment used to say + /// "right of the gutter isn't needed: the caret x can't precede + /// `text_left`" — an invariant Stage 5 deletes. A comment asserting + /// something a later stage falsifies is worse than silence, so it + /// is rewritten rather than merely joined by a new condition. fn code_caret_rect_in_clip(&mut self) -> Option { let rect = self.caret_rect()?; let bottom = document_text_bottom(self.config.height, self.fm, self.band_inset()); let right = self.text_bounds_right() as f32; - (rect.y < bottom && rect.y + rect.h > TEXT_TOP && rect.x < right).then_some(rect) + (rect.y < bottom + && rect.y + rect.h > TEXT_TOP + && rect.x < right + && self.survives_code_clip_left(rect.x, rect.w)) + .then_some(rect) } /// The pre-change follow decision (framing Q#F6): whether the own @@ -9775,8 +9975,6 @@ impl State { if hi <= lo { return; } - // UX gutter: washes/squiggles are code-relative, past the gutter. - let text_left = self.text_left(); for run in self.buffer.layout_runs() { let line_base = line_offsets.get(run.line_i).copied().unwrap_or(0); let line_end = line_offsets @@ -9832,13 +10030,15 @@ impl State { Some(bar) => (TEXT_TOP + run.line_top + run.line_height - bar, bar), None => (TEXT_TOP + run.line_top, run.line_height), }; - rects.push(MinimapRect { - x: text_left + x0, - y, - w: x1 - x0, - h, - color, - }); + // Stage 5: code-relative extents ride the offset, and + // this quad batch has no scissor of its own — glyphon's + // `TextBounds` covers the glyph layers only. So the + // shared clip crops them at the gutter here. + let Some((x, w)) = self.crop_to_code_clip_left(self.code_x_to_screen(x0), x1 - x0) + else { + continue; + }; + rects.push(MinimapRect { x, y, w, h, color }); } } } @@ -18777,4 +18977,682 @@ mod tests { let error = parse_args(&extra).expect_err("help operands must fail"); assert_eq!(error, "--help does not accept operands"); } + + // --- Horizontal scroll (QoL Stage 5, framing Q#G5) ------------------- + // + // Every test below drives the offset through `ensure_caret_painted`, + // the only thing that moves it (Q#G2 — automatic follow, no command + // surface). Two of them are deliberately the exception: the wrap and + // snapshot resets must be observed BEFORE any cursor motion, because + // a motion afterwards repairs the offset anyway and a witness that + // waits for one cannot tell "reset" from "repaired later" — which is + // the bug. + // + // The gutter is on in all of them. With it off `code_clip_left` is 0 + // and every left-edge assertion below is vacuously true, so a suite + // that forgot to turn it on would pass against an unclipped build. + + /// One long line, gutter on, wrapping off — the configuration the + /// Stage 5 witnesses share. `truncate` is set through the real + /// `apply_line_wrap` rather than by poking `buffer.set_wrap`, so the + /// buffer never sits on cosmic-text's constructor default. + fn truncating_state(width: u32, height: u32, text: &str) -> Option<(State, BufferId)> { + let mut state = headless_or_skip(width, height, text)?; + let bid = BufferId::next(); + state.current_buffer_id = Some(bid); + state.line_numbers = LineNumberMode::Absolute; + state.apply_line_wrap(bid, false); + state.reshape(); + Some((state, bid)) + } + + /// Follow the caret to `byte`, returning the resulting offset. + fn follow_to(state: &mut State, bid: BufferId, byte: u64) -> f32 { + state.own_cursor = Some(OwnCursor { + buffer_id: bid, + byte, + }); + state.ensure_caret_painted(); + state.code_scroll_left + } + + /// A line long enough that its end is far off the right edge of the + /// windows used here. + fn long_line() -> String { + "abcdefghij".repeat(30) + } + + /// Q#G5 — **the gutter is unchanged by scrolling.** + /// + /// Not "nothing paints left of the edge": with line numbers on, the + /// gutter deliberately holds digit glyphs, so that assertion would + /// fail on a correct build. The checkable form of the same intent is + /// byte-identity of the gutter band across a scroll — a code painter + /// bleeding leftward changes exactly those pixels. + #[test] + fn the_gutter_band_is_byte_identical_across_a_horizontal_scroll() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + // A wash over the WHOLE line, so that once the view scrolls its + // rect genuinely starts left of the gutter. Without a decoration + // here this test would say nothing about the quad batch — which + // is the batch with no scissor of its own, and so the only one + // that can bleed into the gutter. + state.current_decorations = vec![Decoration { + range: ByteRange { + start: 0, + end: text.len() as u64, + }, + kind: DecorationKind::Selection, + }]; + let before = state.render_offscreen(); + let offset = follow_to(&mut state, bid, text.len() as u64); + assert!(offset > 0.0, "precondition: the caret's column scrolled"); + let after = state.render_offscreen(); + + let gutter_right = state.code_clip_left() as u32; + assert!(gutter_right > 0, "precondition: the gutter is on"); + let bottom = document_text_bottom(240, state.fm, state.band_inset()) as u32; + assert_eq!( + region(&before, 320, 0, gutter_right, 0, bottom), + region(&after, 320, 0, gutter_right, 0, bottom), + "scrolling the code must not touch one pixel of the gutter" + ); + // Without this the test passes on a build where scrolling does + // nothing at all. + assert_ne!( + region(&before, 320, gutter_right, 320, 0, bottom), + region(&after, 320, gutter_right, 320, 0, bottom), + "the code area must actually have moved" + ); + } + + /// Q#G5 — **the glyphs themselves move**, which is the one thing + /// every other witness here takes for granted. + /// + /// The gutter test above asserts the code area changes under a + /// scroll, but a decoration wash and the caret both satisfy that on + /// their own — it passes with `TextArea.left` pinned to `text_left`. + /// So this isolates the glyph layer: **no decorations**, and the + /// pixels examined are a source line with **no caret on it**, whose + /// only ink is text glyphon placed. + /// + /// The fixture makes the claim binary rather than a diff: line 1 is + /// blank for its first 295 columns and marked at its end, so at + /// offset 0 its band holds no ink at all, and ink appearing there is + /// unambiguously text that scrolled in from off the right edge. + #[test] + fn scrolling_moves_the_text_glyphs_not_only_the_decorations() { + let text = format!("{}\n{}#####", long_line(), " ".repeat(295)); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + let line_h = state.fm.code_line_height(); + // Row band of source line 1, which never holds the caret. + let (y0, y1) = ( + (TEXT_TOP + line_h) as u32 + 1, + (TEXT_TOP + 2.0 * line_h) as u32 - 1, + ); + let left = state.code_clip_left() as u32; + + let before = state.render_offscreen(); + let bg = bg_sample(&before, 320); + assert!( + !(left..320).any(|x| (y0..y1).any(|y| differs_from_bg(&before, 320, bg, x, y))), + "precondition: line 1's visible span is blank at offset 0" + ); + + // Follow the caret on line 0 — line 1 is dragged along by the + // shared offset, and has no cursor of its own. + let end_of_first_line = long_line().len() as u64; + assert!(follow_to(&mut state, bid, end_of_first_line) > 0.0); + let after = state.render_offscreen(); + assert!( + (left..320).any(|x| (y0..y1).any(|y| differs_from_bg(&after, 320, bg, x, y))), + "line 1's end must have scrolled into view — with `TextArea.left` \ + pinned to the code origin this band stays blank forever" + ); + } + + /// Q#G5 — **a caret scrolled off the left is not painted.** + /// + /// Both predicates, because they are what the scroll indicator and + /// the font/resize re-follow read. Revision 1 of the framing assumed + /// this came for free; it did not — `code_caret_rect_in_clip` tested + /// three edges and not this one. + #[test] + fn a_caret_scrolled_off_the_left_reports_not_painted() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + // Move the cursor home WITHOUT following: the offset stays put, + // so byte 0 now sits behind the gutter. This is the state a + // frame can genuinely be in — the daemon's `CursorByte` and the + // follow are separate steps. + state.own_cursor = Some(OwnCursor { + buffer_id: bid, + byte: 0, + }); + assert!( + state.code_scroll_left > 0.0, + "precondition: the view is still scrolled right" + ); + let raw = state.caret_rect().expect("the caret still has geometry"); + assert!( + raw.x < state.code_clip_left(), + "precondition: byte 0 is left of the gutter edge, at {}", + raw.x + ); + assert!( + state.code_caret_rect_in_clip().is_none(), + "an off-left caret must not be painted" + ); + assert!( + !state.caret_painted_in_code_clip(), + "and the predicate the re-follow reads must agree" + ); + } + + /// The caret's left-edge predicate, tested **at** the edge — the + /// audit the completion defect prompted. + /// + /// The caret's use of `survives_code_clip_left` is correct where the + /// completion anchor's was not: a caret quad is `CARET_WIDTH` wide, + /// so that IS its horizontal extent. What this pins is the + /// consequence, which is otherwise only derivable: because + /// `horizontal_follow` snaps the offset to whole columns, a caret is + /// never *partly* behind the gutter. It is either at a visible + /// column or a whole advance left of one, and `CARET_WIDTH` (2px) is + /// far smaller than any code advance — so the straddle case the + /// completion path got wrong cannot arise here at all. + /// + /// Written because that argument is load-bearing and invisible. A + /// future change to the snap, or a font whose advance approached + /// 2px, would break it silently. + /// + /// Mutation-checked: substituting `rect.h` for `rect.w` — the exact + /// error the completion path shipped — fails this test. An + /// over-width *smaller than one advance* does not, and that is the + /// invariant rather than a gap: under column snapping an extent + /// error too small to cross a column cannot put the caret in the + /// gutter, which is why only the height-sized error is visible. + #[test] + fn the_caret_never_straddles_the_gutter_edge_under_column_snapping() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + let advance = state.mono_advance(); + assert!( + advance > CARET_WIDTH, + "the no-straddle argument needs an advance wider than the caret" + ); + let clip = state.code_clip_left(); + + // Walk the caret across the boundary one column at a time and + // check every frame: painted carets are wholly inside the code + // area, hidden ones are wholly outside it. + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + let scrolled = state.code_scroll_left; + for col in 0..text.len() as u64 { + state.own_cursor = Some(OwnCursor { + buffer_id: bid, + byte: col, + }); + state.code_scroll_left = scrolled; // no follow: hold the view + let Some(rect) = state.caret_rect() else { + continue; + }; + if state.code_caret_rect_in_clip().is_some() { + assert!( + rect.x >= clip, + "a PAINTED caret at column {col} starts at {}, inside \ + the gutter edge {clip}", + rect.x + ); + } else { + assert!( + rect.x + rect.w <= clip || rect.x >= state.text_bounds_right() as f32, + "a HIDDEN caret at column {col} spans [{}, {}) — it \ + overlaps the code area, so hiding it loses the cursor", + rect.x, + rect.x + rect.w + ); + } + } + } + + /// Q#G5 — **math boxes and their rules obey the same clip.** + /// + /// The glyph mini-buffers are clipped by their layer's `TextBounds`; + /// the fraction rules are quads in the background batch with no + /// scissor of their own, so they are cropped by hand and this is the + /// test that says so. + #[test] + fn math_rules_are_cropped_at_the_gutter_when_scrolled_off_left() { + let text = format!(r"$\frac{{ab}}{{cd}}$ {}", long_line()); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + let (_, rules) = state.build_math_paint(); + assert!( + !rules.is_empty(), + "precondition: the fraction produced a rule quad" + ); + let unscrolled = rules[0].x; + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + + let clip = state.code_clip_left(); + let (glyphs, rules) = state.build_math_paint(); + for rule in &rules { + assert!( + rule.x >= clip - 0.01 && rule.w > 0.0, + "a rule at x={} w={} escaped the clip at {clip}", + rule.x, + rule.w + ); + } + // Cropped or culled — but not left where it was. A rule that + // never moved would also satisfy the loop above. + assert!( + rules.is_empty() || (rules[0].x - unscrolled).abs() > 0.5, + "the rule must ride the offset" + ); + for (_, left, _) in &glyphs { + assert!( + *left < unscrolled, + "the glyph origins ride the offset too ({left} vs {unscrolled})" + ); + } + } + + /// Q#G5 — **an off-left completion anchor HIDES the popup.** + /// + /// It does not close it. Closure is `CompletionPopup { anchor: None + /// }` and belongs to the daemon; a viewport lane must not quietly + /// redefine when a completion ends. So: nothing paints while the + /// anchor is off-left, the session state survives, and scrolling + /// back brings the popup straight back. + #[test] + fn an_off_left_completion_anchor_hides_the_popup_without_closing_it() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(480, 240, &text) else { + return; + }; + state.view_range = (0, text.len() as u64); + state.completion = Some(CompletionLocal { + buffer_id: bid, + anchor: 0, + prefix_len: 0, + rows: vec![CompletionPopupRow { + label: "candidate".into(), + kind: 3, + detail: None, + }], + selected: Some(0), + total: 1, + }); + assert!( + state.completion_anchor_px().is_some(), + "precondition: the anchor is visible at offset 0" + ); + + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + state.view_range = (0, text.len() as u64); + assert!( + state.completion_anchor_px().is_none(), + "an off-left anchor must not paint" + ); + assert!( + state.completion_dropdown_layout().is_none(), + "and neither must the dropdown that hangs off it" + ); + assert!( + state.completion.is_some(), + "HIDDEN, not closed — the daemon owns the session and its keys" + ); + + assert!(follow_to(&mut state, bid, 0).abs() < f32::EPSILON); + state.view_range = (0, text.len() as u64); + assert!( + state.completion_anchor_px().is_some(), + "scrolling back must bring the same popup back" + ); + } + + /// Review finding — **the completion boundary is a POINT, and it is + /// exact to within a fraction of a pixel.** + /// + /// The far-off-left test above cannot catch this. The first version + /// of the predicate passed `line_height` as the horizontal extent — + /// a vertical dimension standing in for a horizontal one — so an + /// anchor anywhere within a line-height left of the gutter survived, + /// and `completion_dropdown_rect` clamps `ax` against the right + /// margin only. The popup painted over the line numbers. An anchor + /// 200px off-left survives neither predicate, which is exactly why + /// that test stayed green through the defect. + /// + /// So this straddles the edge instead: the same anchor a twentieth + /// of a pixel either side of it, which no width-based predicate can + /// separate. + #[test] + fn a_completion_anchor_a_hair_left_of_the_gutter_hides() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(480, 240, &text) else { + return; + }; + state.view_range = (0, text.len() as u64); + state.completion = Some(CompletionLocal { + buffer_id: bid, + anchor: 0, + prefix_len: 0, + rows: vec![CompletionPopupRow { + label: "candidate".into(), + kind: 3, + detail: None, + }], + selected: Some(0), + total: 1, + }); + let (code_x, _, _) = state.code_byte_px(0).expect("the anchor has geometry"); + // Offset that puts the anchor exactly on the clip edge. Set + // directly: this witnesses the PREDICATE, not the follow, and + // the follow cannot land on a fractional boundary by design. + let flush = state.text_left() + code_x - state.code_clip_left(); + + state.code_scroll_left = flush + 0.05; // a hair LEFT of the edge + assert!( + state.completion_anchor_px().is_none(), + "an anchor {:.2}px left of the gutter must hide — a width-based \ + predicate lets it through by most of a line height", + 0.05 + ); + assert!( + state.completion.is_some(), + "still HIDDEN, not closed, at the boundary too" + ); + assert!( + state.completion_dropdown_rect().is_none(), + "and nothing downstream may resurrect a hidden anchor" + ); + + state.code_scroll_left = flush - 0.05; // a hair RIGHT of the edge + let (ax, _, _) = state.completion_anchor_px().expect( + "a hair right of the edge is visible — the predicate is \ + a boundary, not a margin", + ); + assert!( + ax >= state.code_clip_left(), + "the anchor the popup is placed at must be inside the code area" + ); + // The claim that `completion_dropdown_rect` needs no left clamp + // of its own, checked rather than asserted in a comment. + let (left, _, _) = state + .completion_dropdown_rect() + .expect("the visible anchor places a popup"); + assert!( + left >= state.code_clip_left(), + "the popup's left edge must not enter the gutter ({left} vs {})", + state.code_clip_left() + ); + } + + /// Q#G5 — **the three consumers agree at a non-zero offset.** + /// + /// Caret, decoration geometry, and hit test, all for one byte. A + /// partial fix — one painter translated and another not — shows up + /// here as disagreement and nowhere else, because each of them looks + /// correct on its own. + #[test] + fn caret_decoration_and_hit_test_agree_at_a_non_zero_offset() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + let target = text.len() as u64 - 4; + assert!(follow_to(&mut state, bid, target) > 0.0); + + let caret_x = state.caret_rect().expect("caret is in the slice").x; + assert!( + caret_x >= state.code_clip_left(), + "precondition: the followed caret is inside the code area" + ); + + state.current_decorations = vec![Decoration { + range: ByteRange { + start: target, + end: target + 1, + }, + kind: DecorationKind::Selection, + }]; + let mut rects = Vec::new(); + let slice = &state.current_text[state.view_range.0 as usize..state.view_range.1 as usize]; + let line_offsets = line_byte_offsets(slice); + state.collect_own_decoration_rects( + &mut rects, + &line_offsets, + state.view_range.0, + state.view_range.1, + ); + let wash = rects + .first() + .copied() + .expect("the selection produced a rect"); + assert!( + (wash.x - caret_x).abs() < 1.0, + "the wash over the caret's byte must start where the caret is \ + ({} vs {caret_x})", + wash.x + ); + + let y = f64::from(TEXT_TOP + state.fm.code_line_height() / 2.0); + assert_eq!( + state.hit_test_source_byte(f64::from(caret_x) + 0.5, y), + Some(target), + "and a click on that pixel must land on that same byte" + ); + } + + /// Q#G5 — **pixel → byte → pixel round trips at a non-zero offset.** + /// + /// The forward transform and its inverse are separate functions, and + /// a sign error in either survives every one-directional test. + #[test] + fn hit_testing_round_trips_through_a_non_zero_offset() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + + let advance = state.mono_advance(); + let y = f64::from(TEXT_TOP + state.fm.code_line_height() / 2.0); + let start = state.text_left() + advance * 2.5; + for step in 0..5 { + let x = start + advance * step as f32; + let byte = state + .hit_test_source_byte(f64::from(x), y) + .expect("a pixel inside the code area hits a byte"); + let (code_x, _, _) = state.code_byte_px(byte).expect("that byte has geometry"); + let back = state.code_x_to_screen(code_x); + assert!( + (back - x).abs() <= advance, + "round trip drifted: {x} -> byte {byte} -> {back}" + ); + } + // The inverse in isolation, exactly. + assert!( + (state.screen_x_to_code(state.code_x_to_screen(41.5)) - 41.5).abs() < 0.01, + "screen_x_to_code must be the exact inverse of code_x_to_screen" + ); + } + + /// Q#G5 — **the wrap transition resets the offset to zero**, and it + /// is observed BEFORE any cursor motion. + /// + /// An implementation that merely ignores the offset while wrapped + /// passes a "wrap looks right" test and fails this one: toggling + /// back to `truncate` would surface the stale viewport instantly. + #[test] + fn toggling_wrap_and_back_resets_the_offset_before_any_motion() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + + state.apply_line_wrap(bid, true); + assert!( + state.code_scroll_left.abs() < f32::EPSILON, + "wrapping zeroes the offset" + ); + state.apply_line_wrap(bid, false); + assert!( + state.code_scroll_left.abs() < f32::EPSILON, + "and it is still zero on the way back — no cursor has moved" + ); + } + + /// Q#G5 — **a buffer snapshot resets the offset to zero**, observed + /// before any `CursorByte` arrives. + /// + /// The pre-cursor scoping is the whole test. A later motion repairs + /// the offset anyway, so a witness that waits for one cannot + /// distinguish "reset on snapshot" from "repaired on first motion" — + /// and it is the window between them, where buffer B renders scrolled + /// sideways for no reason the user can see, that this rule exists for. + #[test] + fn a_buffer_snapshot_resets_the_offset_before_any_cursor_arrives() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + + let doc = loro::LoroDoc::new(); + doc.get_text(LORO_TEXT_CONTAINER) + .insert(0, "buffer B") + .expect("insert snapshot text"); + let other = BufferId::next(); + let _ = state.apply_attach_message(InstanceMessage::BufferSnapshot { + buffer_id: other, + crdt_snapshot: doc.export(loro::ExportMode::Snapshot).expect("export"), + }); + + assert!( + state.own_cursor.is_none(), + "precondition: no cursor has arrived for the new buffer yet" + ); + assert!( + state.code_scroll_left.abs() < f32::EPSILON, + "the new buffer must not inherit the old one's viewport" + ); + // And it must actually RENDER at the origin, not merely hold a + // zeroed field: the first glyph of buffer B sits at the code + // origin, which is the thing the user would see going wrong. + let first_glyph_x = state + .buffer + .layout_runs() + .next() + .and_then(|run| run.glyphs.first().map(|g| g.x)) + .expect("buffer B shaped at least one glyph"); + assert!( + (state.code_x_to_screen(first_glyph_x) - state.text_left()).abs() < 0.01, + "buffer B must render at its code origin" + ); + } + + /// Q#G5 / Q#G4 — **the minimap does not move.** + /// + /// It derives from the summary, the surface dimensions and + /// `scroll_top`, with no horizontal input, so this pins an existing + /// property rather than asking for new work. That is exactly why it + /// is worth writing: an offset threaded one seam too far would break + /// it silently. + #[test] + fn a_horizontal_offset_leaves_the_minimap_vertices_unchanged() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(480, 300, &text) else { + return; + }; + state.current_summary = Some(FileStyleSummaryState { + generation: 1, + lines: vec![ + CellStyle { + fg: CellColor::Rgb(255, 255, 255), + ..CellStyle::default() + }; + 40 + ], + }); + state.current_line_shapes = minimap_line_shapes(&text); + state.minimap_cache = None; + + let before = state.minimap_vertex_bytes(); + assert!( + !before.is_empty(), + "precondition: the minimap actually paints, or this is vacuous" + ); + let top = state.scroll_top; + + assert!(follow_to(&mut state, bid, text.len() as u64) > 0.0); + assert_eq!(state.scroll_top, top, "vertical state must be unchanged"); + state.minimap_cache = None; + assert_eq!( + before, + state.minimap_vertex_bytes(), + "a horizontal offset must not reach the minimap" + ); + } + + /// Q#G5 — **TUI parity, unconditional.** + /// + /// The same buffer and the same cursor column put the same character + /// first in both frontends. This is checkable rather than asserted + /// because there is one rule: `pmacs_protocol::scroll::follow_left`, + /// which `src/editor.rs` also calls. What this test proves is the + /// part that is genuinely GPU-local — that the px ↔ column + /// conversion around that call is **exact**, which is what Q#G3 + /// (monospace code fonts only) buys and what the snap-back to a + /// column multiple in `horizontal_follow` preserves. + #[test] + fn the_gpu_offset_is_the_tui_column_rule_converted_exactly() { + let text = long_line(); + let Some((mut state, bid)) = truncating_state(320, 240, &text) else { + return; + }; + let advance = state.mono_advance(); + let width = state.text_bounds_right() as f32 - state.text_left(); + // Derived from the window and the font, not from the offset + // under test — otherwise this would compare a value to itself. + let cols = (width / advance).floor() as u32; + assert!(cols > 0 && (text.len() as u32) > cols, "precondition"); + + for cursor_col in [cols, cols + 1, text.len() as u32 - 1] { + let offset = follow_to(&mut state, bid, u64::from(cursor_col)); + let expected = pmacs_protocol::scroll::follow_left(0, cursor_col, cols); + // Every char in the fixture is one column wide, so the + // cursor's byte IS its column and the TUI's `view_left` + // would be `expected`. + let in_columns = offset / advance; + assert!( + (in_columns - expected as f32).abs() < 0.01, + "column {cursor_col}: the GPU is at {in_columns} columns, \ + the TUI would be at {expected}" + ); + // Exactness is the claim, so the offset must land on a + // column boundary rather than merely near one. + assert!( + (offset - expected as f32 * advance).abs() < 0.01, + "the offset must be an exact multiple of the advance" + ); + // Reset so each case starts from a known edge, matching the + // `follow_left(0, ..)` above. + let _ = follow_to(&mut state, bid, 0); + } + } } diff --git a/pmacs-protocol/src/scroll.rs b/pmacs-protocol/src/scroll.rs index bab0689..9696aad 100644 --- a/pmacs-protocol/src/scroll.rs +++ b/pmacs-protocol/src/scroll.rs @@ -102,6 +102,51 @@ pub fn classify( } } +/// Move a horizontal viewport's left edge so `cursor_col` is visible, +/// returning the new edge. All three arguments and the result are +/// **columns**. +/// +/// # Why this is shared, and why in columns +/// +/// This is the horizontal twin of [`classify`], and it is here for the +/// same reason spelled out in the module docs: the two frontends were +/// about to hold one rule twice. The TUI stores its edge as a column +/// (`Window::view_left`); `pmacs-gpu` stores pixels, because its +/// geometry comes from cosmic-text advances. Long-lines Stage 5 Q#G1 +/// settles that difference as a **conversion, not a second rule** — the +/// GPU rejects non-monospace code fonts (Q#G3), so px ↔ column is exact +/// through the resolved advance, and the GPU divides on the way in and +/// multiplies on the way out. +/// +/// Columns, not pixels, is the shared unit because it is the one both +/// sides can name. A pixel rule would force the TUI into float +/// arithmetic over a quantity that is integral by construction, and an +/// off-by-one from rounding there is a character the user cannot read — +/// the whole complaint this arc answers. +/// +/// # The rule +/// +/// Scroll the minimum distance that puts the cursor back inside, so a +/// cursor already visible never moves the view. `width == 0` means +/// nothing has been laid out yet: no column is visible, so no edge is +/// better than another and the current one stands. +#[must_use] +pub fn follow_left(left: u32, cursor_col: u32, width: u32) -> u32 { + if width == 0 { + return left; + } + if cursor_col < left { + cursor_col + } else if cursor_col >= left.saturating_add(width) { + // `+ 1` puts the cursor's own column at the right edge rather + // than one past it. No underflow: this arm implies + // `cursor_col >= width`. + cursor_col.saturating_add(1) - width + } else { + left + } +} + #[cfg(test)] mod tests { use super::*; @@ -215,4 +260,52 @@ mod tests { "correct by arithmetic here, not by saturation" ); } + + /// A cursor already inside the window never moves the edge — the + /// property that separates "follow" from "center". + #[test] + fn a_visible_cursor_leaves_the_edge_alone() { + for col in 10..90 { + assert_eq!(follow_left(10, col, 80), 10, "column {col} is visible"); + } + } + + /// Both edges, minimally. + #[test] + fn the_edge_moves_the_minimum_distance_in_each_direction() { + // Left: the cursor's own column becomes the first visible one. + assert_eq!(follow_left(10, 4, 80), 4); + // Right: the cursor's own column becomes the LAST visible one, + // which is `+ 1 - width`, not `- width`. Dropping the `+ 1` + // parks the caret one column off the right edge — invisible, + // and the exact defect this arc reports. + assert_eq!(follow_left(10, 90, 80), 11); + assert_eq!(follow_left(0, 79, 80), 0, "the last column still fits"); + assert_eq!(follow_left(0, 80, 80), 1, "one past it scrolls by one"); + } + + /// Nothing laid out yet: no column is visible, so the edge stands + /// rather than snapping to a cursor whose geometry is unknown. + #[test] + fn a_zero_width_viewport_holds_its_edge() { + assert_eq!(follow_left(7, 0, 0), 7); + assert_eq!(follow_left(7, 9999, 0), 7); + } + + /// The saturating arms are reachable arithmetic, not decoration. + /// + /// At `cursor_col == u32::MAX` the saturation absorbs the `+ 1`, so + /// the edge lands one column short of showing that column. Asserted + /// as the value it actually produces rather than the value the rule + /// would like: a line 4·10⁹ columns wide does not occur, and a test + /// that lied about this arm to look tidy would be worse than the + /// one-column imprecision it hid. + #[test] + fn extreme_columns_do_not_panic() { + assert_eq!(follow_left(0, u32::MAX, 80), u32::MAX - 80); + assert_eq!(follow_left(u32::MAX, 0, 80), 0); + // `left + width` overflows; the cursor is nonetheless left of + // the edge, so the first arm decides and nothing wraps. + assert_eq!(follow_left(u32::MAX - 1, 5, 80), 5); + } } diff --git a/src/editor.rs b/src/editor.rs index d9bfdfb..8d80a6c 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -4277,20 +4277,21 @@ impl CompletionPopupKey { /// so the offset is pinned to 0 rather than merely ignored. Leaving a /// stale non-zero value would surface the moment the buffer toggled /// back to `truncate`. +/// +/// **The arithmetic itself lives in `pmacs_protocol::scroll`** (Stage 5, +/// Q#G5), beside `classify`, and for the same reason its module docs +/// give: `pmacs-gpu` needs the identical rule and cannot see this crate. +/// Held twice, the two copies drift — which is not hypothetical, it is +/// what happened to the scroll indicator earlier in this very arc. What +/// stays here is the part that is genuinely the TUI's: which window, +/// which wrap mode, and which width. fn horizontal_follow(window: &mut crate::window::Window, cursor_col: u32) { if window.last_wrap == crate::view::WrapMode::Wrap { window.view_left = 0; return; } - let cols = window.last_content_cols; - if cols == 0 { - return; // not rendered yet; nothing to be visible within - } - if cursor_col < window.view_left { - window.view_left = cursor_col; - } else if cursor_col >= window.view_left.saturating_add(cols) { - window.view_left = cursor_col + 1 - cols; - } + window.view_left = + pmacs_protocol::scroll::follow_left(window.view_left, cursor_col, window.last_content_cols); } /// Scroll one window so its cursor stays visible, reckoning in