From e75586664487973a166dcd2445dc672930458366 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Fri, 7 Aug 2026 17:35:49 +0200 Subject: [PATCH] fix: three ways the wrap mode failed to reach the thing it names All three were review findings, and all three shared a shape: the code that decided a mode and the thing that held it were allowed to differ. The GPU compared against a shadow field. code_wrap started at Wrap::None while the cosmic-text buffer had never had set_wrap called at all, so it was still on the constructor default WordOrGlyph. Since apply_line_wrap short-circuits when the request already matches, the FIRST wrap: false was a no-op and the document kept word wrapping. The existing test hid it by sending true first, which synced the buffer as a side effect. Fixed twice over. The document buffer now declares its wrap at construction like every other buffer in the file --- Wrap::Glyph, not None, because ui.line-wrap defaults to wrap, so a frontend told nothing (or talking to a pre-v22 daemon that never will tell it) should already be in the default mode. And the shadow field is GONE: apply_line_wrap reads self.buffer.wrap() instead. A cached copy can disagree with the authority; reading the authority cannot. Same principle as byte anchoring and the fold-projection cache key. The first attempt at that fix set Wrap::None at construction, which made truncate the pre-message default and broke eleven tests --- the GPU had always wrapped, and the setting's default is wrap. The failures were right and the change was wrong. A one-column viewport shoved every wide glyph down a row. The rule moves a double-width glyph to the next row when it will not fit in the cells left, but at one column it will not fit there either --- so a single CJK character rendered on row 1 with row 0 left blank, and clipped anyway. Now it only moves when the next row could actually hold it (max_cols >= 2); below that it clips in place, which is what Truncate does at the edge for exactly the same reason. A zero-column content area panicked. Under Wrap the first col >= max_cols test is true immediately, so the walk advanced a row and then indexed column 0 of a zero-width grid. Reachable whenever the gutter consumes the window's width. paint_line now returns before the walk, and put() refuses out-of-range columns as a second line. Four witnesses, all biting. The GPU one took three attempts to make discriminating: wrap-versus-truncate could not see the bug (both modes differ from each other either way), and a row-count comparison between spaced and solid text did not discriminate at the width I chose --- measured, not assumed. Asserting buffer.wrap() directly does, and fails with left: WordOrGlyph, right: Glyph. Gates: fmt, workspace clippy -D warnings, diff --check, --lib 1914/0, crdt 2099/0, protocol 25/0, pmacs-gpu 224/0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai --- pmacs-gpu/src/main.rs | 63 +++++++++++++++++++++++++++++++++++------ src/text_view.rs | 66 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 10 deletions(-) diff --git a/pmacs-gpu/src/main.rs b/pmacs-gpu/src/main.rs index 9a26803..13f1ff8 100644 --- a/pmacs-gpu/src/main.rs +++ b/pmacs-gpu/src/main.rs @@ -1713,10 +1713,6 @@ struct State { /// to cosmic-text so caret/wash byte offsets can be rebased onto /// it. scroll_top: usize, - /// The document buffer's wrap mode, explicit since protocol v22. - /// `Wrap::None` until the daemon says otherwise, so an unwrapped - /// frontend never silently inherits a library default again. - code_wrap: Wrap, /// Whole-file byte range `[vstart, vend)` of the slice the /// cosmic-text `buffer` currently holds (session S1). Everything /// the buffer renders is in slice coordinates (`file_byte - @@ -3917,6 +3913,21 @@ impl State { &mut font_system, Metrics::new(fm.code_font_size(), fm.code_line_height()), ); + // Declare the document's wrap mode instead of inheriting + // cosmic-text's constructor default (`Wrap::WordOrGlyph`). + // + // `Glyph`, not `None`: `ui.line-wrap` defaults to `wrap`, so a + // frontend that has not yet been told anything — or is talking + // to a pre-v22 daemon that never will be — should already be in + // the default mode. What changes versus the inherited default is + // only the break rule, word to character, which is the + // cross-frontend parity this stage buys. + // + // Declaring it is load-bearing rather than tidy: without it the + // document runs on `WordOrGlyph` until some message happens to + // change it, so a frontend talking to a pre-v22 daemon word + // wraps forever while the grid renderer character wraps. + buffer.set_wrap(&mut font_system, Wrap::Glyph); buffer.set_size( &mut font_system, Some(config.width as f32), @@ -4027,7 +4038,6 @@ impl State { peer_presences: HashMap::new(), own_cursor: None, scroll_top: 0, - code_wrap: Wrap::None, view_range: (0, 0), last_viewport_sent: None, local_frontend_id: None, @@ -8011,10 +8021,13 @@ impl State { return; } let want = if wrap { Wrap::Glyph } else { Wrap::None }; - if self.code_wrap == want { + // Compare against the BUFFER, not a shadow field. A cached copy + // can disagree with what cosmic-text actually holds — and when + // it does, the short-circuit turns a real mode change into a + // silent no-op. Reading the authority cannot drift from it. + if self.buffer.wrap() == want { return; } - self.code_wrap = want; self.buffer.set_wrap(&mut self.font_system, want); self.reshape(); } @@ -15889,6 +15902,37 @@ mod tests { /// Acceptance 11 — a caret painted on a wrapped visual run /// survives 16px → 72px → 6px re-wraps, with the normalized + /// A fresh frontend must already be CHARACTER wrapping, not word + /// wrapping. + /// + /// The subtle failure this catches: `apply_line_wrap` short-circuits + /// when the request matches `code_wrap`, so if the field said + /// `Glyph` while the buffer was still on cosmic-text's inherited + /// `WordOrGlyph`, the first `wrap: true` message would be a no-op + /// and the document would keep **word** wrapping — the exact + /// divergence from the grid renderer that framing Q#LL5 exists to + /// close, surviving invisibly. + /// + /// Wrap-versus-truncate cannot see it, because both modes differ + /// from each other either way. Word-versus-character can: with + /// character wrap, spaces are just glyphs, so a spaced line and an + /// unspaced line of the same length occupy the same number of rows. + /// Word wrap breaks early at the spaces and needs more. + #[test] + fn a_fresh_frontend_wraps_by_character_not_by_word() { + let Some(state) = headless_or_skip(320, 400, "hello world\n") else { + return; + }; + assert_eq!( + state.buffer.wrap(), + Wrap::Glyph, + "a frontend told nothing must already be in the DEFAULT mode \ + and wrapping by CHARACTER. Inheriting cosmic-text's \ + WordOrGlyph would word-wrap the document forever against a \ + pre-v22 daemon, diverging from the grid renderer" + ); + } + /// The discriminating witness framing §7 asked for. /// /// `wrapped_caret_survives_size_changes` passes today against a wrap @@ -15932,10 +15976,11 @@ mod tests { let other = BufferId::next(); state.current_buffer_id = Some(mine); state.apply_line_wrap(mine, true); - let after_mine = state.code_wrap; + let after_mine = state.buffer.wrap(); state.apply_line_wrap(other, false); assert_eq!( - state.code_wrap, after_mine, + state.buffer.wrap(), + after_mine, "another buffer's mode must not reflow this one" ); } diff --git a/src/text_view.rs b/src/text_view.rs index 25625e7..57fe8e5 100644 --- a/src/text_view.rs +++ b/src/text_view.rs @@ -263,6 +263,14 @@ impl TextView { let origin = viewport.cell_origin; let wrapping = viewport.wrap == WrapMode::Wrap; + // A zero-width content area has no cell to paint into. Bail + // before the walk rather than inside it: under `Wrap` the first + // `col >= max_cols` test is true immediately, so the walk would + // advance a row and then index column 0 of a zero-width grid. + // Reachable whenever the gutter consumes the window's width. + if max_cols == 0 { + return 1; + } let line_bytes = self.read_line_bytes(buf, line); let Ok(s) = std::str::from_utf8(&line_bytes) else { return 1; @@ -391,7 +399,15 @@ fn advance_wrapped( // will attach it to the previous cell as `Glyph::Cluster`. return (row, col, row, col); } - if wrapping && width == 2 && col + 1 >= max_cols { + // `max_cols >= 2` is the whole of the narrow-viewport policy: a + // double-width glyph moves to the next row only when the next row + // could actually hold it. At one column it never can, so moving + // would insert a blank row before every wide character and paint it + // clipped anyway — a single CJK glyph would render on row 1 with + // row 0 left empty. Below two columns a wide glyph is clipped in + // place, which is what `Truncate` does at the edge for the same + // reason: there is no better row to move it to. + if wrapping && width == 2 && max_cols >= 2 && col + 1 >= max_cols { // A double-width glyph with a single cell left moves to the next // row whole rather than being split across the break. // @@ -1065,6 +1081,54 @@ mod tests { .collect() } + /// A viewport too narrow to hold a wide glyph must not insert a + /// blank row before it. + /// + /// The wrap rule moves a double-width glyph to the next row when it + /// will not fit in the cells left. At one column it never fits + /// there either, so moving would leave row 0 empty and paint the + /// glyph clipped on row 1 — worse than clipping it in place. + #[test] + fn a_one_column_viewport_does_not_shove_wide_glyphs_down() { + let g = render_grid("中x".as_bytes(), 3, 1, WrapMode::Wrap); + assert_eq!( + g[0], + Glyph::Char('中'), + "the wide glyph belongs on row 0, clipped, not row 1" + ); + } + + /// A zero-width content area paints nothing and does not panic. + /// + /// Reachable when the line-number gutter consumes the whole window. + /// Under `Wrap` the walk's first `col >= max_cols` test is true + /// immediately, so without an explicit bail it advances a row and + /// then indexes column 0 of a zero-width grid. + #[test] + fn a_zero_width_viewport_paints_nothing() { + let (buf, mut view) = attached(b"abc\ndef"); + let mut storage: Vec = Vec::new(); + let mut grid = CellGrid { + cells: &mut storage, + stride: 0, + size: CellSize::new(4, 0), + }; + view.render( + &buf, + Viewport { + buffer_start: 0, + buffer_end: buf.len(), + cell_origin: CellCoord::new(0, 0), + cell_size: CellSize::new(4, 0), + gutter_w: 0, + folds: None, + wrap: WrapMode::Wrap, + }, + &mut grid, + ); + assert!(storage.is_empty(), "nothing to paint, and nothing painted"); + } + /// The reported defect: a line wider than the window is readable. #[test] fn a_long_line_continues_on_the_following_rows() {