Repository navigation
Conversation
egonelbre
requested review from
andydotxyz,
benoitkugler and
whereswaldon
as code owners
September 29, 2026 09:39
Contributor
Author
|
PS: Here I'm less confident in my own review of the code as I do not have the necessary in-depth knowledge. |
Contributor
Only tests referenced mapRunesToClusterIndices v1/v2 and mapRuneToClusterIndex. Rename v3 to mapRunesToClusterIndices.
A Face carries the variation coordinates. Two Faces on one Font with different coords shaped with the same cached harfbuzz.Font.
Size is already in 26.6 units, the same scaleShift the shaper removes from the harfbuzz positions. Sizes 12.5 and 13 now shape differently, and Output.Size matches the metrics. Tests using Size 72 and 36 relied on the ceiling to 2 and 1 px. They now pass those sizes directly.
Glyphs are in visual order, so with a TowardTopLeft progression index 0 is the logical end of the run. AddLetterSpacing and trimStartLetterSpacing assumed index 0 was the start. They now read the run progression to find the logical start and end.
cutRun and postProcessLine modified the caller Output.Glyphs in place, so wrapping the same runs again produced stale advances. Copy the glyph slice only when it changes.
WrapParagraph skipped trailing whitespace trimming for a single run that fits. Prepare and WrapNextLine trimmed it.
processBreakOption discarded the Peek ok flag and indexed a nil mapping. The last available run now ends the text, checked against the usual width and truncation rules.
UAX go-text#9 L1 resets whitespace at the end of each line to the paragraph embedding level, so it sits at the line end whatever direction the run containing it has. The wrapper left such whitespace inside the run, so a space at the end of an RTL run in an LTR paragraph was drawn at the visual start of the run, in the middle of the line. postProcessLine now applies L1 at each wrapped line boundary (resetTrailingBidiWhitespace). Trailing whitespace inside a run that opposes the paragraph direction is cut off into its own Output, which takes the paragraph direction and base level, with its glyphs reversed into a copy. A wrapped Line can therefore hold more Outputs than the caller fed in. Clusters that mix whitespace with visible characters stay whole. advanceSpaceAware measures an opposing run without its trailing L1 whitespace, so the space that L1 moves to the line end no longer decides whether the word before it fits. Only spaces L1 covers count; a no-break space stays inside the run and keeps its width. A run with Level 0 and a direction opposing the paragraph was built by hand rather than by Segmenter, and is left alone.
Contributor
Author
|
@benoitkugler done #296 |
whereswaldon
reviewed
Oct 6, 2026
whereswaldon
left a comment
whereswaldon
left a comment
Member
There was a problem hiding this comment.
I've been able to review through 563ce7f so far. I'm out of time for today, so I'll circle back when I can.
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
3 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.
Sixteen commits, one fix each. Grouped by area.
Bidi ordering:
line[i]andline[end-1-i]. That only works while the indices are still in logical order. Once a higher level has reversed an inner sequence the swap scrambles them. Levels{0,2,2,1,1,1,0}, an LTR paragraph with an RTL span holding two LTR runs, came out with the LTR runs swapped and the RTL runs shuffled. The two "bidi example" test cases with nested levels expected that output and now expect whatUAX #9specifies.Outputat the paragraph level, and measures the run without it when deciding whether the word fits. No-break spaces stay inside the run.bidiclosed a run only when the direction changed, so level 0 text followed by level 2 text inside an LRE shared one run and one reported level. A run now ends on any level change. Callers see more runs, and kerning no longer applies across an LRE or LRI boundary. Pango and ICU itemize the same way.Line wrapping:
cutRunand post-processing mutated the caller's glyph slice, so wrapping the same runs twice used stale advances. The slice is copied only when it changes.Segmenterpassed the caller'sFacethrough to every run instead of selecting one, when the input had a face set.Spacing:
Shaper:
Font, so twoFaces with different variation coordinates shared one shaped result. It is keyed on theFacenow.SetFontCacheSizeshrank it, it stayed over the limit. A negative size no longer panics.Output.Sizedisagreed with the metrics. Tests that relied on the ceiling pass integer sizes now.Runesand cluster counts used the unclamped run range while the glyphs used the clamped one.