Type: required lam-alef is not an alternate in fonts that put it in liga (#394)

Highlight Alternate Glyphs compares each glyph with required shaping
alone, which turns liga and calt off. Older Arabic fonts (such as the
legacy Arial of the Microsoft core fonts) put the mandatory lam-alef
ligature in liga instead of rlig, so every lam-alef was highlighted.
A lam + alef cluster (any alef form, marks allowed) is now never an
alternate candidate, whatever feature the font uses.

Adds a test font built in code (lam-alef under liga or rlig) and
host-independent regressions. Also two equivalent one-line repairs
strict Clippy needs on main: a nonminimal_bool in the Path Precision
Tools inspect check and is_none() in a gradient panel test.

crates/engine/src/cmd/addons/pathtools.rs

@@ -355,7 +355,7 @@ fn inspect(s: &mut Session, p: &Value) -> Result<Value> {
     let id = checked_id(s, p.get("id").unwrap_or(&Value::Null), C)?;
     let (x, y) = (number_or(p, "x", f64::NAN, C)?, number_or(p, "y", f64::NAN, C)?);
     let tolerance = number_or(p, "tolerance", 3.0, C)?;
-    if !(x.is_finite() && y.is_finite()) || !(tolerance >= 0.0 && tolerance.is_finite()) {
+    if !(x.is_finite() && y.is_finite() && tolerance >= 0.0 && tolerance.is_finite()) {
         return Err(bad(C, "give `x` and `y` (and a `tolerance` of 0 or more)"));
     }
     let path = s.doc()?.doc.node(id).and_then(|n| n.path_data()).ok_or_else(|| bad(C, "`id` is not a path"))?;

crates/render/src/tests/alternate.rs

@@ -118,7 +118,9 @@ fn arabic_joining_forms_are_not_alternates() {
     assert_eq!(alternates(&t), 0);
 }
 
-/// #394: the lam-alef ligature is required (`rlig`), so it isn't highlighted either.
+/// #394: the lam-alef ligature is required (`rlig`), so it isn't highlighted either. With an
+/// older font that files it under `liga` (as the legacy Arial of Microsoft's core fonts does) it
+/// is still required: [`lam_alef_in_liga_is_not_an_alternate`] checks that without an OS font.
 #[test]
 fn lam_alef_is_not_an_alternate() {
     // Looking up an installed font loads it, which changes the fonts.
@@ -129,6 +131,38 @@ fn lam_alef_is_not_an_alternate() {
     }
 }
 
+/// #394: a font whose lam-alef ligature is in `liga` (older Arabic fonts) or `rlig` gets no
+/// highlight on it: the ligature is required by the characters, not chosen by an optional
+/// feature. Built in code, so it runs whatever fonts the machine has.
+#[test]
+fn lam_alef_in_liga_is_not_an_alternate() {
+    use vectorcraft_text::test_fonts::{lam_alef_family, lam_alef_font};
+    let _adding = crate::tests_fontchange::adding_fonts();
+    let db = vectorcraft_text::FontDb::global();
+    for feature in [b"liga", b"rlig"] {
+        db.add_font(lam_alef_font(feature).unwrap());
+        let style = vectorcraft_doc::text::CharStyle { size: 36.0, font_family: lam_alef_family(feature).into(), ..Default::default() };
+        for s in ["\u{0644}\u{0627}", "\u{0644}\u{0623}", "\u{0628}\u{0644}\u{0627}", "\u{0644}\u{064E}\u{0627}"] {
+            let t = text(s, style.clone());
+            // The font's ligature glyph stands for the pair.
+            assert!(glyphs(&t).iter().any(|g| g.1 > 2), "{}: {s} is shaped to one ligature glyph", lam_alef_family(feature));
+            assert_eq!(alternates(&t), 0, "{}: {s}", lam_alef_family(feature));
+        }
+    }
+}
+
+/// Only a lam followed by an alef (marks between or after allowed) is the required lam-alef.
+#[test]
+fn only_lam_alef_clusters_are_the_required_ligature() {
+    for s in ["\u{0644}\u{0627}", "\u{0644}\u{0622}", "\u{0644}\u{0623}", "\u{0644}\u{0625}", "\u{0644}\u{064E}\u{0627}", "\u{0644}\u{0627}\u{064B}"]
+    {
+        assert!(crate::text_geom::is_lam_alef(s), "{s:?}");
+    }
+    for s in ["", "\u{0644}", "\u{0627}\u{0644}", "\u{0644}\u{0628}", "\u{0628}\u{0644}\u{0627}", "\u{0644}\u{0627}\u{0627}", "fi", "\u{064E}"] {
+        assert!(!crate::text_geom::is_lam_alef(s), "{s:?}");
+    }
+}
+
 /// #394: an optional feature the characters turn on (a stylistic set, swash, stylistic or
 /// discretionary alternate…) is highlighted on the glyphs it changes, and only on those.
 #[test]

crates/render/src/text_geom.rs

@@ -48,7 +48,8 @@ impl TextGeom {
     /// The boxes of the glyphs of `t` (the text this geometry is of) that optional OpenType features
     /// chose (Preferences › Highlight Alternate Glyphs): candidates the text's required shaping
     /// alone doesn't give ([`required_shaping`]). Arabic letters joining, Indic conjuncts and the
-    /// lam-alef ligature are what the script needs, not alternates. Measured once.
+    /// lam-alef ligature are what the script needs, not alternates (the lam-alef ligature also when
+    /// a font files it under `liga`: [`is_lam_alef`]). Measured once.
     pub(crate) fn alternate_glyphs(&self, t: &TextObject) -> &BezPath {
         self.alternate_glyphs.get_or_init(|| {
             let mut out = BezPath::new();
@@ -84,6 +85,17 @@ pub(crate) fn is_opentype_alternate(nominal_gid: u32, shaped_gid: u32, cluster_c
     shaped_gid != 0 && (cluster_chars > 1 || shaped_gid != nominal_gid)
 }
 
+/// Whether `cluster` is a lam followed by an alef (plain, with madda, or with hamza above or below),
+/// with any Arabic marks between or after them: the lam-alef ligature, which the Unicode
+/// Standard's Arabic chapter treats as obligatory, not a stylistic choice. Fonts made before
+/// `rlig` was common put it in `liga`, which [`required_shaping`] turns off, so it is required
+/// by its characters whatever feature the font files it under.
+pub(crate) fn is_lam_alef(cluster: &str) -> bool {
+    let is_mark = |c: &char| matches!(c, '\u{0610}'..='\u{061A}' | '\u{064B}'..='\u{065F}' | '\u{0670}' | '\u{06D6}'..='\u{06DC}' | '\u{06DF}'..='\u{06E4}' | '\u{06E7}' | '\u{06E8}' | '\u{06EA}'..='\u{06ED}');
+    let mut letters = cluster.chars().filter(|c| !is_mark(c));
+    matches!((letters.next(), letters.next(), letters.next()), (Some('\u{0644}'), Some('\u{0622}' | '\u{0623}' | '\u{0625}' | '\u{0627}'), None))
+}
+
 /// A glyph [`is_opentype_alternate`] marks, in its cluster of the text (`byte`, `len`), with its
 /// box.
 #[derive(Debug)]
@@ -181,7 +193,7 @@ pub(crate) fn text_geom_snapped(t: &TextObject, snap: Option<Affine>) -> TextGeo
         let cluster_chars = cluster.chars().count();
         let Some(first) = cluster.chars().next() else { continue };
         let nominal = db.face_by_id(g.font_id).map(|f| f.glyph_for(first)).unwrap_or(0);
-        if is_opentype_alternate(nominal, g.gid, cluster_chars) {
+        if is_opentype_alternate(nominal, g.gid, cluster_chars) && !is_lam_alef(cluster) {
             alternate_candidates.push(AlternateCandidate { byte: g.byte, len: g.len, font_id: g.font_id, gid: g.gid, cell });
         }
     }

crates/text/src/test_fonts.rs

@@ -343,6 +343,56 @@ pub fn half_form_font(half: bool) -> Option<Vec<u8>> {
     Some(builder.build())
 }
 
+/// The family of [`lam_alef_font`] whose lam-alef ligature is in `feature`.
+pub fn lam_alef_family(feature: &[u8; 4]) -> &'static str {
+    match feature {
+        b"liga" => "Lamtest Liga",
+        b"rlig" => "Lamtest Rlig",
+        _ => "Lamtest Other",
+    }
+}
+
+/// A test-only Arabic cmap over bundled Source Sans 3: lam uses l, alef I, alef with hamza above
+/// J and beh b. The `GSUB` table's `arab` script has one ligature lookup, under `feature`, that
+/// joins lam + alef into H and lam + alef with hamza above into K. Older fonts put the mandatory
+/// lam-alef ligature in `liga` rather than `rlig`; this builds either, without an OS font. `None`
+/// when it can't be built.
+pub fn lam_alef_font(feature: &[u8; 4]) -> Option<Vec<u8>> {
+    use write_fonts::tables::cmap::Cmap;
+    use write_fonts::tables::gsub::{Gsub, Ligature, LigatureSet, LigatureSubstFormat1, SubstitutionLookup, SubstitutionLookupList};
+    use write_fonts::tables::layout::{CoverageFormat1, Feature, FeatureList, FeatureRecord, LangSys, Lookup, Script, ScriptList, ScriptRecord};
+    use write_fonts::types::GlyphId16;
+
+    const BASE: &[u8] = include_bytes!("../../../assets/fonts/SourceSans3-Regular.ttf");
+    let font = skrifa::FontRef::new(BASE).ok()?;
+    let charmap = font.charmap();
+    let mapping = [('\u{0644}', 'l'), ('\u{0627}', 'I'), ('\u{0623}', 'J'), ('\u{0628}', 'b')];
+    let mappings = mapping.into_iter().map(|(c, source)| Some((c, write_fonts::types::GlyphId::new(charmap.map(source)?.to_u32()))));
+    let cmap = Cmap::from_mappings(mappings.collect::<Option<Vec<_>>>()?).ok()?;
+    let family = lam_alef_family(feature);
+    let full = format!("{family} Regular");
+    let postscript = format!("{}-Regular", family.replace(' ', ""));
+    let name = name_table(&[(1, family), (2, "Regular"), (4, &full), (6, &postscript)])?;
+    let mut builder = FontBuilder::new();
+    builder.add_table(&cmap).ok()?;
+    builder.add_raw(Tag::new(b"name"), name);
+    let gid = |c| Some(GlyphId16::new(u16::try_from(charmap.map(c)?.to_u32()).ok()?));
+    let coverage = CoverageFormat1::new(vec![gid('l')?]);
+    let ligatures = vec![Ligature::new(gid('H')?, vec![gid('I')?]), Ligature::new(gid('K')?, vec![gid('J')?])];
+    let subst = LigatureSubstFormat1::new(coverage.into(), vec![LigatureSet::new(ligatures)]);
+    let features = FeatureList::new(vec![FeatureRecord::new(Tag::new(feature), Feature::new(None, vec![0]))]);
+    let lookups = SubstitutionLookupList::new(vec![SubstitutionLookup::Ligature(Lookup::new(Default::default(), vec![subst]))]);
+    let scripts = ScriptList::new(vec![ScriptRecord::new(Tag::new(b"arab"), Script::new(Some(LangSys::new(vec![0])), vec![]))]);
+    builder.add_table(&Gsub::new(scripts, features, lookups)).ok()?;
+    for r in font.table_directory.table_records() {
+        let tag = Tag::new(&r.tag().to_be_bytes());
+        if !builder.contains(tag) {
+            builder.add_raw(tag, font.table_data(r.tag())?.as_bytes());
+        }
+    }
+    Some(builder.build())
+}
+
 /// The family of [`pair_kerning_font`].
 pub const PAIR_KERNING_FAMILY: &str = "Pairtest Sans";
 

crates/ui-egui/src/panels/gradient/tests.rs

@@ -660,7 +660,7 @@ fn the_panel_menu_saves_deletes_and_hides_presets() {
     let mut p = Panel::new();
     let texts = p.frame_of(&mut app, vec![], Modifiers::NONE, menu);
     let save = text_at(&texts, "Save Gradient Preset…").unwrap_or_else(|| panic!("{texts:?}"));
-    assert!(!app.session.active_saved_gradient_preset().is_some());
+    assert!(app.session.active_saved_gradient_preset().is_none());
     p.menu_click(&mut app, save);
     let d = app.ui.dialog.take().expect("the name dialog");
     assert_eq!((d.fields["__command"].as_str(), d.fields["name"].as_str()), (Some("gradient.presets.save"), Some("Gradient Preset 1")));