From bec8fc9aae0847824fe21a68229b7a7cfc6f427f Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 21:49:00 +0200 Subject: [PATCH] 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,