fix(grid): keep the wide-glyph wrap filler out of reflowed text - #92
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93993fef5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let blank = self.blank(); | ||
| let x = self.cursor.x as usize; | ||
| if let Some(row) = self.grid.get_mut(self.cursor.y as usize) { | ||
| Self::mark_wide_pad(row, x, &blank); |
There was a problem hiding this comment.
When a client-mode source contains a wide glyph wrapped from the last column (for example, abc界 at width 4), this call adds WIDE_PAD, but terminal/seed.rs can only replay the cell as an ordinary space. The replica therefore differs immediately, and widening both terminals later makes the replica retain that space and shift the following glyph while the source drops it. The seed path needs to recreate or explicitly encode this layout marker.
Useful? React with 👍 / 👎.
| /// Filler at the end of a soft-wrapped row where a wide glyph did not | ||
| /// fit and moved to the next row. Not content: reflow, copy and search | ||
| /// skip it. | ||
| const WIDE_PAD = 1 << 15; |
There was a problem hiding this comment.
With scrollback persistence enabled, introducing this new non-default cell state also requires updating snapshot::rewrap in crates/noa-grid/src/snapshot.rs: it currently copies an existing WIDE_PAD into the logical cell stream, and emit_logical_line does not mark fillers when it moves a wide pair to the next row. Restoring at a different width can consequently place later content one column too far right, and a subsequent resize can treat newly created unmarked fillers as real spaces.
Useful? React with 👍 / 👎.
| if row.wrapped | ||
| && len == row.cells.len() | ||
| && len > 0 | ||
| && row.cells[len - 1].attrs.contains(CellAttrs::WIDE_PAD) |
There was a problem hiding this comment.
When DECSLRM's right margin is before the physical row end, a wide glyph can create WIDE_PAD at that margin rather than at row.cells[len - 1]. After margins are disabled and the terminal is resized, this condition fails, so the filler remains in the reflow stream and inserts a visible blank column even though copy and search skip the same cell. Reflow should remove the marked cell wherever the soft-wrap boundary occurred, not only at the physical row end.
Useful? React with 👍 / 👎.
When a wide glyph does not fit in the last column, it moves to the next row
and leaves one blank cell behind. That cell looked exactly like a real space,
so a column-count resize carried it into the reflowed text and left stray
blanks mid-word at the old wrap points ("あた り", "1,391,8 31件"). The cell
was also left untouched, so stale content could survive under it.
Blank that cell when printing or reflow wraps a wide glyph. In the last
column, also flag it `WIDE_PAD`. `Row::ends_with_wide_pad` defines where the
flag counts: only in the last column of a soft-wrapped row. A flagged cell
anywhere else is an ordinary blank, so a stale flag can never hide visible
text. Reflow drops the filler from the end of a soft-wrapped row, and copy and
search skip it.
A wrap at a narrower right margin just blanks the cell. Flagging it there
would leave fillers mid-row once the margins go away, and a row could carry
more than one of them. DCH and rectangle scrolls clear the flag on the cells
they move.
Restoring saved scrollback at a new width rewraps it with `snapshot::rewrap`, which kept the filler as a regular cell. As a result, "abc界Z" saved at 4 columns placed 界 at column 4 instead of 3 when restored at 8 columns. Drop the filler from the end of a soft-wrapped row, as live reflow does. When rewrap moves a wide glyph down, flag the column it vacates, so a later live reflow treats that column as filler too.
The seed rebuilds a replica's screen with plain VT output, which has no way to express the filler. Before this change, the replica received it as an ordinary space and copied "abc 界Z" where the server copied "abc界Z". Its own reflow then disagreed with the server as well. - For a row that ends in a filler, print U+3000 in the last column with the filler's own background and grapheme clustering off. The replica then lays down the filler and soft-wraps by itself, however the next row now looks, and the next row is repainted from the source. - Paint filler cells with ECH rather than a printed space. This keeps a filler's background on the last row, where no wrap can be rebuilt, and never joins a preceding cluster ending in a ZWJ. - Recreating a cursor's deferred-wrap latch reprints the cell under it, which turns a filler there into a plain space. That cursor must stay latched, so the filler cannot be rebuilt afterwards. The new seed-only `CSI > $ w` flags the cell under the cursor without moving it or clearing the latch. Both the absolute and origin-relative cursor writers emit it after such a reprint.
93993fe to
ef2b848
Compare
1 participant
When a wide glyph does not fit in the last column, it moves to the next row and leaves one blank cell behind. That cell looked exactly like a real space, so a column-count resize carried it into the reflowed text and left stray blanks mid-word at the old wrap points ("あた り", "1,391,8 31件"). The cell was also left untouched, so stale content could survive under it.
Three commits, each green on its own:
WIDE_PAD.Row::ends_with_wide_paddefines where the flag counts: only in the last column of a soft-wrapped row. A flagged cell anywhere else is an ordinary blank, so a stale flag cannot hide visible text. Reflow drops the filler, and copy and search skip it. A wrap at a narrower right margin only blanks the cell. DCH and rectangle scrolls clear the flag on the cells they move.snapshot::rewrapdrops the filler the same way, and flags the column it vacates when it moves a wide glyph down.CSI > $ wre-flags a filler that a cursor-latch reprint turned into a plain space.Reviewer notes:
CSI > $ waddsHandler::seed_mark_wide_pad, with a no-op default. It follows the existing seed-onlyCSI > $ s/CSI > $ t, and the>marker keeps it clear of DECRQPSR (CSI Ps $ w).rewrap_never_separates_a_wide_glyph_from_its_spacerchanged. The vacated column is now a flagged filler instead of a default cell.Validation:
cargo test --workspace --exclude noa-pty --exclude noa-ipcandcargo fmt --all -- --checkpass on each commit.cargo clippy -p noa-grid -p noa-vt -p noa-core --all-targetsis clean on the final commit.noa-ptyandnoa-ipcneed unsandboxed device and socket access and were not run here.Not addressed:
0in "92,70件"). That could not be reproduced through grid, split-feed, or render paths, and this change is not known to fix it.