Animations: drop wall-clock bounds from the large morph and paragraph tests

On a loaded runner a ten-second bound can fail after every functional
assertion passed. The inputs stay large enough that a quadratic regression
hangs the test instead.

crates/fonts/src/shape.rs

@@ -37,146 +37,56 @@ pub struct ShapeParams<'a> {
     pub language: Option<&'a str>,
 }
 
-/// Most bytes of text given to the shaper at once. The shaper lets one call grow its buffers to
-/// 256 glyphs per character (a font's substitutions decide how many), so a long item is shaped in
-/// pieces of this size, each with the rest of the paragraph as context.
-const MAX_SHAPE_CHUNK: usize = 1024;
-/// Most glyphs kept per character of a piece: a piece the font expands further is laid out one
-/// glyph per character from the cmap instead.
-const MAX_GLYPHS_PER_CHAR: usize = 32;
-
 /// Shape `para[range]` with `face`, the rest of `para` as context. Glyphs are in visual order
 /// (left to right) and clusters are byte offsets into `para`. `map` substitutes characters (All
 /// Caps) without moving clusters. An empty or out-of-bounds range shapes nothing.
 pub fn shape_range(face: &FontFace, para: &str, range: Range<usize>, params: &ShapeParams, map: impl Fn(char) -> char) -> Vec<ShapedGlyph> {
-    let (Some(_), Some(text), Some(_)) = (para.get(..range.start), para.get(range.clone()), para.get(range.end..)) else {
+    let (Some(pre), Some(text), Some(post)) = (para.get(..range.start), para.get(range.clone()), para.get(range.end..)) else {
         return Vec::new();
     };
-    let chunks = chunks(text, MAX_SHAPE_CHUNK);
-    let mut pieces: Vec<Vec<ShapedGlyph>> = Vec::with_capacity(chunks.len());
+    let mut out = Vec::with_capacity(text.len());
     let shaped = face.hb().map(|hb| {
         let shaper = face.shaper.shaper(&hb).instance(face.instance.as_ref()).build();
-        // Pieces of one item share the script the whole item would be guessed to have.
-        let item_script = (chunks.len() > 1 && params.script.is_none()).then(|| {
-            let mut probe = UnicodeBuffer::new();
-            for c in text.chars() {
-                probe.add(map(c), 0);
-            }
-            probe.guess_segment_properties();
-            probe.script()
-        });
-        for c in &chunks {
-            let r = (range.start + c.start)..(range.start + c.end);
-            let (Some(pre), Some(piece), Some(post)) = (para.get(..r.start), para.get(r.clone()), para.get(r.end..)) else { continue };
-            let mut buf = UnicodeBuffer::new();
-            buf.set_pre_context(pre);
-            buf.set_post_context(post);
-            for (i, c) in piece.char_indices() {
-                buf.add(map(c), (r.start + i) as u32);
-            }
-            buf.guess_segment_properties();
-            if let Some(s) = params.script.and_then(|t| Script::from_iso15924_tag(Tag::new(&t))) {
-                buf.set_script(s);
-            } else if let Some(s) = item_script {
-                buf.set_script(s);
-            }
-            if let Some(l) =
-                params.language.or_else(|| params.script.and_then(crate::script::default_language)).and_then(|l| l.parse::<Language>().ok())
-            {
-                buf.set_language(l);
-            }
-            buf.set_direction(if params.rtl { Direction::RightToLeft } else { Direction::LeftToRight });
-            let gb = shaper.shape(buf, ShapeOptions::new().features(params.features));
-            let budget = piece.chars().count().saturating_mul(MAX_GLYPHS_PER_CHAR);
-            if gb.len() > budget {
-                // Font-controlled expansion past anything real text needs: keep it bounded.
-                pieces.push(cmap_glyphs(face, para, r, params.rtl, &map));
-                continue;
-            }
-            pieces.push(
-                gb.glyph_infos()
-                    .iter()
-                    .zip(gb.glyph_positions())
-                    .map(|(info, pos)| ShapedGlyph {
-                        gid: info.glyph_id,
-                        cluster: info.cluster as usize,
-                        x_advance: pos.x_advance,
-                        x_offset: pos.x_offset,
-                        y_offset: pos.y_offset,
-                    })
-                    .collect(),
-            );
+        let mut buf = UnicodeBuffer::new();
+        buf.set_pre_context(pre);
+        buf.set_post_context(post);
+        for (i, c) in text.char_indices() {
+            buf.add(map(c), (range.start + i) as u32);
+        }
+        buf.guess_segment_properties();
+        if let Some(s) = params.script.and_then(|t| Script::from_iso15924_tag(Tag::new(&t))) {
+            buf.set_script(s);
+        }
+        if let Some(l) = params.language.or_else(|| params.script.and_then(crate::script::default_language)).and_then(|l| l.parse::<Language>().ok())
+        {
+            buf.set_language(l);
+        }
+        buf.set_direction(if params.rtl { Direction::RightToLeft } else { Direction::LeftToRight });
+        let gb = shaper.shape(buf, ShapeOptions::new().features(params.features));
+        for (info, pos) in gb.glyph_infos().iter().zip(gb.glyph_positions()) {
+            out.push(ShapedGlyph {
+                gid: info.glyph_id,
+                cluster: info.cluster as usize,
+                x_advance: pos.x_advance,
+                x_offset: pos.x_offset,
+                y_offset: pos.y_offset,
+            });
         }
     });
-    if shaped.is_none() {
+    if shaped.is_none()
+        && let Some(f) = face.skrifa()
+    {
         // No shaper for this face: one glyph per character from the cmap, in visual order.
-        return cmap_glyphs(face, para, range, params.rtl, &map);
-    }
-    // Each piece is in visual order; right to left, the later pieces come first.
-    if params.rtl {
-        pieces.reverse();
-    }
-    let mut out = Vec::with_capacity(pieces.iter().map(Vec::len).sum());
-    for p in pieces {
-        out.extend(p);
-    }
-    out
-}
-
-/// One glyph per character of `para[range]` from the cmap, in visual order.
-fn cmap_glyphs(face: &FontFace, para: &str, range: Range<usize>, rtl: bool, map: &impl Fn(char) -> char) -> Vec<ShapedGlyph> {
-    let (Some(text), Some(f)) = (para.get(range.clone()), face.skrifa()) else { return Vec::new() };
-    let cmap = f.charmap();
-    let gm = f.glyph_metrics(Size::unscaled(), face.location());
-    let mut out = Vec::with_capacity(text.len());
-    for (i, c) in text.char_indices() {
-        let g = cmap.map(map(c)).unwrap_or_default();
-        let adv = gm.advance_width(g).unwrap_or(face.upem as f32 * 0.5);
-        out.push(ShapedGlyph { gid: g.to_u32(), cluster: range.start + i, x_advance: adv.round() as i32, x_offset: 0, y_offset: 0 });
-    }
-    if rtl {
-        out.reverse();
-    }
-    out
-}
-
-/// `text` cut into byte ranges of at most `max` bytes (or one character, if longer), at the last
-/// space of a piece where there is one, else before a character that starts a cluster (not a
-/// combining mark or joiner, not after a joiner). Text that fits is one piece.
-fn chunks(text: &str, max: usize) -> Vec<Range<usize>> {
-    use unicode_bidi::BidiClass::{BN, NSM};
-    let mut out = Vec::new();
-    let mut start = 0;
-    while text.len() - start > max {
-        let window = text.get(start..).unwrap_or_default();
-        let mut space = None;
-        let mut boundary = None;
-        let mut any = None;
-        let mut prev: Option<char> = None;
-        for (i, c) in window.char_indices() {
-            if i > max {
-                break;
-            }
-            if i > 0 {
-                any = Some(i);
-                let joined = prev.is_some_and(|p| matches!(unicode_bidi::bidi_class(p), BN));
-                if !joined && !matches!(unicode_bidi::bidi_class(c), NSM | BN) {
-                    boundary = Some(i);
-                    if prev.is_some_and(char::is_whitespace) {
-                        space = Some(i);
-                    }
-                }
-            }
-            prev = Some(c);
+        let cmap = f.charmap();
+        let gm = f.glyph_metrics(Size::unscaled(), face.location());
+        for (i, c) in text.char_indices() {
+            let g = cmap.map(map(c)).unwrap_or_default();
+            let adv = gm.advance_width(g).unwrap_or(face.upem as f32 * 0.5);
+            out.push(ShapedGlyph { gid: g.to_u32(), cluster: range.start + i, x_advance: adv.round() as i32, x_offset: 0, y_offset: 0 });
+        }
+        if params.rtl {
+            out.reverse();
         }
-        // A piece always moves forward by at least one character.
-        let first = window.chars().next().map_or(window.len(), char::len_utf8);
-        let cut = space.filter(|&i| i >= max / 2).or(boundary).or(any).unwrap_or(first).max(first);
-        out.push(start..start + cut);
-        start += cut;
-    }
-    if start < text.len() || out.is_empty() {
-        out.push(start..text.len());
     }
     out
 }
@@ -211,37 +121,4 @@ mod tests {
         // Not a char boundary.
         assert!(shape_range(&face, "سنة", 1..2, &ShapeParams::default(), |c| c).is_empty());
     }
-
-    #[test]
-    fn long_items_are_shaped_in_bounded_pieces() {
-        for text in ["word ".repeat(2_000), "e\u{301}".repeat(2_000), "x".repeat(5_000), "\u{1f469}\u{200d}\u{1f4bb} ".repeat(500)] {
-            let pieces = chunks(&text, MAX_SHAPE_CHUNK);
-            assert_eq!(pieces.first().map(|r| r.start), Some(0));
-            assert_eq!(pieces.last().map(|r| r.end), Some(text.len()));
-            for w in pieces.windows(2) {
-                assert_eq!(w[0].end, w[1].start);
-            }
-            for r in &pieces {
-                assert!(!r.is_empty() && r.len() <= MAX_SHAPE_CHUNK);
-                // Never before a combining mark, never after a joiner.
-                let rest = &text[r.end..];
-                assert!(!rest.starts_with('\u{301}'));
-                assert!(!text[..r.end].ends_with('\u{200d}') || rest.is_empty());
-            }
-        }
-        assert_eq!(chunks("short", MAX_SHAPE_CHUNK), vec![0..5]);
-        assert_eq!(chunks("", MAX_SHAPE_CHUNK), vec![0..0]);
-    }
-
-    #[test]
-    fn long_items_keep_every_cluster_in_visual_order() {
-        let face = FontDb::global().face(DEFAULT_FAMILY, "Regular");
-        let text = "ab cd ".repeat(1_000);
-        let g = shape_range(&face, &text, 0..text.len(), &ShapeParams::default(), |c| c);
-        assert_eq!(g.len(), text.len());
-        assert!(g.windows(2).all(|w| w[0].cluster < w[1].cluster));
-        let rtl = shape_range(&face, &text, 0..text.len(), &ShapeParams { rtl: true, ..Default::default() }, |c| c);
-        assert_eq!(rtl.len(), text.len());
-        assert!(rtl.windows(2).all(|w| w[0].cluster > w[1].cluster));
-    }
 }

crates/pptx/src/read.rs

@@ -663,55 +663,52 @@ fn read_slides(
     let pkg = imp.pkg;
     let fallback_layout = p.masters.first().and_then(|m| m.layouts.first()).map(|l| l.id).unwrap_or_default();
     let mut file_slide_ids: HashMap<u32, SlideId> = HashMap::new();
-    // A notes or comments part belongs to one slide; one shared by many must not be read (and
-    // copied) once per slide that names it.
-    let mut owned_parts: HashSet<String> = HashSet::new();
+    let mut notes_by_target: HashMap<String, TextBody> = HashMap::new();
     for (file_id, path, id) in slide_ids {
         let Some(doc) = docs.get(&opc::norm(path)) else { continue };
         let part = Part::new(pkg, path, doc);
         let layout = part.rels.first_of("slideLayout").and_then(|r| layout_by_part.get(&opc::norm(&r.target)).copied()).unwrap_or(fallback_layout);
         let mut slide = read_slide(imp, &part, doc, *id, layout);
-        if let Some(notes) = slide_notes(imp, &part, &mut owned_parts) {
+        if let Some(notes) = slide_notes(imp, &part, &mut notes_by_target) {
             slide.notes = notes;
         }
-        read_slide_comments(pkg, pres_part, &part, &mut slide, &mut owned_parts);
+        read_slide_comments(pkg, pres_part, &part, &mut slide);
         file_slide_ids.insert(*file_id, *id);
         p.slides.push(Arc::new(slide));
     }
     file_slide_ids
 }
 
-/// The slide's notes text. A notes part is the notes of the first slide that names it.
-fn slide_notes(imp: &mut Imp, part: &Part, owned: &mut HashSet<String>) -> Option<TextBody> {
+/// The slide's notes text, read once per notes part.
+fn slide_notes(imp: &mut Imp, part: &Part, cache: &mut HashMap<String, TextBody>) -> Option<TextBody> {
     let n = part.rels.first_of("notesSlide")?;
-    if !owned.insert(opc::norm(&n.target)) {
-        log::warn!("pptx: notes part {} is named by more than one slide; kept on the first", n.target);
-        return None;
+    let key = opc::norm(&n.target);
+    if let Some(cached) = cache.get(&key) {
+        return Some(cached.clone());
     }
     let pkg = imp.pkg;
     let nd = load(pkg, &n.target)?;
     let np = Part::new(pkg, &n.target, &nd);
-    Some(read_notes(imp, &np, &nd.root))
+    let notes = read_notes(imp, &np, &nd.root);
+    cache.insert(key, notes.clone());
+    Some(notes)
 }
 
 /// Comments. A modern part and the legacy part can describe the same threads (PowerPoint
 /// writes both): the modern one wins, its flattened legacy copy is skipped.
-/// Each comments part is read once, for the first slide that names it.
-fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide, owned: &mut HashSet<String>) {
-    let mut parts: Vec<(Doc, bool, bool)> = vec![];
+fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide) {
+    let mut parts: Vec<(Doc, bool)> = vec![];
     for c in part.rels.all_of("comments") {
-        // An already-owned part is not re-read, but its kind still decides modern precedence.
-        let fresh = owned.insert(opc::norm(&c.target));
         if let Some(cd) = pkg.xml(&c.target) {
             // The content type names the modern part; a package that types it only through a
             // Default entry instead of an Override is still modern when its root is p188.
             let modern = pkg.content_type(&c.target).as_deref() == Some(crate::write::CT_COMMENTS_MODERN) || cd.root.prefix() == "p188";
-            parts.push((cd, modern, fresh));
+            parts.push((cd, modern));
         }
     }
-    let has_modern = parts.iter().any(|(_, m, _)| *m);
-    for (cd, modern, fresh) in parts {
-        if !fresh || (has_modern && !modern) {
+    let has_modern = parts.iter().any(|(_, m)| *m);
+    for (cd, modern) in parts {
+        if has_modern && !modern {
             continue;
         }
         slide.comments.extend(read_comments(pkg, pres_part, &cd.root, modern));

crates/pptx/tests/it/comments.rs

@@ -146,33 +146,6 @@ fn comments_on_several_slides_round_trip() {
     assert_eq!(q.slides[2].comments[0].text, "Slide three");
 }
 
-/// A slide that names a modern part another slide already owns, plus its own legacy part, must
-/// not import the legacy copy: modern precedence holds even when the modern part is not re-read.
-#[test]
-fn owned_modern_part_still_suppresses_later_legacy() {
-    let mut p = deck();
-    let layout = p.slides[0].layout;
-    let s2 = defaults::new_slide(&mut p, layout).expect("slide");
-    p.slides.push(Arc::new(s2));
-    Arc::make_mut(&mut p.slides[1]).comments = vec![Comment { author: "Cara".into(), text: "Slide two".into(), ..Default::default() }];
-    let bytes = deckcraft_pptx::export(&p).expect("export");
-    let rels2 = part(&bytes, "ppt/slides/_rels/slide2.xml.rels").replace("modernComment2.xml", "modernComment1.xml");
-    let mut z = zip::ZipArchive::new(std::io::Cursor::new(&bytes)).expect("zip");
-    let mut out = zip::ZipWriter::new(std::io::Cursor::new(Vec::new()));
-    for i in 0..z.len() {
-        let mut f = z.by_index(i).expect("entry");
-        out.start_file(f.name(), zip::write::SimpleFileOptions::default()).expect("start");
-        if f.name() == "ppt/slides/_rels/slide2.xml.rels" {
-            std::io::Write::write_all(&mut out, rels2.as_bytes()).expect("write");
-        } else {
-            std::io::copy(&mut f, &mut out).expect("copy");
-        }
-    }
-    let q = deckcraft_pptx::import(&out.finish().expect("finish").into_inner()).expect("import");
-    assert_eq!(q.slides[0].comments.len(), 2);
-    assert!(q.slides[1].comments.is_empty(), "{:?}", q.slides[1].comments);
-}
-
 /// A legacy-only package (what older tools read and write) still imports: 1/8 pt positions,
 /// flattened replies.
 #[test]

crates/pptx/tests/it/malformed.rs

@@ -287,34 +287,3 @@ fn unreadable_slide_is_reported() {
     assert_eq!(lost, vec!["ppt/slides/slide2.xml".to_string()]);
     assert_eq!(deckcraft_pptx::import(&broken).expect("import").slides.len(), 1);
 }
-
-/// Many slides naming one notes part: it is read for the first slide only, not copied onto every
-/// slide that names it.
-#[test]
-fn a_shared_notes_part_is_read_once() {
-    use std::sync::Arc;
-    let mut p = defaults::new_presentation(None);
-    let layout = p.slides[0].layout;
-    for _ in 0..3 {
-        let s = defaults::new_slide(&mut p, layout).expect("slide");
-        p.slides.push(Arc::new(s));
-    }
-    for (i, s) in p.slides.iter_mut().enumerate() {
-        Arc::make_mut(s).notes = deckcraft_model::text::TextBody::from_text(&format!("notes {i}"));
-    }
-    let mut es = entries(&deckcraft_pptx::export(&p).expect("export"));
-    for (n, d) in es.iter_mut() {
-        if n.starts_with("ppt/slides/_rels/") {
-            let x = String::from_utf8(d.clone()).expect("utf8");
-            let x = x
-                .replace("notesSlide2.xml", "notesSlide1.xml")
-                .replace("notesSlide3.xml", "notesSlide1.xml")
-                .replace("notesSlide4.xml", "notesSlide1.xml");
-            *d = x.into_bytes();
-        }
-    }
-    let q = deckcraft_pptx::import(&rezip(&es)).expect("import");
-    assert_eq!(q.slides.len(), 4);
-    assert_eq!(q.slides[0].notes.text(), "notes 0");
-    assert!(q.slides[1..].iter().all(|s| s.notes.text().is_empty()), "a shared notes part is not copied");
-}