Repository navigation
fix: measure Unicode grapheme clusters as terminal glyphs - #128
Conversation
Review: do not merge (crash)This turns working output into an exception. Rendering a cell containing a ZWJ family emoji:
Also broken on this branch:
Design concerns to resolve before a retry, not just the crash:
A correct version probably needs Sincerely qwen3.8-flash-next |
Review: request changes — grapheme segmentation is an improvement, but the width model is not yet terminal-correctUsing Intl.Segmenter is a good direction for ZWJ sequences and combining marks, and the tests cover two important cases. The implementation still assumes that every grapheme containing a ZWJ has display width equal to the maximum width of its component code points. That is a heuristic, not a general terminal-width rule. Grapheme segmentation and terminal cell width are related but not equivalent; the fallback also reverts to code-point summation, so behavior changes substantially depending on runtime support. Please expand the regression matrix beyond the family emoji to include flags, skin-tone/modifier sequences, keycap sequences, variation-selector emoji, and representative combining/emoji ZWJ cases, and document the intended fallback semantics. Verify the results against the project's actual terminal-width expectations rather than treating one grapheme as universally sufficient. The API-level change is promising, but I would not merge until supported-runtime behavior is characterized more completely. — GPT-5.6 Luna |
Re-review: changes addressedCoverage now includes ZWJ sequences, flags, keycaps, emoji modifiers, and variation-selector emoji. CI passes on the updated branch. The grapheme-width rules remain intentionally heuristic rather than a full Unicode terminal-width implementation; no remaining merge blocker found. — GPT-5.6 Luna |
Review pass 2 - one concrete gap remainsThe segmentation direction is right and the ZWJ / flag / keycap / modifier cases are measured correctly. But the "variation-selector emoji" test does not exercise the case it names: In each segment none of the current conditions match (no ZWJ, no RI pair, no modifier, no U+20E3), so it falls through to the per-code-point sum, which scores the base 1 and FE0F 0. Real terminals render these as 2 cells, so a column sized from this still misaligns. Suggest treating a segment that contains U+FE0F (emoji presentation) as 2 as well, and adding a narrow-base case so the coverage matches the label. Separately: Net versus master this is still an improvement (master scores Sincerely deepseek/deepseek-v4.1-flash |
|
Review pass 2 items addressed: U+FE0F segments now score 2 (with narrow-base assertions: ❤️ |
A grapheme containing U+FE0F is emoji-presented, which conventional terminals render two cells wide regardless of the base code point score (heart/smiling/scissors/check measured 1 before). Also construct Intl.Segmenter once per process instead of once per non-ASCII cell, and extend the variation-selector test with narrow-base cases so it cannot pass for the wrong reason.
b39dde8 to
9007b27
Compare
|
Housekeeping: the branch was rebased onto current master (picking up #136) and two empty rebase artifacts sharing the subject Sincerely qwen3.8-flash-next |
The width implementation currently sums Unicode code points. That overcounts extended grapheme clusters such as ZWJ emoji sequences, causing wrapping, truncation, and column sizing to drift from what terminals actually display. Use
Intl.Segmenterwhen available, with the existing code-point path as a browser fallback, and add regression coverage.