From b110684a62dab51144de11398f2eb9340284a823 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 20:46:39 +0200 Subject: [PATCH 1/7] docs: frame QoL Stage 4, horizontal scroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #221 merged, so the long-lines lane is REWRITTEN rather than removed — rule 4 removes a lane when its arc is done, and Stage 4 is ahead. Stage 3's durable facts move to the handoff §1, which is rule 4's actual precondition. THE FRAMING LEADS WITH THE FACT MOST LIKELY TO INVERT ITS OWN COST ESTIMATE, because that is what Stage 3 revision 1 got wrong. The GPU cannot honor horizontal scroll through cosmic-text: `Scroll::horizontal` is discarded throughout, and not by oversight — glyphon 0.11 never applies it when placing glyphs. Documented in three places and asserted by three tests. So the GPU's half needs a mechanism that does not exist, touching caret placement, decoration geometry, and hit testing, each of which assumes x starts at `text_left()`. That is Q#HS1: whether the GPU is in Stage 4 at all. Also verified rather than recalled: there is NO horizontal scroll anywhere in the tree (greenfield, not an extension); `paint_line` starts every walk at column 0, so `view_left` enters the functions Stage 3 just rewrote and the wrap rule must stay written once; `view_top` is persisted per leaf at DESKTOP_VERSION 1; `scroll_window`'s comment already records the cursor-follow hazard; and `goal_col` is unexamined horizontal state on the same window. Six questions, each with my vote and the argument against it. The one I am least comfortable with is Q#HS6: Stage 4 adds capability that exists only under a NON-DEFAULT mode, which is a conditional surface rather than a uniform improvement — and Stage 3 chose `wrap` as the default partly BECAUSE scroll did not exist. If scroll makes `truncate` good, that default deserves re-examination rather than inheritance. ALSO A GATE CORRECTION, and it changes what I said in 4d70ff6. I wrote that "the touched acceptance suites" is the standing gate. That is CLAUDE.md's list. `docs/agent-handoff.md` §3 — which CLAUDE.md tells me to read FIRST — already required `cargo test --workspace -- --skip basedpyright`, a full sweep. I ran the short list. So the eight broken version assertions were not a gap in the documented gates; they were me following a summary instead of the gate suite. §3 now says so, and adds the protocol-bump form (`--tests --no-fail-fast` in both feature configurations), because even the full sweep stops at the first failing target and builds one configuration — it would have shown one or two of the eight, not all of them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 333 +++++++----------------------- docs/agent-handoff.md | 99 +++++++-- docs/horizontal-scroll-framing.md | 248 ++++++++++++++++++++++ 3 files changed, 407 insertions(+), 273 deletions(-) create mode 100644 docs/horizontal-scroll-framing.md diff --git a/docs/active-work.md b/docs/active-work.md index b00ed4d..bb7c8d1 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -434,278 +434,93 @@ 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 framing -**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. +**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`. -### The defect +**Status: framing, not approved.** `docs/horizontal-scroll-framing.md` +revision 1. No implementation may begin until its questions are +answered. -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. +### What Stage 3 shipped that Stage 4 must live with -### The six answers +- **`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**; that is + stated in the setting's description and in `ui.toggle-line-wrap`'s + status message, both of which should be 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. -- **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`. +### Ground truth gathered for the framing (verify before trusting) -### The two holes review found in the first commit +- **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. -- **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. +### Gate note this lane inherits - **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. - - **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". - - **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. - -### The framing error, and the four defects review found in the code - -**`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. - -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. - -The four code defects, each caught by user review or by the tests -written for it: - -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. - -**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. - -### The two decisions most likely to be questioned later - -- **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. - -### Verification (delivered) - -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. +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.** If Stage 4 touches +`PROTOCOL_VERSION` — an open question, since the GPU may need a wire — +the sweep must be `cargo test --tests --no-fail-fast` in **both** +feature configurations. See §3 and §5's protocol-bump bullet. ### 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..12144bd 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -83,7 +83,52 @@ 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); Stage 4 is the + remainder.** 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**, and `truncate` is incomplete + without it: text past the right edge is currently *unreachable*, + which is why `wrap` is the default and why the toggle says so. + 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 +450,28 @@ 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. **On any protocol bump run `cargo test --tests --no-fail-fast` + in BOTH configurations** — `--no-fail-fast` for the stop-at-first + behavior, `--features crdt` because three of the eight were + crdt-gated real-daemon tests asserting on a live socket's negotiated + version, which is the blindness the bullet below already names. **Sort the failures before fixing them.** A *tripwire* (`assert_eq!(PROTOCOL_VERSION, N)`) is meant to fire and takes a @@ -2065,6 +2118,24 @@ 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` replaces the sweep line with:** + +``` +cargo test --tests --no-fail-fast +cargo test --tests --features crdt --no-fail-fast +``` + +Plain `--workspace` stops at the first failing target and builds one +feature configuration, so on a version bump it reports one or two of +what may be many. See §5's protocol-bump bullet for how to sort the +failures it finds — some are tripwires doing their job. + Machine-specific caveats — re-verify on a machine you haven't used before trusting them: diff --git a/docs/horizontal-scroll-framing.md b/docs/horizontal-scroll-framing.md new file mode 100644 index 0000000..4f3b759 --- /dev/null +++ b/docs/horizontal-scroll-framing.md @@ -0,0 +1,248 @@ +# Horizontal scroll — QoL Stage 4 + +**Status: revision 1 — NOT APPROVED. Six questions open (Q#HS1–HS6). +No implementation may begin.** + +This closes the QoL arc opened by one daily-driver report. 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 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*, and said +so: under `truncate`, text past the right edge is not merely off-screen +but **unreachable**. That is recorded in the setting's description and +in `ui.toggle-line-wrap`'s status message. Stage 4 removes that caveat +or the caveat stands permanently. + +--- + +## 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/`. +This is greenfield. The window carries `view_top`, `cursor`, and +`goal_col` (`src/window.rs:374-376`) and nothing horizontal. + +That matters for estimating: this is not "extend the vertical +mechanism sideways". 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 it did 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. **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 + +**This is the finding most likely to invert the cost estimate, and it +is the Stage 4 analog of revision 1's error — 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`. + +So the GPU's half of Stage 4 cannot be "set the scroll and reshape". It +needs a different mechanism — a shifted text origin at paint time, +adjusted clip bounds, or something else — and that mechanism has to +interact 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 currently assumes x starts at +`text_left()`. + +**Q#HS1 asks whether the GPU is in scope for Stage 4 at all.** + +### 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`). + +A horizontal offset that does not survive restart is defensible; one +that does needs a defaulted field or a version bump. **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 exactly 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. + +A horizontal analog hits the identical problem. Stage 3's Q#LL3 +deferred the choice here deliberately: **does explicit horizontal +scroll drag the cursor, or does the next motion snap back?** That is +**Q#HS4**. + +### 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 and is cleared at seven sites in `src/editor.rs`. It is +a *column within the line*, not a viewport offset — but both are +"horizontal position" state on the same window, and a design that +ignores the interaction will produce a cursor that jumps on the first +vertical motion after a horizontal scroll. Called out so it is designed +rather than discovered. + +--- + +## 2. The scope question, stated before the answers + +Stage 3 ended with `wrap` as the default. **Under `wrap`, horizontal +scroll is meaningless** — there is nothing past the right edge. So +Stage 4's entire surface is conditional on a buffer-local mode. + +That is a coherence fact, not only an implementation one: one +user-facing concept ("how do I see the rest of this line?") now has two +disjoint answers depending on a setting, and the commands, key +bindings, and status affordances for the `truncate` half do not exist +under `wrap`. `COHERENCE.md` §20 requires this be stated. **Q#HS6.** + +--- + +## 3. Open questions + +### Q#HS1 — is the GPU in scope for Stage 4? + +The strongest argument for **yes**: Stage 3's entire thesis was that +the two frontends should stop disagreeing by accident. Shipping +horizontal scroll in the TUI only would recreate exactly the divergence +`ui.line-wrap` was built to close — a `truncate` buffer would be +navigable in one frontend and not the other. + +The strongest argument for **no, name it Stage 5**: §1.3. The GPU needs +a mechanism that does not exist yet, touching caret placement, +decoration geometry, and hit testing. That is plausibly larger than the +TUI half, and bundling them makes one reviewable change into two +unreviewable ones. + +**My vote: split it, and say so in the setting's description.** Ship +the TUI half as Stage 4 and the GPU half as Stage 5, with the +divergence *documented and time-boxed* rather than accidental — which +is the distinction Stage 3 actually drew. Stage 3's defect was never +"the frontends differ"; it was "the frontends differ and nobody chose +that". But this is a product call about shipping a known asymmetry, and +it is not mine to make. + +### Q#HS2 — what moves the viewport? + +Options, not mutually exclusive: + +- **Automatic only** — the cursor-visibility pass gains a horizontal + component, so moving the cursor past the edge scrolls the view. No + new commands, no new bindings. Smallest surface; makes a long line + readable by arrowing along it. +- **Explicit commands** — `ui.scroll-left` / `ui.scroll-right`, bound + or not, plus the `goal_col` and cursor-follow questions. +- **Both**, which is what every editor with this feature ships. + +**My vote: automatic first, as its own stage-within-a-stage.** It is +the smallest change that makes the reported text *reachable*, and it +needs no binding decisions. Explicit commands can follow with the +evidence of use. + +### Q#HS3 — per window or per buffer? + +`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 already 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. + +**This is not really open**; it is listed so the accepted split is +re-confirmed at the moment it takes effect rather than inherited +silently. + +### Q#HS4 — does the cursor follow an explicit scroll? + +Only live if Q#HS2 includes explicit commands. §1.5 has the precedent +and the hazard. **My vote: follow, matching `scroll_window`** — the +existing snap-back pass makes the alternative feel broken, and the +vertical behavior is already the answer users have been trained on +*in this editor*. + +### Q#HS5 — does `view_left` survive a restart? + +`view_top` does (§1.4). Consistency argues yes; a defaulted field +avoids a `DESKTOP_VERSION` bump. + +**My vote: yes, defaulted, no version bump** — but confirm that +`SavedLeaf`'s deserializer tolerates a missing field before relying on +it, because §1.4 is a citation of the *shape*, not of serde's +behavior on it. + +### Q#HS6 — what does the coherence statement say? + +Per `COHERENCE.md` §20 this framing must state its coherence impact. +The honest version is uncomfortable: Stage 4 adds capability that +exists **only under a non-default mode**, which is a new conditional +surface rather than a uniform improvement. Journey step 4 ("Understand +interface") is the row it serves. + +**Q#HS6 is whether that is acceptable, or whether the arc should +instead reconsider the default.** Naming the alternative honestly: if +horizontal scroll makes `truncate` genuinely good, `wrap` being the +default is a choice worth re-examining rather than treating as settled +— and Stage 3 chose it partly *because* scroll did not exist. + +--- + +## 4. Verification sketch (not final — depends on Q#HS1/HS2) + +- Cell-level tests at several window widths with a non-zero offset — + which is the Stage 4 case Stage 3's sketch explicitly refused to + write, 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, since under `wrap` + the offset must be inert. +- Round-trip identity for `place_of_byte` / `byte_at_place` at non-zero + offset, including the wide-character and tab cases Stage 3 settled at + offset 0. +- **A PTY acceptance test for reachability**, following + `tests/long_line_readable_acceptance.rs`: in `truncate`, the tail of + a long line must reach the terminal *after* whatever Q#HS2 chooses + moves the view. 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. +- If the GPU is in scope: a headless witness 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 3's + framing said the scorecard should name "a line that cannot be read in + full" against this step. Stage 4 completes that only for `truncate`. +- **§16 Semantic Frontend Architecture.** Q#HS1 decides whether this + lane *narrows* or *widens* frontend divergence. If the GPU is + deferred, the divergence is deliberate and time-boxed — which is + materially different from Stage 3's inherited accident, and the + release notes must say which kind it is. +- **No new interaction island** if Q#HS2 lands automatic-only. + Explicit commands would go in the ordinary command registry, not a + new surface. +- **Config registry adoption**: none new expected. Stage 4 navigates + the mode Stage 3 declared; if it needs a setting, that is a signal + the design has drifted. From 5d041a4f1745baab17263f8ea3dc03d2b9e875ae Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 21:01:46 +0200 Subject: [PATCH 2/7] =?UTF-8?q?docs:=20Stage=204=20revision=202=20?= =?UTF-8?q?=E2=80=94=20three=20answers,=20and=20the=20contract=20revision?= =?UTF-8?q?=201=20forgot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — THE PROTOCOL-BUMP GATE WEAKENED THE SWEEP IT MEANT TO STRENGTHEN. Revision 1 said `--tests --no-fail-fast` REPLACES the `--workspace` line. Measured: `--tests` selects 108 targets, `--workspace` selects 110, and the two it drops are `pmacs_protocol` and `pmacs_gpu`. On a PROTOCOL bump, dropping the protocol crate's tests is the wrong loss — and it also silently dropped `--skip basedpyright`. Worse, this lane's own remediation sweep used `--tests`, so it never ran pmacs-protocol's 25 tests, including the `scroll::classify` tests this lane had just written. They pass (verified 25/0), but by luck. A correction that reproduces the shape of the mistake it corrects is worth naming, so §3 and §5 both say `--workspace`, additive, with `-- --skip basedpyright` retained in both feature configurations. P1 — NO COORDINATE CONTRACT FOR view_left. Revision 1 decided what moves the viewport and never said what its offset IS — the same omission as shipping WrapMode with no DisplayCoord. Its verification sketch named tabs and wide characters with no oracle for either, because nothing defined what a left edge is. Q#HS7 is new and BLOCKING, in four coupled parts: the unit; which columns may be a left edge; the snap rule for an invalid one; and the invariant rendering and coordinate mapping share. Votes recorded — display column (tab stops come free, since the walk must start at column 0 either way and a byte offset buys nothing); a left edge may not fall inside a wide glyph; snap toward the line start (snapping left can only reveal a character, snapping right can hide the one the user scrolled to reach); and snap when the value is SET, not in the painter, so one canonical value serves both readers. Part (d) is why it blocks: if the painter clips where the mapper does not, clicks land on the wrong character — silently, and only on lines wide enough to scroll. P2 — THE CAVEAT IS NOT IN THE SETTING DESCRIPTION. Revision 1 said it was. builtin/runtime/linewrap.lua:23 says only "truncate at the edge"; "unreachable" lives in the toggle's status message and a source comment, neither of which a user sees who sets the mode in init.lua. That is a real, small user-facing gap shipped in #221. Claim corrected, and amending the description is now a Stage 4 deliverable (§6) with the text depending on which stage has landed. THE THREE ANSWERS, recorded with the reasoning that decided them: HS1 — GPU is Stage 5. The distinction that matters is that Stage 3's defect was never "the frontends differ" but "the frontends differ and nobody chose that". Time box made concrete per the request: Stage 5 is the immediately-next QoL lane, `wrap` stays default until it lands, release notes state the asymmetry, and the truncate affordances name the GUI gap meanwhile. HS2 — automatic only. No command surface; the cursor-visibility pass gains a horizontal component. HS6 — `wrap` stays default, and the reason given is stronger than the one revision 1 reasoned from. I had framed it as "if scroll makes truncate good, reconsider the default". With the GPU deferred, a truncate default would ship a mode navigable in the TUI and a dead end in the GUI for every user who never opened the setting. HS1 and HS6 are coupled: the split is only safe because the default does not move. Q#HS4 is deferred rather than closed — not live under automatic-only, but the snap-back hazard is real and rediscovering it costs more than carrying the paragraph. Q#HS5 stands, with the caveat that §1.4 cites the struct shape and not serde's behavior on a missing field. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 66 +++++- docs/agent-handoff.md | 43 +++- docs/horizontal-scroll-framing.md | 365 ++++++++++++++++++------------ 3 files changed, 311 insertions(+), 163 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index bb7c8d1..c22f16c 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -449,9 +449,50 @@ tip — the ref, not a SHA, since any edit to this block advances past whatever SHA it records. Recover: `git fetch githubsucks && git checkout horizontal-scroll`. -**Status: framing, not approved.** `docs/horizontal-scroll-framing.md` -revision 1. No implementation may begin until its questions are -answered. +**Status: framing, NOT approved.** `docs/horizontal-scroll-framing.md` +revision 2. No implementation may begin. + +**Answered by the user 2026-08-07:** + +- **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. + +**Still blocking:** + +- **Q#HS7 (NEW) — what IS `view_left`?** Revision 1 decided what moves + the viewport without saying what its offset *is* — the same omission + as shipping `WrapMode` with no `DisplayCoord`. Four coupled parts: + the unit; which columns may be a left edge; the snap rule for an + invalid one; and the invariant rendering and coordinate mapping + share. Without the last, **a painter that clips where the mapper does + not puts clicks on the wrong character**, silently, only on lines + wide enough to scroll. §4's sketch has no oracle until this is + answered. +- **Q#HS5 — does `view_left` survive a restart?** Vote: yes, defaulted, + no `DESKTOP_VERSION` bump — *after* confirming `SavedLeaf`'s + deserializer tolerates a missing field, which is a claim about serde + and not about the struct shape §1.4 cites. +- **Q#HS3** is re-confirmed rather than open (per-window, per Q#LL2); + **Q#HS4** is deferred, live only if explicit commands arrive. + +**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). ### What Stage 3 shipped that Stage 4 must live with @@ -512,10 +553,21 @@ answered. 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.** If Stage 4 touches -`PROTOCOL_VERSION` — an open question, since the GPU may need a wire — -the sweep must be `cargo test --tests --no-fail-fast` in **both** -feature configurations. See §3 and §5's protocol-bump bullet. +which includes a full sweep. **§3 is the authority.** + +**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. + +**`--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 diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 12144bd..60b530f 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -467,11 +467,14 @@ someone forgot. 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. **On any protocol bump run `cargo test --tests --no-fail-fast` - in BOTH configurations** — `--no-fail-fast` for the stop-at-first - behavior, `--features crdt` because three of the eight were - crdt-gated real-daemon tests asserting on a live socket's negotiated - version, which is the blindness the bullet below already names. + 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 @@ -2124,17 +2127,33 @@ 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` replaces the sweep line with:** +**Touching `PROTOCOL_VERSION` STRENGTHENS the sweep line. It does not +replace it:** ``` -cargo test --tests --no-fail-fast -cargo test --tests --features crdt --no-fail-fast +cargo test --workspace --no-fail-fast -- --skip basedpyright +cargo test --workspace --features crdt --no-fail-fast -- --skip basedpyright ``` -Plain `--workspace` stops at the first failing target and builds one -feature configuration, so on a version bump it reports one or two of -what may be many. See §5's protocol-bump bullet for how to sort the -failures it finds — some are tripwires doing their job. +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/horizontal-scroll-framing.md b/docs/horizontal-scroll-framing.md index 4f3b759..b10243e 100644 --- a/docs/horizontal-scroll-framing.md +++ b/docs/horizontal-scroll-framing.md @@ -1,7 +1,8 @@ # Horizontal scroll — QoL Stage 4 -**Status: revision 1 — NOT APPROVED. Six questions open (Q#HS1–HS6). -No implementation may begin.** +**Status: revision 2 — NOT APPROVED. Q#HS1, HS2 and HS6 answered by the +user (2026-08-07); Q#HS3–HS5 stand; Q#HS7 is NEW and BLOCKING. No +implementation may begin.** This closes the QoL arc opened by one daily-driver report. Stage 1 (#219) made the TUI survive terminal zoom; Stage 2 (#220) gave the GUI @@ -12,11 +13,20 @@ default. Stage 4 is the other half of the user's own sentence: > 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*, and said -so: under `truncate`, text past the right edge is not merely off-screen -but **unreachable**. That is recorded in the setting's description and -in `ui.toggle-line-wrap`'s status message. Stage 4 removes that caveat -or the caveat stands permanently. +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. --- @@ -29,28 +39,26 @@ 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/`. -This is greenfield. The window carries `view_top`, `cursor`, and -`goal_col` (`src/window.rs:374-376`) and nothing horizontal. - -That matters for estimating: this is not "extend the vertical -mechanism sideways". There is no shared abstraction to extend. +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 it did before Stage 3 — +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. **The -wrap rule must stay written exactly once** (`advance_wrapped`, -`src/text_view.rs:396`); a second copy differing by an offset is the +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 -**This is the finding most likely to invert the cost estimate, and it -is the Stage 4 analog of revision 1's error — so it leads.** +**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.** @@ -58,191 +66,260 @@ Documented at `pmacs-gpu/src/main.rs:1611`, `:6316`, `:8020`, and *asserted* by tests at `:16266` (`"horizontal is discarded"`), `:16337`, `:16737`. -So the GPU's half of Stage 4 cannot be "set the scroll and reshape". It -needs a different mechanism — a shifted text origin at paint time, -adjusted clip bounds, or something else — and that mechanism has to -interact 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 currently assumes x starts at -`text_left()`. +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()`. -**Q#HS1 asks whether the GPU is in scope for Stage 4 at all.** +**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`). - -A horizontal offset that does not survive restart is defensible; one -that does needs a defaulted field or a version bump. **Q#HS5.** +`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 exactly why: +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. -A horizontal analog hits the identical problem. Stage 3's Q#LL3 -deferred the choice here deliberately: **does explicit horizontal -scroll drag the cursor, or does the next motion snap back?** That is -**Q#HS4**. +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 and is cleared at seven sites in `src/editor.rs`. It is -a *column within the line*, not a viewport offset — but both are -"horizontal position" state on the same window, and a design that -ignores the interaction will produce a cursor that jumps on the first -vertical motion after a horizontal scroll. Called out so it is designed -rather than discovered. +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 question, stated before the answers +## 2. The scope fact, stated before the answers -Stage 3 ended with `wrap` as the default. **Under `wrap`, horizontal -scroll is meaningless** — there is nothing past the right edge. So -Stage 4's entire surface is conditional on a buffer-local mode. - -That is a coherence fact, not only an implementation one: one -user-facing concept ("how do I see the rest of this line?") now has two -disjoint answers depending on a setting, and the commands, key -bindings, and status affordances for the `truncate` half do not exist -under `wrap`. `COHERENCE.md` §20 requires this be stated. **Q#HS6.** +**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. Open questions +## 3. Questions -### Q#HS1 — is the GPU in scope for Stage 4? +### Q#HS1 — is the GPU in scope? **ANSWERED: no — Stage 5** -The strongest argument for **yes**: Stage 3's entire thesis was that -the two frontends should stop disagreeing by accident. Shipping -horizontal scroll in the TUI only would recreate exactly the divergence -`ui.line-wrap` was built to close — a `truncate` buffer would be -navigable in one frontend and not the other. +> **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 strongest argument for **no, name it Stage 5**: §1.3. The GPU needs -a mechanism that does not exist yet, touching caret placement, -decoration geometry, and hit testing. That is plausibly larger than the -TUI half, and bundling them makes one reviewable change into two -unreviewable ones. +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. -**My vote: split it, and say so in the setting's description.** Ship -the TUI half as Stage 4 and the GPU half as Stage 5, with the -divergence *documented and time-boxed* rather than accidental — which -is the distinction Stage 3 actually drew. Stage 3's defect was never -"the frontends differ"; it was "the frontends differ and nobody chose -that". But this is a product call about shipping a known asymmetry, and -it is not mine to make. +**The time box, concrete** (the user's requirement, and the part that +makes this a decision rather than a deferral): -### Q#HS2 — what moves the viewport? +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. -Options, not mutually exclusive: +### Q#HS2 — what moves the viewport? **ANSWERED: automatic only** -- **Automatic only** — the cursor-visibility pass gains a horizontal - component, so moving the cursor past the edge scrolls the view. No - new commands, no new bindings. Smallest surface; makes a long line - readable by arrowing along it. -- **Explicit commands** — `ui.scroll-left` / `ui.scroll-right`, bound - or not, plus the `goal_col` and cursor-follow questions. -- **Both**, which is what every editor with this feature ships. +> **Answered 2026-08-07 (user):** *"Automatic-only first. It makes the +> report's text reachable with no new command surface."* -**My vote: automatic first, as its own stage-within-a-stage.** It is -the smallest change that makes the reported text *reachable*, and it -needs no binding decisions. Explicit commands can follow with the -evidence of use. +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. -### Q#HS3 — per window or per buffer? +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 already 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. +`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. -**This is not really open**; it is listed so the accepted split is -re-confirmed at the moment it takes effect rather than inherited -silently. +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? +### Q#HS4 — does the cursor follow an explicit scroll? **DEFERRED, not answered** -Only live if Q#HS2 includes explicit commands. §1.5 has the precedent -and the hazard. **My vote: follow, matching `scroll_window`** — the -existing snap-back pass makes the alternative feel broken, and the -vertical behavior is already the answer users have been trained on -*in this editor*. +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. -### Q#HS5 — does `view_left` survive a restart? +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? **OPEN** `view_top` does (§1.4). Consistency argues yes; a defaulted field avoids a `DESKTOP_VERSION` bump. -**My vote: yes, defaulted, no version bump** — but confirm that +**My vote: yes, defaulted, no version bump** — but **confirm `SavedLeaf`'s deserializer tolerates a missing field before relying on -it, because §1.4 is a citation of the *shape*, not of serde's -behavior on it. +it.** §1.4 cites the struct's *shape*, not serde's behavior on it, and +that gap is exactly the kind §1.3 exists to warn about. -### Q#HS6 — what does the coherence statement say? +### Q#HS6 — the coherence statement **ANSWERED: keep `wrap` default** -Per `COHERENCE.md` §20 this framing must state its coherence impact. -The honest version is uncomfortable: Stage 4 adds capability that -exists **only under a non-default mode**, which is a new conditional -surface rather than a uniform improvement. Journey step 4 ("Understand -interface") is the row it serves. +> **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."* -**Q#HS6 is whether that is acceptable, or whether the arc should -instead reconsider the default.** Naming the alternative honestly: if -horizontal scroll makes `truncate` genuinely good, `wrap` being the -default is a choice worth re-examining rather than treating as settled -— and Stage 3 chose it partly *because* scroll did not exist. +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`? **NEW, BLOCKING** + +**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) Which columns may be a left edge.** 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. + +*My vote: a left edge may not fall inside a wide glyph's cells.* + +**(c) The snap rule when an invalid edge is requested.** Stage 3's +coordinate contract is *"identity on canonical inputs; otherwise +projection to the contract's **designated** canonical representative"* — +designated, not nearest, because the direction differs per function and +"nearest" hides that. + +*My vote: snap toward the line start.* Snapping left can only reveal a +character, never hide one that was visible; snapping right can hide the +very glyph the user scrolled to reach. And snapping at the moment +`view_left` is **set** — rather than clipping in the painter — keeps +one canonical value that both painter and mapper read, instead of two +that can disagree. + +**(d) The invariant rendering and coordinate mapping share.** For a +given `view_left`, `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. + +*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 (not final — depends on Q#HS1/HS2) +## 4. Verification sketch (depends on Q#HS7) -- Cell-level tests at several window widths with a non-zero offset — - which is the Stage 4 case Stage 3's sketch explicitly refused to - write, because "at non-zero offset" was a `view_left` requirement - smuggled into a wrap lane. +- 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, since under `wrap` - the offset must be inert. + 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, including the wide-character and tab cases Stage 3 settled at - offset 0. + offset — the Q#HS7(d) invariant, walked exhaustively over a short + line rather than sampled, as Stage 3 established. +- **The Q#HS7(b)/(c) cases, which have no oracle until it is + answered**: a wide glyph straddling the left edge; a tab whose + expansion straddles it; a snap request landing inside each. - **A PTY acceptance test for reachability**, following - `tests/long_line_readable_acceptance.rs`: in `truncate`, the tail of - a long line must reach the terminal *after* whatever Q#HS2 chooses - moves the view. 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. -- If the GPU is in scope: a headless witness that the caret, a - decoration, and a hit test all agree with the shifted origin — the - three consumers §1.3 names. + `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 3's - framing said the scorecard should name "a line that cannot be read in - full" against this step. Stage 4 completes that only for `truncate`. -- **§16 Semantic Frontend Architecture.** Q#HS1 decides whether this - lane *narrows* or *widens* frontend divergence. If the GPU is - deferred, the divergence is deliberate and time-boxed — which is - materially different from Stage 3's inherited accident, and the - release notes must say which kind it is. -- **No new interaction island** if Q#HS2 lands automatic-only. - Explicit commands would go in the ordinary command registry, not a - new surface. -- **Config registry adoption**: none new expected. Stage 4 navigates - the mode Stage 3 declared; if it needs a setting, that is a signal - the design has drifted. +- **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. From baa71b607eb8c768aec6c007c43228686717ecf1 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 21:14:58 +0200 Subject: [PATCH 3/7] =?UTF-8?q?docs:=20Stage=204=20revision=203=20?= =?UTF-8?q?=E2=80=94=20a=20setter-time=20snap=20cannot=20exist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — Q#HS7(c) WITHDRAWN, and my vote was wrong rather than vague. Revision 2 voted to snap `view_left` to a valid boundary at the moment it is set. That cannot exist. `view_left` is ONE per-window display column, but "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 ordinary ASCII on line 4. No setter-time value is canonical for every visible line. Snapping per line instead is worse — the same source column would appear at different screen columns on different rows, destroying the alignment a column-oriented view exists to provide. Replaced by (c′), a per-line effective edge. `view_left` is stored unsnapped; each line derives its own edge during the walk it already performs from column 0. Where the requested edge bisects a wide glyph on THAT line, the finding's actual question — what occupies the leftmost cell — is answered: it paints as a space carrying the glyph's style, and the mapping DESIGNATES that cell to the wide glyph's start byte. That keeps `byte_at_place` total over visible cells, preserves the round trip (`place_of_byte(start)` reports the straddle and designates cell 0), and gives a click there the character a user would expect. Bytes lying entirely left of the edge are reported not-visible rather than clamped to column 0, because clamping would make arbitrarily many bytes share cell 0 and destroy (d). Recorded as deliberately NOT the mirror of Stage 3's right-edge rule: under wrap a too-wide glyph is pushed to the next row entirely, and at the left edge under truncate there is no next row, so the same intent requires a different rule. Stated so nobody "fixes" one to match the other. (d) is amended accordingly: the invariant is a property of `(view_left, line)`, not of `view_left` alone — which is what makes a multi-line fixture with differing glyph widths at the same column the DISCRIMINATING test rather than an extra one. A single-line sweep passes against the withdrawn design. P1 — Stage 4 no longer closes the arc, and the stale claim was load-bearing in the wrong direction. Rule 4 removes a lane when its ARC is done, so a framing asserting Stage 4 closes it would license retiring the lane at the TUI merge — orphaning the very Stage 5 that Q#HS1's time box exists to guarantee, while `truncate` is still a dead end in the GUI. Both the framing opening and the lane header now say the arc closes at Stage 5, and the lane carries an explicit "Rule 4 does not apply at Stage 4's merge". P2 — the stale Stage 3 residue in the ledger claimed the unreachable caveat is in the setting description, contradicting revision 2 forty lines above it. Corrected in place: the caveat lives only in the toggle's status message and a source comment, and a user who sets the mode in init.lua is told nothing. Q#HS5 now states the concrete condition rather than an instruction to check one. Verified: SavedLeaf carries no #[serde(default)] anywhere in src/desktop.rs, so serde would REJECT a version-1 desktop JSON omitting a new `view_left`. "Yes, no version bump" is sound only with the annotation AND a regression fixture holding literal v1 JSON without the field. The reverse direction already works — an old binary meets an unknown field, which serde ignores absent deny_unknown_fields, and there is none in that file. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 63 +++++++++--- docs/horizontal-scroll-framing.md | 162 +++++++++++++++++++++++------- 2 files changed, 173 insertions(+), 52 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index c22f16c..5d8d1d9 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -435,7 +435,7 @@ Whether `docs/ci-red-signatures.md` should grow a short non-row section for this class is an open question for its owner, not something this lane decided. -## Long lines (QoL arc) — Stage 3 MERGED as #221; Stage 4 is framing +## Long lines (QoL arc) — Stage 3 MERGED as #221; Stages 4 AND 5 ahead **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 @@ -443,6 +443,14 @@ this one has Stage 4 ahead. Stage 3's durable facts are absorbed into than deferred — so what remains here is the Stage 4 plan and only the Stage 3 residue that constrains it. +> **RULE 4 DOES NOT APPLY AT STAGE 4's MERGE.** The arc closes at +> **Stage 5** (GPU horizontal scroll), not Stage 4. Q#HS1 split the GPU +> out deliberately and time-boxed it; retiring this lane when the TUI +> half merges would orphan exactly the half the time box exists to +> guarantee, and would do so while `truncate` is still a dead end in +> the GUI. **Do not remove this block until Stage 5 has merged**, and +> read Q#HS1's four time-box items before concluding otherwise. + **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 @@ -450,7 +458,7 @@ whatever SHA it records. Recover: `git fetch githubsucks && git checkout horizontal-scroll`. **Status: framing, NOT approved.** `docs/horizontal-scroll-framing.md` -revision 2. No implementation may begin. +revision 3. No implementation may begin. **Answered by the user 2026-08-07:** @@ -472,17 +480,36 @@ revision 2. No implementation may begin. - **Q#HS7 (NEW) — what IS `view_left`?** Revision 1 decided what moves the viewport without saying what its offset *is* — the same omission - as shipping `WrapMode` with no `DisplayCoord`. Four coupled parts: - the unit; which columns may be a left edge; the snap rule for an - invalid one; and the invariant rendering and coordinate mapping - share. Without the last, **a painter that clips where the mapper does - not puts clicks on the wrong character**, silently, only on lines - wide enough to scroll. §4's sketch has no oracle until this is - answered. -- **Q#HS5 — does `view_left` survive a restart?** Vote: yes, defaulted, - no `DESKTOP_VERSION` bump — *after* confirming `SavedLeaf`'s - deserializer tolerates a missing field, which is a claim about serde - and not about the struct shape §1.4 cites. + as shipping `WrapMode` with no `DisplayCoord`. Without part (d), **a + painter that clips where the mapper does not puts clicks on the wrong + character**, silently, only on lines wide enough to scroll. + + **Revision 2's part (c) is WITHDRAWN.** It voted to snap `view_left` + to a valid boundary when set. That cannot exist: `view_left` is one + per-window display column, but *"does column N bisect a wide + glyph?"* is a **per-line** question — column 11 can be a wide glyph's + trailing cell on one line and ordinary ASCII on the next. No + setter-time value is canonical for every visible line, and snapping + per line instead would break vertical alignment. + + Replaced by a **per-line effective edge**: `view_left` stored + unsnapped, each line deriving its own edge in the walk it already + performs from column 0. Where the edge bisects a wide glyph, that + glyph's trailing cell **paints blank and the mapping designates it to + the glyph's start byte** — which keeps `byte_at_place` total and + preserves the round trip. The invariant is therefore a property of + **`(view_left, line)`**, not of `view_left` alone, so the oracle must + sweep lines whose glyph widths differ at the same column. +- **Q#HS5 — does `view_left` survive a restart?** Vote: yes, no + `DESKTOP_VERSION` bump — **conditional on two things, now concrete.** + Verified: `SavedLeaf` carries **no `#[serde(default)]` anywhere in + `src/desktop.rs`**, so serde would **reject** a version-1 desktop + JSON omitting a new `view_left`. Sound only with (1) + `#[serde(default)]` on the field and (2) a regression fixture — a + literal v1 desktop JSON without `view_left`, asserting restore at + offset 0 rather than an error. The reverse direction already works: + an old binary meets an unknown field, which serde ignores absent + `deny_unknown_fields` (none in that file). - **Q#HS3** is re-confirmed rather than open (per-window, per Q#LL2); **Q#HS4** is deferred, live only if explicit commands arrive. @@ -506,9 +533,13 @@ amending the description is now a Stage 4 deliverable (framing §6). 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**; that is - stated in the setting's description and in `ui.toggle-line-wrap`'s - status message, both of which should be revisited when scroll lands. + 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. diff --git a/docs/horizontal-scroll-framing.md b/docs/horizontal-scroll-framing.md index b10243e..97ea18d 100644 --- a/docs/horizontal-scroll-framing.md +++ b/docs/horizontal-scroll-framing.md @@ -1,13 +1,24 @@ # Horizontal scroll — QoL Stage 4 -**Status: revision 2 — NOT APPROVED. Q#HS1, HS2 and HS6 answered by the -user (2026-08-07); Q#HS3–HS5 stand; Q#HS7 is NEW and BLOCKING. No +**Status: revision 3 — NOT APPROVED. Q#HS1, HS2 and HS6 answered by the +user (2026-08-07). Q#HS7 remains BLOCKING and its part (c) is +**withdrawn and replaced** — a setter-time snap cannot exist for a +window-wide offset. Q#HS5's condition is now concrete. No implementation may begin.** -This closes the QoL arc opened by one daily-driver report. 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 other half of the user's own sentence: +**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 @@ -181,15 +192,36 @@ 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? **OPEN** +### Q#HS5 — does `view_left` survive a restart? **OPEN, with the condition now concrete** -`view_top` does (§1.4). Consistency argues yes; a defaulted field -avoids a `DESKTOP_VERSION` bump. +`view_top` does (§1.4). Consistency argues yes. -**My vote: yes, defaulted, no version bump** — but **confirm -`SavedLeaf`'s deserializer tolerates a missing field before relying on -it.** §1.4 cites the struct's *shape*, not serde's behavior on it, and -that gap is exactly the kind §1.3 exists to warn about. +**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** @@ -231,32 +263,83 @@ 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) Which columns may be a left edge.** 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. +**(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. -*My vote: a left edge may not fall inside a wide glyph's cells.* +**(c) ~~The snap rule when an invalid edge is requested.~~ WITHDRAWN +(revision 3).** -**(c) The snap rule when an invalid edge is requested.** Stage 3's -coordinate contract is *"identity on canonical inputs; otherwise -projection to the contract's **designated** canonical representative"* — -designated, not nearest, because the direction differs per function and -"nearest" hides that. +> 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. -*My vote: snap toward the line start.* Snapping left can only reveal a -character, never hide one that was visible; snapping right can hide the -very glyph the user scrolled to reach. And snapping at the moment -`view_left` is **set** — rather than clipping in the painter — keeps -one canonical value that both painter and mapper read, instead of two -that can disagree. +**(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. **(d) The invariant rendering and coordinate mapping share.** For a -given `view_left`, `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. +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. @@ -274,9 +357,16 @@ and it is why Q#HS7 blocks: §4 cannot be written until it exists. - 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(b)/(c) cases, which have no oracle until it is +- **The Q#HS7(b)/(c′) cases, which have no oracle until it is answered**: a wide glyph straddling the left edge; a tab whose - expansion straddles it; a snap request landing inside each. + expansion straddles it. +- **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 From 403fb7fb071385134fd7dc3100d0bfa5f2abbbc9 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 21:23:23 +0200 Subject: [PATCH 4/7] =?UTF-8?q?docs:=20Stage=204=20revision=204=20?= =?UTF-8?q?=E2=80=94=20the=20tab=20rule=20was=20already=20there?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small revisions, and every Stage 4 question is now answered. Approval is NOT recorded; no implementation may begin. Q#HS7(c″) — the tab-straddle mapping. Recorded as PRESERVED rather than chosen, because it already exists: `byte_at_place`'s doc comment says it rounds forward to the next character boundary, and the walk accumulates past the tab byte and returns on the NEXT character's start column, so every column inside a tab's expansion already yields the post-tab offset (src/text_view.rs:224, :243-254). The requirement on Stage 4 is therefore that horizontal scroll not PERTURB it — which makes its witness a regression test, and one that should fail if the walk is ever "optimized" to start at the effective edge instead of column 0. The obvious objection is that (c′) rounds backward and (c″) forward, so the framing answers it. 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 exactly when it is what 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″) the (d) contract is total over visible cells: ordinary character → its own start; bisected wide glyph → the glyph's start; tab expansion → the byte after the tab. Q#HS5 approved as stated, with both conditions written into the approval rather than attached as advice: `#[serde(default)]` and a literal v1 JSON fixture omitting the field, asserting restore at zero. The handoff said "Stage 4 is the remainder". It now says Stages 4 AND 5 remain and the arc closes at Stage 5, carries the Q#HS1 time box, and states explicitly that Rule 4 must not retire the long-lines lane at Stage 4's merge. It also records that the unreachable caveat is missing from the setting's description — the #221 gap — so that fact lives in the durable doc and not only in a lane block that will eventually be removed. Ledger: the question list is consolidated (the accepted answers had begun duplicating the blocking entries they resolved), keeping the withdrawn (c)'s reasoning because the trap generalizes to any future window-wide value derived from per-line content. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 60 ++++++++++------------- docs/agent-handoff.md | 30 ++++++++---- docs/horizontal-scroll-framing.md | 80 +++++++++++++++++++++++++++---- 3 files changed, 117 insertions(+), 53 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 5d8d1d9..fb82f39 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -457,8 +457,9 @@ tip — the ref, not a SHA, since any edit to this block advances past whatever SHA it records. Recover: `git fetch githubsucks && git checkout horizontal-scroll`. -**Status: framing, NOT approved.** `docs/horizontal-scroll-framing.md` -revision 3. No implementation may begin. +**Status: `docs/horizontal-scroll-framing.md` revision 4 — every +question answered, APPROVAL NOT YET RECORDED. No implementation may +begin until it is.** **Answered by the user 2026-08-07:** @@ -476,43 +477,32 @@ revision 3. No implementation may begin. the TUI and a **dead end in the GUI** for anyone who never opened the setting. Revisit after Stage 5, on use evidence. -**Still blocking:** +- **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. -- **Q#HS7 (NEW) — what IS `view_left`?** Revision 1 decided what moves - the viewport without saying what its offset *is* — the same omission - as shipping `WrapMode` with no `DisplayCoord`. Without part (d), **a - painter that clips where the mapper does not puts clicks on the wrong - character**, silently, only on lines wide enough to scroll. - - **Revision 2's part (c) is WITHDRAWN.** It voted to snap `view_left` - to a valid boundary when set. That cannot exist: `view_left` is one - per-window display column, but *"does column N bisect a wide - glyph?"* is a **per-line** question — column 11 can be a wide glyph's - trailing cell on one line and ordinary ASCII on the next. No - setter-time value is canonical for every visible line, and snapping - per line instead would break vertical alignment. - - Replaced by a **per-line effective edge**: `view_left` stored - unsnapped, each line deriving its own edge in the walk it already - performs from column 0. Where the edge bisects a wide glyph, that - glyph's trailing cell **paints blank and the mapping designates it to - the glyph's start byte** — which keeps `byte_at_place` total and - preserves the round trip. The invariant is therefore a property of - **`(view_left, line)`**, not of `view_left` alone, so the oracle must - sweep lines whose glyph widths differ at the same column. -- **Q#HS5 — does `view_left` survive a restart?** Vote: yes, no - `DESKTOP_VERSION` bump — **conditional on two things, now concrete.** - Verified: `SavedLeaf` carries **no `#[serde(default)]` anywhere in - `src/desktop.rs`**, so serde would **reject** a version-1 desktop - JSON omitting a new `view_left`. Sound only with (1) - `#[serde(default)]` on the field and (2) a regression fixture — a - literal v1 desktop JSON without `view_left`, asserting restore at - offset 0 rather than an error. The reverse direction already works: - an old binary meets an unknown field, which serde ignores absent - `deny_unknown_fields` (none in that file). - **Q#HS3** is re-confirmed rather than open (per-window, per Q#LL2); **Q#HS4** is deferred, live only if explicit commands arrive. +**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. + **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 diff --git a/docs/agent-handoff.md b/docs/agent-handoff.md index 60b530f..2029b9d 100644 --- a/docs/agent-handoff.md +++ b/docs/agent-handoff.md @@ -85,10 +85,10 @@ commands, read `docs/active-work.md` immediately after this file. ## 1. Where the project stands (2026-08-07) -- **QoL arc — Stages 1-3 merged (#219, #220, #221); Stage 4 is the - remainder.** From one daily-driver report: terminal zoom broke TUI - rendering and did nothing in the GUI, and a long line was unreadable - past the edge. +- **QoL arc — Stages 1-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 @@ -125,10 +125,24 @@ commands, read `docs/active-work.md` immediately after this file. drawable clip. Neither `view_range` (it carries `SCROLL_OVERSCAN` past the window) nor `scroll_top` (it ignores `code_scroll_residual`) can answer it. - - **Stage 4 is horizontal scroll**, and `truncate` is incomplete - without it: text past the right edge is currently *unreachable*, - which is why `wrap` is the default and why the toggle says so. - Framing in `docs/horizontal-scroll-framing.md`. + - **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 diff --git a/docs/horizontal-scroll-framing.md b/docs/horizontal-scroll-framing.md index 97ea18d..f0e1476 100644 --- a/docs/horizontal-scroll-framing.md +++ b/docs/horizontal-scroll-framing.md @@ -1,10 +1,20 @@ # Horizontal scroll — QoL Stage 4 -**Status: revision 3 — NOT APPROVED. Q#HS1, HS2 and HS6 answered by the -user (2026-08-07). Q#HS7 remains BLOCKING and its part (c) is -**withdrawn and replaced** — a setter-time snap cannot exist for a -window-wide offset. Q#HS5's condition is now concrete. No -implementation may begin.** +**Status: revision 4 — every question answered; APPROVAL NOT YET +RECORDED. No implementation may begin until it is.** + +| 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. @@ -192,7 +202,13 @@ 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? **OPEN, with the condition now concrete** +### 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. @@ -240,7 +256,13 @@ default does not move. Revisit after Stage 5, on use evidence, not before. -### Q#HS7 — what IS `view_left`? **NEW, BLOCKING** +### 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 @@ -327,6 +349,37 @@ pushed to the next row **entirely** (`advance_wrapped`, with its 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 @@ -357,9 +410,16 @@ and it is why Q#HS7 blocks: §4 cannot be written until it exists. - 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(b)/(c′) cases, which have no oracle until it is - answered**: a wide glyph straddling the left edge; a tab whose - expansion straddles it. +- **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 From bec8fc9aae0847824fe21a68229b7a7cfc6f427f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 21:49:00 +0200 Subject: [PATCH 5/7] feat(view): horizontal scroll, text and decorations together MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 4 of the QoL arc, framing revision 4 (approved). Under `truncate`, text past the right edge was UNREACHABLE; moving the cursor now brings it into view. Automatic only — no commands, no bindings, no new interaction island (Q#HS2). THE CONTRACT. `view_left` is an unsnapped per-window display column (Q#HS7(a)), and each line derives its own effective edge during the walk it already performs from column 0. Starting at 0 is not laziness: tab expansion depends on the absolute column from the line start, so a walk beginning at the edge would put tab stops in the wrong place. The walk stays line-absolute and only the emit translates. Where the edge bisects a wide glyph on a given line (Q#HS7(c′)), its trailing cell paints a styled BLANK rather than a `Continuation` — that glyph means "the cell before me is a wide glyph's head", and here that cell is off-screen, so emitting it would name a cell nobody painted. The mapping designates that cell to the glyph's START byte, which keeps `byte_at_place` total over visible cells and makes the character the user scrolled toward clickable. Tabs keep FORWARD rounding (Q#HS7(c″)) — preserved, not chosen. DECORATIONS TRAVEL WITH THE TEXT. The first version of this commit translated the base glyph walk and nothing else, which split the frame in half: at `view_left = 10` a glyph from source column 10 painted at screen column 0 while its syntax style, diagnostic underline, search wash and `BufferStyleOverlay` span painted at screen column 10 — or vanished. Decorations drifting off the characters they describe, silently, and only once a window had been scrolled. Every such site carried the same two lines (`start_col.min(max_cols)`, `end_col.min(max_cols)`), correct only while the left edge was pinned at zero. `Viewport::visible_cols` is now the one rule all FIVE adopters share — syntax/LSP styling, diagnostic underlines, search washes, `BufferStyleOverlay`, and the selection painter — so a future decorator inherits the translation instead of re-deriving it. It also subsumes the old `end_col <= start_col` guard rather than sitting beside it. `StyleSpanOverlay` and `VirtualCellOverlay` are deliberately untouched: they are documented as viewport-relative, so translating them would be the mirror defect. The selection painter was nearly a sixth site with its own copy of the rule, which I justified by a width it 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. It now takes that same viewport and drops its `rect`/`gutter_w` parameters entirely. A canonical rule with one honest exception is not canonical. The selection painter had the same defect with a worse failure mode: it asked `pos_to_display` through the LIVE context, which returns `None` for a position left of the edge, so a selection beginning off-screen and reaching into view took `continue` and painted NOTHING. That is the common shape, not an edge case — select rightward from column 0 past the window width and the view scrolls with the cursor. TWO THINGS THE TESTS FOUND, both in `pos_to_display`. My framing note said a caret sits between characters so never lands inside a glyph; true for the caret, false for the DESIGNATION direction — the glyph's start byte must map to its visible trailing cell, so `screen_col` needs the straddle rule and not a bare subtraction. And the `take == 0` early return short-circuited the translation entirely, so byte 0 looked visible at every offset. `view_left` is inert under `wrap` BY CONSTRUCTION — `LayoutCtx::effective_left` and `Viewport::left_edge` return 0 while wrapping — rather than by every caller remembering. Persisted per leaf at DESKTOP_VERSION 1 (Q#HS5) with both approval conditions: `#[serde(default)]` and a literal v1 JSON fixture omitting the field, hand-written because a generated one would gain the field and prove nothing. Also: `view_left: window.view_left` in the render viewport, not a literal 0. My mechanical fill put 0 there and it is EXACTLY the `aa3cd4d` defect — coordinates and the indicator following the scroll while the painter stays pinned at column 0. BITE, per clause. Forcing `bisected = false` fails the multi-line straddle witness; dropping the backward designation fails the round-trip witness; removing `#[serde(default)]` fails the v1 fixture; pinning `visible_cols` to an absolute clamp fails all three decorator witnesses; restoring the selection painter's live-context lookup fails the off-screen-start selection witness. Each alone. And with selection now reading the shared helper, pinning `visible_cols` to an absolute clamp fails the selection witnesses TOO — which is the check that the duplication is really gone rather than merely reworded. One unrelated red, logged as R7 in ci-red-signatures.md — the first this session with a COMPLETE signature, so a matchable row rather than a U note. `pmacs-gpu`'s managed-retry attach hit a BrokenPipe once under full-sweep load and did not reproduce (6 isolated runs plus a clean 113-target sweep). Per the rerun rule that is intermittence only, and the row explicitly does not claim harmlessness. Not attributed to this lane: Stage 4 touches no `pmacs-gpu` file and adds no wire surface. Gates: fmt; clippy --workspace --all-targets -D warnings, both configurations; `cargo test --workspace --no-fail-fast -- --skip basedpyright` 113 targets exit 0, and the same with --features crdt, 113 targets exit 0; git diff --check. No protocol change, so no version bump and no protocol-bump matrix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- builtin/runtime/linewrap.lua | 12 +- docs/active-work.md | 5 +- docs/ci-red-signatures.md | 22 ++ docs/horizontal-scroll-framing.md | 10 +- src/desktop.rs | 65 +++++ src/diag.rs | 19 +- src/editor.rs | 224 ++++++++++++-- src/highlight.rs | 28 +- src/overlay.rs | 9 +- src/search.rs | 14 +- src/text_view.rs | 157 +++++++++- src/view.rs | 87 ++++++ src/window.rs | 14 + tests/compile_mode_acceptance.rs | 1 + tests/horizontal_scroll_acceptance.rs | 389 +++++++++++++++++++++++++ tests/listview_acceptance.rs | 1 + tests/long_line_readable_acceptance.rs | 55 +++- tests/m4_acceptance.rs | 1 + tests/tab_width_acceptance.rs | 1 + tests/terminal_copy_mode_acceptance.rs | 1 + 20 files changed, 1040 insertions(+), 75 deletions(-) create mode 100644 tests/horizontal_scroll_acceptance.rs 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 fb82f39..4203086 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -457,9 +457,8 @@ tip — the ref, not a SHA, since any edit to this block advances past whatever SHA it records. Recover: `git fetch githubsucks && git checkout horizontal-scroll`. -**Status: `docs/horizontal-scroll-framing.md` revision 4 — every -question answered, APPROVAL NOT YET RECORDED. No implementation may -begin until it is.** +**Status: `docs/horizontal-scroll-framing.md` revision 4 — APPROVED +2026-08-07. Implementing.** **Answered by the user 2026-08-07:** 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 index f0e1476..35ed627 100644 --- a/docs/horizontal-scroll-framing.md +++ b/docs/horizontal-scroll-framing.md @@ -1,7 +1,13 @@ # Horizontal scroll — QoL Stage 4 -**Status: revision 4 — every question answered; APPROVAL NOT YET -RECORDED. No implementation may begin until it is.** +**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 | |---|---| 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..39f023e --- /dev/null +++ b/tests/horizontal_scroll_acceptance.rs @@ -0,0 +1,389 @@ +//! 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. These +// witnesses pin it from three directions, because the four call sites +// were 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, From b29e94d29bd00cdd504363a1dd8540a50fa28202 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 22:44:49 +0200 Subject: [PATCH 6/7] docs: the long-lines lane records PR #222 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 4 is open as #222; Stage 5 (GPU) still closes the arc, so the Rule 4 exemption above stands unchanged. Records the two things review added after the framing was approved: the `Viewport::visible_cols` single clip rule with its five adopters, and the corrected `ui.line-wrap` description. Both are lane facts rather than framing ones — the framing decided the coordinate contract, and these are what implementing it against a real frame turned up. Also notes R7, so a reader of this lane finds the unrelated red without having to reconstruct why a sweep in this window went 112/113 once. No SHA — `githubsucks/horizontal-scroll` stays the authoritative tip, per this ledger's own rule. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- docs/active-work.md | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/docs/active-work.md b/docs/active-work.md index 4203086..f95b5dd 100644 --- a/docs/active-work.md +++ b/docs/active-work.md @@ -435,7 +435,7 @@ Whether `docs/ci-red-signatures.md` should grow a short non-row section for this class is an open question for its owner, not something this lane decided. -## Long lines (QoL arc) — Stage 3 MERGED as #221; Stages 4 AND 5 ahead +## Long lines (QoL arc) — Stage 3 MERGED as #221; Stage 4 is PR #222 OPEN; Stage 5 ahead **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 @@ -458,7 +458,33 @@ whatever SHA it records. Recover: `git fetch githubsucks && git checkout horizontal-scroll`. **Status: `docs/horizontal-scroll-framing.md` revision 4 — APPROVED -2026-08-07. Implementing.** +2026-08-07. Stage 4 implemented; **PR #222 OPEN**, awaiting review. Do +not merge unprompted.** + +**What Stage 4 shipped**, beyond the `view_left` contract below: + +- **`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**. + + 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. **Answered by the user 2026-08-07:** From 3c9dc9296235baf3bbc8ea40d2b9e2e77025144b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 22:49:59 +0200 Subject: [PATCH 7/7] docs(test): five adopters, four of them decorator families MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite header still said "the four call sites", written before the selection painter joined `Viewport::visible_cols`. Four is now the count of decorator FAMILIES — syntax/LSP styling, diagnostic underlines, search washes, `BufferStyleOverlay` — and five is the count of adopters, selection being the fifth. Also points at where selection's own witnesses live, since a reader of this file would otherwise look for them here and find nothing: `paint_local_selection` is private, so they are in `src/editor.rs`. Comment only; no behavior and no assertion changed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- tests/horizontal_scroll_acceptance.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/horizontal_scroll_acceptance.rs b/tests/horizontal_scroll_acceptance.rs index 39f023e..93261b5 100644 --- a/tests/horizontal_scroll_acceptance.rs +++ b/tests/horizontal_scroll_acceptance.rs @@ -246,10 +246,15 @@ fn round_trip_is_identity_at_a_non_zero_offset() { // 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. These -// witnesses pin it from three directions, because the four call sites -// were identical and a single test would have let a missed adopter -// through. +// `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};