diff --git a/builtin/runtime/linewrap.lua b/builtin/runtime/linewrap.lua index 39aae65..30d48b7 100644 --- a/builtin/runtime/linewrap.lua +++ b/builtin/runtime/linewrap.lua @@ -20,7 +20,15 @@ pmacs.config.define { name = "ui.line-wrap", - description = "How a line wider than the window is shown: wrap onto following rows, or truncate at the edge.", + -- The description names what `truncate` COSTS, because the toggle's + -- status message is not enough: a user who sets this in `init.lua` + -- never invokes the toggle and so never sees it. #221 shipped that + -- gap (framing §6). + -- + -- In the TUI the cost is now only the GUI's, because Stage 4 gave the + -- grid renderer horizontal scrolling. Stage 5 closes the rest, and + -- this sentence shrinks back when it lands. + description = "How a line wider than the window is shown: wrap onto following rows, or truncate at the edge. Truncated text is reachable by moving the cursor past the edge in the terminal UI; in the GUI it is not yet reachable at all.", -- A closed set, so an unknown value is impossible rather than -- handled. Adding "word" later is a clean additive change --- which -- is the plan, since character wrap is what both frontends can do @@ -65,7 +73,7 @@ pmacs.command.define { local next_mode = current == "wrap" and "truncate" or "wrap" pmacs.config.set_local(buf, "ui.line-wrap", next_mode) if next_mode == "truncate" then - pmacs.editor.set_status("line wrap off — text past the edge is unreachable until horizontal scrolling lands") + pmacs.editor.set_status("line wrap off — move the cursor past the edge to scroll (GUI: not yet)") else pmacs.editor.set_status("line wrap on") end diff --git a/docs/active-work.md b/docs/active-work.md index b00ed4d..f95b5dd 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -434,278 +434,191 @@ a job executed — **the log is**. 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 Stage 3) — PR #221 OPEN, awaiting review -**Branch `long-lines`**, originally based on `githubsucks/main` @ -`218d2e7` (the #219 merge); **main merged in at #220's landing**, so -the branch now carries Stage 2. The two share no code — that merge is -currency, not dependency, taken so the Stage 3 PR opens against a base -it has already been tested on. `githubsucks/long-lines` is the authoritative tip — the ref, -not a SHA, since any edit to this block advances past whatever SHA it -records. Recover: `git fetch githubsucks && git checkout long-lines`. +## Long lines (QoL arc) — Stage 3 MERGED as #221; Stage 4 is PR #222 OPEN; Stage 5 ahead -**This block was written with the lane's first commit, before any PR -existed** — the standing correction from #171, #215 and #220. **PR -#221** opened 2026-08-07 against `main` @ `912bf57`; the number is -recorded here rather than left to be reconstructed from the branch. +**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. -- **Framing `docs/long-lines-framing.md` revision 20, APPROVED.** All - eight questions settled, §5d.6 resolved. Revision 20 **withdraws - §1.1** — see "The framing error" below. -- **Independent of #220** (now merged). Stage 2 and Stage 3 share no - code; the merge in this branch is currency, not dependency. -- **Implementation complete.** Seven commits, `937544c..840a338`. - Every gate green locally, macOS unverifiable here as always. +> **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. -### The defect +**Branch `horizontal-scroll`**, based on `githubsucks/main` @ `02f3ec3` +(the #221 merge). `githubsucks/horizontal-scroll` is the authoritative +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`. -A line wider than the window is unreadable past the edge **in the -TUI**. The GPU is not in that state: it already wraps. So the -cross-frontend defect is **not** unreadability — it is that *neither -behavior was chosen*. The TUI truncates because a cell walk breaks at -`max_cols`; the GPU wraps because cosmic-text's `Wrap::WordOrGlyph` -default was never overridden. Two accidents that disagree, with no way -for the user to express a preference in either. +**Status: `docs/horizontal-scroll-framing.md` revision 4 — APPROVED +2026-08-07. Stage 4 implemented; **PR #222 OPEN**, awaiting review. Do +not merge unprompted.** -### The six answers +**What Stage 4 shipped**, beyond the `view_left` contract below: -- **Q#LL1** — ships `wrap` + `truncate`, **default `wrap`**; horizontal - scroll is **Stage 4**. No default preserves both frontends, so this - knowingly changes the TUI's behavior and leaves the GPU's alone. -- **Q#LL2** — the mode is **buffer-local**; `Viewport` carries the - *resolved* mode as it already carries `folds`, so `TextView` stays - config-agnostic. -- **Q#LL4** — do **not** adopt `editing.fill-column`; name ours - `ui.line-wrap` (`ConfigKind::Enum`). *(The recorded reason was wrong; - the answer was not. See "The framing error".)* -- **Q#LL5** — **character wrap in both frontends**; the GPU document - buffer gets its first explicit `set_wrap`, `Wrap::Glyph`. -- **Q#LL6** — no global map (every vertical consumer is local, so - layout is per-line); `view_top`'s sub-line component is a **byte**; - `DisplayCoord` gains `sub_row` rather than redefining `row`. +- **`Viewport::visible_cols` — one clip rule, five adopters.** Review + found the first version had translated the base glyph walk and + nothing else, so syntax styling, diagnostic underlines, search + washes, `BufferStyleOverlay` and the selection painter all kept + painting at absolute columns: decorations drifting off the characters + they describe, only once a window had been scrolled. The selection + painter was worst — it asked `pos_to_display` through the live + context, which returns `None` left of the edge, so a selection + starting off-screen painted **nothing at all**. -### The two holes review found in the first commit + A second review round caught that my reason for letting selection + keep its own copy of the rule (a width the viewport supposedly + lacked) was **false**: the render viewport is already + `rect.size.cols - gutter_w` with an origin past the gutter. It now + takes that viewport. `StyleSpanOverlay` / `VirtualCellOverlay` stay + untouched — viewport-relative by contract. +- **The `ui.line-wrap` description now names what `truncate` costs**, + closing the #221 gap where only the toggle's status message said it. +- **`R7`** in `docs/ci-red-signatures.md` — an unrelated, unreproduced + `pmacs-gpu` managed-retry `BrokenPipe` under full-sweep load. First + incident this session with a complete signature, so a matchable row + rather than a `U` note. -- **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. +**Answered by the user 2026-08-07:** - **Two further corrections, from review of the second commit.** - `format_scroll_indicator` is **duplicated, not shared** - (`src/editor.rs:5509`, `pmacs-gpu/src/main.rs:10114`), and the GPU - passes a source-line count --- so the first fix would have corrected - one frontend and left the other wrong, *this lane's own defect - reproduced by the section meant to close it*. And the lazy total's - cache key omitted **fold state**, which changes per window with no - edit, resize or mode change; now keyed on the fold projection's own - contents, which cannot be forgotten, rather than a maintained - revision counter that can. +- **Q#HS1 — the GPU is Stage 5, not Stage 4.** A conscious, bounded + divergence rather than a repeat of Stage 3's accidental one. The time + box is concrete: Stage 5 is the *immediately-next* QoL lane after + Stage 4 merges, `wrap` stays the default until it lands, Stage 4's + release notes state the asymmetry, and the `truncate` affordances + name the GUI gap while it exists. +- **Q#HS2 — automatic only.** The cursor-visibility pass gains a + horizontal component; no commands, no bindings, no new interaction + island. Explicit `ui.scroll-*` is deliberately out. +- **Q#HS6 — `wrap` stays the default.** Coupled to Q#HS1: with the GPU + deferred, a `truncate` default would ship a mode that is navigable in + the TUI and a **dead end in the GUI** for anyone who never opened the + setting. Revisit after Stage 5, on use evidence. - **Then the aggregate was abandoned entirely (revision 17).** The GPU - shapes **only the viewport slice** — Session S1 found the whole rope - made large-file editing `O(file)` per keystroke — so its layout - cannot yield a total, and re-shaping the document for a status-line - readout would reintroduce that cost. `NN%` is now **byte-based in - both frontends**; `All`/`Top`/`Bot` stay exact because they are local - predicates. This retires the cache *and* the fold-key fix above, - which is kept in the framing marked superseded so a reader can tell - "the key was fixed" from "there is no key". +- **Q#HS7 — ACCEPTED.** `view_left` is an unsnapped window display + column; the effective edge is derived **per line**; a bisected wide + glyph's trailing cell renders as styled blank and is designated to + the **glyph's start** byte; a straddling tab's surviving cells keep + the **existing** forward rounding to the byte after the tab + (`src/text_view.rs:224` — preserved, not chosen). Together these make + the mapping **total over visible cells**, which is the (d) invariant. + The discriminating witness is multi-line, with glyph widths differing + at the same column. +- **Q#HS5 — APPROVED: yes, persist, no `DESKTOP_VERSION` bump** — + conditional on `#[serde(default)]` **and** a literal v1 JSON fixture + omitting the field, asserting restore at zero. Both conditions are + part of the approval. - **Then the retained formatter turned out unable to express the new - contract (revision 18).** Every branch of `format_scroll_indicator` - derives from `total_lines`, so a byte total would compare rows - against bytes and a fake total restores the false `All`. Resolved by - keeping the existing formatter **untouched for `truncate`** — output - identical by construction — and adding a `classify(first_visible, - last_visible, byte_pos, byte_len)` for `wrap` that never sees a row - count. **Resolved: the classifier lives in `pmacs-protocol`** — - `ScrollPosition` plus a pure `classify`, with string rendering left - in each frontend. Per §16: each frontend computes its own local - layout facts, the shared crate owns the semantic decision they feed. - **No wire message, no version bump**, and the one-copy-fixed defect - becomes unrepresentable rather than reviewer-guarded. +- **Q#HS3** is re-confirmed rather than open (per-window, per Q#LL2); + **Q#HS4** is deferred, live only if explicit commands arrive. -### The framing error, and the four defects review found in the code +**The reasoning worth keeping from the withdrawn Q#HS7(c).** Revision +2 voted to snap `view_left` to a valid boundary when set. That cannot +exist, and the reason generalizes: `view_left` is ONE per-window +column, but *"does column N bisect a wide glyph?"* is a **per-line** +question. No setter-time value is canonical for every visible line, +and snapping per line instead would break the vertical alignment a +column-oriented view exists to provide. Recorded because the same trap +waits for any future window-wide value derived from per-line content. -**`editing.fill-column` does not exist.** Framing §1.1 called it an -orphaned registry setting "of the exact shape Stage 1 just fixed" and -carried a deliverable to sharpen its description. Both cited -occurrences are inside `#[cfg(test)] mod tests` — -`src/config_registry.rs` and `src/lua_bindings/config.rs` — fixture -names in round-trip tests covering one setting per `ConfigKind`. Two of -those five names are real; three, including this one, are defined -nowhere else. +**One correction carried into revision 2.** Revision 1 claimed the +"unreachable past the edge" caveat is recorded in the setting's +description. It is not — `builtin/runtime/linewrap.lua:23` says only +"truncate at the edge"; the word appears in the toggle's status message +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). -Nineteen revisions and three review rounds inherited it. The mechanism -is worth keeping: a grep hit at a `src/` path, a genuine `r.define(...)` -call that is real API usage rather than a mock, and `#[cfg(test)]` about -fifty lines above the citation. **A file:line citation is not a -substitute for reading the scope it sits in** — and once a conclusion is -in a document, later rounds reason about its consequences rather than -re-check it. Cost: three paragraphs of framing and one carried -deliverable that had no object. Stage 3 ships a comment at both fixture -sites instead. +### What Stage 3 shipped that Stage 4 must live with -The four code defects, each caught by user review or by the tests -written for it: +- **`ui.line-wrap` is BUFFER-local** (`ConfigKind::Enum`, + `wrap`/`truncate`, default `wrap`). Q#LL2 recorded the consequence + and deliberately deferred it: `view_left` is unambiguously + **per-window**, because two panes on one buffer must scroll + independently exactly as they already hold independent `view_top`s + (`src/desktop.rs:92`, `src/window.rs:374`). So the two halves of one + user-facing concept land at different scopes. Emacs effectively does + this and it is survivable — but Stage 3 signed up for it as *a + decision*, and Stage 4 is where the bill arrives. +- **`truncate` is the mode Stage 4 makes navigable.** Today text past + the right edge is not merely off-screen but **unreachable** — and + that is stated **only** in `ui.toggle-line-wrap`'s status message and + a source comment, **not** in the setting's description + (`builtin/runtime/linewrap.lua:23` says just "truncate at the edge"). + A user who sets the mode in `init.lua` and never invokes the toggle + is told nothing. Amending the description is a Stage 4 deliverable + (framing §6); the status message is revisited when scroll lands. +- **Under `wrap`, horizontal scroll is meaningless.** Stage 4's surface + is therefore conditional on the mode, which is a coherence question + (one concept, two behaviors) and not only an implementation one. +- **The scroll indicator's `wrap` path takes byte percentages** from + `pmacs-protocol::scroll`. It is vertical-only and Stage 4 does not + change it — recorded because "scroll" in this lane means horizontal + and the two must not be conflated in review. -1. **The toggle wrote the global layer.** `ui.line-wrap` is - buffer-local, so `config.get(name)`/`config.set(name, ...)` reported - and flipped the *global* value — leaving a pinned buffer untouched - while silently moving every unpinned one. Invisible in any - single-buffer test. Fixed in `4a26f00`; witnesses use two buffers. -2. **The renderer never got the mode.** The frame resolved - `ui.line-wrap`, stored it on the window, and fed it to coordinate - mapping and the indicator — while the `Viewport` literal still - carried a hard-coded `Truncate`. Every "is the mode right?" test - passed and the text stayed clipped. Fixed in `aa3cd4d`; the new - witnesses read the **grid**, not the resolved value. -3. **The GPU indicator reckoned in source lines.** Pre-existing (the - GPU has always wrapped), but nameable only once `ui.line-wrap` - decided which formula applies. Fixed in `840a338` via - `code_byte_painted` — layout decides, not arithmetic over it. The - two cheap predicates are both wrong: `view_range` includes - `SCROLL_OVERSCAN`, `scroll_top` ignores the sub-line residual. -4. **An empty `view_range` is not an empty layout.** Found *by* the - tests in 3, not confirmed by them: a file ending in a newline has a - final empty line, and a viewport parked on it is `(len, len)` with - one real row. The guard borrowed from the caret path made **every - newline-terminated file report a percentage instead of `Bot`** at - the bottom. +### Ground truth gathered for the framing (verify before trusting) -**A process note that earned itself twice.** Two edits in this lane -were silently lost to `str.replace` calls that matched nothing (once -after an unrelated exception aborted the write). Both times the code -looked edited and was not — defect 2 above is one of them. Use the -Edit tool, which errors on mismatch, for anything load-bearing. +- **There is no horizontal scroll anywhere in the tree.** No + `view_left`, `scroll_left`, or `hscroll` in `src/` or `builtin/`. + This is greenfield, not an extension. +- **`paint_line` starts every walk at column 0** (`src/text_view.rs`), + which is the same walk Stage 3 rewrote for wrapping. A `view_left` + enters here, and the wrap rule (`advance_wrapped`) must stay written + exactly once. +- **The GPU cannot honor horizontal scroll through cosmic-text.** + `Scroll::horizontal` is discarded throughout, because **glyphon 0.11 + never applies it when placing glyphs** — documented at + `pmacs-gpu/src/main.rs:1611`, `:6316`, `:8020` and asserted by tests + at `:16266`, `:16337`, `:16737`. The GPU's Stage 4 half needs a + different mechanism entirely. **This is the fact most likely to + invert the cost estimate**, exactly as the "both frontends consume + the same `CellGrid`" error did in Stage 3 revision 1. +- **`view_top` is persisted per leaf** in `SavedLeaf` alongside + `cursor`, at `DESKTOP_VERSION = 1` (`src/desktop.rs:33`). A + `view_left` that survives a restart needs either a defaulted field or + a version bump — a decision, not an afterthought. +- **`scroll_window` carries the cursor with the scroll** to defeat the + renderer's "auto-scroll to keep cursor visible" pass, whose comment + already records the hazard: an unconditional snap-back makes explicit + scrolling feel stuck (`src/editor.rs:3624-3628`). A horizontal analog + faces the identical problem, and Q#LL3 deferred the choice — drag the + cursor, or let the next motion snap back — to this stage. +- **`goal_col` is the existing column-memory field** + (`src/window.rs:376`, cleared at seven sites in `src/editor.rs`). Its + relationship to a horizontal offset is unexamined and is a framing + question, not an implementation detail. -### The two decisions most likely to be questioned later +### Gate note this lane inherits -- **GUI users lose word wrap.** Character-wrap parity is cheap and - Emacs-consistent, but the GPU has word-wrapped since it existed. - Accepted deliberately (user, 2026-08-06). **Must appear in the PR - description and release notes**, not only in the framing. -- **The audit strategy is asymmetric on purpose.** The coordinate - functions gain a required context parameter so the **compiler - enumerates** every call site; `DisplayCoord` gains an **additive** - field so untouched consumers stay *correct* rather than merely - findable. Compiler-enforced where possible, correct-by-default where - not. +Stage 3 put eight broken version assertions on CI by running +`CLAUDE.md`'s short gate list instead of `docs/agent-handoff.md` §3's, +which includes a full sweep. **§3 is the authority.** -### Verification (delivered) +**Stage 4 should not need a protocol bump at all** — Q#HS1 puts the GPU +in Stage 5, and `view_left` is per-window TUI state with no wire. If +that changes, §3's protocol-bump form is +`cargo test --workspace --no-fail-fast -- --skip basedpyright` in +**both** feature configurations. -Everything framing §7 sketched, plus three groups it did not anticipate -(framing §7.1). - -**The gate list below is the one that FAILED to catch this lane's CI -red, and it is kept only to show the hole.** "The touched acceptance -suites" is selected from the diff, and a `PROTOCOL_VERSION` bump breaks -version-assertion tests that appear nowhere in it. Five failed on CI's -first round. Use **`cargo test --tests --no-fail-fast`** on any -protocol bump — `--no-fail-fast` because cargo stops at the first -failing target, which is why CI showed one failure and the local -full-corpus run then found three more. Recorded in -`docs/agent-handoff.md`. - -``` -cargo fmt --check -cargo clippy --workspace --all-targets -- -D warnings -cargo clippy --workspace --all-targets --features crdt -- -D warnings -cargo test --lib # 1917 / 0 -cargo test --lib --features crdt # 2102 / 0 -cargo test --tests --no-fail-fast # 108 targets, exit 0 -cargo test --tests --features crdt --no-fail-fast # 108 targets, exit 0 -PMACS_REQUIRE_GPU=1 cargo test -p pmacs-gpu # 228 / 0 -git diff --check -``` - -**Both configurations, and that mattered.** The default sweep went -clean while `--features crdt` still had **three** failures, in -crdt-gated real-daemon tests (`vterm_stage3_acceptance` ×2, -`bottom_panel_stage2b_gpu_acceptance` ×1) that assert on a real -socket's negotiated version. This is the handoff's existing "a local -sweep is blind to whichever feature configuration it does not build" -lesson, hit again by a different lane — so **eight** version -assertions broke in total, not five. - -The eight version-assertion failures, and what each one was: - -| test | was | why it broke | -|---|---|---| -| `acc51_a_v20_peer_..._even_when_capable` (×5 jobs) | `PROTOCOL_VERSION - 1` | **bug** — an absolute contract ("below the panel version") as arithmetic on a moving constant. Now `PANEL_MIN_VERSION - 1`, the idiom `src/` already used in five places | -| `the_baseline_stays_and_the_counter_offer_activates` | `PANEL_MIN_VERSION == PROTOCOL_VERSION` | **bug** — asserted a coincidence true only while panels were the newest feature. Now the two durable bounds | -| `the_panel_stage_takes_protocol_v21` | `PROTOCOL_VERSION == 21` | **bug** — the current wire as a proxy for the panel stage's own version, in a test whose name says which it means | -| `a54_real_daemon_..._panel_hosted_terminal` *(crdt)* | `session_protocol_version == "21"` | **bug** — the negotiated session version is *this binary's* wire, so the literal held only while panels were newest. Now `PROTOCOL_VERSION`, plus an explicit `>= PANEL_MIN_VERSION` for the capability the literal carried implicitly | -| `a37_real_daemon_..._one_terminal_session` *(crdt)* | `session_protocol_version == "21"` | **bug** — same | -| `a13_17_26_..._version_cost` | `PROTOCOL_VERSION == 21`, `6..=21`, `!supported(22)` | **tripwire working as designed** — it says in its own comment that it tracks the current wire. Took the conscious edit | -| `the_baseline_stays_...` (same test) | `PROTOCOL_VERSION == 21` | **tripwire working as designed** | -| `terminal_mode_keeps_reporting_presence_...` *(crdt)* | `PROTOCOL_VERSION == 21` | **tripwire working as designed** | - -**Five bugs and three tripwires.** The distinction is the useful part. -A tripwire that fires on a bump is doing its job, and "fixing" it means -editing it deliberately — the baseline pin `ADVERTISED_PROTOCOL_VERSION -== 20` is the one that must *never* be edited, and it never fired. - -The five bugs share one shape: **an absolute contract expressed as -arithmetic on, or equality with, a moving constant.** `PROTOCOL_VERSION -- 1` for "below the panel version"; `PANEL_MIN_VERSION == -PROTOCOL_VERSION` for a coincidence; `PROTOCOL_VERSION == 21` standing -in for the panel stage's own version; `"21"` for a negotiated session -version. Each was true when written and silently false afterwards. -`src/` already had the right idiom — `PANEL_MIN_VERSION - 1`, five -occurrences — and every outlier was in `tests/`. - -`tests/long_line_readable_acceptance.rs` is the one that answers the -**report** rather than a mechanism: the shipped binary, a real PTY, a -line 200 columns wide in an 80-column terminal, and an assertion that -the tail marker reaches the host. It bites — -`scripts/bite HEAD~2 src/editor.rs --test long_line_readable_acceptance` -against the pre-`aa3cd4d` editor never paints `TAILZQX` in 20s. Its -`truncate` control (an isolated `init.lua` pinning the mode) is what -makes that marker discriminating. - -**What the PTY test does not prove.** The vterm suites assert on raw -output bytes; there is no screen model and no `vt100`/`termwiz`/`vte` -in the workspace. It proves the tail was *written to the terminal*, not -that it occupies the row a human would point at. That is nonetheless -the whole of the original report — under truncation those bytes are -never emitted at all. - -**One caveat on the GPU line, stated rather than smoothed over.** One -`-p pmacs-gpu` run went `227 passed; 1 failed` before every run since -went 228/0. **The failing test name was not captured** — that command -was piped through `tail -3`, which kept the summary and discarded the -failure block. 36 later full runs are clean, 6 under deliberate -concurrent load. Per `docs/ci-red-signatures.md`'s rerun rule that -establishes intermittence only, and without a selector it does not even -establish that. Logged there as **U1**, explicitly *not* matched -against A1 (also GPU-headless-under-load) because matching requires a -selector and fragments this occurrence does not have. - -**And one more, logged as U2.** -`process::tests::m6_1_pty_raw_mode_disables_kernel_echo` failed once -during a full `--tests --no-fail-fast` run and did not reproduce in a -later full sweep (108 targets, exit 0) or 3 isolated `--lib` runs. It -is in no registry row, so it is a new incident; leaked -`pmacs --daemon` processes remain an unexcluded rival explanation. +**`--workspace`, not `--tests`**, and the distinction is not cosmetic: +`--tests` selects 108 targets where `--workspace` selects 110, and the +two it drops are **`pmacs_protocol` and `pmacs_gpu`**. Stage 3's own +*remediation* sweep used `--tests`, so it never ran `pmacs-protocol`'s +25 tests — including the `scroll::classify` tests that lane had just +written. They passed, but by luck, and a correction that reproduces the +shape of its own mistake is worth naming. ### Not in scope -Horizontal scroll in full (Stage 4). `M-q` / auto-fill / reflow. Word -wrap as a mode value — a named future third choice, not this stage. -Bidi/RTL. Soft-wrap gutter indicators. +`M-q` / auto-fill / reflow. Word wrap as a mode value — a named future +third choice. Bidi/RTL. Soft-wrap gutter indicators. ## Tree primitive (P5) — MERGED as #217; adoption is the open work diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 48a29fd..2029b9d 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -83,7 +83,66 @@ reads it the way you just did. For volatile branches, checkpoints, verification, and recovery commands, read `docs/active-work.md` immediately after this file. -## 1. Where the project stands (2026-08-06) +## 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. + - **#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 + already existed, quantizing the step so the round-trip guarantee is + exact rather than approximately true. + - **#221** added **`ui.line-wrap`** — `ConfigKind::Enum` + (`wrap`/`truncate`), default `wrap`, **buffer-local**, with + `ui.toggle-line-wrap`. Both frontends honor it: the grid renderer + through `Viewport`, the GPU through + **`InstanceMessage::LineWrapFacts` at protocol v22**, resent on + attach, config change, **and buffer switch** (the mode is per + buffer, so a `FontFacts`-shaped global design is silently wrong). + `ADVERTISED_PROTOCOL_VERSION` stays pinned at 20. + + Three durable consequences: + - **GUI users lost word wrap.** The GPU document buffer's first + explicit `set_wrap` is `Wrap::Glyph`. Character wrap is what the + grid can implement identically without UAX #14; a whitespace + approximation of cosmic-text's real breaking was judged worse + than honest divergence. Word wrap is a clean additive third + choice. + - **`pmacs-protocol::scroll`** owns `ScrollPosition` and a pure + `classify(first_visible, last_visible, byte_pos, byte_len)`. + `pmacs-gpu` depends on `pmacs-protocol` and never on the `pmacs` + lib, so the status readout had been duplicated *structurally* — + and during this lane's own review a fix landed in one copy while + the other kept reporting `All` for a wrapped one-line buffer. + Each frontend supplies local layout facts and renders the string; + the shared crate owns the decision. No wire message, no version + bump. **Under `truncate` the old line-space formatter is + untouched**, so its output is identical by construction. + - **The GPU answers "is this byte on screen?" with + `code_byte_painted`** — `code_byte_px` intersected with the + 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. + + `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`. - **`main` @ `db1bbe9`.** The **tree primitive #217** — `listview` rows take optional `depth`/`id`, collapse is primitive-owned, folding is @@ -405,20 +464,31 @@ someone forgot. failure, so **check `pgrep -f "pmacs --daemon"` before trusting a local red**. Lane recorded in `docs/active-work.md`. - **A `PROTOCOL_VERSION` bump's blast radius is every version-sensitive - test, and NONE of them appear in the diff.** "The touched acceptance - suites" is the standing gate, and for a protocol bump it is the wrong - selector: long-lines Stage 3 bumped v21→v22, ran the suites it had - edited, and broke **eight** version assertions across six suites. CI - showed exactly **one**, because **cargo stops at the first failing - target**; the rest surfaced only afterwards, and one at a time would - have cost four more red rounds. + test, and NONE of them appear in the diff.** Long-lines Stage 3 + bumped v21→v22 and broke **eight** version assertions across six + suites. CI showed exactly **one**, because **cargo stops at the first + failing target**; the rest surfaced only afterwards, and one at a + time would have cost four more red rounds. - **On any protocol bump run `cargo test --tests --no-fail-fast` in - BOTH feature configurations.** `--no-fail-fast` because of the - stop-at-first-target behavior above, and `--features crdt` because - three of the eight were in crdt-gated real-daemon tests that assert - on a live socket's negotiated version — invisible to a default sweep, - which is the blindness the bullet below already names. + **The first thing to say is that §3's gate list would have caught + it.** `cargo test --workspace -- --skip basedpyright` is in that list + and was not run — `CLAUDE.md` carries a shorter list ending at "the + touched acceptance suites", and that shorter list is what the lane + followed. **When the two disagree, §3 is the authority**; the short + form is a summary, and a summary of a gate suite is not a gate suite. + + **But §3's sweep alone would still have understated it**, which is + why this bullet exists rather than just a pointer. Plain + `--workspace` stops at the first failing target and builds one + feature configuration, so it would have shown one or two of the + eight. §3 now carries the strengthened form: **`--workspace + --no-fail-fast -- --skip basedpyright` in BOTH configurations.** + + **Note `--workspace`, not `--tests`** — the lane's own remediation + sweep used `--tests`, which silently drops the `pmacs_protocol` and + `pmacs_gpu` targets. On a *protocol* bump that omits the protocol + crate's tests, which is how a correction can reproduce the shape of + the mistake it is correcting. **Sort the failures before fixing them.** A *tripwire* (`assert_eq!(PROTOCOL_VERSION, N)`) is meant to fire and takes a @@ -2065,6 +2135,40 @@ cargo test --workspace -- --skip basedpyright # full sweep git diff --check ``` +**The full sweep is not optional, and `CLAUDE.md`'s shorter list is not +a substitute.** That list stops at "the touched acceptance suites"; +this one continues. Long-lines Stage 3 followed the short form and put +eight broken version assertions on CI. When the two disagree, **this +list wins**. + +**Touching `PROTOCOL_VERSION` STRENGTHENS the sweep line. It does not +replace it:** + +``` +cargo test --workspace --no-fail-fast -- --skip basedpyright +cargo test --workspace --features crdt --no-fail-fast -- --skip basedpyright +``` + +Every part is load-bearing: + +- **`--workspace`, never `--tests`.** `--tests` selects 108 targets + where `--workspace` selects 110, and the two it drops are + **`pmacs_protocol` and `pmacs_gpu`**. On a protocol bump, dropping + the protocol crate's own unit tests is precisely the wrong loss. + Long-lines Stage 3 swept with `--tests` and so never ran + `pmacs-protocol`'s 25 tests — including the `scroll::classify` tests + that same lane had just written. They passed, but by luck. +- **`--no-fail-fast`**, because cargo stops at the first failing + target: without it a bump that breaks eight assertions reports one. +- **`--features crdt`**, because crdt-gated real-daemon tests assert on + a live socket's negotiated version and are invisible otherwise. +- **`-- --skip basedpyright`** for the same reason the line above + carries it. + +See §5's protocol-bump bullet for how to sort the failures this finds — +some are tripwires doing their job, and one pin +(`ADVERTISED_PROTOCOL_VERSION`) must never be edited at all. + Machine-specific caveats — re-verify on a machine you haven't used before trusting them: diff --git a/docs/ci-red-signatures.md b/docs/ci-red-signatures.md index 0f2154b..288b75b 100644 --- a/docs/ci-red-signatures.md +++ b/docs/ci-red-signatures.md @@ -423,6 +423,28 @@ without a name there is nothing to call intermittent. with `grep -E "FAILED|panicked|test result"`, which keeps failure context, or capture the full log to a file and summarize from it. +### R7 — managed-retry attach hits a broken pipe under full-sweep load + +The first incident this session with a **complete** signature, so it is +a matchable row rather than a `U` note. Recorded during long-lines +Stage 4; the lane touches no `pmacs-gpu` code at all. + +| field | value | +|---|---| +| **selector** | `-p pmacs-gpu attach::tests::managed_retry_survives_transients_and_uses_the_successful_stream` | +| **job / flavor** | local (Linux), `cargo test --workspace --features crdt --no-fail-fast`, i.e. under full-sweep load | +| **required fragments** | `transient sequence must attach` + `Handshake(Io(` + `BrokenPipe` (or `code: 32`) | +| **status** | **new incident, unreproduced — causal status UNRESOLVED** | +| **what IS established** | one occurrence at `pmacs-gpu/src/attach.rs:1680`; the test drives a scripted transient-then-success sequence over a real socket pair | +| **what is NOT** | whether the broken pipe is the *fixture's* writer closing early or a real retry-path defect. **This row is not a claim that it is harmless** | +| **rerun evidence** | 6 isolated runs green, plus a full `--workspace --features crdt` sweep green (113 targets). Per the rerun rule this establishes **intermittence only** | +| **retirement** | hardening that removes the named mechanism plus a discriminating witness — or a diagnosis showing the fixture, not the code, closes the pipe | + +**Not attributed to this lane**, and the reasoning is not merely "my +diff looks unrelated": Stage 4 adds no wire surface, no protocol +version change, and touches no file in `pmacs-gpu`. A merge-base +control would settle it if this recurs. + ### U2 — `m6_1_pty_raw_mode_disables_kernel_echo`, one local occurrence Has a selector, which U1 lacks — but still no fragments, so it cannot diff --git a/docs/horizontal-scroll-framing.md b/docs/horizontal-scroll-framing.md new file mode 100644 index 0000000..35ed627 --- /dev/null +++ b/docs/horizontal-scroll-framing.md @@ -0,0 +1,481 @@ +# Horizontal scroll — QoL Stage 4 + +**Status: revision 4 — APPROVED 2026-08-07. Every question answered. +Implementation may begin within this scope.** + +> *"Q#HS7's per-line effective edge and preserved tab-forward mapping +> are coherent and fully specified. Q#HS5 is approved with the required +> serde default and literal-v1-fixture conditions. The Stage 4/Stage 5 +> split and Rule 4 protection are now durably reflected in the handoff +> and ledger."* + +| question | state | +|---|---| +| Q#HS1 — GPU in scope? | **answered**: no, Stage 5, time-boxed (§3) | +| Q#HS2 — what moves the viewport? | **answered**: automatic only | +| Q#HS3 — window or buffer? | re-confirmed: per window | +| Q#HS4 — cursor follows explicit scroll? | **deferred** — not live under HS2 | +| Q#HS5 — persist `view_left`? | **approved**: yes, no version bump, on two conditions | +| Q#HS6 — the default | **answered**: `wrap` stays | +| Q#HS7 — what IS `view_left`? | **accepted**: (a), (b), (c′), (c″), (d) | + +Revision 4 adds only Q#HS7(c″) — the tab-straddle mapping — and fixes +the handoff's "Stage 4 is the remainder" to name Stages 4–5. + +**Stage 4 does NOT close the QoL arc.** Revision 1 said it did, and +that was written before Q#HS1 moved GPU horizontal scroll to Stage 5. +The claim is not merely stale — it is load-bearing in the wrong +direction: `docs/active-work.md`'s **Rule 4 removes a lane when its +ARC is done**, so a framing asserting Stage 4 closes the arc would +license retiring this lane at the TUI merge, **orphaning the very +Stage 5 that Q#HS1's time box exists to guarantee**. The arc closes at +**Stage 5**. + +Stage 1 (#219) made the TUI survive terminal zoom; Stage 2 (#220) gave +the GUI native zoom; Stage 3 (#221) added `ui.line-wrap` and made +`wrap` the default. Stage 4 is the TUI half of the other half of the +user's own sentence: + +> long lines need to either **wrap somehow or be scrollable**. […] This +> should also be something that the user can configure, whether to wrap +> or scrollable. + +Stage 3 shipped the *mode*. It did not ship the *navigation*: under +`truncate`, text past the right edge is not merely off-screen but +**unreachable**. + +**Revision 1 claimed that caveat "is recorded in the setting's +description". It is not.** `builtin/runtime/linewrap.lua:23` says only +*"How a line wider than the window is shown: wrap onto following rows, +or truncate at the edge."* The word "unreachable" appears in +`ui.toggle-line-wrap`'s status message and in a source comment — neither +of which a user sees if they set `ui.line-wrap = "truncate"` in +`init.lua` and never invoke the toggle. **That is a real, if small, +user-facing gap shipped in #221**, and §6 makes amending the +description a Stage 4 deliverable rather than leaving the false claim +standing. + +--- + +## 1. What is actually there today + +**Verified in the tree at `02f3ec3`, not recalled.** Stage 3's revision +1 inverted its whole cost model by assuming both frontends consumed the +same `CellGrid`; every claim below carries a citation for that reason. + +### 1.1 There is no horizontal scroll anywhere + +No `view_left`, `scroll_left`, or `hscroll` in `src/` or `builtin/`. +The window carries `view_top`, `cursor`, and `goal_col` +(`src/window.rs:374-376`) and nothing horizontal. This is greenfield — +not "extend the vertical mechanism sideways", because there is no +shared abstraction to extend. + +### 1.2 The grid walk starts every line at column 0 + +`paint_line` (`src/text_view.rs:266`) walks from the line's first +character with no offset parameter, exactly as before Stage 3 — +wrapping changed *where rows break*, not *where the walk starts*. +`place_of_byte` and `byte_at_place` have the same shape. + +So `view_left` enters the same functions Stage 3 just rewrote, and +**the wrap rule must stay written exactly once** (`advance_wrapped`, +`src/text_view.rs:396`). A second copy differing by an offset is the +defect Stage 3 spent its review budget avoiding. + +### 1.3 The GPU cannot use cosmic-text's horizontal scroll + +**The finding most likely to invert the cost estimate, so it leads.** + +`Scroll::horizontal` is discarded throughout the GPU, and not by +oversight: **glyphon 0.11 never applies it when placing glyphs.** +Documented at `pmacs-gpu/src/main.rs:1611`, `:6316`, `:8020`, and +*asserted* by tests at `:16266` (`"horizontal is discarded"`), `:16337`, +`:16737`. + +The GPU's half therefore cannot be "set the scroll and reshape". It +needs a mechanism that does not exist — a shifted text origin at paint +time, adjusted clip bounds, or something else — interacting correctly +with the gutter, the caret (`code_byte_px`), decoration geometry +(`push_glyph_extent_rects`), and hit testing (`gutter_aware_rel_x`), +each of which assumes x starts at `text_left()`. + +**Answered in Q#HS1: the GPU is Stage 5.** + +### 1.4 `view_top` is persisted; a `view_left` would want to be + +`SavedLeaf` carries `path`, `cursor`, and `view_top` at +`DESKTOP_VERSION = 1` (`src/desktop.rs:33`, `:276-280`); the restore +path clamps `view_top` against the line count (`:512`). **Q#HS5.** + +### 1.5 The cursor-follow hazard is already documented + +`scroll_window` (`src/editor.rs:3628`) carries the cursor with a +vertical scroll, and its comment says why: + +> The cursor must follow the scroll: the renderer has an "auto-scroll +> to keep cursor visible" pass that would otherwise snap `view_top` +> straight back to wherever the cursor sits, so the user's mouse-wheel +> scroll would feel stuck after one notch. + +Under Q#HS2's answer (automatic-only) this pass is not a hazard but +**the entire mechanism** — Stage 4 adds its horizontal component. The +hazard returns with explicit commands, which is why Q#HS4 is deferred +rather than closed. + +### 1.6 `goal_col` exists and its relationship to `view_left` is unexamined + +`goal_col` (`src/window.rs:376`) remembers a target column across +vertical motion, cleared at seven sites in `src/editor.rs`. It is a +*column within the line*; `view_left` is a *viewport offset*. Both are +horizontal state on the same window, and a design ignoring the +interaction produces a cursor that jumps on the first vertical motion +after a horizontal scroll. Feeds **Q#HS7**. + +--- + +## 2. The scope fact, stated before the answers + +**Under `wrap`, horizontal scroll is meaningless** — nothing sits past +the right edge. So Stage 4's entire surface is conditional on a +buffer-local mode: one user-facing question ("how do I see the rest of +this line?") gets two disjoint answers depending on a setting. Stated +here because `COHERENCE.md` §20 requires it, and answered in Q#HS6. + +--- + +## 3. Questions + +### Q#HS1 — is the GPU in scope? **ANSWERED: no — Stage 5** + +> **Answered 2026-08-07 (user):** split the GPU work into Stage 5. +> *"This is a conscious, bounded divergence, not a repeat of Stage 3's +> accidental one. Make the time box concrete and keep `wrap` default +> until parity lands."* + +The distinction is the load-bearing part. Stage 3's defect was never +"the frontends differ" — it was "the frontends differ and **nobody +chose that**". A divergence that is decided, recorded, and bounded is a +different object from one inherited from a library default. + +**The time box, concrete** (the user's requirement, and the part that +makes this a decision rather than a deferral): + +1. **Stage 5 is the immediately-next QoL lane after Stage 4 merges** — + not backlogged behind another arc. If something displaces it, that + displacement is itself a decision to record here. +2. **`wrap` stays the default until Stage 5 lands** (independently + reaffirmed in Q#HS6). This is what keeps the divergence invisible to + anyone who has not opted in: a default-configuration user is never + exposed to it. +3. **Stage 4's release notes must state the asymmetry** — horizontal + scroll works in the TUI and not yet the GUI — in the same way #221's + had to state the word-wrap loss. +4. **While the gap exists, the `truncate` affordances must name it.** + §6's description amendment is where that lands, so a GUI user + choosing `truncate` learns the limitation from the setting rather + than from the behavior. + +### Q#HS2 — what moves the viewport? **ANSWERED: automatic only** + +> **Answered 2026-08-07 (user):** *"Automatic-only first. It makes the +> report's text reachable with no new command surface."* + +The cursor-visibility pass gains a horizontal component, so moving the +cursor past the edge scrolls the view. No new commands, no binding +decisions, no new interaction island. + +Explicit `ui.scroll-left` / `ui.scroll-right` are **not** in Stage 4. +They can follow with evidence of use, and they are what re-opens Q#HS4. + +### Q#HS3 — per window or per buffer? **NOT ACTUALLY OPEN** + +`view_left` is **per window**, unambiguously: two panes on one buffer +must scroll independently, exactly as they already hold independent +`view_top`s (`src/desktop.rs:92`). Stage 3's Q#LL2 recorded this and +accepted the consequence — the *mode* is buffer-local while the +*offset* is per-window, so one user-facing concept spans two scopes. + +Listed so the accepted split is re-confirmed where it takes effect +rather than inherited silently. + +### Q#HS4 — does the cursor follow an explicit scroll? **DEFERRED, not answered** + +Not live under Q#HS2's answer: automatic-only means every viewport move +already originates from a cursor move. §1.5 holds the precedent and the +hazard for whenever explicit commands arrive. + +Deferring rather than deleting, because the hazard is real and +rediscovering it costs more than carrying the paragraph. + +### Q#HS5 — does `view_left` survive a restart? **APPROVED: yes** + +> **Approved 2026-08-07 (user):** persist `view_left` **without** a +> desktop-version bump, **provided implementation adds +> `#[serde(default)]` and a literal v1 JSON fixture omitting the field, +> asserting restoration at zero.** Both conditions are part of the +> approval, not advice attached to it. + +`view_top` does (§1.4). Consistency argues yes. + +**Verified, not assumed:** `SavedLeaf` is a plain +`#[derive(Serialize, Deserialize)]` (`src/desktop.rs:85`) with **no +`#[serde(default)]` on any field and none anywhere in the file**. So +serde will **reject** a version-1 desktop JSON that omits a newly added +`view_left` — a missing field is a deserialization error, not a zero. +Revision 2 cited the struct's shape as though it settled serde's +behavior on it; it did not. + +**"Yes, persisted, no `DESKTOP_VERSION` bump" is sound only with both +of:** + +1. **`#[serde(default)]` on the new field.** This is the whole of the + new-binary-reads-old-file direction. +2. **A regression fixture**: a literal version-1 desktop JSON with no + `view_left`, deserialized in a test, asserting it restores at offset + 0 rather than erroring. Without this, (1) is an untested claim about + a crate's behavior — which is precisely the failure this question + was reopened for. + +**The other direction already works, and that is why no bump is +needed.** An old binary reading a new file passes the version check +(`version` is still 1, `src/desktop.rs:366`) and then meets an unknown +`view_left` field — which serde **ignores** by default, and +`src/desktop.rs` sets no `deny_unknown_fields` anywhere (verified). So +both directions are safe at `DESKTOP_VERSION = 1` **given (1)**, and +neither is safe without it. + +### Q#HS6 — the coherence statement **ANSWERED: keep `wrap` default** + +> **Answered 2026-08-07 (user):** *"Keep `wrap` as default for now. +> Revisit only after GPU parity and use evidence; changing to +> `truncate` before Stage 5 would make the default unreachable in the +> GUI."* + +That last clause is the argument revision 1 missed. I had framed this +as "if scroll makes `truncate` good, the default deserves +re-examination" — but with the GPU deferred to Stage 5, a `truncate` +default would ship a mode that is **navigable in the TUI and a dead end +in the GUI**, for every user who never opened the setting. Q#HS1 and +Q#HS6 are therefore coupled: the split is only safe *because* the +default does not move. + +Revisit after Stage 5, on use evidence, not before. + +### Q#HS7 — what IS `view_left`? **ACCEPTED (revision 4)** + +> **Accepted 2026-08-07 (user):** *"keep `view_left` as an unsnapped +> window display column; derive the effective edge per line; render a +> bisected wide glyph's trailing cell as styled blank and designate it +> to the glyph start. The multi-line discriminating witness is exactly +> right."* Plus (c″) below, on the user's recommendation. + +**Revision 1 decided what moves the viewport without ever saying what +the viewport offset is.** That is the same omission Stage 3 would have +made had it shipped `WrapMode` without `DisplayCoord`: the mode is +useless until the coordinate contract is written down, and the contract +is where every sharp edge lives. + +Revision 1's verification sketch named tabs and wide characters. **It +had no oracle for either**, because nothing defined what a left edge +is. Four things must be settled together: + +**(a) The unit.** Candidates: a source byte offset within the line; a +display column (cells from the line start, after tab expansion); or a +cell boundary with an explicit validity rule. + +*My vote: display column.* Tab expansion depends on the absolute column +from the line start, so the walk must begin at column 0 and compute +forward **regardless** of the offset — which makes a byte offset buy +nothing and lose tab correctness. Starting at 0 and suppressing paint +until `col >= view_left` preserves tab stops **for free**, and costs no +more than `paint_line` already pays under wrapping. + +**(b) What happens at a left edge that bisects a glyph.** A tab +straddling the edge is unambiguous: its expansion is width-1 spaces, so +the remaining ones paint. **A wide (width-2) glyph is not** — a grid +cannot paint half of one. + +**(c) ~~The snap rule when an invalid edge is requested.~~ WITHDRAWN +(revision 3).** + +> Revision 2 voted *"a left edge may not fall inside a wide glyph"* plus +> *"snap toward the line start, at the moment `view_left` is set"*. +> **That cannot hold, and the reason is structural rather than a detail +> to tune.** +> +> `view_left` is **one** per-window display column. "Does column N +> bisect a wide glyph?" is a **per-line** question: column 11 can be a +> wide glyph's trailing cell on line 3 and an ordinary ASCII cell on +> line 4. **No single setter-time value is canonical for every visible +> line**, so a snap performed once is simply wrong for most of them — +> and the invariant in (d), which the whole question exists to serve, +> would stay undefined exactly where it matters. +> +> Snapping *per line* is the other way to read it, and it is worse: the +> same source column would then appear at different screen columns on +> different rows, destroying the vertical alignment that a +> column-oriented view exists to provide. + +**(c′) The per-line effective edge, which replaces it.** + +`view_left` is stored **unsnapped** — the requested display column, +constrained only to `>= 0` and whatever maximum the design picks. Each +line derives its own **effective edge** during the walk it already +performs from column 0. + +When the requested edge bisects a wide glyph *on this line*, that +glyph's trailing cell is the leftmost visible cell. It cannot be +painted as half a glyph, so: + +- **It paints as a space**, carrying the glyph's own cell style. +- **The mapping designates that cell to the wide glyph's START byte.** + +Both halves are load-bearing, and the second is the part the finding +correctly says was missing: + +- The cell visually belongs to that character, so a click there + selecting it is what a user expects. +- It keeps `byte_at_place` **total** over visible cells — every painted + cell maps to some byte, with no hole at column 0. +- It preserves the round trip: `place_of_byte(glyph_start)` reports the + straddle and designates cell 0, so `byte_at_place(0) == glyph_start`. + +**And the direction rule `place_of_byte` needs at the left edge:** a +byte whose cells lie *entirely* left of the effective edge is **not +visible**, and `place_of_byte` must report that rather than clamping to +column 0. Clamping would make arbitrarily many bytes share cell 0 and +destroy (d). Only the straddling glyph designates cell 0. + +**This is deliberately NOT the mirror of Stage 3's right-edge rule**, +and the asymmetry should be stated so nobody "fixes" one to match the +other. Under `wrap`, a wide glyph that will not fit at the right edge is +pushed to the next row **entirely** (`advance_wrapped`, with its +`max_cols >= 2` guard). At the left edge under `truncate` there is no +next row to push to, so the blank-plus-designation rule is what the +same intent requires here. + +**(c″) A tab whose expansion straddles the edge — PRESERVED, not +chosen.** + +Each visible tab-expansion cell maps to **the byte immediately after +the tab**. This is not a new rule: it is what `byte_at_place` already +does, and its doc comment says so — *"Rounds forward to the next +character boundary… matching the unwrapped `display_to_pos`"* +(`src/text_view.rs:224`). The walk accumulates `walked` past the tab +byte and returns on the *next* character's `start_col`, so every column +inside the expansion already yields the post-tab offset +(`src/text_view.rs:243-254`). + +So the requirement on Stage 4 is **that horizontal scroll not perturb +it**: with the expansion's leading cells scrolled off, the surviving +cells must still map post-tab, exactly as they do at offset 0. + +**Why this direction differs from (c′)'s, which is the obvious +objection.** A wide glyph's two cells belong to **one character**; +forward-rounding its trailing cell would designate it to the *next* +character and leave the straddling glyph with **no visible cell mapping +to it at all** — unreachable by click precisely when it is the thing +the user scrolled toward. A tab's expansion cells are whitespace +*between* the tab byte and the next character, and forward-rounding +them is already how clicking in indentation lands at the start of the +text. Different directions, one principle: **every visible cell is +designated to the byte a user would mean by clicking it.** + +With (c′) and (c″) together, the (d) contract is total over visible +cells: ordinary character → its own start byte; bisected wide glyph → +the glyph's start byte; tab expansion → the byte after the tab. + +**(d) The invariant rendering and coordinate mapping share.** For a +given `view_left` **and line**, `byte_at_place` must invert +`place_of_byte` on every canonical input, and `paint_line` must place +exactly the bytes `place_of_byte` claims. If the painter clips where the +mapper does not, **clicks land on the wrong character** — silently, and +only for lines wide enough to scroll. + +**"And line" is what (c′) forced**, and it is the whole of that +finding: the invariant is not a property of `view_left` alone. It is a +property of `(view_left, line)`, because the effective edge is derived +per line. A test that fixes one line and sweeps offsets will not see +the failure; the oracle has to sweep **lines whose glyph widths differ +at the same column**. + +*This is the invariant the verification sketch needs as its oracle*, +and it is why Q#HS7 blocks: §4 cannot be written until it exists. + +--- + +## 4. Verification sketch (depends on Q#HS7) + +- Cell-level tests at several window widths **at non-zero offset** — + the case Stage 3's sketch explicitly refused, because "at non-zero + offset" was a `view_left` requirement smuggled into a wrap lane. +- **A `wrap` control for every claim**, asserting the wrap path is + byte-identical with a horizontal offset present. Under `wrap` the + offset must be **inert**, not merely harmless. +- Round-trip identity for `place_of_byte` / `byte_at_place` at non-zero + offset — the Q#HS7(d) invariant, walked exhaustively over a short + line rather than sampled, as Stage 3 established. +- **The Q#HS7(c′) case**: a wide glyph straddling the left edge — the + trailing cell blank, and `byte_at_place` on it returning the glyph's + **start** byte. +- **The Q#HS7(c″) case**: a tab whose expansion straddles the edge, + with every surviving cell still mapping to the byte **after** the + tab. This one is a **regression** witness rather than a new claim — + `byte_at_place` already behaves this way at offset 0 + (`src/text_view.rs:224`), so the test asserts scroll did not perturb + it, and it should fail if the walk is "optimized" to start at the + effective edge instead of column 0. +- **A multi-line fixture whose glyph widths DIFFER at the same column** + — the case (c′) exists for, and the one a single-line sweep cannot + reach. At one `view_left`, one line must take the straddle path and + another the ordinary path, with the (d) invariant holding on both. + A test that fixes one line and sweeps offsets passes against the + withdrawn setter-time snap, which is what makes this the + discriminating fixture rather than an extra one. +- **A PTY acceptance test for reachability**, following + `tests/long_line_readable_acceptance.rs`. That file's `truncate` + control currently asserts the tail is **absent** — Stage 4 must + update it, and **that update is itself the proof the caveat is + gone**. +- **No GPU witness in Stage 4** (Q#HS1). Stage 5 owes one that the + caret, a decoration, and a hit test all agree with the shifted + origin — the three consumers §1.3 names. + +--- + +## 5. Coherence impact (§20 requirement) + +- **Journey step 4, "Understand interface — Partial."** Stage 4 + completes "a line that cannot be read in full" for `truncate` **in + the TUI only**. The scorecard row should say so rather than reading + as closed. +- **§16 Semantic Frontend Architecture.** Q#HS1 *widens* frontend + divergence for the duration of the Stage 4→5 gap. Per the answer, + this is deliberate and time-boxed, and materially different from + Stage 3's inherited accident — but the release notes must say which + kind it is, or a reader cannot tell them apart. +- **No new interaction island** — automatic-only adds no command + surface (Q#HS2). +- **Config registry: no new settings expected.** Stage 4 navigates the + mode Stage 3 declared. If it needs a setting, that is a signal the + design has drifted, not a feature. + +--- + +## 6. A Stage 4 deliverable that is not scroll + +**Amend `ui.line-wrap`'s description** (`builtin/runtime/linewrap.lua`). +Today it says only *"…or truncate at the edge"*, which does not tell a +user that the edge is a wall. The honest text depends on where Stage 4 +lands: + +- **Before Stage 4**, `truncate` means unreachable in both frontends. +- **After Stage 4**, it means reachable in the TUI and unreachable in + the GUI until Stage 5 (Q#HS1's time box, item 4). +- **After Stage 5**, the caveat is gone and the sentence should shrink + back. + +Carried here rather than filed elsewhere because it is the one place +the arc's user-visible honesty is currently wrong, and revision 1 +asserted it was already right. diff --git a/src/desktop.rs b/src/desktop.rs index 16c15cc..a79ce43 100644 --- a/src/desktop.rs +++ b/src/desktop.rs @@ -90,6 +90,20 @@ pub struct SavedLeaf { pub cursor: u64, /// First visible source line. pub view_top: usize, + /// First visible display column — horizontal scroll (Stage 4). + /// + /// **`#[serde(default)]` is load-bearing, not tidiness.** Nothing + /// else in this file carries it, so without it serde would REJECT + /// every desktop written before this field existed: a missing field + /// is a deserialization error, not a zero. That is why no + /// `DESKTOP_VERSION` bump is needed — and why removing this + /// attribute would silently orphan every user's saved desktop. + /// + /// The reverse direction needs nothing: an older binary meets an + /// unknown field, which serde ignores absent `deny_unknown_fields` + /// (this file sets none). + #[serde(default)] + pub view_left: u32, } /// Serde mirror of [`Orientation`] (which is not itself serde). @@ -277,6 +291,7 @@ pub fn snapshot(core: &EditorCore, session_key: String) -> Option path: path.display().to_string(), cursor: win.cursor, view_top: win.view_top, + view_left: win.view_left, }) }; @@ -384,6 +399,7 @@ struct RestoreLeaf { window: WindowId, cursor: u64, view_top: usize, + view_left: u32, } /// Do the structural rebuild: open buffers, prune the old LOCAL layout, @@ -476,6 +492,11 @@ pub fn restore_into( if let Some(win) = c.windows.get_mut(&leaf.window) { win.cursor = leaf.cursor; win.view_top = leaf.view_top; + // Re-applied for the same reason `cursor` and `view_top` + // are: `buffer.after-load` can move the window, and the + // desktop is authoritative over whatever a hook (saveplace) + // did (Q#DS3). + win.view_left = leaf.view_left; } } core.borrow_mut().set_active_window_id(active_wid); @@ -514,11 +535,19 @@ fn build_restore_node( let mut win = Window::new(wid, buffer_id, text_view); win.cursor = cursor; win.view_top = view_top; + // Not clamped, unlike `view_top` against the line count. + // There is no cheap column bound (it would mean measuring + // the widest visible line), and none is needed: the + // horizontal follow pass moves the offset to the cursor on + // the first frame, so a stale value from a since-shortened + // file corrects itself rather than persisting. + win.view_left = leaf.view_left; core.windows.insert(wid, win); leaves.push(RestoreLeaf { window: wid, cursor, view_top, + view_left: leaf.view_left, }); save_slots.push(Some(wid)); Some(LayoutNode::Leaf(wid)) @@ -597,11 +626,13 @@ mod tests { path: "/a.rs".into(), cursor: 10, view_top: 2, + view_left: 0, }), SavedNode::Leaf(SavedLeaf { path: "/b.rs".into(), cursor: 0, view_top: 0, + view_left: 0, }), ], }, @@ -611,11 +642,45 @@ mod tests { assert_eq!(serde_json::from_str::(&json).unwrap(), d); } + /// A desktop written **before** `view_left` existed must still load + /// (Stage 4, framing Q#HS5 — a condition of that approval, not a + /// nicety). + /// + /// This is a literal v1 document, not one produced by serializing + /// the current struct: a generated fixture would gain the field and + /// prove nothing. `SavedLeaf` carries no other `#[serde(default)]`, + /// so without that attribute serde treats the missing field as an + /// **error** and every saved desktop in the wild stops loading — + /// which is exactly why no `DESKTOP_VERSION` bump was needed and why + /// deleting the attribute must fail here rather than in the field. + #[test] + fn a_desktop_saved_before_horizontal_scroll_still_loads() { + let v1 = r#"{ + "version": 1, + "session_key": "cwd.abc", + "buffers": [{"path": "/a.rs", "modified": false}], + "root": {"Leaf": {"path": "/a.rs", "cursor": 7, "view_top": 3}}, + "active_leaf": 0 + }"#; + let saved: SavedDesktop = serde_json::from_str(v1) + .expect("a pre-Stage-4 desktop must load, not error on a missing field"); + let SavedNode::Leaf(leaf) = &saved.root else { + panic!("expected a single leaf"); + }; + assert_eq!(leaf.cursor, 7, "the fields that existed are unchanged"); + assert_eq!(leaf.view_top, 3); + assert_eq!( + leaf.view_left, 0, + "and the new one restores unscrolled rather than erroring" + ); + } + fn leaf(path: &str) -> SavedLeaf { SavedLeaf { path: path.into(), cursor: 0, view_top: 0, + view_left: 0, } } diff --git a/src/diag.rs b/src/diag.rs index 4ba4d6d..7a28b91 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -553,7 +553,6 @@ impl View for DiagnosticView { let start_line_buf = line_at_offset(&line_offsets, viewport.buffer_start as u32); let max_rows = viewport.cell_size.rows; - let max_cols = viewport.cell_size.cols; let cell_origin = viewport.cell_origin; // Column-0 line markers (gutter signs, T M4.6): most severe @@ -629,12 +628,14 @@ impl View for DiagnosticView { }; let (start_col, end_col) = underline_cols_for_line(line_bytes, byte_start, byte_end); - if end_col <= start_col { + // `visible_cols` returns `None` for an empty range too, so the + // old `end_col <= start_col` guard is subsumed rather than + // dropped. + let Some((clamped_start, clamped_end)) = viewport.visible_cols(start_col, end_col) + else { continue; - } + }; let cell_row = cell_origin.row + row_offset; - let clamped_start = start_col.min(max_cols); - let clamped_end = end_col.min(max_cols); for col in clamped_start..clamped_end { let cell = cells.at(CellCoord::new(cell_row, cell_origin.col + col)); cell.style = merge_styles(cell.style, style); @@ -648,7 +649,9 @@ impl View for DiagnosticView { cells, cell_origin, viewport.gutter_w, - max_cols, + // Gutter-anchored, so unaffected by the horizontal offset: + // a sign lives left of the text area, not in it. + viewport.cell_size.cols, &line_markers, theme.as_ref(), ); @@ -1141,6 +1144,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); @@ -1207,6 +1211,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); @@ -1284,6 +1289,7 @@ mod tests { gutter_w: 2, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); @@ -1354,6 +1360,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); diff --git a/src/editor.rs b/src/editor.rs index 01cfc08..d9bfdfb 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -4263,6 +4263,36 @@ impl CompletionPopupKey { } } +/// Move `view_left` so the cursor's column is on screen (Stage 4, +/// framing Q#HS2 — automatic only). +/// +/// The horizontal mirror of the `view_top` rule below, and deliberately +/// the same shape: scroll only as far as it takes to bring the cursor +/// back inside, so a cursor already visible never moves the view. That +/// is what makes this the whole of Stage 4's navigation — there are no +/// explicit scroll commands, so every viewport move originates here, +/// and Q#HS4's snap-back hazard cannot arise. +/// +/// A no-op under `wrap`: there is nothing past the right edge to reach, +/// 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`. +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; + } +} + /// Scroll one window so its cursor stays visible, reckoning in /// **visible** lines when a fold map is supplied (Arc 6 Q#FD18). /// @@ -4283,10 +4313,22 @@ fn prepare_window_cursor_visible( inner_rows: u32, folds: Option<&crate::fold_view::VisibleLineMap>, ) { - let cursor_row = window + // Ask in LINE-absolute columns by pinning `view_left` to 0 — this + // pass decides what the offset should BE, so consulting the current + // one would make it self-referential. `pos_to_display` returns + // `None` for a position left of the edge (framing Q#HS7(c′)), which + // is exactly the case this pass exists to fix; reading it through + // the live context would report row 0 and scroll the window to the + // top instead. + let unscrolled = crate::view::LayoutCtx { + view_left: 0, + ..window.layout_ctx() + }; + let coord = window .text_view - .pos_to_display(buf, window.cursor, window.layout_ctx()) - .map_or(0, |d| d.row as usize); + .pos_to_display(buf, window.cursor, unscrolled); + let cursor_row = coord.map_or(0, |d| d.row as usize); + horizontal_follow(window, coord.map_or(0, |d| d.col)); match folds { // The logical cursor may sit on a hidden line (a shared fold, or // goto-line into one); the row that actually renders — and so @@ -4375,6 +4417,14 @@ fn paint_window_content( // the failure this whole one-resolution arrangement exists to // prevent. wrap: window.last_wrap, + // Same discipline as `wrap` directly above, and for the same + // reason it was needed: `aa3cd4d` shipped a hard-coded + // `Truncate` here while every other consumer read the resolved + // value, so the cursor was placed for wrapped text over text + // that was still clipped. A literal `0` here would reproduce it + // exactly — coordinates and the indicator would follow the + // scroll while the painter stayed pinned at column 0. + view_left: window.view_left, }; // Composition (T M2.9): base text_view paints first, then the // gutter numbers — before the overlays, so a diagnostic overlay @@ -4396,7 +4446,7 @@ fn paint_window_content( for overlay in &mut window.overlays { overlay.render(buf, viewport, grid); } - paint_local_selection(grid, buf, window, &rect, inner_rows, gutter_w, folds, theme); + paint_local_selection(grid, buf, window, viewport, inner_rows, folds, theme); // Mode line for this window. Painted last so the line // itself is always visible regardless of overlay activity. let coord = window @@ -5009,12 +5059,16 @@ fn paint_local_selection( grid: &mut crate::cell::CellGrid<'_>, buf: &crate::buffer::Buffer, window: &crate::window::Window, - rect: &crate::window::Rect, + // The SAME viewport the text and every decorator were painted + // through. It already carries the gutter-adjusted width + // (`rect.size.cols - gutter_w`) and an origin shifted past the + // gutter, so the selection shares one clip rule with them rather + // than re-deriving it — the first version of Stage 4 duplicated the + // rule here on the false premise that this painter needed a + // different width (Q#UX2 handled it via `gutter_w`, which the + // viewport has already applied). + viewport: crate::view::Viewport<'_>, inner_rows: u32, - // UX gutter: the reserved left-strip width; selection cells are the - // text-relative display column shifted right by this (Q#UX2). 0 when - // the gutter is off, so this is a no-op then. - gutter_w: u32, // Arc 6 Stage 2: this window's collapsed regions, or `None`. folds: Option<&crate::fold_view::VisibleLineMap>, theme: &crate::highlight::Theme, @@ -5047,10 +5101,9 @@ fn paint_local_selection( ..crate::cell::Style::default() }, ); - if inner_rows == 0 || rect.size.cols == 0 || sel_start >= sel_end { + if inner_rows == 0 || viewport.cell_size.cols == 0 || sel_start >= sel_end { return; } - let text_cols = rect.size.cols.saturating_sub(gutter_w); // Row `r` shows the `r`-th VISIBLE line at or after `view_top`. let mut next_line = folds.map_or(window.view_top, |map| map.visible_head_of(window.view_top)); @@ -5070,32 +5123,42 @@ fn paint_local_selection( continue; } - let Some(start_coord) = - window - .text_view - .pos_to_display(buf, paint_start, window.layout_ctx()) + // Asked in LINE-absolute columns, then clipped below. + // + // Through the live context this dropped whole visible segments: + // `pos_to_display` returns `None` for a position left of the + // edge (framing Q#HS7(c′)), so a selection beginning off-screen + // and reaching well into view took the `continue` and painted + // nothing — the most common shape there is, since selecting + // rightward from column 0 then scrolling produces exactly it. + let unscrolled = crate::view::LayoutCtx { + view_left: 0, + ..window.layout_ctx() + }; + let Some(start_coord) = window + .text_view + .pos_to_display(buf, paint_start, unscrolled) else { continue; }; - let Some(end_coord) = window - .text_view - .pos_to_display(buf, paint_end, window.layout_ctx()) - else { + let Some(end_coord) = window.text_view.pos_to_display(buf, paint_end, unscrolled) else { continue; }; if start_coord.row as usize != display_row || end_coord.row as usize != display_row { continue; } - let start_col = start_coord.col.min(text_cols); - let end_col = end_coord.col.min(text_cols); - if start_col >= end_col { + // The one shared clip rule (Stage 4). Five adopters now read it: + // syntax/LSP styling, diagnostic underlines, search washes, + // `BufferStyleOverlay`, and this. + let Some((start_col, end_col)) = viewport.visible_cols(start_coord.col, end_coord.col) + else { continue; - } + }; for col in start_col..end_col { let cell = grid.at(CellCoord::new( - rect.origin.row + row_offset, - rect.origin.col + gutter_w + col, + viewport.cell_origin.row + row_offset, + viewport.cell_origin.col + col, )); cell.style = crate::overlay::merge_styles(cell.style, overlay); } @@ -8543,6 +8606,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let mut grid = CellGrid { cells: &mut backing, @@ -8679,6 +8743,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; // Two no-op overlays: probe the dispatch cost only. @@ -11429,3 +11494,112 @@ mod tests { ); } } + +#[cfg(test)] +mod horizontal_scroll_selection_tests { + use super::*; + use crate::cell::{Cell, CellCoord, CellGrid, CellSize, Style}; + use crate::view::WrapMode; + use crate::window::{Selection, Window, WindowId}; + + /// A selection that begins LEFT of the horizontal edge and reaches + /// into view must paint its visible tail (Stage 4 review P1). + /// + /// The selection painter asked `pos_to_display` through the live + /// layout context, which returns `None` for a position left of the + /// edge (framing Q#HS7(c′)) — so the whole segment took `continue` + /// and painted nothing. That is the *common* shape, not an edge + /// case: select rightward from column 0, keep going past the window + /// width, and the view scrolls with the cursor. + #[test] + fn a_selection_starting_off_screen_paints_its_visible_tail() { + let buf = crate::buffer::Buffer::from_bytes( + crate::buffer::BufferId::next(), + "t", + b"ABCDEFGHIJKL", + ); + let text_view = crate::text_view::TextView::new(&buf); + let mut window = Window::new(WindowId::next(), buf.id(), text_view); + window.last_wrap = WrapMode::Truncate; + window.last_content_cols = 4; + // Scrolled so screen column 0 shows source column 4. + window.view_left = 4; + // Selected from the line start through byte 6 — bytes 0..4 are + // off-screen left, bytes 4..6 ("EF") are the visible tail. + // `Selection` holds only the anchor; the other end is the + // window's cursor. + window.selection = Some(Selection { anchor: 0 }); + window.cursor = 6; + + let mut storage = vec![Cell::default(); 4]; + let mut grid = CellGrid { + cells: &mut storage, + stride: 4, + size: CellSize::new(1, 4), + }; + let viewport = crate::view::Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(1, 4), + gutter_w: 0, + folds: None, + wrap: WrapMode::Truncate, + view_left: 4, + }; + let theme = crate::highlight::Theme::default_dark(); + paint_local_selection(&mut grid, &buf, &window, viewport, 1, None, &theme); + + let washed: Vec = (0..4) + .map(|c| storage[c].style != Style::default()) + .collect(); + assert_eq!( + washed, + vec![true, true, false, false], + "the visible tail (E, F) must carry the selection wash; \ + painting nothing at all is the defect, and painting at \ + absolute columns 0..6 would wash the whole window" + ); + } + + /// The control: a selection entirely left of the edge paints nothing. + #[test] + fn a_selection_entirely_off_screen_paints_nothing() { + let buf = crate::buffer::Buffer::from_bytes( + crate::buffer::BufferId::next(), + "t", + b"ABCDEFGHIJKL", + ); + let text_view = crate::text_view::TextView::new(&buf); + let mut window = Window::new(WindowId::next(), buf.id(), text_view); + window.last_wrap = WrapMode::Truncate; + window.last_content_cols = 4; + window.view_left = 4; + window.selection = Some(Selection { anchor: 0 }); + window.cursor = 3; + + let mut storage = vec![Cell::default(); 4]; + let mut grid = CellGrid { + cells: &mut storage, + stride: 4, + size: CellSize::new(1, 4), + }; + let viewport = crate::view::Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(1, 4), + gutter_w: 0, + folds: None, + wrap: WrapMode::Truncate, + view_left: 4, + }; + let theme = crate::highlight::Theme::default_dark(); + paint_local_selection(&mut grid, &buf, &window, viewport, 1, None, &theme); + + assert!( + (0..4).all(|c| storage[c].style == Style::default()), + "a selection ending before the edge must not wash anything" + ); + } +} diff --git a/src/highlight.rs b/src/highlight.rs index dfa27c9..67983bf 100644 --- a/src/highlight.rs +++ b/src/highlight.rs @@ -471,7 +471,6 @@ impl View for SyntaxHighlightView { let start_line = line_at_offset(&self.cache.line_offsets, viewport.buffer_start as u32); let max_rows = viewport.cell_size.rows; - let max_cols = viewport.cell_size.cols; let cell_origin = viewport.cell_origin; let total_lines = self.cache.line_offsets.len() as u32; @@ -531,8 +530,11 @@ impl View for SyntaxHighlightView { continue; } let cell_row = cell_origin.row + row_offset; - let clamped_start = start_col.min(max_cols); - let clamped_end = end_col.min(max_cols); + let Some((clamped_start, clamped_end)) = + viewport.visible_cols(start_col, end_col) + else { + continue; + }; for col in clamped_start..clamped_end { let cell = cells.at(CellCoord::new(cell_row, cell_origin.col + col)); cell.style = merge_styles(cell.style, style); @@ -688,7 +690,6 @@ impl View for LspStyleView { let start_line = line_at_offset(&line_offsets, viewport.buffer_start as u32); let max_rows = viewport.cell_size.rows; - let max_cols = viewport.cell_size.cols; let cell_origin = viewport.cell_origin; let total_lines = line_offsets.len() as u32; @@ -751,12 +752,14 @@ impl View for LspStyleView { continue; } let (start_col, end_col) = byte_range_to_columns(line_bytes, start_b, end_b); - if end_col <= start_col { + // `visible_cols` returns `None` for an empty range too, so the + // old `end_col <= start_col` guard is subsumed rather than + // dropped. + let Some((clamped_start, clamped_end)) = viewport.visible_cols(start_col, end_col) + else { continue; - } + }; let cell_row = cell_origin.row + row_offset; - let clamped_start = start_col.min(max_cols); - let clamped_end = end_col.min(max_cols); for col in clamped_start..clamped_end { let cell = cells.at(CellCoord::new(cell_row, cell_origin.col + col)); cell.style = merge_styles(cell.style, style); @@ -1113,6 +1116,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = state.core.borrow().registry.clone(); let reg = registry.borrow(); @@ -1212,6 +1216,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = state.core.borrow().registry.clone(); let reg = registry.borrow(); @@ -1333,6 +1338,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = state.core.borrow().registry.clone(); let reg = registry.borrow(); @@ -1394,6 +1400,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = buf; // keep buf alive hv.render(®istry, viewport, &mut grid); @@ -1452,6 +1459,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = buf; // keep buf alive hv.render(®istry, viewport, &mut grid); @@ -1518,6 +1526,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = buf; hv.render(®istry, viewport, &mut grid); @@ -1586,6 +1595,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; hv.render(&buf, viewport, &mut grid); grid.get(CellCoord::new(0, col)).style @@ -1829,6 +1839,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = buf; hv.render(®istry, viewport, &mut grid); @@ -1893,6 +1904,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let registry = buf; hv.render(®istry, viewport, &mut grid); diff --git a/src/overlay.rs b/src/overlay.rs index 80ec62b..e02dd9e 100644 --- a/src/overlay.rs +++ b/src/overlay.rs @@ -402,8 +402,12 @@ fn render_buffer_style_span( (style_start - line_start) as usize, line_prefix.len(), ); - let start_col = start_col.min(viewport.cell_size.cols); - let end_col = end_col.min(viewport.cell_size.cols); + // Buffer coordinates, so they translate (Stage 4). Its siblings + // `StyleSpanOverlay` and `VirtualCellOverlay` are documented as + // viewport-relative and deliberately do NOT. + let Some((start_col, end_col)) = viewport.visible_cols(start_col, end_col) else { + continue; + }; for col in start_col..end_col { let coord = CellCoord::new( viewport.cell_origin.row + row_offset, @@ -498,6 +502,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, } } diff --git a/src/search.rs b/src/search.rs index a2e84b0..d342126 100644 --- a/src/search.rs +++ b/src/search.rs @@ -443,7 +443,6 @@ impl View for SearchView { // the line is collapsed away (the wash then paints nothing). let row_of = |line: u32| viewport.row_offset_of(start_line_buf as usize, line as usize); let max_rows = viewport.cell_size.rows; - let max_cols = viewport.cell_size.cols; let cell_origin = viewport.cell_origin; // Themes Q#TH5: a set wash face replaces the default overlay @@ -523,12 +522,14 @@ impl View for SearchView { let within_end = (paint_end - line_start) as usize; let (start_col, end_col) = byte_range_to_columns(line_bytes, within_start, within_end); - if end_col <= start_col { + // `visible_cols` returns `None` for an empty range too, so the + // old `end_col <= start_col` guard is subsumed rather than + // dropped. + let Some((clamped_start, clamped_end)) = viewport.visible_cols(start_col, end_col) + else { continue; - } + }; let cell_row = cell_origin.row + row_offset; - let clamped_start = start_col.min(max_cols); - let clamped_end = end_col.min(max_cols); for col in clamped_start..clamped_end { let cell = cells.at(CellCoord::new(cell_row, cell_origin.col + col)); cell.style = merge_styles(cell.style, style); @@ -761,6 +762,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); @@ -791,6 +793,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid2, ); @@ -834,6 +837,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); diff --git a/src/text_view.rs b/src/text_view.rs index 777e809..830c419 100644 --- a/src/text_view.rs +++ b/src/text_view.rs @@ -217,6 +217,52 @@ impl TextView { } } + /// Translate a LINE column to a SCREEN column at horizontal offset + /// `left`, or `None` when the byte is not visible (framing + /// Q#HS7(c′)). + /// + /// The straddle case is why this is not a bare subtraction, and it + /// was the first version's bug. A wide glyph starting at `left - 1` + /// has its **trailing** cell on screen at column 0, so its start + /// byte must designate that cell — otherwise the character the user + /// scrolled toward has no visible cell mapping to it at all, and the + /// round trip against `display_to_pos` breaks. + /// + /// A **tab** is deliberately excluded. Its expansion cells map + /// FORWARD to the byte after it (Q#HS7(c″), the pre-Stage-4 + /// behavior), so the tab byte itself is simply off-screen; letting + /// it claim cell 0 would put two bytes on one cell. + fn screen_col(buf: &Buffer, pos: Position, col: u32, left: u32) -> Option { + if col >= left { + return Some(col - left); + } + // Left of the edge — visible only if the glyph *starting* here + // reaches past it. + let mut probe = [0u8; 4]; + let end = (pos + 4).min(buf.len()); + let n = (end - pos) as usize; + if n == 0 { + return None; + } + buf.snapshot_rope().slice(pos, end, &mut probe[..n]); + let ch = std::str::from_utf8(&probe[..n]) + .ok() + .and_then(|s| s.chars().next()) + .or_else(|| { + // A truncated read can split the final codepoint; decode + // the longest valid prefix instead of giving up. + std::str::from_utf8(&probe[..n]) + .err() + .map(|e| e.valid_up_to()) + .and_then(|v| std::str::from_utf8(&probe[..v]).ok()) + .and_then(|s| s.chars().next()) + })?; + if ch == '\t' { + return None; + } + (advance_char(col, ch) > left).then_some(0) + } + /// Byte offset (relative to `line`'s start) at visual row `sub_row`, /// column `col`, under character wrap — the inverse of /// [`Self::place_of_byte`]. @@ -301,9 +347,26 @@ impl TextView { let r = first_row.checked_add(sub.checked_sub(skip_rows)?)?; (r < max_rows).then_some(origin.row + r) }; + // The walk stays in LINE-absolute columns and only `put` + // translates to the screen (framing Q#HS7(a)). Tab expansion + // depends on the absolute column from the line start, so a walk + // that began at the edge would put tab stops in the wrong place; + // starting at 0 and translating on output preserves them for + // free, at the cost `paint_line` already pays under wrapping. + // + // `left` is 0 whenever this line wraps, so the wrap path below is + // byte-identical to Stage 3. + let left = viewport.left_edge(); let put = |cells: &mut CellGrid<'_>, sub: u32, col: u32, glyph: Glyph| { + // Entirely left of the edge: not this viewport's cell. + let Some(screen) = col.checked_sub(left) else { + return; + }; + if screen >= max_cols { + return; + } if let Some(row) = grid_row(sub) { - let cell = cells.at(CellCoord::new(row, origin.col + col)); + let cell = cells.at(CellCoord::new(row, origin.col + screen)); cell.glyph = glyph; cell.style = Style::default(); cell.attachment = None; @@ -312,7 +375,7 @@ impl TextView { let (mut sub_row, mut col) = (0u32, 0u32); for ch in s.chars() { - if !wrapping && col >= max_cols { + if !wrapping && col >= max_cols.saturating_add(left) { break; } let (start_row, start_col, end_row, end_col) = @@ -327,8 +390,23 @@ impl TextView { } } else if end_col > start_col || start_row > sub_row { put(cells, start_row, start_col, Glyph::Char(ch)); - if end_col.saturating_sub(start_col) == 2 && start_col + 1 < max_cols { - put(cells, start_row, start_col + 1, Glyph::Continuation); + if end_col.saturating_sub(start_col) == 2 + && start_col + 1 < max_cols.saturating_add(left) + { + // A wide glyph the left edge BISECTS cannot draw its + // leading cell, so its trailing cell shows a blank + // rather than a `Continuation` — which is a marker + // meaning "the cell before me is a wide glyph's + // head", and here that cell is off-screen. Emitting + // it would name a cell nobody painted (framing + // Q#HS7(c′)). + let bisected = start_col < left; + let trailing = if bisected { + Glyph::Char(' ') + } else { + Glyph::Continuation + }; + put(cells, start_row, start_col + 1, trailing); } } sub_row = end_row; @@ -465,7 +543,14 @@ impl View for TextView { // answer, which means trimming the in-progress bytes). let take = (pos - line_start) as usize; if take == 0 { - return Some(DisplayCoord::new(row_idx as u32, 0)); + // Still translated: at a non-zero offset the line's first + // byte is off-screen (or straddling), and returning column 0 + // unconditionally was the first version's bug — it made byte + // 0 look visible at every offset. + return Some(DisplayCoord::new( + row_idx as u32, + Self::screen_col(buf, pos, 0, ctx.effective_left())?, + )); } // Copy [line_start, pos) into a stack buffer for the common short-line // case, hitting the heap only for unusually long prefixes. This removes @@ -480,7 +565,21 @@ impl View for TextView { }; buf.snapshot_rope().slice(line_start, pos, bytes); let col = valid_prefix_width(bytes); - Some(DisplayCoord::new(row_idx as u32, col)) + // Translate to the screen. A caret sits BETWEEN characters, so it + // never lands inside a glyph — the straddle case belongs to + // `display_to_pos` and the painter, not here. + // + // `None` for a position left of the edge is deliberate and is the + // contract in framing Q#HS7(c′): clamping to column 0 instead + // would make arbitrarily many positions share one cell and + // destroy the round trip. Callers already handle `None` (it is + // what an out-of-range `pos` returns), and the horizontal + // visibility pass keeps the cursor on screen so this is not + // reachable for the caret itself. + Some(DisplayCoord::new( + row_idx as u32, + Self::screen_col(buf, pos, col, ctx.effective_left())?, + )) } fn display_to_pos( @@ -502,14 +601,38 @@ impl View for TextView { let line_bytes = self.read_line_bytes(buf, row); let s = std::str::from_utf8(&line_bytes).ok()?; + // Screen column back to line column. The walk below is otherwise + // unchanged, so tab stops stay right (framing Q#HS7(a)). + let left = ctx.effective_left(); + let target = coord.col.saturating_add(left); + let mut walked_cols: u32 = 0; let mut walked_bytes: usize = 0; for (byte_idx, ch) in s.char_indices() { - if walked_cols >= coord.col { + if walked_cols >= target { walked_bytes = byte_idx; return Some(line_start + walked_bytes as u64); } - walked_cols = advance_char(walked_cols, ch); + let next = advance_char(walked_cols, ch); + // The bisected wide glyph, and ONLY at the leftmost visible + // cell (framing Q#HS7(c′)). Its trailing cell is screen + // column 0, and it is designated to the glyph's START byte: + // the cell belongs to that character, so a click there must + // select it, and nothing else can — its leading cell is off + // screen. + // + // Deliberately narrow. Everywhere else a column landing + // inside a glyph keeps rounding FORWARD, which is the + // pre-Stage-4 behavior and what `byte_at_place` documents; + // widening this would change unscrolled mappings. Tabs keep + // forward rounding here too — their expansion is whitespace + // BETWEEN the tab byte and the next character, so landing + // after it is what clicking indentation should do + // (Q#HS7(c″)). + if coord.col == 0 && left > 0 && ch != '\t' && walked_cols < target && next > target { + return Some(line_start + byte_idx as u64); + } + walked_cols = next; walked_bytes = byte_idx + ch.len_utf8(); } // Past the line's last codepoint: clamp to the line's visible end. @@ -954,6 +1077,7 @@ mod tests { let ctx = LayoutCtx { cols: 4, wrap: WrapMode::Wrap, + view_left: 0, }; // 'e' is byte 4: line 0, second visual row, column 0. assert_eq!( @@ -976,6 +1100,7 @@ mod tests { let ctx = LayoutCtx { cols: 4, wrap: WrapMode::Wrap, + view_left: 0, }; assert_eq!( view.pos_to_display(&buf, 4, ctx), @@ -1001,6 +1126,7 @@ mod tests { let ctx = LayoutCtx { cols, wrap: WrapMode::Wrap, + view_left: 0, }; for (byte, _) in text.char_indices() { let coord = view @@ -1024,6 +1150,7 @@ mod tests { let ctx = LayoutCtx { cols: 4, wrap: WrapMode::Wrap, + view_left: 0, }; // '中' starts at byte 2 and is three bytes long. let at_start = view.pos_to_display(&buf, 2, ctx); @@ -1060,7 +1187,7 @@ mod tests { /// Render `text` into a `rows` x `cols` grid and return the glyph of /// every cell, row-major. fn render_grid(text: &[u8], rows: u32, cols: u32, wrap: WrapMode) -> Vec { - render_grid_from(text, rows, cols, wrap, 0) + render_grid_from(text, rows, cols, wrap, 0, 0) } /// As [`render_grid`], but starting the viewport at byte `start` — @@ -1070,6 +1197,7 @@ mod tests { rows: u32, cols: u32, wrap: WrapMode, + view_left: u32, start: u64, ) -> Vec { let (buf, mut view) = attached(text); @@ -1090,6 +1218,7 @@ mod tests { gutter_w: 0, folds: None, wrap, + view_left, }, &mut grid, ); @@ -1132,6 +1261,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Wrap, + view_left: 0, }; view.render(&buf, vp, &mut grid); assert!( @@ -1202,6 +1332,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Wrap, + view_left: 0, }, &mut grid, ); @@ -1286,7 +1417,7 @@ xy", #[test] fn the_viewport_can_start_partway_down_a_wrapped_line() { // Byte 4 is 'e', the first character of the second visual row. - let g = render_grid_from(b"abcdefghij", 2, 4, WrapMode::Wrap, 4); + let g = render_grid_from(b"abcdefghij", 2, 4, WrapMode::Wrap, 0, 4); assert_eq!(row_text(&g, 4, 0), "efgh", "the first row is skipped"); assert_eq!(row_text(&g, 4, 1), "ij "); } @@ -1303,7 +1434,7 @@ xy", let row = view.row_of_byte(&buf, 0, byte as u64, cols); // Anchoring the viewport at that byte must put the // character on the viewport's FIRST row. - let g = render_grid_from(text.as_bytes(), 3, cols, WrapMode::Wrap, byte as u64); + let g = render_grid_from(text.as_bytes(), 3, cols, WrapMode::Wrap, 0, byte as u64); let full = render_grid(text.as_bytes(), 12, cols, WrapMode::Wrap); let expect = row_text(&full, cols, row); assert_eq!( @@ -1334,6 +1465,7 @@ xy", gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); @@ -1364,6 +1496,7 @@ xy", gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); @@ -1397,6 +1530,7 @@ xy", gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); @@ -1431,6 +1565,7 @@ xy", gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }, &mut grid, ); diff --git a/src/view.rs b/src/view.rs index ab77cf1..06bc7dc 100644 --- a/src/view.rs +++ b/src/view.rs @@ -171,6 +171,19 @@ pub struct LayoutCtx { pub cols: u32, /// The window's resolved wrap mode. pub wrap: WrapMode, + /// First visible display column — the window's horizontal scroll + /// offset (Stage 4, framing Q#HS7). + /// + /// **Stored unsnapped.** It is one per-window column, while "does + /// this column bisect a wide glyph?" is a *per-line* question, so no + /// single snapped value could be canonical for every visible line. + /// Each line derives its own **effective edge** during the walk it + /// already performs from column 0 (framing Q#HS7(c′)). + /// + /// Inert under [`WrapMode::Wrap`]: a wrapped line has nothing past + /// the right edge to scroll toward, so the wrap path ignores this + /// entirely and stays byte-identical to Stage 3. + pub view_left: u32, } impl LayoutCtx { @@ -184,6 +197,7 @@ impl LayoutCtx { Self { cols: 0, wrap: WrapMode::Truncate, + view_left: 0, } } @@ -192,6 +206,18 @@ impl LayoutCtx { pub const fn wrapping(self) -> bool { matches!(self.wrap, WrapMode::Wrap) && self.cols > 0 } + + /// The horizontal offset that actually applies. + /// + /// Always `0` when wrapping, which is what makes `view_left` inert + /// under `wrap` **by construction** rather than by every caller + /// remembering to check. A wrapped line has no content past the + /// right edge, so a non-zero offset there could only hide text that + /// nothing would ever scroll back to. + #[must_use] + pub const fn effective_left(self) -> u32 { + if self.wrapping() { 0 } else { self.view_left } + } } /// How a line wider than the viewport is shown --- the long-lines @@ -265,9 +291,68 @@ pub struct Viewport<'a> { /// pre-Stage-3 sites read as *deliberately* unwrapped rather than /// merely untouched. pub wrap: WrapMode, + /// First visible display column (Stage 4). See + /// [`LayoutCtx::view_left`]; required rather than defaulted for the + /// same reason `wrap` is. + pub view_left: u32, } impl Viewport<'_> { + /// The horizontal offset that actually applies — `0` when wrapping, + /// for the reason [`LayoutCtx::effective_left`] gives. + #[must_use] + pub const fn left_edge(&self) -> u32 { + if matches!(self.wrap, WrapMode::Wrap) { + 0 + } else { + self.view_left + } + } + + /// Clip a **line**-column range to what is on screen, returning + /// **screen** columns — or `None` when none of it is visible. + /// + /// # Why every buffer-coordinate decorator must use this + /// + /// Stage 4 translated the base text walk and nothing else, which + /// split the frame in half: at `view_left = 10` a glyph at source + /// column 10 painted at screen column 0 while its syntax style, + /// diagnostic underline, search wash and selection painted at screen + /// column 10 — or vanished. Decorations drifted off the characters + /// they describe, silently, and only once a window was scrolled. + /// + /// Every such site had the same two lines (`start_col.min(max_cols)`, + /// `end_col.min(max_cols)`) — correct only while the left edge was + /// pinned at zero. One helper replaces all of them so a future + /// decorator inherits the translation instead of re-deriving it. + /// + /// **Five adopters**, and the count is the point: syntax/LSP + /// styling, diagnostic underlines, search washes, + /// [`crate::overlay::BufferStyleOverlay`], and the selection + /// painter. The selection was nearly the exception — Stage 4's first + /// version duplicated the rule there, justified by a width this + /// painter supposedly needed and the viewport lacked. That was + /// false: the render viewport's `cell_size.cols` is already + /// `rect.size.cols - gutter_w`, and its origin already sits past the + /// gutter. A canonical rule with one honest exception is not + /// canonical, so the exception went. + /// + /// **Not for [`crate::overlay::StyleSpanOverlay`] or + /// [`crate::overlay::VirtualCellOverlay`]**: those are documented as + /// viewport-relative, so their columns are already screen columns + /// and translating them twice would be the mirror defect. + #[must_use] + pub fn visible_cols(&self, start_col: u32, end_col: u32) -> Option<(u32, u32)> { + let left = self.left_edge(); + let right = left.saturating_add(self.cell_size.cols); + let start = start_col.max(left); + let end = end_col.min(right); + // A range that begins off-screen left and reaches past the edge + // is CLIPPED, not skipped — that is the selection defect this + // returns `Some` for. + (end > start).then(|| (start - left, end - left)) + } + /// Row offset within this viewport for source `line`, given the /// viewport's first (visible) source line. /// @@ -484,6 +569,7 @@ mod tests { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; assert_eq!(vp.row_offset_of(4, 4), Some(0)); assert_eq!(vp.row_offset_of(4, 9), Some(5)); @@ -505,6 +591,7 @@ mod tests { gutter_w: 0, folds: Some(&map), wrap: WrapMode::Truncate, + view_left: 0, }; assert_eq!(vp.row_offset_of(0, 1), Some(1), "the head keeps its row"); assert_eq!(vp.row_offset_of(0, 3), None, "hidden lines have no row"); diff --git a/src/window.rs b/src/window.rs index 4b6514d..eea37bf 100644 --- a/src/window.rs +++ b/src/window.rs @@ -372,6 +372,18 @@ pub struct Window { pub selection: Option, /// First buffer line shown at the top of this window's viewport. pub view_top: usize, + /// First display column shown at the left of this window's viewport + /// — the horizontal scroll offset (Stage 4, framing Q#HS7). + /// + /// **Per window**, exactly as `view_top` is: two panes on one buffer + /// must scroll independently. Note the deliberate asymmetry with + /// `ui.line-wrap`, which is **buffer**-local — the two halves of one + /// user-facing concept live at different scopes, accepted in Stage + /// 3's Q#LL2 as a decision rather than discovered here. + /// + /// Always `0` while this window's buffer wraps; see + /// [`LayoutCtx::effective_left`](crate::view::LayoutCtx::effective_left). + pub view_left: u32, /// Sticky display column for vertical motion. pub goal_col: Option, /// Number of text rows that fit in this window's viewport at last @@ -426,6 +438,7 @@ impl Window { cursor: 0, selection: None, view_top: 0, + view_left: 0, goal_col: None, last_visible_rows: 0, last_content_cols: 0, @@ -450,6 +463,7 @@ impl Window { crate::view::LayoutCtx { cols: self.last_content_cols, wrap: self.last_wrap, + view_left: self.view_left, } } diff --git a/tests/compile_mode_acceptance.rs b/tests/compile_mode_acceptance.rs index 1b5150c..1801641 100644 --- a/tests/compile_mode_acceptance.rs +++ b/tests/compile_mode_acceptance.rs @@ -290,6 +290,7 @@ fn render_active_window_to_grid( gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let mut grid = CellGrid { cells: &mut backing, diff --git a/tests/horizontal_scroll_acceptance.rs b/tests/horizontal_scroll_acceptance.rs new file mode 100644 index 0000000..93261b5 --- /dev/null +++ b/tests/horizontal_scroll_acceptance.rs @@ -0,0 +1,394 @@ +//! Horizontal scroll acceptance (`QoL` Stage 4, +//! `docs/horizontal-scroll-framing.md`). +//! +//! Stage 3 shipped `ui.line-wrap`; under `truncate` the text past the +//! right edge was **unreachable**. Stage 4 makes it reachable by moving +//! the cursor — automatic only, no commands (Q#HS2). +//! +//! # The contract these tests are the oracle for +//! +//! `view_left` is an **unsnapped** per-window display column, and each +//! line derives its own **effective edge** (Q#HS7(c′)). A setter-time +//! snap was the first design and 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. +//! +//! That is why the discriminating witness here is **multi-line with +//! differing glyph widths at the same column**. A single-line sweep +//! passes against the withdrawn design and proves nothing. + +use pmacs::buffer::{Buffer, BufferId}; +use pmacs::cell::{Cell, CellCoord, CellGrid, CellSize, Glyph}; +use pmacs::text_view::TextView; +use pmacs::view::{DisplayCoord, LayoutCtx, View, Viewport, WrapMode}; + +fn attached(text: &[u8]) -> (Buffer, TextView) { + let buf = Buffer::from_bytes(BufferId::next(), "test", text); + let view = TextView::new(&buf); + (buf, view) +} + +fn ctx(cols: u32, wrap: WrapMode, view_left: u32) -> LayoutCtx { + LayoutCtx { + cols, + wrap, + view_left, + } +} + +/// Render and return each grid row's text. +fn rows_of(text: &[u8], rows: u32, cols: u32, wrap: WrapMode, view_left: u32) -> Vec { + let (buf, mut view) = attached(text); + let mut storage = vec![Cell::default(); (rows * cols) as usize]; + let mut grid = CellGrid { + cells: &mut storage, + stride: cols, + size: CellSize::new(rows, cols), + }; + view.render( + &buf, + Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(rows, cols), + gutter_w: 0, + folds: None, + wrap, + view_left, + }, + &mut grid, + ); + (0..rows) + .map(|r| { + (0..cols) + .map(|c| match storage[(r * cols + c) as usize].glyph { + Glyph::Char(ch) => ch, + // Rendered as a distinct marker so a test can tell + // "wide glyph's second cell" from "blank". + Glyph::Continuation => '\u{1}', + Glyph::Cluster(_) => ' ', + }) + .collect() + }) + .collect() +} + +/// The report's own case: text past the edge becomes visible. +#[test] +fn scrolling_right_reveals_text_past_the_edge() { + let text = b"ABCDEFGHIJKL"; + assert_eq!(rows_of(text, 1, 4, WrapMode::Truncate, 0)[0], "ABCD"); + assert_eq!(rows_of(text, 1, 4, WrapMode::Truncate, 4)[0], "EFGH"); + assert_eq!( + rows_of(text, 1, 4, WrapMode::Truncate, 8)[0], + "IJKL", + "the tail of a long line is reachable, which is the whole of the \ + report Stage 3 could only half-answer" + ); +} + +/// `view_left` must be **inert** under `wrap`, not merely harmless. +/// +/// A wrapped line has nothing past the right edge, so an offset there +/// could only hide text nothing would scroll back to. Pinned to 0 by +/// `LayoutCtx::effective_left` rather than by callers remembering. +#[test] +fn wrap_ignores_a_horizontal_offset() { + let text = b"ABCDEFGH"; + let unscrolled = rows_of(text, 2, 4, WrapMode::Wrap, 0); + for offset in [1, 4, 7, 99] { + assert_eq!( + rows_of(text, 2, 4, WrapMode::Wrap, offset), + unscrolled, + "offset {offset} changed a wrapped render; it must be inert" + ); + } + assert_eq!(unscrolled[0], "ABCD"); + assert_eq!(unscrolled[1], "EFGH"); +} + +/// **The discriminating witness** (framing Q#HS7(c′)/(d)). +/// +/// At one `view_left`, one line takes the straddle path and another the +/// ordinary path. A setter-time snap has a single value to choose and +/// must be wrong for one of these two lines; a per-line effective edge +/// is right for both. +#[test] +fn one_offset_straddles_on_one_line_and_not_another() { + // Line 0: a wide glyph occupying columns 1-2, so column 2 bisects it. + // Line 1: all narrow, so column 2 is an ordinary boundary. + let text = "a\u{4e00}bcd\nabcd".as_bytes(); + let out = rows_of(text, 2, 3, WrapMode::Truncate, 2); + + assert_eq!( + out[0].chars().next(), + Some(' '), + "the bisected glyph's trailing cell is a styled BLANK — not a \ + Continuation, which would name a leading cell nobody painted" + ); + assert_eq!( + &out[0][1..], + "bc", + "and the rest of that line follows it normally" + ); + assert_eq!( + out[1], "cd ", + "the same offset on an all-narrow line is an ordinary boundary" + ); +} + +/// The bisected glyph's trailing cell is designated to the glyph's +/// **start** byte, so clicking it selects the character it belongs to. +/// +/// Forward-rounding here would designate the NEXT character and leave +/// the straddling glyph with no visible cell mapping to it at all — +/// unreachable exactly when it is what the user scrolled toward. +#[test] +fn the_bisected_cell_maps_back_to_its_own_glyph() { + let (buf, view) = attached("a\u{4e00}bcd".as_bytes()); + let c = ctx(3, WrapMode::Truncate, 2); + // 'a' is byte 0; the wide glyph is bytes 1..4. + assert_eq!( + view.display_to_pos(&buf, DisplayCoord::new(0, 0), c), + Some(1), + "screen column 0 is the wide glyph's trailing cell" + ); + // …and the round trip: the glyph reports that same cell. + assert_eq!( + view.pos_to_display(&buf, 1, c).map(|d| d.col), + Some(0), + "place_of_byte designates cell 0, so byte_at_place inverts it" + ); +} + +/// A tab straddling the edge keeps **forward** rounding (Q#HS7(c″)). +/// +/// This is a REGRESSION witness, not a new claim: `display_to_pos` +/// already rounds forward for a column inside a tab's expansion. Stage 4 +/// must not perturb it — and it would, if the walk were "optimized" to +/// start at the effective edge instead of column 0, because tab stops +/// are computed from the line start. +#[test] +fn a_straddling_tab_still_rounds_forward() { + // Tab expands to columns 0..8 at the default tab width; 'x' is byte 1. + let (buf, view) = attached(b"\txyz"); + let unscrolled = + view.display_to_pos(&buf, DisplayCoord::new(0, 4), ctx(8, WrapMode::Truncate, 0)); + assert_eq!(unscrolled, Some(1), "precondition: forward rounding today"); + + // Same absolute column 4, now reached as screen column 0 with the + // expansion's leading cells scrolled off. + assert_eq!( + view.display_to_pos(&buf, DisplayCoord::new(0, 0), ctx(8, WrapMode::Truncate, 4)), + Some(1), + "scroll must not change where a tab-interior column lands" + ); +} + +/// Tab stops are preserved because the walk still starts at column 0. +#[test] +fn tab_stops_survive_a_horizontal_offset() { + // "a\tb": the tab advances to the next multiple of the tab width, so + // 'b' sits at column 8 regardless of what is scrolled off. + let full = rows_of(b"a\tb", 1, 12, WrapMode::Truncate, 0); + assert_eq!(full[0].chars().nth(8), Some('b'), "precondition"); + + let scrolled = rows_of(b"a\tb", 1, 12, WrapMode::Truncate, 6); + assert_eq!( + scrolled[0].chars().next(), + Some(' '), + "column 6 is still inside the tab's expansion" + ); + assert_eq!( + scrolled[0].chars().nth(2), + Some('b'), + "'b' is at absolute column 8, so screen column 8-6=2 — a walk \ + restarted at the edge would put it at 0" + ); +} + +/// Round-trip identity at a non-zero offset, walked exhaustively — the +/// Q#HS7(d) invariant, on the ordinary (non-straddling) path. +#[test] +fn round_trip_is_identity_at_a_non_zero_offset() { + let (buf, view) = attached(b"abcdefghij"); + let c = ctx(4, WrapMode::Truncate, 3); + // Bytes 3.. are at or right of the edge; earlier ones are off-screen + // and report None rather than clamping. + for pos in 0..3u64 { + assert_eq!( + view.pos_to_display(&buf, pos, c), + None, + "byte {pos} is left of the edge: not visible, never clamped \ + to column 0 — clamping would make many bytes share one cell" + ); + } + for pos in 3..=10u64 { + let coord = view.pos_to_display(&buf, pos, c).expect("visible"); + assert_eq!( + view.display_to_pos(&buf, coord, c), + Some(pos), + "round trip must be identity at byte {pos}" + ); + } +} + +// --------------------------------------------------------------------------- +// Decorations must travel WITH the text (review P1) +// +// Stage 4's first commit translated the base glyph walk and nothing +// else. Every buffer-coordinate decorator — syntax/LSP styling, +// diagnostic underlines, search washes, `BufferStyleOverlay`, and the +// selection painter — kept clamping `start_col..end_col` straight onto +// `cell_origin.col`. At `view_left = 10` the glyph from source column 10 +// painted at screen column 0 while its style painted at screen column 10 +// or vanished: decorations drifting off the characters they describe, +// silently, and only once a window had been scrolled. +// +// `Viewport::visible_cols` is the one rule they now share, across FIVE +// adopters: four decorator families — syntax/LSP styling, diagnostic +// underlines, search washes, `BufferStyleOverlay` — plus the selection +// painter, whose own witnesses live in `src/editor.rs` because +// `paint_local_selection` is private. +// +// These witnesses pin the rule from three directions, because the +// decorator sites were textually identical and a single test would have +// let a missed adopter through. +// --------------------------------------------------------------------------- + +use std::sync::{Arc, Mutex}; + +use pmacs::cell::Style; +use pmacs::overlay::{BufferStyleOverlay, BufferStyleSpan}; + +/// `(glyph, is_styled)` per cell of row 0 — decoration read against the +/// character it is supposed to be describing. +fn row0_with_styles( + text: &[u8], + cols: u32, + view_left: u32, + spans: Vec, +) -> Vec<(char, bool)> { + let (buf, mut view) = attached(text); + let mut storage = vec![Cell::default(); cols as usize]; + let mut grid = CellGrid { + cells: &mut storage, + stride: cols, + size: CellSize::new(1, cols), + }; + let viewport = Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(1, cols), + gutter_w: 0, + folds: None, + wrap: WrapMode::Truncate, + view_left, + }; + view.render(&buf, viewport, &mut grid); + let store: pmacs::overlay::SharedBufferStyleSpans = Arc::new(Mutex::new(spans)); + let mut overlay = BufferStyleOverlay::new(store); + overlay.render(&buf, viewport, &mut grid); + (0..cols as usize) + .map(|c| { + let g = match storage[c].glyph { + Glyph::Char(ch) => ch, + _ => ' ', + }; + (g, storage[c].style != Style::default()) + }) + .collect() +} + +fn styled(start: u64, end: u64) -> BufferStyleSpan { + BufferStyleSpan { + start, + end, + style: Style { + bold: true, + ..Style::default() + }, + } +} + +/// A style span sits on the characters it names, at a non-zero offset. +#[test] +fn a_style_span_travels_with_its_characters() { + // Style covers bytes 4..6 ("EF"), which scroll to screen columns 0..2. + let out = row0_with_styles(b"ABCDEFGHIJ", 4, 4, vec![styled(4, 6)]); + let text: String = out.iter().map(|(g, _)| *g).collect(); + assert_eq!(text, "EFGH", "precondition: the glyphs did translate"); + assert_eq!( + out.iter().map(|(_, s)| *s).collect::>(), + vec![true, true, false, false], + "the style must land on E and F — before the fix it painted at \ + absolute columns 4..6, i.e. screen columns 4..6, off this window" + ); +} + +/// A span beginning off-screen and reaching into view is CLIPPED, not +/// dropped — the boundary the selection painter got wrong. +#[test] +fn a_span_starting_off_screen_still_paints_its_visible_tail() { + // Style covers bytes 2..6 ("CDEF"); C and D are scrolled off. + let out = row0_with_styles(b"ABCDEFGHIJ", 4, 4, vec![styled(2, 6)]); + assert_eq!( + out.iter().map(|(_, s)| *s).collect::>(), + vec![true, true, false, false], + "the visible tail (E, F) must still be styled; skipping the whole \ + span because it starts left of the edge is the defect" + ); +} + +/// And a span entirely left of the edge paints nothing. +#[test] +fn a_span_entirely_off_screen_paints_nothing() { + let out = row0_with_styles(b"ABCDEFGHIJ", 4, 4, vec![styled(0, 3)]); + assert!( + out.iter().all(|(_, s)| !*s), + "a span that ends before the edge must not paint — clamping it to \ + column 0 instead would smear it onto unrelated text" + ); +} + +/// Under `wrap` the decorator translation is inert too, matching the +/// base walk. +#[test] +fn decorations_ignore_the_offset_under_wrap() { + let (buf, _) = attached(b"ABCDEFGH"); + let _ = buf; + let a = row0_with_styles(b"ABCDEFGH", 4, 0, vec![styled(0, 2)]); + // Same span, non-zero offset, wrapping: `left_edge()` pins to 0. + let (buf2, mut view2) = attached(b"ABCDEFGH"); + let mut storage = vec![Cell::default(); 4]; + let mut grid = CellGrid { + cells: &mut storage, + stride: 4, + size: CellSize::new(1, 4), + }; + let viewport = Viewport { + buffer_start: 0, + buffer_end: buf2.len(), + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(1, 4), + gutter_w: 0, + folds: None, + wrap: WrapMode::Wrap, + view_left: 4, + }; + view2.render(&buf2, viewport, &mut grid); + let store: pmacs::overlay::SharedBufferStyleSpans = Arc::new(Mutex::new(vec![styled(0, 2)])); + let mut overlay = BufferStyleOverlay::new(store); + overlay.render(&buf2, viewport, &mut grid); + let wrapped: Vec = (0..4) + .map(|c| storage[c].style != Style::default()) + .collect(); + assert_eq!( + wrapped, + a.iter().map(|(_, s)| *s).collect::>(), + "a wrapped render must ignore the offset for decorations exactly \ + as it does for glyphs" + ); +} diff --git a/tests/listview_acceptance.rs b/tests/listview_acceptance.rs index 6653fbd..7979287 100644 --- a/tests/listview_acceptance.rs +++ b/tests/listview_acceptance.rs @@ -163,6 +163,7 @@ fn paint_active_window(s: &EditorState, rows: u32, cols: u32) -> Vec Viewport<'static> { gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, } } diff --git a/tests/terminal_copy_mode_acceptance.rs b/tests/terminal_copy_mode_acceptance.rs index 13f3758..b85f3a3 100644 --- a/tests/terminal_copy_mode_acceptance.rs +++ b/tests/terminal_copy_mode_acceptance.rs @@ -487,6 +487,7 @@ fn render_active_window_to_grid( gutter_w: 0, folds: None, wrap: WrapMode::Truncate, + view_left: 0, }; let mut grid = CellGrid { cells: &mut backing,