Skip to content

applib/graphics: add a minimal BiDi engine - #1972

Merged
gmarull merged 1 commit into
coredevices:mainfrom
kaluaim:minimal-bidi-engine
Sep 17, 2026
Merged

gmarull merged 1 commit into
coredevices:mainfrom
kaluaim:minimal-bidi-engine

Conversation

@kaluaim

@kaluaim kaluaim commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Following the discussion in #1631 (comment) — mirroring paired brackets on its own does not cover mixed-direction text, so this does the reordering properly.

before / after

Non-RTL text never reaches the engine

bidi_is_needed() gates the whole path on a raw byte scan over the same range the previous utf8_contains_rtl() covered. Text with none of those bytes costs one comparison per byte, never decodes UTF-8, and keeps taking the existing left-to-right path. No behaviour change for Latin-only text.

What it fixes

  • Bracket mirroring (L4). RTL runs are reversed but the glyphs are never mirrored, so (مرحبا) draws as )مرحبا(. main has no mirroring at all.
  • Neutral resolution (N1/N2). Spaces and punctuation join whichever run happens to be open instead of resolving against their neighbours, so the two halves of a bracket pair can land in different runs and be treated inconsistently.
  • Combining marks (W1). A mark is reversed away from the letter it attaches to, and can resolve into a run of its own.
  • European numbers (W7). A number after a Latin word is treated as right-to-left, so مرحبا abc 123 def renders the embedded phrase as def 123 abc.
  • Paragraph separators (class B). Direction resolution runs past \n into the following paragraph.

What it preserves

The weak-LTR digit and numeric-separator handling already in rtl_support.c is kept, not added. It moves from a special case inside the reversal into the run classification: European and Arabic-Indic numbers resolve to their own left-to-right run, which is the level UAX 9 assigns them in either paragraph direction. ٢٠٢٦ and ١٢:٣٤ behave as they do today. One visible change for Arabic: 25% after an Arabic word now renders %25, as the phone does (W2).

Resolving neutrals properly also removes the need to peel trailing spaces into a separate segment, so rtl_segment_content_end() goes away with it. Lam-alef ligature shaping and the segment cap are untouched.

Scope

Implemented: P2/P3 (paragraph direction), W1 (marks), W2 and W4–W7 (Arabic letters are AL, so a following European number becomes Arabic and its terminators and signs no longer bind; numbers keep their separators), N1/N2 (neutrals), I1/I2 and L2 (embedding levels 0–2 and run reordering), L4 (mirroring, 10 pairs), class B (paragraph separators).

Not implemented: bracket pairs (N0), explicit directional controls (LRM/RLM/RLE/PDF), isolates, embedding levels above two, the full ~380-character mirror set. N0 and a linear single-pass neutral resolver are tracked as a follow-up; until then a Latin word with a parenthetical inside an RTL paragraph (رسالة من John (Work)) draws both brackets as opening ones.

Changes

rtl_support.c becomes bidi.c, rewritten. The run splitter in text_layout.c collapses into a single bidi_next_run() call, and mirroring is applied in both the width pass and the draw pass so the two agree.

About 2.3 KB of flash (.text 2132 B + .rodata 90 B on cortex-m4 -Os, plus 124 B for the W2 change per its author's measurement), no RAM, no allocation. It replaces rtl_support.c at 550 B and text_layout.c is net 24 lines smaller, so the firmware grows by roughly 1.8 KB.

Testing

test_rtl_support.c is replaced by tests/fw/test_bidi.c — 34 cases. The digit and separator coverage from the old file is kept, re-expressed as run boundaries rather than reversal output, alongside bracket mirroring in both paragraph directions, combining marks across every block that carries them, \n as a paragraph boundary, regional-indicator pairing, and the fast-path gate. All pass, clean under ASan and UBSan.

@kaluaim
kaluaim marked this pull request as ready for review August 31, 2026 05:19
@kaluaim
kaluaim force-pushed the minimal-bidi-engine branch from 672aac0 to 569a334 Compare September 2, 2026 07:22

@gmarull gmarull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated correctness review of the BiDi engine. The engine itself held up well: fuzzing bidi_next_run() / bidi_reverse_run() over 300k random mixed-script strings under ASan/UBSan found no memory-safety issues and no failure to make progress, and W1/W4-W7/N1/N2/L4 produce correct output for brackets, 12:34, 50%, -5, 3.5, the Arabic comma and AN-vs-EN. The findings below are mostly in how text_layout.c consumes it.


// L4: mirrored glyphs such as brackets face the other way inside
// an RTL run.
rcp = bidi_mirror_codepoint(rcp);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression: flag emoji inside an RTL run are drawn unfolded, so the measured width no longer matches what is drawn.

prv_class() classifies regional indicators as BidiClassON, so in "مرحبا 🇸🇦 وداعا" the neutrals resolve RTL on both sides and the whole string comes out as a single RTL run (checked against the engine directly).

Pass 1 measures that run through prv_shape_pair() -> emoji_shape_pair(), which folds the RI pair into one FLAG_CODEPOINT advance and sets skip_ligature_member. This draw loop reads codepoints straight out of rtl_buffer and never calls prv_shape_pair(), so the two regional indicators are rendered individually (missing/wildcard glyphs) and walked_width_px advances by both — everything after them on the line is shifted.

On main this worked: the old splitter treated an RI as neither punctuation nor RTL, so it broke the segment and the flag was drawn by the LTR branch, which does fold. The RI pairing added in bidi_reverse_run() preserves the pair's byte order but does not make this loop fold it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the RTL draw loop now folds through emoji_shape_pair() and consumes the second indicator, matching pass 1 and the LTR branch.

Your read of the cause is exactly right, and I had this wrong: I had assumed the LTR branch did not fold either, so I treated it as pre-existing. It does fold (line 97), the RTL branch did not, and classifying regional indicators as ON is what moved the flag into the RTL run in the first place. Confirmed "مرحبا 🇸🇦 وداعا" resolves to a single RTL run here.

The pair ordering fix already in bidi_reverse_run() keeps the two indicators in order through the reversal, so folding after it produces the right flag rather than a mirrored country code.

Comment thread src/fw/applib/graphics/text_layout.c Outdated
// Pass 2: Reorder the runs for the paragraph direction (UAX 9 L2)
// In an RTL paragraph the visual order of the runs is reversed so the
// first logical run appears on the right, where reading starts.
if (line_is_rtl && num_segments > 1) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L2 reordering is skipped entirely for LTR paragraphs, so a number after an RTL run lands on the wrong side.

In an LTR paragraph an EN that follows an RTL run sits at level 2 nested inside the level-1 RTL region, and L2 moves it to the left of that region. Gating the reorder on line_is_rtl never does that.

Concrete case — "Total مرحبا 123" splits into L("Total "), R("مرحبا "), L("123") and renders as the glyph sequence

T o t a l ␣ ␣ ا ب ح ر م 1 2 3

i.e. two adjacent spaces, and the digits fused to the wrong side of the Arabic. Expected is Total 123 ابحرم.

This is inside the PR's stated scope — only "embedding levels above two" is listed as out of scope, and this is level 2. RTL paragraphs are fine, because there L and EN both resolve to is_rtl == false and bidi_next_run() merges them into one run, which is what makes the whole-array reversal correct in that direction.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed properly — runs now carry an embedding level (UAX 9 I1/I2, 0-2) instead of a direction bit, and pass 2 is real L2: from the highest level present down to the lowest odd level, reversing each contiguous group at that level or above.

Your scope point was the one that settled it. I had tried a cheaper version first that reconstructed the level-1 region by bridging any LTR run held between two RTL runs. It fixed your case and broke the ordinary one, because a bool cannot tell level-0 Latin from a level-2 number:

A مرحبا 12 وداعا Z    ->  A اعادو 12 ابحرم Z    fixed
A مرحبا DEF وداعا Z   ->  A اعادو DEF ابحرم Z   broken, was correct

With levels both hold:

Total مرحبا 123      ->  [Total |L0] [مرحبا |L1] [123|L2]  ->  Total 123 ابحرم
A مرحبا DEF وداعا Z  ->  [A |L0] [مرحبا|L1] [ DEF |L0] [وداعا|L1] [ Z|L0]

This changes bidi_next_run() to report uint8_t *run_level. The PR text now lists I1/I2 and L2 as implemented.

Comment thread src/fw/applib/graphics/text_layout.c Outdated
utf8_t *line_start = (utf8_t *)line->start;
utf8_t *line_end = (utf8_t *)text_box_params->utf8_bounds->end;
utf8_t *ptr = line_start;
const bool line_is_rtl = bidi_paragraph_is_rtl(line_start, line_end);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2/P3 is resolved per visual line rather than per paragraph.

line_start is line->start, so for wrapped text this picks the first strong character on this line, not in the paragraph. An Arabic paragraph that wraps so that line 2 begins with an embedded English word gets line_is_rtl == false for that line: its runs are not reordered, and prv_line_justify() (same call at line 1388) left-aligns it while line 1 is right-aligned — the base direction flips mid-paragraph.

P2 is defined over the paragraph, so the direction wants resolving once from the paragraph start and carrying on Line / TextBoxParams. Pre-existing in shape (prv_utf8_starts_with_rtl() had the same problem), but bidi_paragraph_is_rtl() is the natural place to fix it now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Paragraph direction now resolves from the start of the paragraph rather than the start of the visual line, at both call sites.

Rather than carry it on Line / TextBoxParams, prv_paragraph_start() scans back to the previous \n and hands that to bidi_paragraph_is_rtl(), which already stops at class B. \n cannot appear as a UTF-8 continuation byte so the scan is a plain byte walk, once per line. Same result as plumbing it through, without the struct change.

You are right that it was pre-existing in shape — prv_utf8_starts_with_rtl() had the same scope — but it is a small fix here and the alignment flip mid-paragraph is worth not shipping.

return true;
}

bool bidi_is_needed(const utf8_t *start, const utf8_t *end) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gate is narrower than the class table, so some scripts prv_class() calls strong-RTL never reach the engine.

The byte scan covers U+0590-U+06FF only, but prv_class() returns BidiClassR for U+0700-U+08FF (Syriac, Thaana, NKo, Samaritan, Mandaic, Arabic Ext-A/B) and for the presentation forms U+FB1D-U+FDFF / U+FE70-U+FEFC.

Text made only of those — including pre-shaped Arabic presentation forms, which some servers send — returns false here, so it renders in logical order and is left-aligned by prv_line_justify(), even though the engine has full class data for it.

Either widen the scan (lead bytes 0xDC-0xDF for U+0700-U+07FF, plus the three-byte prefixes for U+0800-U+08FF and U+FB1D and up), or drop the unreachable R classifications so the two agree.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by widening the scan rather than dropping the classifications, so the gate and the class table now agree in the direction that keeps the data useful.

Lead bytes added: 0xD7-0xDF for U+05C0-U+07FF, 0xE0 with 0xA0-0xA3 for U+0800-U+08FF, and 0xEF with 0xAC-0xB7 / 0xB9-0xBB for the presentation forms. Continuation bytes are 0x80-0xBF so none of them can match a lead tested here and the byte-at-a-time scan stays safe.

Dropping the R classifications was the other option you offered, but the presentation forms are the case that decided it: pre-shaped Arabic is real input, arabic_shape_text() emits U+FE70-U+FEFC itself, and classifying those as L would be wrong rather than merely unreachable. Verified Syriac, Thaana, NKo, Arabic Extended-A and both presentation-form blocks now pass the gate.

//! Resolve the direction of the span starting at @p pos and report where it
//! ends. A span is one strong character, a number with its weak neighbours, or
//! a stretch of neutrals resolved together.
static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t *end,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neutral resolution re-scans the line for every span, which is super-linear on the render path.

Each neutral span calls prv_prev_side() (backward scan to line_start) and prv_next_side() (forward scan to end), and every EN either of them meets calls prv_number_follows_ltr(), which scans backward to line_start again.

Measured on an M-series host at -O2, splitting one 122-byte Arabic-plus-digit-groups line into runs costs ~80 us, vs ~6 us for a more typical mixed line. Scaled to a 64-80 MHz Cortex-M4 that is single-digit milliseconds per line, repeated on every render of the text. MAX_BIDI_SEGMENTS 16 caps the number of bidi_next_run() calls but not the rescans inside each one.

Carrying the last-strong direction forward while walking the line once would make this linear.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The complexity claim is right and I can reproduce the shape of it. Measuring run splitting alone on an M-series host at -O2, us per byte across a doubling series:

shape              n=2    n=4    n=8    n=16    us/byte
arabic + digits   0.41   0.91   1.92   3.92    flat 0.016   linear
digits + punct    0.40   1.29   4.52  15.53    0.020->0.149  super-linear

The digits + punct shape roughly doubles per doubling, so prv_prev_side() / prv_number_follows_ltr() walking back to line_start is the cost, as you describe.

I could not reproduce the magnitude. My worst constructed case is 15.5 us at 104 bytes against your 80 us at 122 bytes, about 5x apart. Could you share the line you measured? I would rather fix this against the shape that actually hurts than against my guess at it.

One thing that got worse, not better: resolving embedding levels for the L2 fix means every EN span now calls prv_number_follows_ltr() inside prv_resolve_span(), not only during neutral resolution. That took the same worst case from 10.9 us to 15.5 us. So this is more worth doing than when you raised it.

Carrying the last strong class forward is the right fix and it needs state across bidi_next_run() calls, so it changes the signature again. I would rather do that as a follow-up than reopen the interface a second time in this PR, but say the word if you would prefer it here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shape is a Latin word followed by a list of numbers, with Arabic anywhere later in the paragraph so the line passes the gate:

"a " + "1 " * 55 + "مرحبا"      122 B   36.6 us
"a " + "1 " * 27 + "مرحبا"       66 B    9.8 us
"a " + "1 " * 14 + "مرحبا"       40 B    3.1 us
"a " + "1 " * 7  + "مرحبا"       26 B    1.1 us
"Score: 10 11 12 … 44 مرحبا"     122 B   21 us

M-series, -O2, run splitting only, on the current head. ~4x per doubling. Every space is a neutral whose prv_prev_side() and prv_next_side() both land on an EN, and each of those calls prv_number_follows_ltr(), which walks back over the whole prefix to the a. It is one run, so MAX_BIDI_SEGMENTS does not bound it, and a line is bounded only by the display width.

Carrying the last strong forward is the right fix and I am fine with it as a tracked follow-up: at ~120 B per line that is low single-digit milliseconds on an M4 per render, not a hang. One thing to weigh: a single-pass resolver with per-line state is also the natural place to do N0 (see the final-pass review), so it may be worth doing the two together rather than fitting N0 into the scanning design first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the shape — reproduced it. Agree it goes to the follow-up, and agree N0 belongs in the same single-pass rewrite rather than being fitted into the scanning design first.

@gmarull gmarull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated correctness review of the BiDi engine. The engine itself held up well: fuzzing bidi_next_run() / bidi_reverse_run() over 300k random mixed-script strings under ASan/UBSan found no memory-safety issues and no failure to make progress, and W1/W4-W7/N1/N2/L4 produce correct output for brackets, 12:34, 50%, -5, 3.5, the Arabic comma and AN-vs-EN. The findings below are mostly in how text_layout.c consumes it.

@kaluaim
kaluaim force-pushed the minimal-bidi-engine branch from 569a334 to 63cfe7e Compare September 9, 2026 04:58

@gmarull gmarull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final pass, on top of the first round. The four fixes hold up: I re-verified the RI folding, the level-based L2, the paragraph-start P2/P3 and the widened gate. The branch also rebases cleanly onto current main and the text layout, iterator and image-fixture tests all pass there.

This time the engine was checked differentially: bidi_next_run() + the L2 reorder from text_layout.c + bidi_reverse_run(), compared against the unicode-bidi reference (via python-bidi) and against Unicode's BidiCharacterTest.txt. Hebrew mixed with Latin, numbers, separators, terminators and neutrals: 0 mismatches in 80k random lines. The deviations are all in three places, below, and the remaining findings go with them.

1. N0 is absent, and that regresses main for a common shape

A Latin word followed by a parenthetical inside an RTL paragraph draws both parentheses as opening ones:

رسالة من John (Work)          →  (John (Work نم ةلاسر
مرحبا iPhone (15) وداعا       →  اعادو (iPhone (15 ابحرم
הודעה מ John (Work)           →  (John (Work מ העדוה
مرحبا (hello) world وداعا     →  اعادو hello) world) ابحرم

( sits between L and L and resolves L; ) sits between L and R (or eos) and takes the paragraph direction, so it is reversed to the far side and mirrored. On main the old splitter never split on punctuation, so John (Work) was one LTR segment and came out right. Contact and app names with a parenthetical are the practical case, so I don't think the "N1/N2 already give both halves the same direction in the cases that arise in practice" argument survives; in BidiCharacterTest.txt 14.5k of 91.5k bracket cases fail. N0 restricted to the ten pairs already in the mirror table would cover it. If it goes to the follow-up instead, the claim in the description and commit message should go now.

2. W2 is missing: Arabic letters are classed R, not AL

After an Arabic letter a European number becomes an Arabic number, so terminators and +/- do not bind to it:

الحرارة 25%    →  25% ةرارحلا     spec / phone:  %25 ةرارحلا
من 10-12 مساء  →  ءاسم 10-12 نم   spec / phone:  ءاسم 12-10 نم
الرصيد $50     →  $50 ديصرلا      spec / phone:  50$ ديصرلا

This is inherited from main's weak-LTR digit rule and the PR preserves it deliberately, but it is what the phone renders differently for the primary script. Hebrew is unaffected (R, not AL).

3. W4 is applied together with W5

A separator after a terminator joins the number, so 50%-60% is one run; per spec W4 runs first and the - is a neutral. 16 of 40k random Hebrew lines, so minor, but it falls out of the same patch.

Patch for 2 and 3

Attached below. It adds BidiClassAL, replaces prv_number_follows_ltr()'s scan with prv_prev_strong() (feeds both W2 and W7), gives prv_scan_number() an after_arabic flag plus an after_digit flag for the W4/W5 order, and steps over a terminator run in one go in the neutral stretch (the extra back-scan would otherwise make مرحبا %%%…1 quadratic). Verified: 0 mismatches across 200k random lines on every alphabet, ASan/UBSan clean on 60k corrupted-UTF-8 inputs, +124 B on cortex-m4 -Os. Only number_keeps_its_terminator needed changing (it encoded the non-standard case; it is retargeted to Hebrew and W2 / W4-order tests are added).

bidi-w2-w4w5.patch (apply with git apply on the PR head)
diff --git a/src/fw/applib/graphics/bidi.c b/src/fw/applib/graphics/bidi.c
index 618df2feb..3624c53c7 100644
--- a/src/fw/applib/graphics/bidi.c
+++ b/src/fw/applib/graphics/bidi.c
@@ -17,6 +17,7 @@
 typedef enum {
   BidiClassL,    // Strong left-to-right
   BidiClassR,    // Strong right-to-left
+  BidiClassAL,   // Strong right-to-left Arabic letter, turns a following EN into AN
   BidiClassEN,   // European number
   BidiClassAN,   // Arabic-Indic number
   BidiClassES,   // European separator, binds two European numbers
@@ -126,17 +127,24 @@ static BidiClass prv_class(Codepoint cp) {
     if (cp == 0x0609 || cp == 0x060A || cp == 0x066A) {  // Per mille, per ten thousand, percent
       return BidiClassET;
     }
-    return BidiClassR;
+    return BidiClassAL;
   }
   if (cp >= 0x0700 && cp <= 0x08FF) {  // Syriac, Thaana, NKo, Arabic Extended-A/B
     // These blocks carry Arabic number signs among their letters.
     if (cp == 0x0890 || cp == 0x0891 || cp == 0x08E2) {
       return BidiClassAN;
     }
-    return BidiClassR;
+    // NKo, Samaritan and Mandaic are R; the rest are Arabic-script letters.
+    if (cp >= 0x07C0 && cp <= 0x085F) {
+      return BidiClassR;
+    }
+    return BidiClassAL;
   }
-  if ((cp >= 0xFB1D && cp <= 0xFDFF) || (cp >= 0xFE70 && cp <= 0xFEFC)) {
-    return BidiClassR;  // Hebrew and Arabic presentation forms
+  if (cp >= 0xFB1D && cp <= 0xFB4F) {
+    return BidiClassR;  // Hebrew presentation forms
+  }
+  if ((cp >= 0xFB50 && cp <= 0xFDFF) || (cp >= 0xFE70 && cp <= 0xFEFC)) {
+    return BidiClassAL;  // Arabic presentation forms
   }
   switch (cp) {
     case 0x00A0:  // No-break space
@@ -185,18 +193,20 @@ static BidiClass prv_class_at(const utf8_t *pos, const utf8_t *end, utf8_t **nex
 //! Direction a class contributes when a neutral looks at it (N1). Numbers act
 //! as right-to-left for this purpose even though they are laid out the other way.
 static bool prv_side_is_rtl(BidiClass cls) {
-  return (cls == BidiClassR) || (cls == BidiClassEN) || (cls == BidiClassAN);
+  return (cls == BidiClassR) || (cls == BidiClassAL) || (cls == BidiClassEN) ||
+         (cls == BidiClassAN);
 }
 
 static bool prv_class_has_side(BidiClass cls) {
   return (cls == BidiClassL) || prv_side_is_rtl(cls);
 }
 
-//! W7: a European number takes the direction of the strong character before it.
-//! Returns true when that character is left-to-right, so the number stops acting
-//! as right-to-left towards the neutrals around it.
-static bool prv_number_follows_ltr(const utf8_t *line_start, const utf8_t *pos,
-                                   const utf8_t *end, bool para_is_rtl) {
+//! Strong class before @p pos (L, R or AL), or the class the start of the
+//! paragraph stands for when nothing strong precedes it. Feeds W2, which turns
+//! a European number after an Arabic letter into an Arabic number, and W7,
+//! which gives one after Latin text that text's direction.
+static BidiClass prv_prev_strong(const utf8_t *line_start, const utf8_t *pos,
+                                 const utf8_t *end, bool para_is_rtl) {
   utf8_t *cur = (utf8_t *)pos;
   while (cur > line_start) {
     cur = utf8_get_previous((utf8_t *)line_start, cur);
@@ -208,15 +218,29 @@ static bool prv_number_follows_ltr(const utf8_t *line_start, const utf8_t *pos,
     if (next == NULL) {
       break;
     }
-    if (cls == BidiClassL) {
-      return true;
+    if ((cls == BidiClassL) || (cls == BidiClassR) || (cls == BidiClassAL)) {
+      return cls;
     }
-    if ((cls == BidiClassR) || (cls == BidiClassB)) {
-      return false;
+    if (cls == BidiClassB) {
+      break;
     }
   }
-  // Nothing strong precedes it, so the paragraph direction decides.
-  return !para_is_rtl;
+  return para_is_rtl ? BidiClassR : BidiClassL;
+}
+
+//! W7: true when the strong character before a European number is left-to-right,
+//! so the number stops acting as right-to-left towards the neutrals around it.
+static bool prv_number_follows_ltr(const utf8_t *line_start, const utf8_t *pos,
+                                   const utf8_t *end, bool para_is_rtl) {
+  return prv_prev_strong(line_start, pos, end, para_is_rtl) == BidiClassL;
+}
+
+//! W2: true when the strong character before a European number is an Arabic
+//! letter, which makes it an Arabic number: separators still bind (W4) but
+//! terminators (W5) and the plus and minus signs no longer do.
+static bool prv_number_follows_arabic(const utf8_t *line_start, const utf8_t *pos,
+                                      const utf8_t *end, bool para_is_rtl) {
+  return prv_prev_strong(line_start, pos, end, para_is_rtl) == BidiClassAL;
 }
 
 //! Direction the class at @p pos contributes to a neighbouring neutral, with W7
@@ -283,30 +307,43 @@ static utf8_t *prv_skip_terminators(utf8_t *pos, const utf8_t *end) {
 }
 
 //! Consume a number along with the separators and terminators that bind to it
-//! (UAX 9 W4-W6). @p pos must point at a number of class @p num_cls.
-static utf8_t *prv_scan_number(utf8_t *pos, const utf8_t *end, BidiClass num_cls) {
+//! (UAX 9 W4-W6). @p pos must point at a digit of class @p num_cls. After an
+//! Arabic letter (@p after_arabic) W2 makes European digits Arabic numbers, so
+//! both kinds of digit join and the Arabic-number rules apply.
+static utf8_t *prv_scan_number(utf8_t *pos, const utf8_t *end, BidiClass num_cls,
+                               bool after_arabic) {
+  const bool european = (num_cls == BidiClassEN) && !after_arabic;
   utf8_t *cur = pos;
+  bool after_digit = true;
   while (cur < end && *cur != '\0') {
     utf8_t *next = NULL;
     BidiClass cls = prv_class_at(cur, end, &next);
     if (next == NULL) {
       break;
     }
-    if (cls == num_cls || cls == BidiClassNSM) {
+    const bool is_digit = (cls == num_cls) ||
+        (after_arabic && ((cls == BidiClassEN) || (cls == BidiClassAN)));
+    if (is_digit || cls == BidiClassNSM) {
+      after_digit = true;
       cur = next;
       continue;
     }
-    // W4: a separator surrounded by numbers of the same class joins them.
-    if ((cls == BidiClassCS) || (cls == BidiClassES && num_cls == BidiClassEN)) {
+    // W4: a separator surrounded by numbers of the same class joins them. W4
+    // runs before W5, so a separator that follows a terminator does not.
+    if ((cls == BidiClassCS) || (cls == BidiClassES && european)) {
       utf8_t *after = NULL;
-      if (prv_class_at(next, end, &after) == num_cls && after != NULL) {
+      const BidiClass after_cls = prv_class_at(next, end, &after);
+      const bool after_is_digit = (after_cls == num_cls) ||
+          (after_arabic && ((after_cls == BidiClassEN) || (after_cls == BidiClassAN)));
+      if (after_digit && after_is_digit && after != NULL) {
         cur = after;
         continue;
       }
       break;
     }
     // W5: terminators next to a European number join it.
-    if (cls == BidiClassET && num_cls == BidiClassEN) {
+    if (cls == BidiClassET && european) {
+      after_digit = false;
       cur = next;
       continue;
     }
@@ -369,21 +406,24 @@ static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t
       *span_end = next;
       return true;
     case BidiClassR:
+    case BidiClassAL:
       *span_level = 1;
       *span_end = next;
       return true;
     case BidiClassAN:
       // Arabic numbers sit one level inside their surroundings either way.
       *span_level = 2;
-      *span_end = prv_scan_number(pos, end, cls);
+      *span_end = prv_scan_number(pos, end, cls,
+                                  prv_number_follows_arabic(line_start, pos, end, para_is_rtl));
       return true;
-    case BidiClassEN:
+    case BidiClassEN: {
       // W7 already turned a number after Latin text into L, so it belongs at
       // the paragraph's own level rather than nested inside an RTL region.
-      *span_level = prv_number_follows_ltr(line_start, pos, end, para_is_rtl) ?
-          prv_level_for_dir(false, para_is_rtl) : 2;
-      *span_end = prv_scan_number(pos, end, cls);
+      const BidiClass strong = prv_prev_strong(line_start, pos, end, para_is_rtl);
+      *span_level = (strong == BidiClassL) ? prv_level_for_dir(false, para_is_rtl) : 2;
+      *span_end = prv_scan_number(pos, end, cls, strong == BidiClassAL);
       return true;
+    }
     case BidiClassB:
       // A paragraph separator stands on its own at the paragraph direction.
       *span_level = prv_level_for_dir(para_is_rtl, para_is_rtl);
@@ -393,14 +433,16 @@ static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t
       break;
   }
 
-  // A terminator run directly ahead of a European number belongs to it (W5).
+  // A terminator run directly ahead of a European number belongs to it (W5),
+  // unless W2 made that number Arabic.
   if (cls == BidiClassET) {
     utf8_t *number = prv_skip_terminators(pos, end);
     utf8_t *after = NULL;
-    if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL) {
+    if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL &&
+        !prv_number_follows_arabic(line_start, number, end, para_is_rtl)) {
       *span_level = prv_number_follows_ltr(line_start, number, end, para_is_rtl) ?
           prv_level_for_dir(false, para_is_rtl) : 2;
-      *span_end = prv_scan_number(number, end, BidiClassEN);
+      *span_end = prv_scan_number(number, end, BidiClassEN, false);
       return true;
     }
   }
@@ -417,9 +459,12 @@ static bool prv_resolve_span(const utf8_t *line_start, utf8_t *pos, const utf8_t
     if (stretch_cls == BidiClassET) {
       utf8_t *number = prv_skip_terminators(stretch_end, end);
       utf8_t *after = NULL;
-      if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL) {
+      if (prv_class_at(number, end, &after) == BidiClassEN && after != NULL &&
+          !prv_number_follows_arabic(line_start, number, end, para_is_rtl)) {
         break;
       }
+      // The whole terminator run is neutral, so step over it at once.
+      stretch_next = number;
     }
     stretch_end = stretch_next;
   }
@@ -494,7 +539,7 @@ bool bidi_paragraph_is_rtl(const utf8_t *start, const utf8_t *end) {
     if (cls == BidiClassL) {
       return false;
     }
-    if (cls == BidiClassR) {
+    if ((cls == BidiClassR) || (cls == BidiClassAL)) {
       return true;
     }
     ptr = next;
diff --git a/tests/fw/test_bidi.c b/tests/fw/test_bidi.c
index 786ebf507..6bf2a00fe 100644
--- a/tests/fw/test_bidi.c
+++ b/tests/fw/test_bidi.c
@@ -252,12 +252,56 @@ void test_bidi__number_keeps_its_separator(void) {
 }
 
 void test_bidi__number_keeps_its_terminator(void) {
+  // Hebrew is R rather than AL, so the digits stay European and W5 binds the
+  // percent sign to them.
   static const ExpectedRun expected[] = {
     { 0, 3, true },
     { 3, 3, false },  // "50%"
     { 6, 3, true },
   };
-  prv_assert_runs("م 50% م", expected, 3);
+  prv_assert_runs("ם 50% ם", expected, 3);
+}
+
+void test_bidi__number_after_arabic_letter_is_arabic(void) {
+  // W2: after an Arabic letter a European number becomes an Arabic number, so
+  // a terminator (W5) or the plus and minus signs (W4) no longer bind to it and
+  // "50%" is drawn as "%50", the way the phone shows it.
+  static const ExpectedRun percent[] = {
+    { 0, 3, true },
+    { 3, 2, false },  // "50"
+    { 5, 4, true },   // "% م"
+  };
+  prv_assert_runs("م 50% م", percent, 3);
+
+  static const ExpectedRun range[] = {
+    { 0, 3, true },
+    { 3, 2, false },  // "10"
+    { 5, 1, true },   // "-"
+    { 6, 2, false },  // "12"
+    { 8, 3, true },
+  };
+  prv_assert_runs("م 10-12 م", range, 5);
+
+  // Common separators still bind (W4), and so do Arabic-Indic digits.
+  static const ExpectedRun joined[] = {
+    { 0, 3, true },
+    { 3, 7, false },  // "12:٣٠"
+    { 10, 3, true },
+  };
+  prv_assert_runs("م 12:٣٠ م", joined, 3);
+}
+
+void test_bidi__separator_after_terminator_does_not_bind(void) {
+  // W4 runs before W5, so a separator that follows a terminator is a neutral
+  // even though the terminator itself joins the number.
+  static const ExpectedRun expected[] = {
+    { 0, 3, true },
+    { 3, 3, false },  // "50%"
+    { 6, 1, true },   // "-"
+    { 7, 3, false },  // "60%"
+    { 10, 3, true },
+  };
+  prv_assert_runs("ם 50%-60% ם", expected, 5);
 }
 
 void test_bidi__trailing_emoji_joins_the_arabic_run(void) {

Nit

bidi.h still says "W4-W6" and "L2 (run reordering, done by the caller)"; stale against what is implemented now.

stretch_end = next;
}

// N1: neutrals between two runs of the same direction take that direction.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where the John (Work) case goes wrong: ( is between L and L and resolves L, ) is between L and R and takes the paragraph direction, so the pair splits. See finding 1 in the review body.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, exactly as you describe — ( J o h n _ ( W o r k …. The claim that N1/N2 covered this in practice was wrong and is gone from the PR. Deferred to the follow-up with the resolver rewrite, per your note on the perf thread.

Comment thread src/fw/applib/graphics/bidi.c Outdated
if (cp == 0x0609 || cp == 0x060A || cp == 0x066A) { // Per mille, per ten thousand, percent
return BidiClassET;
}
return BidiClassR;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arabic letters are AL in UAX 9, not R. The only rule that tells them apart is W2 (a European number after AL becomes AN), but that is the rule that puts %, $, #, ° and +/- on the correct side of a number in Arabic text. The attached patch adds BidiClassAL here and in the 0x0700-0x08FF and presentation-form ranges.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied as-is. AL now covers U+0600, Arabic Extended, and the Arabic presentation forms; Hebrew, NKo, Samaritan and Mandaic stay R. Verified الحرارة 25%%25 and שלום 25% unchanged.

Comment thread src/fw/applib/graphics/bidi.c Outdated
cur = next;
continue;
}
// W4: a separator surrounded by numbers of the same class joins them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W4 runs before W5, so a separator that follows a terminator does not see a number on its left: 50%-60% is EN EN ON EN EN, not one number. The patch tracks after_digit here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In via the patch. Verified ם 50%-60% ם splits into two numbers with a neutral - between them.

RTL rendering reverses the codepoints of each right-to-left run but never
mirrors the glyphs that call for it, so a bracket keeps facing the same
way after the reversal and "(مرحبا)" draws as ")مرحبا(". Neutral
characters are attached to whichever run happens to be open rather than
resolved against their neighbours, so the brackets around an embedded
Latin word can land in different runs and be treated inconsistently. A
combining mark is reversed away from the letter it attaches to.

Replace rtl_support.c with bidi.c, a subset of Unicode UAX 9 sized for a
single line of watch text: P2/P3 for the paragraph direction, W1 so a
mark takes the class of the character it follows, W4-W7 so a number keeps
the separators and terminators that belong to it, N1/N2 for neutrals,
I1/I2 and L2 so runs carry an embedding level and reorder by it, and L4
for mirrored glyphs. Arabic letters are AL, so a European number after
one becomes an Arabic number (W2) and terminators and signs no longer bind
to it, matching what the phone renders. Bracket pairs (N0), explicit
directional controls, isolates and embedding levels above two are out of
scope; N0 and a linear neutral resolver are tracked as a follow-up.

The weak-LTR digit and numeric separator behaviour already in
rtl_support.c is preserved, but moves from a special case inside the
reversal into the run classification: European and Arabic-Indic numbers
resolve to their own left-to-right run, which is the level UAX 9 assigns
them in either paragraph direction. Resolving neutrals also removes the
need to peel trailing spaces into their own segment, so
rtl_segment_content_end() goes away with it.

bidi_is_needed() gates the whole path on a raw byte scan covering every
block the classifier calls strong right-to-left, so the gate and the class
table agree and nothing is right-aligned while still being drawn in
logical order. Text with none of those bytes costs one comparison per byte
and keeps taking the existing left-to-right path, unchanged.

Runs carry an embedding level rather than a direction bit, because a
number following a right-to-left run sits one level deeper than the text
around it: without the level, "Total <arabic> 123" cannot be told apart
from ordinary Latin and L2 leaves the digits on the wrong side of the
Arabic. Paragraph direction is resolved from the start of the paragraph
rather than the start of the visual line, so a wrapped paragraph does not
change base direction part way down.

The run splitter in text_layout.c collapses into one bidi_next_run() call
and mirroring is applied in both the width pass and the draw pass so the
two agree. Lam-alef ligature shaping and the segment cap are untouched.

test_rtl_support.c is replaced by test_bidi.c, which keeps its digit and
separator coverage expressed as run boundaries rather than as reversal
output.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Khalid Nuaim <i@kalua.im>
@kaluaim
kaluaim force-pushed the minimal-bidi-engine branch from 63cfe7e to 61c55c3 Compare September 17, 2026 06:50
@kaluaim

kaluaim commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Patch applied verbatim on top of the previous head, then rebased onto current main and run through clang-format 22. All 34 tests pass, and your three W2 lines now place the terminator on the spec side: after الحرارة the % sits to the left of 25, after من the range 10-12 reverses to 12-10 with the minus as a neutral, and after الرصيد the $ sits to the right of 50. Hebrew is unchanged.

N0 goes to the follow-up, paired with the single-pass resolver as you suggest — the claim is removed from the description and the commit message, and the John (Work) shape is called out there as a known limitation until then.

The differential rig against python-bidi and BidiCharacterTest.txt is a stronger check than anything I ran on this; thank you for it.

@gmarull gmarull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final pass on 61c55c3. Approving.

Verified on this head:

  • bidi.c and test_bidi.c are byte-identical to the previous head plus the W2/W4-W5 patch once both sides go through clang-format 22, and every other file in the PR is unchanged apart from what the rebase picked up from main (the interdiff is empty). Formatting is clean and it still merges cleanly onto current main.
  • Differential run against python-bidi / unicode-bidi, 40k random lines per alphabet: 0 mismatches on Arabic, Hebrew, extended digits, separators, terminators and neutrals. The only remaining deviations are bracket pairs, which is the N0 follow-up called out in the description; BidiCharacterTest.txt is 77081/91588 with every failure in that bucket.
  • The three W2 shapes render the spec way (%25 ةرارحلا, ءاسم 12-10 نم, 50$ ديصرلا) and ם 50%-60% ם splits at the -. Hebrew output is unchanged.
  • ASan/UBSan clean on 60k corrupted-UTF-8 inputs through bidi_next_run() / bidi_reverse_run() / bidi_mirror_codepoint().
  • cortex-m4 -Os: 2223 B → 2347 B, the +124 B the description states.
  • Full unit test suite on this head: 333/333 pass.

The bidi.h summary now matches what is implemented, and the description and commit message no longer claim N0. The quadratic a 1 1 1 … مرحبا case is unchanged (36 us at 122 B) and goes to the follow-up with N0 as agreed.

@gmarull
gmarull merged commit dda08dd into coredevices:main Sep 17, 2026
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants