fix: render Thai/Lao complex-script combining marks (incl. SARA AM) correctly#14124
Open
ultramcu wants to merge 5 commits into
Open
fix: render Thai/Lao complex-script combining marks (incl. SARA AM) correctly#14124ultramcu wants to merge 5 commits into
ultramcu wants to merge 5 commits into
Conversation
…warpdotdev#1) The terminal grid was rendering Thai vowel marks (ั, ี, …) at the wrong position or dropping them entirely. The bug was a chain of issues across font fallback, byte/char indexing, and HarfBuzz glyph positioning. Five root causes, fixed end-to-end: 1. DirectWrite fallback locale on Windows was hardcoded to "en-US", which biased `IDWriteFontFallback::MapCharacters` toward Latin-script fonts even when asked for a Thai codepoint. Switched to "" (system locale). 2. Linux/FreeBSD font fallback used a hardcoded "en" instead of reading `LC_ALL` / `LC_CTYPE` / `LANG`. Now picks up the user's actual locale. 3. `CellGlyphCache::glyph_for_string` returned `None` whenever the layout produced more than one glyph, silently dropping every Thai cluster (base consonant + combining vowel = 2 glyphs). Renamed to `glyphs_for_string` returning a `Vec`, so all glyphs make it through. 4. `AttributedStringBuilder::character_index_to_cell_map` was filled one entry per char, but `paint_line` indexes it with `glyph.index` — which cosmic_text emits as a UTF-8 *byte* offset, not a char index. For ASCII the two coincide; for any 3-byte codepoint (Thai, CJK, emoji) the lookup landed on the wrong cell. Now fill one entry per byte. 5. (The actual visible bug.) `glyphs_for_string` collected only `(GlyphId, FontId)` and discarded `Glyph::position_along_baseline`. Every glyph then got drawn at the same cell origin, so combining marks stacked on top of the base instead of sitting where HarfBuzz placed them — visually they bled into the previous cell or disappeared. Now carry the position through and apply it in `render_cell_glyph`. The ligature path (`render_grid_with_ligatures`) was also fixed for the same byte-indexing problem and gained a deferred-rendering pass for `CharOrStr::Str` cells, which use direct glyph_for_char + DirectWrite fallback (the cached `text_layout_cache` path can't load system Thai fonts on demand). Tests: - New `mixed_ascii_thai_records_missing_glyphs` exercises the text-layout missing-glyph path with mixed Latin + Thai. - New `test_attributed_string_builder_byte_indexed_cell_map` locks in the byte-indexed (not char-indexed) cell map for multi-byte codepoints, with an ASCII regression check alongside it. - Manually verified end-to-end on Windows with `python -c \"print('สวัสดี')\"` — output now matches PowerShell. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
PR warpdotdev#10357 fixes dropped/mispositioned Thai combining marks, but SARA AM (U+0E33 / Lao U+0EB3) is a width-1 spacing vowel stored in its own grid cell, so it still rendered detached from the preceding consonant: the nikhahit dot landed beside or inside the consonant instead of above it. When a consonant is followed by SARA AM, re-shape the two together as a single cluster via glyphs_for_string so HarfBuzz's Thai shaper positions the nikhahit dot over the consonant (GPOS) and places the spacing aa tail after it. The consonant cell draws the full shaped cluster; the SARA AM cell then draws nothing (unless it has no preceding cell, e.g. at line start). This reuses the same GPOS-positioned glyph path warpdotdev#10357 added for in-cell combining marks. Storage keeps the original codepoint, so copy/search/find stay byte-faithful. Adds test_decompose_sara_am.
Route render_grid_with_ligatures' deferred Str cells through CellGlyphCache::glyphs_for_string so emoji VS/ZWJ shape as one cluster, combining marks use GPOS positions, and styled fonts are preserved.
In the ligature rendering path, SARA AM (Thai ำ / Lao ໍ) was handled by decomposing it into a standalone nikhahit pushed to col-1 and a sara_aa pushed to col. The standalone nikhahit had no HarfBuzz shaping context so its position above the base consonant was approximate at best. Switch to the same look-ahead strategy used by render_cell_glyph in the non-ligature path: when a cell is followed by SARA AM, defer both the consonant content and the SARA AM char together as one string. HarfBuzz then shapes the cluster end-to-end and GPOS places the nikhahit dot precisely above the consonant. The SARA AM cell itself is skipped (tracked via skip_sara_am_at) and only falls back to standalone sara_aa rendering when it appears at the start of a line with no preceding cell. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
added
the
external-contributor
Contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Overview
This PR updates terminal grid rendering and font fallback so Thai/Lao combining marks, including SARA AM, are shaped through HarfBuzz and adds Unicode rendering tests.
Concerns
- For this user-facing terminal rendering change, please include screenshots or a screen recording demonstrating the Thai/Lao rendering working end to end. The PR description's Screenshots / Videos section is empty.
- The new positioned-glyph cache stores layout positions without including font size in the cache key, which can reuse stale mark offsets after font-size changes.
- The SARA AM look-ahead currently consumes SARA AM after any preceding cell rather than only an attachable Thai/Lao base cluster, so standalone or invalid sequences after spaces/punctuation/Latin text can be suppressed or shaped with the wrong base.
Verdict
Found: 0 critical, 3 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| } | ||
| // Consonant followed by SARA AM: defer both together so HarfBuzz shapes | ||
| // the cluster and positions the nikhahit dot above the consonant via GPOS. | ||
| CharOrStr::Char(c) if following_sara_am.is_some() => { |
Contributor
There was a problem hiding this comment.
ำ get shaped with the previous cell and the SARA AM cell is skipped; guard this and the non-ligature path to only consume valid base clusters.
| let cached = self | ||
| .string_cache | ||
| .entry((string.to_owned(), font_id)) | ||
| .or_insert_with(|| { |
Contributor
There was a problem hiding this comment.
glyphs_for_string now caches baseline-relative positions, but the key omits font_size; after zooming or changing terminal font size, cached offsets from the old size will be reused and combining marks/ZWJ clusters can render at stale positions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Renders Thai/Lao (and other complex-script) combining marks correctly in the terminal grid, including the Thai/Lao SARA AM vowel (ำ / ຳ) — previously the grid split it from its base consonant so the nikhahit dot detached (e.g.
คำ,น้ำ,ทำ,สำคัญ).This continues @chatre7's work from #10357, which was auto-closed for inactivity. All of his commits are preserved here with their original authorship — thank you, @chatre7 🙏. The branch has been rebased onto the latest
master(resolving the divergence since the original PR, including the Rust 2024 edition migration and thegrid_rendererchanges), re-formatted, and rebuilt + run locally to confirm it still renders correctly.What it does:
CharOrStr::Strcell) and a following SARA AM into a single shaped cluster, so HarfBuzz places the nikhahit dot above the consonant via GPOS instead of drawing it standalone; a line-start SARA AM still falls back to its sara-aa tail.Supersedes #10357.
Linked Issue
Closes #8357
ready-to-specorready-to-implement.Testing
./script/runBuilt and ran a local OSS bundle from this branch (rebased onto current
master, Rust 2024 edition) on macOS: Thai text with SARA AM renders with the vowel attached to its consonant.cargo check -p warpandcargo fmt --checkare clean. The existingtest_decompose_sara_amunit test covers the decomposition helper.Screenshots / Videos
Agent Mode
CHANGELOG-BUG-FIX: Fix rendering of Thai/Lao complex-script combining marks, including the SARA AM vowel attaching to its base consonant.