applib: render bidirectional text with the Unicode bidi algorithm#2
applib: render bidirectional text with the Unicode bidi algorithm#2kaluaim wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous segment-based RTL handling in the text renderer with a minimal UAX 9 (Unicode Bidirectional Algorithm) implementation (implicit levels only), fixing incorrect reordering/mirroring in mixed-direction lines (e.g., bracket pairs spanning LTR/RTL runs).
Changes:
- Add a new bidi engine (
bidi.c/.h) to resolve bidi classes/levels, reorder to visual order, and mirror paired punctuation. - Update
text_layout.cto use the bidi engine during the render pass for lines containing RTL characters, and align justification with the same base-direction rule. - Add a dedicated bidi test suite (
test_bidi.c) and wirebidi.cinto relevant test build scripts; simplify RTL support tests to only cover RTL detection.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/fw/wscript_build | Add bidi.c to firmware test build inputs and register new test_bidi.c. |
| tests/fw/ui/wscript_build | Include bidi.c in UI-related rendering test sources. |
| tests/fw/test_rtl_support.c | Remove reversal/segment tests; keep only utf8_contains_rtl gating coverage. |
| tests/fw/test_bidi.c | New unit tests covering base direction, weak types, separators, bracket pairing/mirroring, and islands. |
| tests/fw/services/timeline/wscript_build | Include bidi.c in timeline rendering test sources. |
| tests/fw/graphics/wscript_build | Include bidi.c in graphics text rendering test sources. |
| tests/fw/apps/watch/kickstart/wscript_build | Include bidi.c in kickstart app rendering test sources. |
| tests/fw/apps/system_apps/workout/wscript_build | Include bidi.c in workout app rendering test sources. |
| tests/fw/apps/system_apps/weather/wscript_build | Include bidi.c in weather app rendering test sources. |
| tests/fw/apps/system_apps/timeline/wscript_build | Include bidi.c in system timeline app rendering test sources. |
| tests/fw/apps/system_apps/music/wscript_build | Include bidi.c in music app rendering test sources. |
| tests/fw/apps/system_apps/launcher/wscript_build | Include bidi.c in launcher rendering/common sources. |
| tests/fw/apps/system_apps/health/wscript_build | Include bidi.c in health app rendering test sources. |
| src/fw/applib/graphics/text_layout.c | Switch RTL render pass to shape + bidi-reorder + visual-order draw; align justification with paragraph base direction. |
| src/fw/applib/graphics/rtl_support.h | Narrow RTL support API to gating only; document relationship to bidi engine. |
| src/fw/applib/graphics/rtl_support.c | Remove old reversal/segment helpers; keep utf8_contains_rtl. |
| src/fw/applib/graphics/bidi.h | New public header for bidi base-level detection and line/UTF-8 reordering. |
| src/fw/applib/graphics/bidi.c | New minimal UAX 9 implementation (weak types, bracket pairing, neutrals, implicit levels, mirroring). |
| src/fw/applib/graphics/arabic_shaping.c | Increase shaping cap to cover a full bidi-processed display line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| int content_width_px = 0; | ||
| Codepoint prev_cp = 0; | ||
| bool skip_ligature_member = false; | ||
| utf8_t *fit_end = line->start; | ||
| utf8_t *p = (utf8_t *)line->start; | ||
| while (p < line_end && *p != '\0' && *p != '\n') { | ||
| utf8_t *pnext = NULL; | ||
| Codepoint cp = utf8_peek_codepoint(p, &pnext); | ||
| if (cp == 0 || pnext == NULL) { | ||
| break; | ||
| } | ||
| if (prv_codepoint_is_invisible(cp)) { | ||
| p = pnext; | ||
| fit_end = p; | ||
| continue; | ||
| } |
| size_t scratch_size = fit_len * 2 + 8; // shaping may expand 2-byte -> 3-byte forms | ||
| utf8_t *shaped = applib_malloc(scratch_size); | ||
| utf8_t *visual = applib_malloc(scratch_size); | ||
| // ~1.3 KiB of bidi working storage; task stacks are 2-4 KiB, so it must | ||
| // live on the heap, not in this frame. |
4adfc2d to
4ddb163
Compare
| bool is_rendering = (char_visitor_cb == render_chars_char_visitor_cb); | ||
|
|
||
| // For segment-based RTL rendering during render pass | ||
| if (is_rendering && line->start != NULL && text_box_params->utf8_bounds != NULL && | ||
| text_box_params->utf8_bounds->end != NULL && | ||
| text_box_params->utf8_bounds->end > line->start && | ||
| utf8_contains_rtl(line->start, text_box_params->utf8_bounds->end)) { |
| size_t scratch_size = fit_len * 2 + 8; // shaping may expand 2-byte -> 3-byte forms | ||
| utf8_t *shaped = applib_malloc(scratch_size); | ||
| utf8_t *visual = applib_malloc(scratch_size); | ||
| // ~1.3 KiB of bidi working storage; task stacks are 2-4 KiB, so it must | ||
| // live on the heap, not in this frame. | ||
| BidiScratch *bidi_ws = applib_malloc(sizeof(BidiScratch)); | ||
| if (shaped && visual && bidi_ws) { |
4ddb163 to
bfd67af
Compare
| // Base direction is a paragraph property (UAX 9 P2/P3): use the | ||
| // paragraph's first strong character so a wrapped continuation line | ||
| // that happens to start with the opposite script keeps the same base. | ||
| int base_level = bidi_base_level_utf8(text_box_params->utf8_bounds->start, line_end); | ||
| visual_len = bidi_reorder_utf8(shaped, shaped_len, visual, scratch_size, base_level, | ||
| bidi_ws); |
| if (effective_alignment == GTextAlignmentLeft && line->start != NULL) { | ||
| if (prv_utf8_starts_with_rtl(line->start, text_box_params->utf8_bounds->end)) { | ||
| if (bidi_base_level_utf8(text_box_params->utf8_bounds->start, | ||
| text_box_params->utf8_bounds->end) == 1) { | ||
| effective_alignment = GTextAlignmentRight; |
| if (cp == 0x000A || cp == 0x000D || cp == 0x0085 || cp == 0x2029) return B; | ||
| if (cp == 0x0009 || cp == 0x000B || cp == 0x001F) return S; | ||
| if (cp == 0x0020) return WS; |
| // Zero-width and other boundary neutrals. | ||
| if (cp == 0x200B || cp == 0x200C || cp == 0x200D || cp == 0xFEFF || | ||
| prv_in(cp, 0x0000, 0x0008) || prv_in(cp, 0x000E, 0x001B)) return BN; |
bfd67af to
d3f9ce6
Compare
d3f9ce6 to
dcd2bcc
Compare
| if (effective_alignment == GTextAlignmentLeft && line->start != NULL) { | ||
| if (prv_utf8_starts_with_rtl(line->start, text_box_params->utf8_bounds->end)) { | ||
| utf8_t *para_start = NULL; | ||
| utf8_t *para_end = NULL; | ||
| prv_paragraph_bounds(text_box_params->utf8_bounds->start, text_box_params->utf8_bounds->end, | ||
| line->start, ¶_start, ¶_end); |
| cursor.origin.x += walked_width_px; | ||
| text_resources_get_glyph(&ctx->font_cache, line->suffix_codepoint, text_box_params->font); | ||
| render_glyph(ctx, line->suffix_codepoint, text_box_params->font, cursor); | ||
| return fit_end; |
|
|
||
| size_t bidi_reorder_line(const Codepoint *cps, size_t n, int base_level, Codepoint *out, | ||
| BidiScratch *ws) { | ||
| if (n == 0 || ws == NULL) return 0; |
dcd2bcc to
6f18000
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/fw/applib/graphics/text_layout.c:706
- The bidi render path hard-caps the fitting scan to BIDI_MAX_CODEPOINTS (including zero-width marks). The measurement/layout path explicitly “shares the standard path below” and does not enforce this cap, so a line can be laid out to include >96 codepoints (e.g. many zero-width marks) but then the render pass will stop early and silently drop any subsequent visible glyphs that still fit in pixel width.
// Walk the logical text accumulating shaped (ligature-folded, mark-aware)
// advances - the same arithmetic the measurement pass uses - to find where
// this line ends within the available width. Also cap the codepoint count:
// the reorder handles at most BIDI_MAX_CODEPOINTS, and zero-width marks
// consume codepoints without consuming width, so width alone cannot bound
// it; without the cap the tail past the limit would be consumed but never
// drawn.
int content_width_px = 0;
size_t fit_cps = 0;
Codepoint prev_cp = 0;
bool skip_ligature_member = false;
utf8_t *fit_end = line->start;
utf8_t *last_cp_start = NULL;
utf8_t *p = (utf8_t *)line->start;
while (p < line_end && *p != '\0' && *p != '\n' && fit_cps < BIDI_MAX_CODEPOINTS) {
utf8_t *pnext = NULL;
Codepoint cp = utf8_peek_codepoint(p, &pnext);
| // Gate on the line's whole paragraph: a display line never crosses a | ||
| // newline, so RTL text in another paragraph must not pull this line | ||
| // through the (allocating) bidi path - but every line of a paragraph that | ||
| // contains RTL anywhere must take it, including LTR-only continuation | ||
| // lines, so their alignment and edge punctuation follow the paragraph | ||
| // base direction. | ||
| prv_paragraph_bounds(text_box_params->utf8_bounds->start, text_box_params->utf8_bounds->end, | ||
| line->start, ¶_start, ¶_end); | ||
| take_bidi_path = utf8_contains_rtl(para_start, para_end); | ||
| } |
6f18000 to
eb546d8
Compare
| // Caps codepoints per shaping call. Sized to cover a whole display line, which | ||
| // the bidi layout shapes in one pass (matches BIDI_MAX_CODEPOINTS). | ||
| #define MAX_SHAPE_CODEPOINTS 96 |
| //! True if the range contains a strong RTL codepoint (bidi class R or AL: | ||
| //! Hebrew, Arabic including supplements and presentation forms). Gates the | ||
| //! bidi layout path with the same class table the reordering itself uses, so | ||
| //! the gate and the base direction can never disagree. |
eb546d8 to
0b90504
Compare
|
Fresh full re-review of current head I found one remaining glyph-loss bug in the joining integration: The new invisible-codepoint handling makes the bidi fit scan agree with the builder, but Concrete shipped-font repro: logical Please make Two smaller cleanup notes:
|
|
Addressed at the current head. word_init() now takes its shaping neighbor from the raw peek unconditionally, and both consumers of the filtered character iterator (word measurement and the standard walk_line path) recover the joining breaker from the raw byte gap the iterator skipped — so a ZWNJ between two Beh measures the same isolated forms everywhere that the fit scan and line builder already produced, and your Beh-ZWNJ-Beh glyph-loss case closes. All four measurement/render stages now derive Arabic joining context from one pair of rules (raw peek forward, raw gap backward). The bidi.h contracts are corrected (orig is documented as apply-time scratch), and git diff --check against the merge base is clean including the codepoint.c/h line endings. This is again a layout-stage transition the engine tests cannot reach — it joins the harness docket as case five. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full re-review of current head I found one remaining ZWNJ glyph-loss path at a soft-wrap boundary: A filtered formatting codepoint can become the raw start of a continuation line, where the standard measurement path treats it as a visible wildcard. Concrete shipped-font repro:
This also reproduces with the existing constant-width |
|
Addressed at the current head, including your regression. A hyphen-split continuation now normalizes its start past filtered formatting codepoints, so a line can never begin on a ZWNJ the iterator will not yield — the codepoint stays in the raw text, where the joining-context scans still see it break the join. Your Beh-ZWNJ-Beh-Beh-Beh recipe is in-tree as test_line_layout__continuation_skips_formatting_codepoint, asserting the continuation pointer advances offset 5 then 7 across two line_add_word calls — you were right that the constant-width stubs expose the endpoint bug without the render harness, and that removes one case from its docket. Also in this head, from the parallel review: the joining scan starts after the last newline before the position (identical result, cheaper), and the ZWJ observation is logged as a known deviation — Unicode classes it join-causing, all four stages here treat it as join-breaking consistently; changing that is a shaping-semantics follow-up. Suite green apart from the known put_bytes flake; whole-PR git diff --check is clean; obelix firmware builds. |
|
Fresh full review of current head
I would address these before merging. |
|
Both addressed at the current head. The shipped number signs are ET — per-mille/prime family U+2030-2034, fullwidth U+FF03-FF05, and the fullwidth currency signs U+FFE0/FFE1/FFE5/FFE6 (the contiguous UCD ranges around your named set) — with a regression asserting W5 keeps the per-mille sign with its number in an RTL line. The five non-bracket mirror pairs are in L4 with explicit mappings (U+223C/223D, U+226E/226F, U+FF1C/FF1E), with a regression for the fullwidth angle mirroring on an odd level. Both rounds this cycle were hand-runs of exactly the manifest-vs-UCD comparison the agreed follow-up automates — the generated table plus conformance test will make this class of finding structural rather than iterative. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full review of current head I found one remaining user-visible class mismatch in the shipped CJK fonts: U+3220–U+3229 are |
|
Addressed at the current head: U+3220-3229 are L (your list-marker repro is a regression test — the marker stays with its following run), the combining marks for symbols U+20D0-20FF including the keycap are NSM (regression: the keycap stays attached to its digit in an RTL line), and the unimplemented isolate controls U+2066-2069 are classed BN so they stay transparent instead of joining neutral runs. The RI+VS16+RI note is accepted as-is: it is a malformed sequence, and measurement and draw agree on the two-flag rendering, so there is no accounting mismatch. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full review of current head I found one remaining assigned class range in the locale resources: U+0483–U+0489 are |
|
Addressed at the current head: U+0483-0489 are NSM (listed before the Cyrillic letter range) and U+1680/U+205F are WS. Your titlo repro is a regression test with one representation note: this engine's visual sequence carries a reversed mark before its base (a ҃ א), and the draw pass overlays the zero-width mark on the base at the same x position — pixel-identical to the mark-after-base sequence FriBidi reports (a א ҃), just a different internal convention for where the attached mark sits in the codepoint stream. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full review of current head I found one render bug in the new titlo regression: The asserted This is the L3/render-attachment part of bidi, not the now-correct |
|
Addressed at the current head with a real L3 implementation rather than a convention argument — you were right that the overlay assumption fails for negative-bearing marks. After L2, each marks+base cluster on an odd level is re-reversed, so the base leads and its combining marks follow in logical order: a zero-advance mark with negative bearing (your titlo metrics: advance 0, offset -8) now draws over its own base's cell, and the visual sequence matches the reference base-then-marks order outright — the previous representation caveat is retired. The titlo regression asserts the reference order, and a new harakat case covers Arabic marks mid-word on a reversed level. The pixel-position regression with real glyph metrics joins the render-harness docket, where the per-form advance fixtures it needs already live. Also from the parallel review: the embedding controls U+202A-202E are now BN like the isolates. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full re-review of current head The global marks+base re-reversal detaches Arabic harakat with the shipped Vazirmatn metrics. For the test's logical Beh + fatha + Jeem, shaping produces
The new visual order Please add actual render-position/pixel regressions for both the Arabic and Cyrillic cases and anchor marks explicitly (or normalize their generated metrics). A single global codepoint-order convention cannot position both shipped metric conventions correctly. |
|
Addressed at the current head with the anchoring you prescribed rather than another order flip. The draw loop now positions zero-advance combining marks explicitly on their base, choosing the pen by the mark's stored bearing: positive or zero left offset draws at the base's own pen (the convention your Vazirmatn fatha metrics expect), negative draws at the pen after the base's advance (your Roboto titlo). The L3 codepoint order stays reference-conformant — pixels are now order-independent for marks, which is exactly why anchoring is the right layer for this. The pixel-position regressions for both conventions join the render-harness docket, which now has real bearing fixtures to encode from your measurements. From the parallel review: the Greek ON signs and the Cyrillic thousands sign are carved out of the letter range, U+2329/232A pair and mirror with the CJK angles, and on the deviation question — yes, intentional: Arabic number signs alone are degenerate input, and right-aligning them is the harmless reading. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full re-review of current head
I would address these before merging. |
|
All three addressed at the current head. U+0482 is back in the Cyrillic L range with your repro as a regression — that misclass was introduced last round by taking a parallel-review claim without checking DerivedBidiClass, which is on me; the Greek ON carve-outs it arrived with were verified and stay. The mark anchor no longer infers a convention from bearing sign: the draw pass computes the mark's bitmap span at both candidate pens and places it where more ink lands on the base's cell — by your own measurements that resolves fatha (base pen), titlo (after-advance pen) and U+06DC (base pen, matching HarfBuzz) with one rule and no per-glyph metadata; the three bearing fixtures go to the render-harness docket as the pixel regressions. BD16 now matches bracket pairs through canonical equivalence (U+2329/232A canonicalize to U+3008/3009 for pairing only; L4 still mirrors the literal forms), and both mixed-equivalence combinations are regression tests reproducing your conformant orders. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full re-review of current head
Small documentation cleanup in the same change: the comment above the I would address the L3 case before merging. |
Replace the per-direction segment reversal in walk_line() with a UAX 9 engine implementing the Basic Display Algorithm: resolve character classes, weak types (W1-W7), bracket pairs (N0/BD16), neutrals (N1/N2) and implicit levels (I1/I2) once for the whole paragraph, then apply the per-line rules (L1 whitespace reset, L2 reversal, L4 mirroring) to each display line's slice. Weak, neutral and bracket resolution therefore cross soft wraps exactly as in an unwrapped paragraph. The old segment approach could not handle mixed-direction lines: brackets split across an LTR/RTL segment boundary mirrored one side only, and neutrals at run boundaries reordered wrongly. Character classes follow the UCD for the codepoints the firmware ships (including fullwidth forms, presentation forms and the CJK bracket pairs), and the resolver matches the applicable class-based Unicode BidiTest and BidiCharacterTest data. Boundary neutrals are transparent to the weak and neutral rules per UAX 9 5.2, a resolved bracket pair propagates its type to following combining marks, and the truncation suffix joins the line at the base level so it lands at the visual end of the reading direction. The resolver handles one paragraph at a time (255 codepoints, so every index fits a uint8_t). All working state - the ~3.6 KiB BidiScratch - lives in a single heap block, because task stacks are 2-4 KiB. Line shaping folds Lam-Alef ligatures and regional-indicator pairs with the paragraph as joining context, so Arabic letterforms stay correct across wraps, and the resolved levels are carried through the fold. Longer paragraphs, over-cap lines and allocation failure all fall back to the standard logical-order path instead of truncating. Direction is resolved per paragraph, as UAX 9 requires: the RTL gate, the base level (P2/P3) and the default alignment all scan exactly the line's own paragraph (in Fill mode a newline renders as a space, so the whole box is one paragraph). One deviation from P3: digit-only Arabic-script text stays RTL, so times and verification codes keep their right alignment. The old rtl_support helpers are removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Khalid Nuaim <i@kalua.im>
|
Addressed at the current head. The L3 cluster scan now looks through retained boundary neutrals, so a mark separated from its base by a ZWNJ re-reverses with the actual base — your Beh-ZWNJ-fatha-Jeem repro resolves to the FriBidi order (Jeem, Beh, ZWNJ, fatha) and is an engine regression; the draw anchor consequently lands the fatha on Beh's cell, and the pixel form of the case joins the render-harness fixtures. The ON carve-out comment now names ano teleia instead of the departed thousands sign, and from the parallel review U+2212 MINUS SIGN is ES, so a negative temperature keeps its sign attached in RTL. Suite green apart from the known put_bytes flake; obelix firmware builds. |
|
Fresh full re-review of current head
The mirror-width mismatch is user-visible and should be fixed before merging. The leading-mark case is a smaller L3 correctness edge. |
What
Replace the per-direction segment reversal in
walk_line()with a UAX 9 engine implementing the Basic Display Algorithm (implicit levels only — no RLE/LRI overrides, which don't occur in watch text): character classes, weak types (W1-W7), bracket pairs (N0/BD16), neutrals (N1/N2) and implicit levels (I1/I2) resolve once per paragraph, then each display line applies L1/L2/L4 to its slice. Weak, neutral and bracket resolution therefore cross soft wraps exactly as in an unwrapped paragraph.The segment approach couldn't handle mixed-direction lines: a bracket pair split across an LTR/RTL segment boundary mirrored one side only — an Arabic word in parentheses after Latin text rendered with two opening parentheses — and neutrals at run boundaries reordered wrongly.
Design notes
BidiScratch, paragraph-sized at 255 codepoints) lives in a singleapplib_mallocblock per rendered RTL line. Task stacks are 2-4 KiB (legacy 2.x/3.x apps get 2 KiB), so none of it can sit in the render path's frame. Allocation failure, over-cap lines and paragraphs beyond 255 codepoints all fall back to the standard logical-order path instead of truncating or rendering blank.bidi_base_level_utf8(), with one deviation: digit-only Arabic-Indic text stays RTL so times/verification codes keep right alignment. Alignment (prv_line_justify) and reordering share this rule, so a line can no longer right-align but reorder as LTR or vice versa.bidi_contains_rtl()scans exactly the line's own paragraph using the engine's class table (Hebrew, Arabic incl. supplements and presentation forms), so gating, alignment and reordering agree. Paragraphs with no RTL never enter the path — no behavior change and no allocations for LTR-only text (the gate itself is a per-line paragraph scan, bounded by watch text sizes).BIDI_MAX_CODEPOINTS(255) — zero-width marks consume codepoints without consuming width, so pixel width alone cannot bound the count.rtl_supportmodule (utf8_reverse_for_rtland the segment machinery) is deleted entirely.Before / after
Test
test_bidi.c: 50 cases — base level (P2/P3 + digit deviation), strong runs, Western + Arabic-Indic digits, time and date group order (W4), separator-needs-two-digits, bracket mirroring in RTL, LTR island in RTL, RTL island in LTR (no mirror).obelix@pvtfirmware builds clean.