Properly support RTL text drawing in CoreText #2022
Conversation
Frameworks/CoreGraphics/CGContext.mm
Outdated
| @@ -2089,6 +2089,12 @@ void CGContextSetPatternPhase(CGContextRef context, CGSize phase) { | |||
| // First glyph's origin is at the given relative position for the glyph run | |||
| CGPoint runningPosition{ glyphRuns[i].relativePosition.x, std::round(glyphRuns[i].relativePosition.y) }; | |||
| for (size_t j = 0; j < run->glyphCount; ++j) { | |||
| if (run->bidiLevel & 1) { | |||
There was a problem hiding this comment.
nit: Since we use this in two different places, but with flipped senses, it might be reasonable to abstract it out into a helper macro or inlinable function. if(!RUN_IS_BIDI(...)) seems clearer than that we currently have.
Frameworks/CoreGraphics/CGContext.mm
Outdated
| } | ||
|
|
||
| // Even if text is RTL set bidiLevel to 0 to draw in correct orientation for proper transformation by glyph |
There was a problem hiding this comment.
nit: comment "we have compensated for the RTL glyph positions, so we don't need DWrite to do it for us."
Frameworks/CoreText/CTLine.mm
Outdated
| ret += std::accumulate(run->_dwriteGlyphRun.glyphAdvances, | ||
| run->_dwriteGlyphRun.glyphAdvances + index, | ||
| run->_relativeXOffset, | ||
| std::minus<CGFloat>()); |
| @@ -505,7 +509,10 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _ | |||
| line->_strRange.length = stringRange; | |||
| line->_glyphCount = glyphCount; | |||
| line->_relativeXOffset = static_cast<_CTRun*>(line->_runs[0])->_relativeXOffset; | |||
| line->_relativeYOffset = static_cast<_CTRun*>(line->_runs[0])->_relativeYOffset; | |||
| if (static_cast<_CTRun*>([line->_runs objectAtIndex:0])->_dwriteGlyphRun.bidiLevel & 1) { | |||
There was a problem hiding this comment.
nit: cache line->_runs[0] to avoid double (or triple!) dynamic dispatch
| @@ -78,7 +78,6 @@ inline void _SafeRelease(T** p) { | |||
| @public | |||
| CFRange _strRange; | |||
| CGFloat _relativeXOffset; | |||
| CGFloat _relativeYOffset; | |||
There was a problem hiding this comment.
no more relative y offset!
how uh
how do we handle top-to-bottom scripts?
There was a problem hiding this comment.
We don't currently support TtB scripts.
| ret += std::accumulate(run->_dwriteGlyphRun.glyphAdvances, | ||
| run->_dwriteGlyphRun.glyphAdvances + index, | ||
| run->_relativeXOffset); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: this is probably an awful idea, but you could use a ternary for the last parameter instead of having the if/else
There was a problem hiding this comment.
I tried that some other place but it really didn't like it, can't remember why though.
|
Given #2027, are we sure nothing really regressed here? |
|
right: it would behoove us to avoid merging anything until 2027 is merged with test fixes. |
|
The recommendation in the issue #1932 is to use ComputeGlyphOrigins but we decided to roll out our own implementation instead? |
|
ComputeGlyphOrigins is not available on windows < anniversary update, so we can't use it here. Will wait for #2027 |
|
Tests passed post #2027, please take another look 🍏 |
|
Rebased on top of develop to resolve merge conflict, tests passed again. |
Now properly draws RTL text with support for text matrix manipulation.
Change CTLineGetOffsetForStringIndex and CTFramesetterCreateFrame to remove assumption of LTR text.
Fixes #1932