From 864c644222a7b9be2d9cf1bcbb80706f43e775a6 Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Tue, 6 Oct 2026 20:49:01 +0300 Subject: [PATCH] review(#1629): one direction mark, no dead RTL paragraph path, tests follow the new layout - One visible-named LRM constant (ttf_glyph) instead of two invisible literals. Stroke and SDF text skip it, so a right-to-left script the shaper does not handle no longer gets a 6-unit gap per mark; vertical MTEXT does not insert it at all. - Shaped cells keep a cluster the shaper merged into the leading mark. - The always-false RTL paragraph flag, its indent/anchor branches and the unused reorder_line_atoms are gone; its test now covers reorder_rtl_runs. - Underline, overline and strike follow each drawn run of a mixed word. - Justify/distribute count only the gaps the measured width holds. - The two RTL alignment tests assert the new left-to-right behaviour; glyph_boxes keeps its own doc comment. Co-Authored-By: Claude Opus 5.5 --- src/entities/mtext.rs | 7 +- src/entities/text.rs | 10 +- src/entities/text_support.rs | 296 +++++++++-------------------------- src/scene/text/lff.rs | 4 + src/scene/text/ttf_glyph.rs | 24 +-- 5 files changed, 99 insertions(+), 242 deletions(-) diff --git a/src/entities/mtext.rs b/src/entities/mtext.rs index 384d76ad7..2fab97e05 100644 --- a/src/entities/mtext.rs +++ b/src/entities/mtext.rs @@ -79,10 +79,6 @@ fn drawing_dir_str(d: &DrawingDirection) -> &'static str { } } -/// Per-visible-character world-space boxes for the MText editor's -/// click-to-select preview. Uses the exact same layout opts as `to_render` -/// so the boxes line up with the rendered glyphs. - /// The text MTEXT lays out: its stored value, except that the spaces between /// a field's value and a right-to-left word right after it read with that /// word — the reference draws `שלום 0 עולם` with no gap after the field @@ -111,6 +107,9 @@ fn display_text(t: &MText, document: &codec::CadDocument) -> String { out } +/// Per-visible-character world-space boxes for the MText editor's +/// click-to-select preview. Uses the exact same layout opts as `to_render` +/// so the boxes line up with the rendered glyphs. pub fn glyph_boxes(t: &MText, document: &codec::CadDocument) -> Vec { let resolved_style = resolve_text_style(&t.style, document); let attach_h_anchor: f32 = match t.attachment_point { diff --git a/src/entities/text.rs b/src/entities/text.rs index 06c7bedc1..e5957d9d2 100644 --- a/src/entities/text.rs +++ b/src/entities/text.rs @@ -705,9 +705,13 @@ mod tests { let doc = codec::CadDocument::default(); let placement = text_run_placement_at_scale(&t, &doc, 1.0); - // For Arabic text with default HA::Left, origin should be shifted left - // so the right edge of the text sits at the insertion point (X = 10.0). - assert!(placement.origin[0] < 10.0, "Arabic text origin must be to the left of insertion point: origin_x={}", placement.origin[0]); + // Left alignment does not depend on the script: Arabic text starts + // at the insertion point like any other. + assert!( + (placement.origin[0] - 10.0).abs() < 1e-9, + "Left-aligned Arabic text starts at the insertion point: origin_x={}", + placement.origin[0] + ); } } diff --git a/src/entities/text_support.rs b/src/entities/text_support.rs index 06f8f9ec3..1e683f2a8 100644 --- a/src/entities/text_support.rs +++ b/src/entities/text_support.rs @@ -864,12 +864,9 @@ pub fn is_rtl_char(c: char) -> bool { ) } -/// Reorder a line's atoms into visual order (left to right) using the Unicode -/// Bidirectional Algorithm (UBA) Rule L2. If the line contains no RTL characters -/// and is not an RTL paragraph, returns the atoms unchanged. /// Left-to-right mark: keeps the shaper's paragraph direction left to right /// and closes a right-to-left run inside a word. -pub(crate) const LRM: char = '‎'; +pub(crate) use crate::scene::text::ttf_glyph::LRM; /// Whether a word is written right to left as a whole: right-to-left /// letters (and their marks) only. @@ -934,113 +931,6 @@ pub fn reorder_rtl_runs(atoms: Vec) -> Vec { atoms } -pub fn reorder_line_atoms(atoms: Vec, is_rtl: bool) -> Vec { - if atoms.len() <= 1 && !is_rtl { - return atoms; - } - let has_rtl = atoms.iter().any(|atom| match &atom.kind { - AtomKind::Word(w) => w.chars().any(is_rtl_char), - _ => false, - }); - if !has_rtl && !is_rtl { - return atoms; - } - - let mut line_text = String::new(); - let mut ranges = Vec::with_capacity(atoms.len()); - for atom in &atoms { - let start = line_text.len(); - match &atom.kind { - AtomKind::Word(w) => line_text.push_str(w), - AtomKind::Space => line_text.push(' '), - AtomKind::Tab => line_text.push('\t'), - AtomKind::Stack { numerator, denominator, .. } => { - line_text.push_str(numerator); - line_text.push('/'); - line_text.push_str(denominator); - } - } - let end = line_text.len(); - ranges.push(start..end); - } - - let base_level = if is_rtl { - Some(unicode_bidi::Level::rtl()) - } else { - Some(unicode_bidi::Level::ltr()) - }; - let bidi = unicode_bidi::BidiInfo::new(&line_text, base_level); - if bidi.paragraphs.is_empty() { - return atoms; - } - - let atom_levels: Vec = ranges - .iter() - .map(|r| { - let mut lvl = None; - // `bidi.levels` is indexed by byte, so walk characters rather than - // bytes to avoid slicing inside a multi-byte character. - for (offset, c) in line_text[r.clone()].char_indices() { - let i = r.start + offset; - if i < bidi.levels.len() { - match unicode_bidi::bidi_class(c) { - unicode_bidi::BidiClass::L - | unicode_bidi::BidiClass::R - | unicode_bidi::BidiClass::AL => { - lvl = Some(bidi.levels[i].number()); - break; - } - _ => {} - } - } - } - lvl.unwrap_or_else(|| { - if r.start < bidi.levels.len() { - bidi.levels[r.start].number() - } else if is_rtl { - 1 - } else { - 0 - } - }) - }) - .collect(); - - let max_level = atom_levels.iter().copied().max().unwrap_or(0); - let min_odd_level = atom_levels - .iter() - .copied() - .filter(|&l| l % 2 != 0) - .min() - .unwrap_or(1); - - let mut order: Vec = (0..atoms.len()).collect(); - for level in (min_odd_level..=max_level).rev() { - let mut start = 0; - while start < order.len() { - if atom_levels[order[start]] >= level { - let mut end = start + 1; - while end < order.len() && atom_levels[order[end]] >= level { - end += 1; - } - order[start..end].reverse(); - start = end; - } else { - start += 1; - } - } - } - - let mut opt_atoms: Vec> = atoms.into_iter().map(Some).collect(); - let mut reordered = Vec::with_capacity(opt_atoms.len()); - for idx in order { - if let Some(atom) = opt_atoms[idx].take() { - reordered.push(atom); - } - } - reordered -} - pub fn run_scale(state: &RunState, entity_h: f32, base_wf: f32) -> f32 { (state.height_mul * entity_h / 9.0) * (state.width_mul * base_wf.abs()) } @@ -1117,7 +1007,9 @@ fn run_family(face: &Face) -> &str { /// Advance of a stroke-font run (glyph units × `scale`). fn stroke_run_width(face: &Face, text: &str, tracking: f32, scale: f32) -> f32 { + // The direction mark takes no room. text.chars() + .filter(|ch| *ch != LRM) .map(|ch| match face.glyph(ch) { Some(g) => (g.advance + face.spacing_after(ch) * tracking) * scale, None => (6.0 + face.letter_spacing() * tracking) * scale, @@ -1620,6 +1512,14 @@ pub struct MTextLayout { /// (text frame, background fill, low-detail LOD substitutes) around the /// text block. pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { + // Vertical text stacks characters one per cell and has no runs to turn. + let display_word = |word: String| { + if opts.vertical_text { + word + } else { + rtl_display_word(word) + } + }; let base_font_name = opts.style.font_name.clone(); let base_font = Face::resolve(&base_font_name); let base_wf_abs = opts.style.width_factor.max(0.01); @@ -1637,7 +1537,6 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { struct SubLine { atoms: Vec, align: Option, - is_rtl: bool, indent_first: f32, indent_left: f32, indent_right: f32, @@ -1686,7 +1585,7 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { } else if ch == ' ' || ch == '\t' { if !word.is_empty() { atoms.push(LayoutAtom { - kind: AtomKind::Word(rtl_display_word(std::mem::take(&mut word))), + kind: AtomKind::Word(display_word(std::mem::take(&mut word))), state: run.state.clone(), char_offset: word_start, }); @@ -1714,7 +1613,7 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { if let Some(prev) = word.chars().last() { if cjk_break_between(prev, ch) { atoms.push(LayoutAtom { - kind: AtomKind::Word(rtl_display_word(std::mem::take(&mut word))), + kind: AtomKind::Word(display_word(std::mem::take(&mut word))), state: run.state.clone(), char_offset: word_start, }); @@ -1727,7 +1626,7 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { } if !word.is_empty() { atoms.push(LayoutAtom { - kind: AtomKind::Word(rtl_display_word(word)), + kind: AtomKind::Word(display_word(word)), state: run.state.clone(), char_offset: word_start, }); @@ -1780,7 +1679,6 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { // one that starts with Hebrew or Arabic: alignment follows the // attachment, and only right-to-left runs read backwards in place // (`reorder_rtl_runs`). - let is_rtl_para = false; // Wrap to the column the text actually flows down, not to the block: // measuring against the full width would let a line run across the @@ -1815,7 +1713,6 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { sub_lines.push(SubLine { atoms, align: para.align, - is_rtl: is_rtl_para, indent_first: para.indent_first, indent_left: para.indent_left, indent_right: para.indent_right, @@ -1839,7 +1736,6 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { sub_lines.push(SubLine { atoms: Vec::new(), align: None, - is_rtl: false, indent_first: 0.0, indent_left: 0.0, indent_right: 0.0, @@ -2215,9 +2111,7 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { let content_left = if rect_w > 0.0 { box_left - + if sub.is_rtl { - sub.indent_right - } else if sub.is_first_in_paragraph { + + if sub.is_first_in_paragraph { sub.indent_first } else { sub.indent_left @@ -2226,16 +2120,7 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { 0.0 }; let content_right = if rect_w > 0.0 { - box_left + rect_w - - if sub.is_rtl { - if sub.is_first_in_paragraph { - sub.indent_first - } else { - sub.indent_left - } - } else { - sub.indent_right - } + box_left + rect_w - sub.indent_right } else { 0.0 }; @@ -2246,13 +2131,7 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { | Some(ParagraphAlign::Distribute) => 0.0, Some(ParagraphAlign::Center) => 0.5, Some(ParagraphAlign::Right) => 1.0, - None => { - if sub.is_rtl { - 1.0 - } else { - attach_h_anchor - } - } + None => attach_h_anchor, }; let visible_atoms = sub @@ -2308,7 +2187,7 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { xmax: ax, ymin: ay.min(by), ymax: ay.max(by), - is_rtl: sub.is_rtl, + is_rtl: false, local: [line_lx + cursor_start, line_ly, line_lx + cursor_start, line_ly + caret_h], }); vis += 1; @@ -2328,17 +2207,17 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { let distribute = matches!(sub.align, Some(ParagraphAlign::Distribute)); let slack = (content_right - content_left).max(0.0) - line_w; if rect_w > 0.0 && (justify || distribute) && slack > 1e-6 { - let spaces = sub - .atoms + // The gaps the measured width holds: none after the last + // visible atom, and no direction marks. + let spaces = sub.atoms[..visible_atoms] .iter() .filter(|a| matches!(a.kind, AtomKind::Space)) .count(); if distribute { - let chars: usize = sub - .atoms + let chars: usize = sub.atoms[..visible_atoms] .iter() .filter_map(|a| match &a.kind { - AtomKind::Word(t) => Some(t.chars().count()), + AtomKind::Word(t) => Some(t.chars().filter(|c| *c != LRM).count()), _ => None, }) .sum(); @@ -2501,47 +2380,40 @@ pub fn layout_mtext(opts: &MTextRenderOpts) -> MTextLayout { bold: atom.state.bold, }), }); - run_x += run_width(&face, run_text, shaped, tracking, run_scale_x); - } - // Underline / overline / strike are lines, not glyphs; a - // run-group's strokes are suppressed by the SDF path, so - // emit the decorations in their own RUN-LESS group. Reuse - // lff's exact positions by tessellating the decorated word - // and taking the strokes it appends after the glyphs. - if atom.state.underline || atom.state.overline || atom.state.strike { - let glyph_n = lff::tessellate_text_run( - [0.0, 0.0], - run_h, - rot, - signed_wf, - oblique, - tracking, - &font_name, - text, - ) - .0 - .len(); - let body = decorated(text, &atom.state); - let (deco, _) = lff::tessellate_text_run( - [0.0, 0.0], - run_h, - rot, - signed_wf, - oblique, - tracking, - &font_name, - &body, - ); - if deco.len() > glyph_n { - all_strokes.push(TextStroke { - strokes: deco[glyph_n..].to_vec(), - origin, - color, - fill_tris: vec![], - plane: None, - run: None, - }); + // Underline / overline / strike are lines, not glyphs; + // a run-group's strokes are suppressed by the SDF path, + // so emit the decorations in their own RUN-LESS group, + // run by run so they span what is drawn. Reuse lff's + // exact positions by tessellating the decorated run and + // taking the strokes it appends after the glyphs. + if atom.state.underline || atom.state.overline || atom.state.strike { + let tessellate = |body: &str| { + lff::tessellate_text_run( + [0.0, 0.0], + run_h, + rot, + signed_wf, + oblique, + tracking, + &font_name, + body, + ) + .0 + }; + let glyph_n = tessellate(run_text).len(); + let deco = tessellate(&decorated(run_text, &atom.state)); + if deco.len() > glyph_n { + all_strokes.push(TextStroke { + strokes: deco[glyph_n..].to_vec(), + origin: [origin[0] + dx as f64, origin[1] + dy as f64], + color, + fill_tris: vec![], + plane: None, + run: None, + }); + } } + run_x += run_width(&face, run_text, shaped, tracking, run_scale_x); } if opts.want_glyph_boxes { let run_h = atom.state.height_mul * entity_h; @@ -3279,8 +3151,8 @@ mod v_anchor_tests { } #[test] - fn test_bidi_atoms() { - fn reorder_atoms(words: &[&str], is_rtl: bool) -> Vec { + fn right_to_left_runs_read_backwards_in_place() { + fn reorder(words: &[&str]) -> Vec { let atoms: Vec = words .iter() .map(|w| super::LayoutAtom { @@ -3293,8 +3165,7 @@ mod v_anchor_tests { char_offset: 0, }) .collect(); - let reordered = super::reorder_line_atoms(atoms, is_rtl); - reordered + super::reorder_rtl_runs(atoms) .into_iter() .map(|a| match a.kind { super::AtomKind::Word(w) => w, @@ -3304,43 +3175,17 @@ mod v_anchor_tests { .collect() } - // Case 1: Pure Arabic - let words1 = ["بسم", " ", "الله", " ", "الرحمن", " ", "الرحيم"]; - let reordered1 = reorder_atoms(&words1, true); - assert_eq!(reordered1, vec!["الرحيم", " ", "الرحمن", " ", "الله", " ", "بسم"]); - - // Case 2: Arabic with numbers - let words2 = ["بسم", " ", "الله", " ", "123", " ", "الرحمن"]; - let reordered2 = reorder_atoms(&words2, true); - assert_eq!(reordered2, vec!["الرحمن", " ", "123", " ", "الله", " ", "بسم"]); - - // Case 3: English with Arabic - let words3 = ["Hello", " ", "بسم", " ", "الله", " ", "world"]; - let reordered3 = reorder_atoms(&words3, false); - assert_eq!(reordered3, vec!["Hello", " ", "الله", " ", "بسم", " ", "world"]); - - // Case 4: Pure English - let words4 = ["Hello", " ", "world"]; - let reordered4 = reorder_atoms(&words4, false); - assert_eq!(reordered4, vec!["Hello", " ", "world"]); - - // Case 5: Mixed Urdu + Arabic + Hebrew + English - let text_b = "یہ اردو ہے۔ مرحبا! שלום שנה 2026 is here!"; - let words_b: Vec<&str> = text_b.split_inclusive(' ').collect(); - let reordered_b = reorder_atoms(&words_b, true); + // A run of right-to-left words turns round with the spaces after it; + // everything else keeps its order. assert_eq!( - reordered_b, - vec!["2026 ", "is ", "here!", "שנה ", "שלום ", "مرحبا! ", "ہے۔ ", "اردو ", "یہ "] + reorder(&["אבג", " ", "דה", " ", "abc"]), + vec![" ", "דה", " ", "אבג", "abc"] ); - - // Case 6: a word starting with multi-byte neutral punctuation must - // not be sliced inside that character. - let words6 = ["שלום", " ", "«x"]; - let reordered6 = reorder_atoms(&words6, false); - assert_eq!(reordered6, vec!["שלום", " ", "«x"]); + assert_eq!(reorder(&["Hello", " ", "world"]), vec!["Hello", " ", "world"]); + // A word starting with multi-byte punctuation ends the run whole. + assert_eq!(reorder(&["שלום", " ", "«x"]), vec![" ", "שלום", "«x"]); } - #[test] fn cjk_paragraph_wraps_between_ideographs() { use crate::entities::text_support::{layout_mtext, MTextRenderOpts, MTextVAnchor, ResolvedTextStyle}; @@ -3460,10 +3305,11 @@ mod v_anchor_tests { }; let layout_mixed = layout_mtext(&opts_mixed); assert!(!layout_mixed.strokes.is_empty()); - // In a wrapped box of width 200.0, RTL paragraph right-aligns: - // the rightmost stroke group should end near 200.0 - let max_origin = layout_mixed.strokes.iter().map(|s| s.origin[0]).fold(f64::NEG_INFINITY, f64::max); - assert!(max_origin > 100.0, "RTL text in 200-width box should be right-aligned near the right margin (got max_origin={})", max_origin); + // A paragraph starting with right-to-left text still follows the + // attachment: left-anchored in a 200-wide box, it starts at the left. + let min_origin = layout_mixed.strokes.iter().map(|s| s.origin[0]).fold(f64::INFINITY, f64::min); + // (The space after the turned run leads, as the reference shows it.) + assert!(min_origin < 5.0, "a left-anchored paragraph starts at the left margin (got min_origin={min_origin})"); // Test single Arabic word "ميل" let opts_mayl = MTextRenderOpts { diff --git a/src/scene/text/lff.rs b/src/scene/text/lff.rs index a4b422977..dc6cefebe 100644 --- a/src/scene/text/lff.rs +++ b/src/scene/text/lff.rs @@ -338,6 +338,10 @@ pub(crate) fn tokenize_run(text: &str) -> Vec { let mut toks = Vec::new(); let mut chars = text.chars().peekable(); while let Some(ch) = chars.next() { + // A direction mark only steers shaping; it draws nothing. + if ch == crate::scene::text::ttf_glyph::LRM { + continue; + } if ch == '\\' { match chars.peek().copied() { Some('L') => { diff --git a/src/scene/text/ttf_glyph.rs b/src/scene/text/ttf_glyph.rs index a40c85690..33b071c18 100644 --- a/src/scene/text/ttf_glyph.rs +++ b/src/scene/text/ttf_glyph.rs @@ -603,8 +603,9 @@ pub fn char_cells(run: &ShapedRun, text: &str) -> Vec<(f32, f32)> { .collect() } -/// Left-to-right mark shaped ahead of every run. -const BASE_MARK: char = '‎'; +/// Left-to-right mark, shaped ahead of every run so its base direction is +/// left to right; layout also uses it to close a right-to-left run. +pub(crate) const LRM: char = '\u{200E}'; type ShapeCache = HashMap<(String, String), Option>>; @@ -907,7 +908,7 @@ fn build_shaped(_family: &str, text: &str) -> Option { // A leading left-to-right mark keeps the paragraph direction left to // right, as the reference lays text out (right-to-left runs still read // backwards); `cells` stay relative to `text`. - let marked = format!("{BASE_MARK}{text}"); + let marked = format!("{LRM}{text}"); buffer.set_text(&mut font_system, &marked, &attrs, Shaping::Advanced, None); buffer.shape_until_scroll(&mut font_system, false); @@ -917,9 +918,11 @@ fn build_shaped(_family: &str, text: &str) -> Option { for run in buffer.layout_runs() { advance = advance.max(run.line_w * px_to_9); for glyph in run.glyphs.iter() { - if glyph.start >= BASE_MARK.len_utf8() { - let m = BASE_MARK.len_utf8(); - cells.push((glyph.start - m, glyph.end - m, glyph.x * px_to_9, (glyph.x + glyph.w) * px_to_9)); + // A cluster the shaper merged into the mark keeps its own part. + let m = LRM.len_utf8(); + if glyph.end > m { + let start = glyph.start.saturating_sub(m); + cells.push((start, glyph.end - m, glyph.x * px_to_9, (glyph.x + glyph.w) * px_to_9)); } let face_index = font_system .db_mut() @@ -991,7 +994,7 @@ fn build_shaped(family: &str, text: &str) -> Option { // A leading left-to-right mark keeps the paragraph direction left to // right, as the reference lays text out (right-to-left runs still read // backwards); `cells` stay relative to `text`. - let marked = format!("{BASE_MARK}{text}"); + let marked = format!("{LRM}{text}"); buf.set_text(&mut fs, &marked, &attrs, Shaping::Advanced, None); buf.shape_until_scroll(&mut fs, false); @@ -1031,9 +1034,10 @@ fn build_shaped(family: &str, text: &str) -> Option { for run in buf.layout_runs() { advance = advance.max(run.line_w * px_to_9); for g in run.glyphs.iter() { - if g.start >= BASE_MARK.len_utf8() { - let m = BASE_MARK.len_utf8(); - cells.push((g.start - m, g.end - m, g.x * px_to_9, (g.x + g.w) * px_to_9)); + // A cluster the shaper merged into the mark keeps its own part. + let m = LRM.len_utf8(); + if g.end > m { + cells.push((g.start.saturating_sub(m), g.end - m, g.x * px_to_9, (g.x + g.w) * px_to_9)); } let face_index = fs.db_mut().face(g.font_id).map(|f| f.index).unwrap_or(0); let Some(font) = fs.get_font(g.font_id, g.font_weight) else {