address review: warn once when a by-paragraph build is truncated

crates/engine/src/cmd/review.rs

@@ -421,30 +421,23 @@ fn add_to_dictionary(_s: &mut Session, p: &Value) -> Result<Value> {
 /// Replace whole-word `from` with `to` in `text`; how many were replaced.
 fn replace_word(text: &mut String, from: &str, to: &str, all: bool) -> usize {
     let is_word = |c: char| c.is_alphanumeric() || c == '\'' || c == '’';
-    // The result is built in one pass; the text is not edited in place for every match.
-    let mut out = String::new();
-    let (mut n, mut at, mut copied) = (0, 0, 0);
+    let mut n = 0;
+    let mut at = 0;
     while let Some(i) = text.get(at..).and_then(|t| t.find(from)).map(|i| i + at) {
         let end = i + from.len();
-        // The character before the match as the edited text would have it.
-        let before = text.get(copied..i).and_then(|t| t.chars().next_back()).or_else(|| out.chars().next_back()).is_some_and(is_word);
+        let before = text.get(..i).and_then(|t| t.chars().next_back()).is_some_and(is_word);
         let after = text.get(end..).and_then(|t| t.chars().next()).is_some_and(is_word);
         if before || after {
             at = end;
             continue;
         }
-        out.push_str(text.get(copied..i).unwrap_or_default());
-        out.push_str(to);
+        text.replace_range(i..end, to);
         n += 1;
-        (at, copied) = (end, end);
+        at = i + to.len();
         if !all {
             break;
         }
     }
-    if n > 0 {
-        out.push_str(text.get(copied..).unwrap_or_default());
-        *text = out;
-    }
     n
 }
 
@@ -771,49 +764,6 @@ fn end_compare(s: &mut Session, _: &Value) -> Result<Value> {
 
 #[cfg(test)]
 mod tests {
-    /// The replacement as it ran before: edited in place for every match.
-    fn replace_word_in_place(text: &mut String, from: &str, to: &str, all: bool) -> usize {
-        let is_word = |c: char| c.is_alphanumeric() || c == '\'' || c == '’';
-        let mut n = 0;
-        let mut at = 0;
-        while let Some(i) = text.get(at..).and_then(|t| t.find(from)).map(|i| i + at) {
-            let end = i + from.len();
-            let before = text.get(..i).and_then(|t| t.chars().next_back()).is_some_and(is_word);
-            let after = text.get(end..).and_then(|t| t.chars().next()).is_some_and(is_word);
-            if before || after {
-                at = end;
-                continue;
-            }
-            text.replace_range(i..end, to);
-            n += 1;
-            at = i + to.len();
-            if !all {
-                break;
-            }
-        }
-        n
-    }
-
-    #[test]
-    fn replace_word_matches_the_in_place_version() {
-        let cases = [
-            ("a a b a", "a", "x", true),
-            ("aa a. a", "a", "bb", true),
-            ("a-a -a-", "-", "ab", true),
-            ("foo. foo.", "foo.", "", true),
-            ("it's it", "it", "so", true),
-            ("x.x. x.", "x.", "y", true),
-            ("a a a", "a", "b", false),
-            ("none", "z", "q", true),
-            ("é é", "é", "e", true),
-        ];
-        for (text, from, to, all) in cases {
-            let (mut a, mut b) = (text.to_string(), text.to_string());
-            let (na, nb) = (replace_word(&mut a, from, to, all), replace_word_in_place(&mut b, from, to, all));
-            assert_eq!((na, a), (nb, b), "{text:?} {from:?} -> {to:?}");
-        }
-    }
-
     use super::*;
     use serde_json::json;
 

crates/pptx/src/write/dml.rs

@@ -762,17 +762,27 @@ pub fn text_body(w: &mut W, x: &mut Exp, o: &mut Out, tag: &str, tb: &TextBody,
 }
 
 fn run_text(w: &mut W, x: &mut Exp, o: &mut Out, text: &str, props: &RunProps) {
+    // The run's properties are the same for every piece and line break: serialize them once.
+    let mut rpr_xml: Option<String> = None;
+    let mut put_rpr = |w: &mut W| match &rpr_xml {
+        Some(xml) => w.s.push_str(xml),
+        None => {
+            let start = w.s.len();
+            rpr(w, x, o, "a:rPr", props);
+            rpr_xml = Some(w.s[start..].to_string());
+        }
+    };
     for (i, piece) in text.split(['\n', '\u{b}']).enumerate() {
         if i > 0 {
             w.open0("a:br");
-            rpr(w, x, o, "a:rPr", props);
+            put_rpr(w);
             w.close("a:br");
         }
         if piece.is_empty() && i > 0 {
             continue;
         }
         w.open0("a:r");
-        rpr(w, x, o, "a:rPr", props);
+        put_rpr(w);
         w.elt("a:t", piece);
         w.close("a:r");
     }

crates/pptx/src/write/timing.rs

@@ -1,7 +1,7 @@
 //! Transitions and the animation timing tree.
 
 use deckcraft_model::anim::{AnimClass, AnimStart, Animation, BuildRole, TextBuild, Transition, build_filled, paragraph_build_roles};
-use deckcraft_model::{Shape, ShapeId, ShapeKind, find_shape};
+use deckcraft_model::{Shape, ShapeId, ShapeKind};
 
 use super::shapes::IdMap;
 use super::{Exp, Out};
@@ -577,6 +577,21 @@ fn click_pars(w: &mut W, x: &mut Exp, ids: &mut Ids, clicks: &Groups, interactiv
     }
 }
 
+/// Most effects one slide's timing writes; animations past it are left out.
+const MAX_EFFECTS: usize = 100_000;
+
+/// The shapes of the tree by id, the first in document order for a repeated id (as `find_shape`
+/// finds it), so each animation resolves its shape without a tree walk.
+fn shape_index<'a>(shapes: &'a [Shape], depth: usize, out: &mut std::collections::HashMap<ShapeId, &'a Shape>) {
+    if depth > 64 {
+        return;
+    }
+    for s in shapes {
+        out.entry(s.id).or_insert(s);
+        shape_index(s.children(), depth + 1, out);
+    }
+}
+
 /// Write `p:timing` for a slide's animations.
 pub fn timing(w: &mut W, x: &mut Exp, anims: &[Animation], shapes: &[Shape], idmap: &IdMap) {
     let mut grp_count: std::collections::HashMap<u32, u32> = Default::default();
@@ -585,16 +600,31 @@ pub fn timing(w: &mut W, x: &mut Exp, anims: &[Animation], shapes: &[Shape], idm
     let mut bld: Vec<(u32, u32, bool)> = vec![];
     // The last paragraph effect written: (shape, class, trigger, grpId).
     let mut prev_para: Option<(ShapeId, AnimClass, Option<ShapeId>, u32)> = None;
+    let mut by_id = std::collections::HashMap::new();
+    shape_index(shapes, 0, &mut by_id);
+    let mut roles_of: std::collections::HashMap<ShapeId, Vec<BuildRole>> = Default::default();
+    let mut effect_count = 0usize;
+    let mut warned = false;
     for a in anims.iter().take(10_000) {
+        if effect_count >= MAX_EFFECTS {
+            if !warned {
+                log::warn!("pptx: more than {MAX_EFFECTS} animation effects; the rest are left out");
+            }
+            break;
+        }
         let Some(spid) = idmap.get(a.shape) else {
             log::warn!("pptx: animation on missing shape {} skipped", a.shape);
             continue;
         };
-        let Some(shape) = find_shape(shapes, a.shape) else { continue };
+        let Some(&shape) = by_id.get(&a.shape) else { continue };
         let text_sp = matches!(shape.kind, ShapeKind::Shape) && shape.text.is_some();
         let is_text_sp = matches!(shape.kind, ShapeKind::Shape) && shape.text.as_ref().is_some_and(|t| !t.is_empty());
         let text = shape.text.as_ref().filter(|_| a.text_build == TextBuild::ByParagraph && is_text_sp);
-        let roles = text.map(paragraph_build_roles).unwrap_or_default();
+        let no_roles = Vec::new();
+        let roles = match text {
+            Some(t) => roles_of.entry(a.shape).or_insert_with(|| paragraph_build_roles(t)),
+            None => &no_roles,
+        };
         let by_para = roles.contains(&BuildRole::Top);
         // PowerPoint keeps a shape's consecutive paragraph effects in one build (`grpId`) marked
         // `build="p"`; the reader turns them back into a by-paragraph build only if that plays them
@@ -632,11 +662,19 @@ pub fn timing(w: &mut W, x: &mut Exp, anims: &[Animation], shapes: &[Shape], idm
                     _ => continue,
                 };
                 let Ok(pi) = u32::try_from(i) else { break };
+                if effect_count + effs.len() >= MAX_EFFECTS {
+                    if !warned {
+                        log::warn!("pptx: more than {MAX_EFFECTS} animation effects; the rest are left out");
+                        warned = true;
+                    }
+                    break;
+                }
                 effs.push(Eff { a, spid, start, paragraph: Some(pi), grp });
             }
         } else {
             effs.push(Eff { a, spid, start: a.start, paragraph: a.paragraph, grp });
         }
+        effect_count += effs.len();
         if text_sp && !joins {
             bld.push((spid, grp, by_para || para_effect));
         }