diff --git a/crates/text/src/harakat.rs b/crates/text/src/harakat.rs index 7e47d3726..b7c4f1060 100644 --- a/crates/text/src/harakat.rs +++ b/crates/text/src/harakat.rs @@ -14,9 +14,8 @@ //! and exporters read the glyph's run as they always did, so they need no change. The text itself //! is never changed. -use skrifa::GlyphId; use skrifa::raw::TableProvider; -use skrifa::raw::tables::gdef::GlyphClassDef; +use skrifa::raw::tables::layout::ClassDef; use vectorcraft_doc::CharStyle; use crate::fontdb::FontFace; @@ -42,19 +41,89 @@ pub(crate) fn is_harakat(c: char) -> bool { /// and change no glyph, advance or position. Stricter than [`crate::shape`]'s `shapes_alike` /// (which also ignores baseline shift, tracking, underline, …). pub(crate) fn paint_only(a: &CharStyle, b: &CharStyle) -> bool { - let mut b = b.clone(); - b.fill = a.fill.clone(); - b.stroke = a.stroke.clone(); - b.stroke_width = a.stroke_width; - b.overprint_fill = a.overprint_fill; - b.overprint_stroke = a.overprint_stroke; - b.stroke_cap = a.stroke_cap; - b.stroke_join = a.stroke_join; - b.stroke_miter_limit = a.stroke_miter_limit; - b.stroke_dash = a.stroke_dash.clone(); - b.stroke_align = a.stroke_align; - b.style_name = a.style_name.clone(); - *a == b + // Exhaustive: a new attribute must explicitly keep or relax this boundary rule. + let CharStyle { + font_family, + font_style, + font_version, + font_axes, + size, + leading, + tracking, + kerning, + optical_kerning, + baseline_shift, + h_scale, + v_scale, + rotation, + underline, + strikethrough, + all_caps, + features, + position, + small_caps, + char_align, + char_direction, + kashidas, + proportional_metrics, + tate_chu_yoko, + tcy_up_down, + tcy_left_right, + standard_vertical_roman, + digits, + font_height_reference, + no_break, + language, + glyph_alternate, + fill, + stroke, + stroke_width, + stroke_miter_limit, + stroke_dash, + overprint_fill: _, + overprint_stroke: _, + stroke_cap: _, + stroke_join: _, + stroke_align: _, + style_name: _, + } = a; + // Previously b's paint was replaced with a's before comparing the whole styles. Keep + // that PartialEq behavior for non-reflexive paint values (NaN), without cloning paint. + let original_paint = (&a.fill, &a.stroke, &a.stroke_width, &a.stroke_miter_limit, &a.stroke_dash); + let normalized_paint = (fill, stroke, stroke_width, stroke_miter_limit, stroke_dash); + original_paint == normalized_paint + && font_family == &b.font_family + && font_style == &b.font_style + && font_version == &b.font_version + && font_axes == &b.font_axes + && size == &b.size + && leading == &b.leading + && tracking == &b.tracking + && kerning == &b.kerning + && optical_kerning == &b.optical_kerning + && baseline_shift == &b.baseline_shift + && h_scale == &b.h_scale + && v_scale == &b.v_scale + && rotation == &b.rotation + && underline == &b.underline + && strikethrough == &b.strikethrough + && all_caps == &b.all_caps + && features == &b.features + && position == &b.position + && small_caps == &b.small_caps + && char_align == &b.char_align + && char_direction == &b.char_direction + && kashidas == &b.kashidas + && proportional_metrics == &b.proportional_metrics + && tate_chu_yoko == &b.tate_chu_yoko + && tcy_up_down == &b.tcy_up_down + && tcy_left_right == &b.tcy_left_right + && standard_vertical_roman == &b.standard_vertical_roman + && digits == &b.digits + && font_height_reference == &b.font_height_reference + && no_break == &b.no_break + && language == &b.language + && glyph_alternate == &b.glyph_alternate } /// Is the run boundary at byte `at` of `text`, between styles `before` and `after`, one that @@ -90,17 +159,15 @@ pub(crate) fn harakat_ranges(text: &str, range: std::ops::Range) -> Vec bool { - let Some(font) = face.skrifa() else { - return false; - }; - let Ok(gdef) = font.gdef() else { - return false; - }; - let Some(Ok(classes)) = gdef.glyph_class_def() else { - return false; - }; - GlyphClassDef::new(classes.get(GlyphId::new(gid))) == GlyphClassDef::Mark + mark_classes(face).as_ref().is_some_and(|classes| classes.get(skrifa::GlyphId::new(gid)) == 3) +} + +/// Borrow the face's mark classification once for a shaping pass. No GDEF, missing classes +/// or malformed tables keep the conservative cluster-start ownership. +pub(crate) fn mark_classes(face: &FontFace) -> Option> { + face.skrifa()?.gdef().ok()?.glyph_class_def()?.ok() } /// Each glyph's own character, from the shaping `by_char` at character cluster level of the diff --git a/crates/text/src/shape.rs b/crates/text/src/shape.rs index 06a14d3df..4ed09b6c0 100644 --- a/crates/text/src/shape.rs +++ b/crates/text/src/shape.rs @@ -1048,8 +1048,11 @@ fn shape_segment(text: &str, context: &Range, seg: &Segment, feats: &OtFe } // Harakat styled apart from their letters (paint only): shaped with them, each glyph then // takes its own character's run ([`crate::harakat`]). - let styled_marks = - boundaries().any(|b| run_at(b - 1).1 != run_at(b).1 && crate::harakat::paint_only_mark_boundary(text, b, run_at(b - 1).1, run_at(b).1)); + let mark_boundaries: Vec = boundaries() + .filter(|&b| run_at(b - 1).1 != run_at(b).1 && crate::harakat::paint_only_mark_boundary(text, b, run_at(b - 1).1, run_at(b).1)) + .collect(); + // Source run starts are sorted. Reuse their classification when checking cluster splits. + let styled_marks = !mark_boundaries.is_empty(); let char_level = std::cell::Cell::new(false); // Shape the segment as if all of it were in `st`, with the features `more` on top. @@ -1064,9 +1067,12 @@ fn shape_segment(text: &str, context: &Range, seg: &Segment, feats: &OtFe let Shaped { raw, proportional, vpal, chars } = &mut shaped; let hb_shaped = face.hb().map(|hb| { let shaper = face.shaper.shaper(&hb).instance(face.instance.as_ref()).build(); + // The baseline and character-level pass use exactly the same resolved features. + // Alternate positioning passes still resolve their own features, with `more` last. + let with_more = |resolved: Vec| resolved.into_iter().chain(more.iter().cloned()).collect::>(); + let along_features = with_more(feats.resolve(st)); // gid, cluster, x and y advance, x and y offset. let shape = |feats: &[Feature], direction: Direction| { - let feats: Vec = feats.iter().chain(more).cloned().collect(); let mut buf = UnicodeBuffer::new(); for (i, c) in text_seg.char_indices() { let cl = (range.start + i) as u32; @@ -1100,8 +1106,8 @@ fn shape_segment(text: &str, context: &Range, seg: &Segment, feats: &OtFe } // A buffer whose characters name no script reads as `UNKNOWN`: its plan has none. let script = Some(buf.script()).filter(|s| *s != harfrust::script::UNKNOWN); - let plan = face.shape_plan(&shaper, buf.direction(), script, language.as_ref(), &feats); - let gb = shaper.shape(buf, ShapeOptions::new().features(&feats).plan(Some(&plan))); + let plan = face.shape_plan(&shaper, buf.direction(), script, language.as_ref(), feats); + let gb = shaper.shape(buf, ShapeOptions::new().features(feats).plan(Some(&plan))); gb.glyph_infos() .iter() .zip(gb.glyph_positions()) @@ -1109,21 +1115,25 @@ fn shape_segment(text: &str, context: &Range, seg: &Segment, feats: &OtFe .collect::>() }; let along = if level.is_rtl() { Direction::RightToLeft } else { Direction::LeftToRight }; - *raw = shape(&feats.resolve(st), along).into_iter().map(|(g, c, xa, _, xo, yo)| (g, c, xa, xo, yo)).collect(); + *raw = shape(&along_features, along).into_iter().map(|(g, c, xa, _, xo, yo)| (g, c, xa, xo, yo)).collect(); let raw = &*raw; let same_glyphs = |v: &[(u32, u32, i32, i32, i32, i32)]| v.len() == raw.len() && v.iter().zip(raw).all(|(a, b)| a.0 == b.0); // The same shaping at character level names each glyph's own character (kept only when // it sets the same glyphs alike, each character in its glyph's cluster). - if styled_marks { + if let Some(classes) = styled_marks.then(|| crate::harakat::mark_classes(face)).flatten() + // Without a mark glyph, no glyph can take a later character's paint. Avoid + // reshaping; the original clusters already give the conservative ownership. + && raw.iter().any(|g| classes.get(skrifa::GlyphId::new(g.0)) == 3) + { char_level.set(true); - let by_char = shape(&feats.resolve(st), along); + let by_char = shape(&along_features, along); char_level.set(false); - *chars = crate::harakat::own_chars(raw, &by_char, range.end, text, |gid| crate::harakat::is_gdef_mark(face, gid)); + *chars = crate::harakat::own_chars(raw, &by_char, range.end, text, |gid| classes.get(skrifa::GlyphId::new(gid)) == 3); } if feats.proportional(st) { // `palt` is positioning only: the same glyphs set on their full widths tell which // advances it changed (kerning and the rest change both alike). - let full = shape(&feats.resolve_fixed_width(st), along); + let full = shape(&with_more(feats.resolve_fixed_width(st)), along); if same_glyphs(&full) { *proportional = full.iter().zip(raw).map(|(a, b)| a.2 != b.2).collect(); } @@ -1134,7 +1144,7 @@ fn shape_segment(text: &str, context: &Range, seg: &Segment, feats: &OtFe // and without either tell how it moves each and changes its advance down the column // (y up, advances negative). Which advances `vpal` itself changed comes from setting // them with it alone. - let ttb = |proportional, kerning| shape(&feats.resolve_vertical(st, proportional, kerning), Direction::TopToBottom); + let ttb = |proportional, kerning| shape(&with_more(feats.resolve_vertical(st, proportional, kerning)), Direction::TopToBottom); let (with, without, alone) = (ttb(true, true), ttb(false, false), ttb(true, false)); if same_glyphs(&with) && same_glyphs(&without) && same_glyphs(&alone) { *vpal = with @@ -1237,7 +1247,7 @@ fn shape_segment(text: &str, context: &Range, seg: &Segment, feats: &OtFe // Except at harakat styled apart only in paint ([`crate::harakat`]). let inside_clusters: Vec = boundaries() .filter(|&b| cluster_starts.binary_search(&b).is_err() && run_at(b - 1).1 != run_at(b).1) - .filter(|&b| !crate::harakat::paint_only_mark_boundary(text, b, run_at(b - 1).1, run_at(b).1)) + .filter(|b| mark_boundaries.binary_search(b).is_err()) .collect(); if !inside_clusters.is_empty() { return shape_apart(inside_clusters, out); diff --git a/crates/text/src/tests/harakat.rs b/crates/text/src/tests/harakat.rs index 63303849c..c097fc48b 100644 --- a/crates/text/src/tests/harakat.rs +++ b/crates/text/src/tests/harakat.rs @@ -152,6 +152,146 @@ fn paint_only_differences_are_told_apart() { assert_eq!(harakat_ranges("اللَّهِ", 0.."اللَّهِ".len()), [6..10, 12..14], "shadda and fatha together"); } +/// The allocation-free comparison must keep the previous comparison's complete contract, +/// including attributes that affect layout rather than shaping and non-reflexive file values. +#[test] +fn paint_only_keeps_every_nonpaint_attribute_and_partial_equality() { + use vectorcraft_doc::{CharAlign, CharPosition, Dash, Digits, FontHeightReference, GlyphChoice, LineCap, LineJoin, ParaDirection, StrokeAlign}; + + fn previous(a: &CharStyle, b: &CharStyle) -> bool { + let mut b = b.clone(); + b.fill = a.fill.clone(); + b.stroke = a.stroke.clone(); + b.stroke_width = a.stroke_width; + b.overprint_fill = a.overprint_fill; + b.overprint_stroke = a.overprint_stroke; + b.stroke_cap = a.stroke_cap; + b.stroke_join = a.stroke_join; + b.stroke_miter_limit = a.stroke_miter_limit; + b.stroke_dash = a.stroke_dash.clone(); + b.stroke_align = a.stroke_align; + b.style_name = a.style_name.clone(); + *a == b + } + + fn check(a: &CharStyle, b: &CharStyle, want: bool, name: &str) { + for (left, right) in [(a, b), (b, a)] { + assert_eq!(paint_only(left, right), previous(left, right), "{name}: old contract"); + } + assert_eq!(paint_only(a, b), want, "{name}"); + } + + let a = CharStyle { + fill: Paint::Pattern { pattern: "named pattern".repeat(8), xf: Affine::IDENTITY }, + stroke: Paint::Solid { color: Color::BLACK, swatch: Some("global swatch".repeat(8)), tint: 0.5 }, + stroke_dash: Some(Dash { pattern: vec![2.0, 3.0, 4.0, 5.0], offset: 0.5, align_corners: true }), + style_name: Some("named character style".repeat(8)), + ..style(FAMILY) + }; + macro_rules! field { + ($field:ident, $value:expr, $want:expr) => {{ + let mut b = a.clone(); + b.$field = $value; + check(&a, &b, $want, stringify!($field)); + }}; + } + field!(font_family, "other family".into(), false); + field!(font_style, "Bold".into(), false); + field!(font_version, Some("other version".into()), false); + field!(font_axes, [("wght".into(), 700.0)].into(), false); + field!(size, 13.0, false); + field!(leading, Some(60.0), false); + field!(tracking, 20.0, false); + field!(kerning, Some(0.0), false); + field!(optical_kerning, true, false); + field!(baseline_shift, 1.0, false); + field!(h_scale, 90.0, false); + field!(v_scale, 90.0, false); + field!(rotation, 3.0, false); + field!(underline, true, false); + field!(strikethrough, true, false); + field!(all_caps, true, false); + field!(features, vec!["-liga".into()], false); + field!(position, CharPosition::Superscript(Default::default()), false); + field!(small_caps, Some(80.0), false); + field!(char_align, CharAlign::EmBoxTop, false); + field!(char_direction, Some(ParaDirection::RightToLeft), false); + field!(kashidas, false, false); + field!(proportional_metrics, true, false); + field!(tate_chu_yoko, true, false); + field!(tcy_up_down, 1.0, false); + field!(tcy_left_right, 1.0, false); + field!(standard_vertical_roman, true, false); + field!(digits, Digits::Hindi, false); + field!(font_height_reference, FontHeightReference::CapHeight, false); + field!(no_break, true, false); + field!(language, Some("ar".into()), false); + field!(glyph_alternate, Some(GlyphChoice { ch: 'a', gid: 1, font: "other".into(), version: String::new() }), false); + field!(fill, Paint::None, true); + field!(stroke, Paint::None, true); + field!(stroke_width, 2.0, true); + field!(overprint_fill, true, true); + field!(overprint_stroke, true, true); + field!(stroke_cap, LineCap::Round, true); + field!(stroke_join, LineJoin::Round, true); + field!(stroke_miter_limit, 2.0, true); + field!(stroke_dash, None, true); + field!(stroke_align, StrokeAlign::Outside, true); + field!(style_name, None, true); + // Only the first style's ignored fields survive the old normalization. A NaN in the + // second style's paint is ignored, but reversing the arguments must still return false. + field!(fill, Paint::solid(Color::Gray { k: f32::NAN }), true); + field!(stroke, Paint::solid(Color::Gray { k: f32::NAN }), true); + field!(stroke_width, f64::NAN, true); + field!(stroke_miter_limit, f64::NAN, true); + field!(stroke_dash, Some(Dash { pattern: vec![f64::NAN], offset: 0.0, align_corners: false }), true); + field!(tracking, f64::NAN, false); +} + +/// A face without usable mark classification keeps cluster-start paint, without changing +/// its glyphs or positioning when a harakat's paint is edited. All variants are in-memory. +#[test] +fn synthetic_harakat_keep_geometry_with_and_without_gdef_marks() { + use skrifa::raw::TableProvider; + use write_fonts::FontBuilder; + use write_fonts::tables::gdef::Gdef; + use write_fonts::tables::layout::ClassDefFormat1; + use write_fonts::types::{GlyphId16, Tag}; + + let bytes = crate::test_fonts::lam_alef_font(b"rlig").unwrap(); + let font = skrifa::FontRef::new(&bytes).unwrap(); + for variant in ["marks", "no GDEF", "no class definition", "base classes"] { + let mut builder = FontBuilder::new(); + for r in font.table_directory.table_records() { + let tag = Tag::new(&r.tag().to_be_bytes()); + if variant == "marks" || tag != Tag::new(b"GDEF") { + builder.add_raw(tag, font.table_data(r.tag()).unwrap().as_bytes()); + } + } + if variant == "no class definition" { + builder.add_table(&Gdef::new(None, None, None, None)).unwrap(); + } else if variant == "base classes" { + let classes = ClassDefFormat1::new(GlyphId16::new(0), vec![1; usize::from(font.maxp().unwrap().num_glyphs())]); + builder.add_table(&Gdef::new(Some(classes.into()), None, None, None)).unwrap(); + } + let db = FontDb::with_font_dirs(vec![]); + assert_eq!(db.add_font(builder.build()), 1, "{variant}"); + let family = crate::test_fonts::lam_alef_family(b"rlig"); + let plain = text_with_harakat("بَبَبَ", family, None); + let coloured = text_with_harakat("بَبَبَ", family, Some(red())); + let before = layout(&db, &plain); + let after = layout(&db, &coloured); + assert!(!before.glyphs.is_empty(), "{variant}"); + assert_eq!(geometry(&before), geometry(&after), "{variant}"); + assert_eq!(coloured.plain_text(), plain.plain_text(), "{variant}"); + if variant == "marks" { + assert!(painted(&coloured, &after, &red()) > 0, "{variant}: harakat own their paint"); + } else { + assert!(after.glyphs.iter().all(|g| run_of(&coloured, g).style.fill != red()), "{variant}: cluster-start paint"); + } + } +} + /// A range is normalised as `edit::style_range` takes one: clamped, each end moved back to the /// start of its character, put in order. Never a silent empty result for a usable range. #[test]