fix: step copy mode horizontal motion over full-width glyphs - #1469
fix: step copy mode horizontal motion over full-width glyphs#1469kovalov wants to merge 2 commits into
Conversation
Greptile SummaryThis PR fixes copy-mode horizontal cursor movement over full-width (CJK) characters by stepping by whole glyphs instead of raw columns, so a single
Confidence Score: 4/5Safe to merge for the common CJK case it targets; cursor alignment will be wrong for characters where UnicodeWidthChar and libghostty's width tables disagree. The fix correctly handles the reported CJK case and is well-tested, but row_cell_glyphs uses a different Unicode width function (UnicodeWidthChar::width) than the rest of the file and libghostty's FFI (crate::ghostty::unicode_codepoint_width). For ambiguous-width characters or emoji where the two tables give different results, the glyph column map will be off and the cursor will land on the wrong cell. src/app/input/copy_mode.rs — specifically the row_cell_glyphs width calculation on line 972 Important Files Changed
|
9795d01 to
2e4bfbf
Compare
|
@ogulcancelik Pls review. I have resolved the conflict. |
📝 WalkthroughWalkthroughCopy-mode horizontal cursor movement now advances across terminal glyph boundaries using visible-row layouts. Wide characters, blank cells, right-edge clamping, and combining marks are handled by new helpers and covered by unit and integration tests. ChangesCopy-mode glyph navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ogulcancelik
added
the
greptile-review
|
@kovalov could you update the pr please? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let mut glyphs = Vec::new(); | ||
| let mut col = 0u16; | ||
| for ch in text.into_iter().flat_map(str::chars) { | ||
| let width = UnicodeWidthChar::width(ch).unwrap_or(1) as u16; |
There was a problem hiding this comment.
row_cell_glyphs uses UnicodeWidthChar::width(ch).unwrap_or(1) to measure character widths, but every other width calculation in this file (and in actions.rs) uses crate::ghostty::unicode_codepoint_width(ch as u32) — the FFI call that reflects how libghostty actually lays out cells. For any character where the two functions disagree (ambiguous-width CJK symbols, some emoji, variation selectors, or chars that unicode-width returns None for), row_cell_glyphs will produce a misaligned column map, so next_cell_col/prev_cell_col will place the cursor on the wrong cell. last_character_col — which this code is explicitly designed to stay consistent with per the doc comment — uses crate::ghostty::unicode_codepoint_width for exactly this reason.
| let width = UnicodeWidthChar::width(ch).unwrap_or(1) as u16; | |
| let width = u16::from(crate::ghostty::unicode_codepoint_width(ch as u32)); | |
| if width == 0 { | |
| continue; | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/input/copy_mode.rs`:
- Around line 963-1004: Update row_cell_glyphs to compute each visible glyph’s
width with Ghostty’s unicode_grapheme_width() helper instead of
UnicodeWidthChar::width per codepoint. Preserve grapheme-cluster handling,
column accumulation, zero-width skipping, and the existing fallback behavior for
unavailable row text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5eb50cc1-6fe7-4cd1-9b94-f29074ff047d
📒 Files selected for processing (1)
src/app/input/copy_mode.rs
| /// (leading_col, cell_width) for each glyph in a visible row, in column order. | ||
| /// Zero-width combining marks fold into the preceding glyph (they occupy no | ||
| /// cells), keeping columns aligned with the terminal's cell layout, matching | ||
| /// `last_character_col`. Empty when the row text is unavailable, which falls | ||
| /// back to single-cell steps. | ||
| fn row_cell_glyphs(text: Option<&str>) -> Vec<(u16, u16)> { | ||
| let mut glyphs = Vec::new(); | ||
| let mut col = 0u16; | ||
| for ch in text.into_iter().flat_map(str::chars) { | ||
| let width = UnicodeWidthChar::width(ch).unwrap_or(1) as u16; | ||
| if width == 0 { | ||
| continue; | ||
| } | ||
| glyphs.push((col, width)); | ||
| col = col.saturating_add(width); | ||
| } | ||
| glyphs | ||
| } | ||
|
|
||
| fn next_cell_col(glyphs: &[(u16, u16)], cursor_col: u16, max_col: u16) -> u16 { | ||
| let step = glyphs | ||
| .iter() | ||
| .find(|(start, _)| *start == cursor_col) | ||
| .map_or(1, |(_, width)| *width); | ||
| // Advancing overshoots the last column only when the current glyph ends at | ||
| // the right margin (e.g. a full-width glyph in the final two columns); stay | ||
| // on the leading cell instead of landing on its trailing spacer. | ||
| let target = cursor_col.saturating_add(step); | ||
| if target > max_col { | ||
| cursor_col | ||
| } else { | ||
| target | ||
| } | ||
| } | ||
|
|
||
| fn prev_cell_col(glyphs: &[(u16, u16)], cursor_col: u16) -> u16 { | ||
| glyphs | ||
| .iter() | ||
| .find(|(start, width)| start.saturating_add(*width) == cursor_col) | ||
| .map_or(cursor_col.saturating_sub(1), |(start, _)| *start) | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the terminal engine backing extract_selection/cursor_state to check its width algorithm.
rg -n --type=rust -C3 '\bfn extract_selection\b|\bis_wide\b|UnicodeWidthChar|UnicodeWidthStr' src/pane src/terminal 2>/dev/null
rg -n 'unicode-width|vt100' Cargo.toml Cargo.lock 2>/dev/nullRepository: ogulcancelik/herdr
Length of output: 1873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/pane/terminal.rs excerpt ---'
sed -n '1,120p' src/pane/terminal.rs
echo
sed -n '1700,1785p' src/pane/terminal.rs
echo
echo '--- terminal engine references ---'
rg -n --hidden --glob '!target' --glob '!node_modules' '\bghostty\b|UnicodeWidthStr|UnicodeWidthChar|width\s*\(' src Cargo.toml Cargo.lockRepository: ogulcancelik/herdr
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Cargo.lock entries for ghostty ---'
rg -n -A6 -B2 '^name = "ghostty"|^name = "unicode-width"|^name = "unicode-segmentation"' Cargo.lock
echo
echo '--- src tree for ghostty references ---'
rg -n --hidden --glob '!target' '\bghostty\b' src Cargo.toml
echo
echo '--- width helpers in src/pane/terminal.rs ---'
rg -n -A3 -B3 'UnicodeWidthStr|UnicodeWidthChar|width\s*\(' src/pane/terminal.rsRepository: ogulcancelik/herdr
Length of output: 43037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
lock = Path("Cargo.lock").read_text()
for pkg in re.finditer(r'(?ms)^name = "([^"]+)"\nversion = "([^"]+)"(?:\n.*?)*?(?=^name = |\Z)', lock):
name, version = pkg.group(1), pkg.group(2)
if any(k in name.lower() for k in ("ghost", "term", "unicode-width")):
print(f"{name} {version}")
PYRepository: ogulcancelik/herdr
Length of output: 474
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '930,1015p' src/app/input/copy_mode.rsRepository: ogulcancelik/herdr
Length of output: 2736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '660,710p' src/ghostty/mod.rs
rg -n -A3 -B3 'unicode_codepoint_width|unicode_grapheme_width|width of a Unicode codepoint' src/ghostty/mod.rs src/ghostty/bindings.rsRepository: ogulcancelik/herdr
Length of output: 8929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'row_cell_glyphs\(|next_cell_col\(|prev_cell_col\(' src/app/input/copy_mode.rsRepository: ogulcancelik/herdr
Length of output: 3506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!target' '2027|grapheme|unicode_grapheme_width|mode_set\(crate::ghostty::MODE_' src/ghostty src/pane src/appRepository: ogulcancelik/herdr
Length of output: 14286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '835,870p' src/pane/terminal.rs
echo
sed -n '2180,2255p' src/pane/terminal.rs
echo
sed -n '2290,2460p' src/pane/terminal.rsRepository: ogulcancelik/herdr
Length of output: 8642
Use Ghostty’s grapheme width helper here. row_cell_glyphs re-derives widths with UnicodeWidthChar::width, but Ghostty’s layout can be grapheme-cluster aware (MODE_GRAPHEME_CLUSTER/2027) and already exposes unicode_grapheme_width() for the exact terminal rules. Per-codepoint widths will still misstep on ZWJ/VS/combining clusters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/input/copy_mode.rs` around lines 963 - 1004, Update row_cell_glyphs
to compute each visible glyph’s width with Ghostty’s unicode_grapheme_width()
helper instead of UnicodeWidthChar::width per codepoint. Preserve
grapheme-cluster handling, column accumulation, zero-width skipping, and the
existing fallback behavior for unavailable row text.
2 participants
Fixes copy-mode horizontal movement stopping mid-glyph on full-width (CJK) characters.
move_copy_cursorstepped horizontal motion by raw columns, soh/llanded on the trailing cell of a wide glyph and needed two presses to cross a single character. This steps horizontal motion by whole glyphs: it reads the visible row text and advances/retreats to the next character boundary viarow_cell_glyphs/next_cell_col/prev_cell_col, falling back to single-cell steps when the row text is unavailable.Reproduction from the issue (
printf 'AXあYb\n'): cursor on the leading cell ofあ(col 2), onelnow moves toY(col 4) instead of the trailing cell, and onehreturns to col 2.Tests:
cell_col_steps_handle_wide_glyphs_and_blanks(unit)copy_mode_horizontal_crosses_full_width_glyph(integration)refs #1000
Summary by CodeRabbit