Skip to content

fix(rio-grid): prevent RTL glyphs from collapsing into one cell - #1929

Open
SagiMedina wants to merge 1 commit into
raphamorim:mainfrom
SagiMedina:fix/glyph-cluster-cell-mapping
Open

SagiMedina wants to merge 1 commit into
raphamorim:mainfrom
SagiMedina:fix/glyph-cluster-cell-mapping

Conversation

@SagiMedina

Copy link
Copy Markdown

RTL glyphs can collapse onto one terminal cell because the grid emitter only advances its cluster-to-cell cursor. For example, source clusters [3, 2, 1, 0] currently produce cell indices [3, 3, 3, 3]. This change maps them to [3, 2, 1, 0], retaining each glyph's logical terminal cell and its foreground color.

The mapping keeps the existing forward walk for ascending clusters and uses a binary search when a glyph belongs to an earlier cell. It supports direction changes within a run without classifying the whole run by its first and last clusters. Both platform offset conventions (UTF-16 on macOS, UTF-8 elsewhere) and the existing wide-cell column mapping are preserved.

This fixes cell attribution for RTL scripts, including Hebrew, Arabic, and Persian. It preserves Rio's logical cell order; it does not implement Unicode BiDi visual reordering, proportional RTL layout, or new combining-mark positioning. Those need coordinated cursor, selection, and mouse mappings.

Related: #1877, which identified the same forward-only mapping bug. This is a narrower alternative that retains terminal cell positions. The broader BiDi feature request #1257 remains open.

Validation on macOS arm64 with Rust 1.96.1:

  • Five regression tests failed against the original mapping, including real CoreText shaping and the production row emitter, and passed after the fix.
  • All 24 rio-grid tests pass. Coverage includes RTL and mixed-direction clusters, UTF-8/UTF-16 offsets, marks and ligatures, actual Hebrew/Arabic/Persian shaping, and row emission into a CPU atlas with per-cell colors and cursor/selection run breaks.
  • cargo test --features wgpu --locked passes across the full workspace (1374 tests passed, 11 ignored, including doc tests).
  • cargo fmt --all -- --check and cargo clippy --all-targets --all-features --locked pass.

Interactive GPU rendering and Android/tmux handoff have not been manually tested with this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant