From b110684a62dab51144de11398f2eb9340284a823 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 20:46:39 +0200 Subject: [PATCH] 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.