From 4b6f307ebbc777426a8bf1704de6eb11986ce683 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Wed, 14 Oct 2026 15:19:02 -0400 Subject: [PATCH] Fix up #169: auto RTL lives only in the engine and right-aligns only unaligned text Drop the layout-time direction detection in crates/text (and its test); the engine's promote_rtl_if_needed sets align Right only when neither the paragraph nor what it inherits (list styles, layout, master) resolves an alignment. right_to_left_paragraphs types its original Arabic text again, after an explicit LTR so the toggle it tests starts from left-to-right. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/text.rs | 64 ++++++++++++++++++++++++++++++++--- crates/engine/src/tests.rs | 22 +++++++++--- crates/text/src/lib.rs | 7 ++-- crates/text/src/tests.rs | 13 ------- 4 files changed, 78 insertions(+), 28 deletions(-) diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 0d43c62f..b2c924a3 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -305,6 +305,7 @@ fn typed(s: &Session, body: &TextBody, at: Pos, text: &str) -> String { } pub(crate) fn insert_text(s: &mut Session, text: &str, props: Option) -> Result { + let inherited = inherited_aligns_for_edit(s); let text = { let st = s.doc()?; match &st.selection.text { @@ -324,7 +325,7 @@ pub(crate) fn insert_text(s: &mut Session, text: &str, props: Option) t.caret = c; for pi in at.0..=c.0 { if let Some(para) = body.paragraphs.get_mut(pi) { - promote_rtl_if_needed(para); + promote_rtl_if_needed(para, &inherited); } } c @@ -333,10 +334,37 @@ pub(crate) fn insert_text(s: &mut Session, text: &str, props: Option) Ok(json!({"caret": [pos.0, pos.1]})) } -fn promote_rtl_if_needed(para: &mut deckcraft_model::text::Paragraph) { +/// Per outline level (0..=8), whether shape `id` on the current slide already resolves an +/// alignment for its paragraphs (its own list style, the layout's or the master's). All `false` +/// when the shape can't be resolved (notes, a shape that is gone). +fn inherited_aligns(s: &Session, id: ShapeId) -> [bool; 9] { + let mut out = [false; 9]; + let Ok(st) = s.doc() else { return out }; + let Some(sh) = st.shape(id) else { return out }; + let Some(ctx) = ctx_for(&st.doc, &st.selection) else { return out }; + for (lvl, set) in out.iter_mut().enumerate() { + let para = deckcraft_model::text::Paragraph { level: lvl as u8, ..Default::default() }; + *set = resolve::para(&ctx, sh, ¶).align.is_some(); + } + out +} + +/// The aligns [`inherited_aligns`] gives for the text being edited (none for notes). +fn inherited_aligns_for_edit(s: &Session) -> [bool; 9] { + match s.doc().ok().and_then(|st| st.selection.text.clone()) { + Some(t) if !t.notes => inherited_aligns(s, t.shape), + _ => [false; 9], + } +} + +/// A paragraph with no direction of its own whose first strong character is right-to-left +/// (Arabic, Hebrew, Kurdish…) becomes right-to-left, and right-aligned when nothing (the +/// paragraph or what it inherits, `inherited`) sets its alignment. +fn promote_rtl_if_needed(para: &mut deckcraft_model::text::Paragraph, inherited: &[bool; 9]) { if para.props.rtl.is_none() && deckcraft_model::bidi::first_strong_is_rtl(¶.text()) { para.props.rtl = Some(true); - if para.props.align.is_none() { + let inherits_align = inherited.get(usize::from(para.level)).copied().unwrap_or(false); + if para.props.align.is_none() && !inherits_align { para.props.align = Some(Align::Right); } } @@ -612,6 +640,7 @@ fn autocorrect_options(s: &mut Session, p: &Value) -> Result { } pub(crate) fn paste_body(s: &mut Session, src: &TextBody) -> Result { + let inherited = inherited_aligns_for_edit(s); let pos = edit_text(s, |body, t| { let (a, b) = t.ordered(); let at = ed::delete(body, a, b); @@ -620,7 +649,7 @@ pub(crate) fn paste_body(s: &mut Session, src: &TextBody) -> Result { t.caret = c; for pi in at.0..=c.0 { if let Some(para) = body.paragraphs.get_mut(pi) { - promote_rtl_if_needed(para); + promote_rtl_if_needed(para, &inherited); } } c @@ -1056,6 +1085,7 @@ fn set(s: &mut Session, p: &Value) -> Result { None => s.doc()?.selection.shapes.first().copied().ok_or_else(|| bad("text.set", "missing `id` (or select a shape)"))?, }; let cell = p.get("cell").and_then(Value::as_array).and_then(|a| Some((a.first()?.as_u64()? as usize, a.get(1)?.as_u64()? as usize))); + let inherited = inherited_aligns(s, id); edit_shapes(s, &json!({"id": id}), "text.set", |sh| { let target = match (&mut sh.kind, cell) { (ShapeKind::Table(t), Some((r, c))) => t.cell_mut(r, c).map(|x| &mut x.text), @@ -1074,7 +1104,7 @@ fn set(s: &mut Session, p: &Value) -> Result { para.level = *lv; para.props = pp.clone(); } - promote_rtl_if_needed(para); + promote_rtl_if_needed(para, &inherited); for r in &mut para.runs { r.props = keep.clone(); } @@ -1284,6 +1314,30 @@ fn replace_font(s: &mut Session, p: &Value) -> Result { Ok(json!({"replaced": n})) } +#[cfg(test)] +mod rtl_promote_tests { + use deckcraft_model::text::{Align, Paragraph, Run}; + + use super::promote_rtl_if_needed; + + fn para(text: &str) -> Paragraph { + Paragraph { runs: vec![Run::new(text)], ..Default::default() } + } + + #[test] + fn right_aligns_only_when_no_alignment_is_set_or_inherited() { + let mut p = para("مرحبا"); + promote_rtl_if_needed(&mut p, &[false; 9]); + assert_eq!((p.props.rtl, p.props.align), (Some(true), Some(Align::Right))); + let mut p = para("مرحبا"); + promote_rtl_if_needed(&mut p, &[true; 9]); + assert_eq!((p.props.rtl, p.props.align), (Some(true), None)); + let mut p = para("Hello مرحبا"); + promote_rtl_if_needed(&mut p, &[false; 9]); + assert_eq!((p.props.rtl, p.props.align), (None, None)); + } +} + #[cfg(test)] mod replace_font_tests { use serde_json::json; diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index e62ce1a8..663324aa 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -488,7 +488,9 @@ fn right_to_left_paragraphs() { let mut s = session(); let title = s.doc().unwrap().current_slide().unwrap().shapes[0].id.0; s.execute("text.edit", &json!({"id": title})).unwrap(); - s.execute("text.insert", &json!({"text": "Year 2024 (Retrait) only"})).unwrap(); + // Explicitly left-to-right first, so typing Arabic doesn't already turn it right-to-left (#169). + s.execute("format.rtl", &json!({"on": false})).unwrap(); + s.execute("text.insert", &json!({"text": "سنة 2024 (Retrait) فقط"})).unwrap(); s.execute("format.rtl", &json!({})).unwrap(); let para = |s: &Session| s.doc().unwrap().current_slide().unwrap().shapes[0].text.as_ref().unwrap().paragraphs[0].props.clone(); assert_eq!(para(&s).rtl, Some(true)); @@ -530,16 +532,26 @@ fn typing_kurdish_or_arabic_auto_detects_rtl_and_advances_caret_smoothly() { } last_x = cx; - // Caret must remain near the right margin (e.g. > 400.0 pt in title placeholder) - // and must NOT jump back to the left margin (e.g. ~7.2 pt). - assert!(cx > 400.0, "caret should not jump to left margin: {cx}"); + // The title is centred: the caret must NOT jump back to the left margin (~7.2 pt). + assert!(cx > 100.0, "caret should not jump to left margin: {cx}"); } // Caret advances leftwards overall as text is appended assert!(last_x < first_x.unwrap(), "caret should advance leftwards: last={last_x} first={:?}", first_x); let para = s.doc().unwrap().current_slide().unwrap().shapes[0].text.as_ref().unwrap().paragraphs[0].props.clone(); assert_eq!(para.rtl, Some(true), "auto-detected as RTL"); - assert_eq!(para.align, Some(deckcraft_model::text::Align::Right), "aligned right"); + assert_eq!(para.align, None, "the title keeps the alignment it inherits"); + + // A text box inherits the deck's default (left) alignment, which it keeps too. + s.execute("text.exit", &json!({})).ok(); + let id = s.execute("insert.textBox", &json!({"rect": [10, 10, 300, 100], "text": ""})).unwrap()["id"].as_u64().unwrap(); + s.execute("text.edit", &json!({"id": id})).unwrap(); + s.execute("text.insert", &json!({"text": "مرحبا"})).unwrap(); + let st = s.doc().unwrap(); + let sh = st.shape(deckcraft_model::ShapeId(id as u32)).unwrap(); + let para = sh.text.as_ref().unwrap().paragraphs[0].props.clone(); + assert_eq!(para.rtl, Some(true)); + assert_eq!(para.align, None, "the text box keeps the alignment it inherits"); } #[test] diff --git a/crates/text/src/lib.rs b/crates/text/src/lib.rs index 70c58a29..b9e28fb1 100644 --- a/crates/text/src/lib.rs +++ b/crates/text/src/lib.rs @@ -712,11 +712,8 @@ fn shape_para(ctx: &Ctx, shape: &Shape, body: &TextBody, pi: usize, scale: f64, rtl: false, }; }; - let mut props = resolve::para(ctx, shape, para); - let rtl = props.rtl.unwrap_or_else(|| deckcraft_model::bidi::first_strong_is_rtl(¶.text())); - if rtl && para.props.align.is_none() { - props.align = Some(Align::Right); - } + let props = resolve::para(ctx, shape, para); + let rtl = props.rtl.unwrap_or(false); let mut styles: Vec