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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016bqGA6s9tTUFzYpbeW3tai
This commit is contained in:
parent
e5076ff277
commit
e755866644
|
|
@ -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"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<Cell> = 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() {
|
||||
|
|
|
|||
Loading…
Reference in New Issue