Give spacing combining marks a column, so Indic scripts stop overprinting - #645
Give spacing combining marks a column, so Indic scripts stop overprinting#645rupok wants to merge 2 commits into
Conversation
…ting
Unicode general category Mc is a spacing combining mark: it advances the
cursor. It was grouped with Mn and Me and given width zero, which is
correct for those two and wrong for Mc.
Every Indic script writes its dependent vowel signs as Mc, so all of them
under-counted their own width. In a terminal view the effect is not
drift but collision — the glyphs draw on top of one another:
|ব| one base character
|বা| the same character plus U+09BE, a spacing vowel sign
Both closing pipes landed in the same column. By four marks the line was
unreadable. Devanagari, Bengali, Gurmukhi, Gujarati, Odia, Tamil, Telugu,
Kannada and Malayalam were all affected.
Mc with a nonzero canonical combining class stays zero-width. Those are
viramas and their equivalents, which conjoin the surrounding consonants
rather than advancing — and keeping them at zero preserves the invariant
handlePrint documents and relies on, that a scalar with a nonzero
combining class is always zero-width. Twenty-seven scalars fall in that
group, including the Grantha, Sharada and Khojki viramas and the musical
combining stems.
The change is in the generator; UnicodeWidthData.swift is regenerated
from it. The reference implementation in UnicodeTests.swift is updated to
match, and one boundary expectation moves: U+0903 DEVANAGARI SIGN VISARGA
is Mc with combining class zero, so it now measures one column.
All 986 tests pass.
migueldeicaza
left a comment
migueldeicaza
left a comment
There was a problem hiding this comment.
Thank you so much for this contribution!
It is fine to submit the generated data along the way, just a small set of comments below, as I think this does not cover Mc properly.
| return 0 | ||
| if category == "Mc": | ||
| return 0 if in_ranges(value, nonzero_combining) else 1 | ||
| if category == "Cf": |
There was a problem hiding this comment.
This PR introduces a mismatch between the unit being changed and the unit being displayed.
We display extended grapheme clusters, and they can contain more than one Mc scalar.
One particular case from the database is this:
0BCA;TAMIL VOWEL SIGN O;Mc;0;L;0BC6 0BBE;;;;N;;;;;
Which decomposes to two Mcs:
let x = "\u{0BCA}".decomposedStringWithCanonicalMapping
for scalar in x.unicodeScalars {
print(
String(format: "U+%04X", scalar.value),
scalar.properties.generalCategory
)
}
Which prints:
U+0BC6 spacingMark
U+0BBE spacingMark
the proposed scalar-based calculation breaks canonical equivalence. The fix must calculate the width for the full grapheme cluster.
A suggested test to catch this would be adding a test to HeadlessTerminal to catch the position after inserting those scenarios (\u{0bca), \u{0bc6}, \u{obbe}) so that each only consumes one cell.
|
Here are a handful of other character that match that scenario:
|
A grapheme cluster can hold more than one Mc. TAMIL VOWEL SIGN O is the
case that shows it:
0BCA;TAMIL VOWEL SIGN O;Mc;0;L;0BC6 0BBE;;;;N;;;;;
Both scalars it decomposes to are Mc, so giving each of them a column made
the decomposed form a cell wider than the composed one — the same text
landing in different places depending on how it had been normalised.
Measured before this commit:
0BCA 1
0BC6 0BBE 2 the same character, decomposed
base + 0BCA 2
base + 0BC6 0BBE 3
A cluster is now worth the width of its base plus one column if it carries
any spacing mark at all, however many it carries. All four cases above
measure the way the composed form always did.
Two things had to move together. clusterWidth replaces the per-scalar
maximum, and a spacing mark now reaches the combining path in handlePrint,
which it could not before: that path is entered on chWidth == 0, and a
spacing mark is the one combining scalar that is not zero-width. The
existing "did this stay one grapheme cluster" test is what decides whether
the mark really belongs to the cell before it. A cell that takes its first
spacing mark widens once; later marks in the same cluster do not widen it
again.
986 existing tests pass, plus five covering the cases above.
|
You're right, and it was worse than the width table — thank you for catching it. I measured the cursor on the branch as it stood, using the HeadlessTerminal test you suggested:
Why it went wrong where it didThe width table was only half of it. var shouldTryCombine = chWidth == 0 || ... ZWJ, variation selector, emoji modifierA spacing mark is the one combining scalar that is not zero-width. Giving The ruleA cluster is worth the width of its base, plus one column if it carries any spacing mark at all — however many it carries. Two changes carry it:
TestsAdded to 986 existing tests still pass, 991 in total. One thing I should flag rather than leave implicit: "one extra column for the marks" is my reading of your test — that each of those three consumes a single cell. If you would rather a cluster's spacing marks contribute nothing and the base carry the whole width, or want the widening handled somewhere other than the combining path, say so and I will redo it that way. |
2 participants
Unicode general category Mc is a spacing combining mark: it advances the cursor. It was grouped with
MnandMeand given width zero, which is right for those two and wrong forMc.Every Indic script writes its dependent vowel signs as
Mc, so all of them under-count their own width. The effect in a terminal view isn't column drift — it's collision. The glyphs draw on top of one another.Reproduction
Both closing pipes land in the same column. By four marks the line is unreadable.
Affected: Devanagari, Bengali, Gurmukhi, Gujarati, Odia, Tamil, Telugu, Kannada, Malayalam.
The exception, and why it matters
Mcwith a nonzero canonical combining class stays zero-width. Those are viramas and their equivalents, which conjoin the surrounding consonants rather than advancing.Keeping them at zero is not just cosmetic — it preserves the invariant
handlePrintdocuments and depends on:Twenty-seven scalars fall in that group, including the Grantha, Sharada and Khojki viramas and the musical combining stems. Without the exception,
testCombiningScalarsAreZeroWidthfails on all of them andhandlePrint's assumption quietly breaks.What changed
scripts/regen_unicode_width_data.py— the actual fix.Mcgets one column unless its canonical combining class is nonzeroSources/SwiftTerm/UnicodeWidthData.swift— regenerated from the script, not editedTests/SwiftTermTests/UnicodeTests.swift— the reference implementation updated to match, plus one boundary expectation: U+0903 DEVANAGARI SIGN VISARGA isMcwith combining class zero, so it now measures one columnTests/SwiftTermTests/IndicWidthTests.swift— new. Covers a spacing mark from each of the nine scripts, confirmsMn/Mestay at zero, and checks that বাংলা advances five columnsAll 986 tests pass.
Notes
Happy to split the regenerated table into its own commit if that reads better, or to adjust the virama exception if you'd rather handle it in
handlePrintinstead — the test comment suggests that was the anticipated alternative.