Apply review fixes

- crates/engine/src/tools/pointer.rs

crates/engine/src/tests.rs

@@ -5023,6 +5023,38 @@ fn pen_pressure_is_kept_and_a_mouse_draws_at_full_width() {
     assert_eq!(got, vec![vec![0.2, 1.0, 0.4], vec![1.0, 1.0]]);
 }
 
+#[test]
+fn a_long_ink_stroke_is_extended_in_place_and_capped() {
+    let mut s = session();
+    s.set_tool(tools::ToolKind::Ink { mode: "pen".into(), color: deckcraft_model::Rgba::rgb(20, 20, 20), width: 4.0 });
+    let ev = |kind, x: f64| tools::PointerEvent { kind, x, y: 80.0, mods: Mods::default(), tol: 3.0, pressure: Some(0.5) };
+    s.pointer(ev(PointerKind::Down, 0.0)).unwrap();
+    let n = tools::MAX_GESTURE_POINTS + 5_000;
+    for i in 1..n {
+        s.pointer(ev(PointerKind::Drag, (i % 500) as f64)).unwrap();
+    }
+    assert_eq!(s.tool.ink_preview.len(), tools::MAX_GESTURE_POINTS);
+    s.pointer(ev(PointerKind::Up, 0.0)).unwrap();
+    let kept: usize = ink_pressures(&s).iter().map(Vec::len).sum();
+    assert!((2..=tools::MAX_GESTURE_POINTS).contains(&kept), "{kept}");
+}
+
+#[test]
+fn double_clicking_a_capped_freeform_keeps_its_vertices() {
+    let mut s = session();
+    let ev = |kind, x: f64, y: f64| tools::PointerEvent { kind, x, y, mods: Default::default(), tol: 3.0, pressure: None };
+    s.set_tool(tools::ToolKind::Shape { preset: "freeform".into() });
+    s.pointer(ev(tools::PointerKind::Down, 100.0, 100.0)).unwrap();
+    // Densely clicked vertices, each within the double-click tolerance of the last.
+    for i in 0..tools::MAX_GESTURE_POINTS + 10 {
+        s.pointer(ev(tools::PointerKind::Down, 300.0 + i as f64 * 0.001, 100.0)).unwrap();
+    }
+    s.pointer(ev(tools::PointerKind::DoubleClick, 300.0, 100.0)).unwrap();
+    let st = s.doc().unwrap();
+    let deckcraft_model::Geom::Custom { paths } = &st.shapes().last().unwrap().geom else { panic!("no freeform") };
+    assert_eq!(paths[0].d.matches(" L ").count(), tools::MAX_GESTURE_POINTS - 1);
+}
+
 fn ink_pressures(s: &Session) -> Vec<Vec<f32>> {
     let st = s.doc().unwrap();
     let Some(sh) = st.shapes().iter().rev().find(|sh| matches!(sh.kind, deckcraft_model::ShapeKind::Ink { .. })) else {

crates/engine/src/tools.rs

@@ -17,6 +17,7 @@ mod geometry;
 mod hit;
 mod pointer;
 
+pub(crate) use drag::MAX_GESTURE_POINTS;
 pub use hit::{line_xfrm, resize_box, table_rows, text_pos};
 
 #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
@@ -275,6 +276,8 @@ pub struct ToolState {
     pub path_preview: Vec<Point>,
     pub path_smooth: bool,
     pub path_closing: bool,
+    /// Whether the last freeform press added a vertex (it adds none at the point cap).
+    pub press_added_vertex: bool,
     /// The shape in Edit Points: its vertices are handles instead of the box's.
     pub edit_points: Option<ShapeId>,
     /// The vertex (path, index) whose control handles show in Edit Points.

crates/engine/src/tools/click.rs

@@ -4,8 +4,12 @@ use super::{Drag, Hit, Point, Result, Session, ShapeId, ShapeKind, TextSel, Tool
 impl Session {
     pub(super) fn double_click(&mut self, p: Point, tol: f64) -> Result<Value> {
         if let Some(Drag::Freeform { mode, mut points, .. }) = self.tool.drag.clone() {
-            // The double-click's own click added a duplicate vertex.
-            while points.len() > 2 && points.last().zip(points.get(points.len() - 2)).is_some_and(|(a, b)| (*a - *b).hypot() <= tol) {
+            // The double-click's own click added a duplicate vertex (none at the point cap, where
+            // the existing vertices are all kept).
+            while self.tool.press_added_vertex
+                && points.len() > 2
+                && points.last().zip(points.get(points.len() - 2)).is_some_and(|(a, b)| (*a - *b).hypot() <= tol)
+            {
                 points.pop();
             }
             self.tool.drag = Some(Drag::Freeform { mode, points, pressed: false });

crates/engine/src/tools/drag.rs

@@ -12,9 +12,16 @@ struct MoveDrag {
     enter_text: Option<(ShapeId, Point)>,
 }
 
+/// Samples one ink or freeform gesture keeps; later pointer moves extend it no further.
+pub(crate) const MAX_GESTURE_POINTS: usize = 20_000;
+
 impl Session {
     /// Update the active drag interaction using the pointer position and modifiers.
     pub(super) fn drag(&mut self, p: Point, mods: Mods, tol: f64) -> Result<Value> {
+        if matches!(self.tool.drag, Some(Drag::Ink { .. })) {
+            self.extend_ink(p);
+            return Ok(Value::Null);
+        }
         let Some(drag) = self.tool.drag.clone() else { return Ok(Value::Null) };
         match drag {
             Drag::Guide { index } => {
@@ -181,7 +188,7 @@ impl Session {
             }
             Drag::Freeform { mode, mut points, pressed } => {
                 // Dragging scribbles (except for curves, which only take clicked points).
-                if pressed && mode != "curve" && points.last().is_none_or(|l| (*l - p).hypot() >= 1.5) {
+                if pressed && mode != "curve" && points.len() < MAX_GESTURE_POINTS && points.last().is_none_or(|l| (*l - p).hypot() >= 1.5) {
                     points.push(p);
                 }
                 let mut v = points.clone();
@@ -191,15 +198,24 @@ impl Session {
                 self.tool.path_preview = v;
                 self.tool.drag = Some(Drag::Freeform { mode, points, pressed });
             }
-            Drag::Ink { mut points } => {
-                points.push(self.ink_sample(p));
-                self.tool.ink_preview = points.clone();
-                self.tool.drag = Some(Drag::Ink { points });
-            }
+            // Handled above.
+            Drag::Ink { .. } => {}
         }
         Ok(Value::Null)
     }
 
+    /// Ink grows by one sample per move: extend it (and its preview) in place rather than
+    /// copying the whole stroke twice per event, up to [`MAX_GESTURE_POINTS`].
+    fn extend_ink(&mut self, p: Point) {
+        let sample = self.ink_sample(p);
+        if let Some(Drag::Ink { points }) = self.tool.drag.as_mut()
+            && points.len() < MAX_GESTURE_POINTS
+        {
+            points.push(sample);
+            self.tool.ink_preview.push(sample);
+        }
+    }
+
     fn drag_move(&mut self, drag: MoveDrag, p: Point, mods: Mods, tol: f64) -> Result<Value> {
         let MoveDrag { ids, start, origs, moved, duplicate, enter_text } = drag;
         let mut d = p - start;

crates/engine/src/tools/pointer.rs

@@ -228,7 +228,10 @@ impl Session {
                 self.tool.drag = Some(Drag::Freeform { mode, points, pressed: false });
                 return self.finish_freeform(true);
             }
-            points.push(p);
+            self.tool.press_added_vertex = points.len() < super::MAX_GESTURE_POINTS;
+            if self.tool.press_added_vertex {
+                points.push(p);
+            }
             self.tool.path_preview = points.clone();
             self.tool.drag = Some(Drag::Freeform { mode, points, pressed: true });
             return Ok(Value::Null);

crates/pptx/src/read.rs

@@ -100,7 +100,8 @@ impl Part {
     }
     /// Like [`Part::keep`], also declaring prefixes that only an enclosing wrapper binds.
     pub fn keep_inner(&self, el: &El) -> String {
-        keep_with(el, &[&self.ns, &self.inner_ns])
+        let ns: Vec<(String, String)> = self.ns.iter().chain(&self.inner_ns).cloned().collect();
+        keep_xml(el, &ns)
     }
 }
 
@@ -135,69 +136,36 @@ fn inner_decls(root: &El, ns: &[(String, String)]) -> Vec<(String, String)> {
     out
 }
 
-/// Longest namespace name copied onto a kept fragment or a resolved `mc` branch. Real ones are
-/// URIs of well under 100 bytes; a longer one is not copied, so one declaration cannot be
-/// multiplied by every fragment that uses its prefix.
-const MAX_NS_VALUE: usize = 2048;
-
-/// Prefixes `el` and its descendants use in element and attribute names, and those listed in
-/// `mc:Ignorable` or `mc:MustUnderstand` (which must stay declared for the markup-compatibility rules to hold),
-/// and those starting a QName-like attribute value.
-fn used_prefixes(el: &El) -> HashSet<&str> {
-    fn rec<'a>(e: &'a El, used: &mut HashSet<&'a str>, depth: usize) {
+/// Serialize `el`, declaring every prefix it uses that the source root declared.
+pub fn keep_xml(el: &El, ns: &[(String, String)]) -> String {
+    let mut used = HashSet::new();
+    fn rec(e: &El, used: &mut HashSet<String>, depth: usize) {
         if depth > crate::xml::MAX_DEPTH {
             return;
         }
         if let Some((p, _)) = e.name.split_once(':') {
-            used.insert(p);
+            used.insert(p.to_string());
         }
-        for (k, v) in &e.attrs {
-            if let Some((p, local)) = k.split_once(':')
+        for (k, _) in &e.attrs {
+            if let Some((p, _)) = k.split_once(':')
                 && p != "xmlns"
                 && p != "xml"
             {
-                used.insert(p);
-                if local == "Ignorable" || local == "MustUnderstand" {
-                    used.extend(v.split_whitespace());
-                }
-            }
-            // QName-valued attributes (`xsi:type="q:Item"`) need their prefix declared too.
-            if k != "xmlns" && !k.starts_with("xmlns:") {
-                for t in v.split_whitespace() {
-                    if let Some((p, _)) = t.split_once(':')
-                        && !p.is_empty()
-                    {
-                        used.insert(p);
-                    }
-                }
+                used.insert(p.to_string());
             }
         }
         for c in e.elements() {
             rec(c, used, depth + 1);
         }
     }
-    let mut used = HashSet::new();
     rec(el, &mut used, 0);
-    used
-}
-
-/// Serialize `el`, declaring every prefix it uses that the source root declared.
-pub fn keep_xml(el: &El, ns: &[(String, String)]) -> String {
-    keep_with(el, &[ns])
-}
-
-fn keep_with(el: &El, decls: &[&[(String, String)]]) -> String {
-    let used = used_prefixes(el);
-    let mut have: HashSet<&str> = el.attrs.iter().map(|(a, _)| a.as_str()).collect();
-    let mut extra = Vec::new();
-    for (k, v) in decls.iter().flat_map(|d| d.iter()) {
+    let mut el = el.clone();
+    for (k, v) in ns {
         let p = k.strip_prefix("xmlns:").unwrap_or("");
-        if !p.is_empty() && used.contains(p) && v.len() <= MAX_NS_VALUE && have.insert(k) {
-            extra.push((k.clone(), v.clone()));
+        if !p.is_empty() && used.contains(p) && !el.attrs.iter().any(|(a, _)| a == k) {
+            el.attrs.push((k.clone(), v.clone()));
         }
     }
-    let mut el = el.clone();
-    el.attrs.extend(extra);
     el.to_xml()
 }
 
@@ -346,18 +314,10 @@ fn kept_parts(pkg: &Package, pres_path: &str) -> Vec<deckcraft_model::KeptPart>
             // The part's own relationships (customXml item → itemProps) and their targets.
             let rp = opc::rels_path(&name);
             if let Some(rels_xml) = text(&rp) {
-                let mut capped = false;
                 for sub in pkg.rels(&name).items.iter().filter(|s| !s.external && keepable(&s.target)) {
-                    // Checked before the part is read: repeated relationships to one large part
-                    // must not copy it again each time.
-                    if out.iter().any(|k| k.name.eq_ignore_ascii_case(&sub.target)) {
-                        continue;
-                    }
-                    if out.len() >= 400 {
-                        capped = true;
-                        continue;
-                    }
-                    if let Some(x) = text(&sub.target) {
+                    if let Some(x) = text(&sub.target)
+                        && !out.iter().any(|k| k.name.eq_ignore_ascii_case(&sub.target))
+                    {
                         out.push(KeptPart {
                             content_type: pkg.content_type(&sub.target).unwrap_or_default(),
                             name: sub.target.clone(),
@@ -366,9 +326,7 @@ fn kept_parts(pkg: &Package, pres_path: &str) -> Vec<deckcraft_model::KeptPart>
                         });
                     }
                 }
-                if !capped {
-                    out.push(KeptPart { name: rp, xml: rels_xml, ..Default::default() });
-                }
+                out.push(KeptPart { name: rp, xml: rels_xml, ..Default::default() });
             }
         }
     }
@@ -405,39 +363,15 @@ fn resolve_mc_rels(el: &mut El, depth: usize, rels: Option<&Rels>) {
                     });
                     let branch = choice.or_else(|| ac.child("Fallback"));
                     if let Some(b) = branch {
-                        // The branch's namespace declarations (the first usable one of each name,
-                        // in order), indexed by prefix once: each child then looks up the prefixes
-                        // it uses instead of scanning every declaration.
-                        let mut decls: Vec<(&String, &String)> = Vec::new();
-                        // An oversized declaration still shadows an outer one of its name: its
-                        // content is left unbound rather than bound to the outer namespace.
-                        let mut by_prefix: HashMap<&str, Option<usize>> = HashMap::new();
-                        for (k, v) in b.attrs.iter().chain(ac.attrs.iter()) {
-                            let prefix = if k == "xmlns" { Some("") } else { k.strip_prefix("xmlns:") };
-                            if let Some(p) = prefix
-                                && !by_prefix.contains_key(p)
-                            {
-                                by_prefix.insert(p, (v.len() <= MAX_NS_VALUE).then_some(decls.len()));
-                                if v.len() <= MAX_NS_VALUE {
-                                    decls.push((k, v));
-                                }
-                            }
-                        }
                         for n in &b.children {
                             if let Node::El(e) = n {
-                                // Carry the namespace declarations from the branch that its content
-                                // uses (all of them per child made children x declarations copies).
-                                let used = used_prefixes(e);
-                                let have: HashSet<&str> = e.attrs.iter().map(|(a, _)| a.as_str()).collect();
-                                let mut wanted: Vec<usize> = std::iter::once("")
-                                    .chain(used.iter().copied())
-                                    .filter_map(|p| by_prefix.get(p).copied().flatten())
-                                    .filter(|&i| !have.contains(decls[i].0.as_str()))
-                                    .collect();
-                                wanted.sort_unstable();
-                                wanted.dedup();
                                 let mut e = e.clone();
-                                e.attrs.extend(wanted.into_iter().map(|i| (decls[i].0.clone(), decls[i].1.clone())));
+                                // Carry namespace declarations from the branch onto its content.
+                                for (k, v) in b.attrs.iter().chain(ac.attrs.iter()) {
+                                    if k.starts_with("xmlns") && !e.attrs.iter().any(|(a, _)| a == k) {
+                                        e.attrs.push((k.clone(), v.clone()));
+                                    }
+                                }
                                 out.push(Node::El(e));
                             }
                         }
@@ -1480,72 +1414,3 @@ fn header_footer(p: &Presentation) -> HeaderFooter {
     }
     hf
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn mc_branches_carry_only_the_declarations_their_content_uses() {
-        // 2,000 declarations on the branch and 2,000 children: each child gets the one it uses
-        // (and the default), not all 2,000.
-        let decls: String = (0..2000).map(|i| format!(r#" xmlns:n{i}="urn:n{i}""#)).collect();
-        let kids: String = (0..2000).map(|i| format!("<n{i}:x/>")).collect();
-        let x = format!(
-            r#"<r xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"><mc:AlternateContent><mc:Fallback xmlns="urn:d"{decls}>{kids}</mc:Fallback></mc:AlternateContent></r>"#
-        );
-        let mut d = crate::xml::parse(x.as_bytes()).unwrap();
-        resolve_mc(&mut d.root, 0);
-        let kids: Vec<&El> = d.root.elements().collect();
-        assert_eq!(kids.len(), 2000);
-        assert_eq!(kids[7].attrs, vec![("xmlns".to_string(), "urn:d".to_string()), ("xmlns:n7".to_string(), "urn:n7".to_string())]);
-    }
-
-    /// The branch's own declaration of a prefix wins over the `AlternateContent`'s; an oversized
-    /// one still shadows it (its content stays unbound); a child's own declaration is left alone.
-    #[test]
-    fn mc_branch_declarations_keep_their_precedence() {
-        let long = "u".repeat(MAX_NS_VALUE + 1);
-        let x = format!(
-            r#"<r xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"><mc:AlternateContent xmlns:p="urn:ac" xmlns:q="urn:q-short"><mc:Fallback xmlns:p="urn:b" xmlns:q="{long}"><p:x/><q:y/><p:z xmlns:p="urn:own"/></mc:Fallback></mc:AlternateContent></r>"#
-        );
-        let mut d = crate::xml::parse(x.as_bytes()).unwrap();
-        resolve_mc(&mut d.root, 0);
-        let kids: Vec<&El> = d.root.elements().collect();
-        let attrs = |i: usize| kids[i].attrs.iter().map(|(k, v)| format!("{k}={v}")).collect::<Vec<_>>();
-        assert_eq!(attrs(0), ["xmlns:p=urn:b"]);
-        assert!(attrs(1).is_empty(), "{:?}", attrs(1));
-        assert_eq!(attrs(2), ["xmlns:p=urn:own"]);
-    }
-
-    #[test]
-    fn mc_branches_keep_prefixes_named_by_must_understand() {
-        let x = r#"<r xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"><mc:AlternateContent><mc:Fallback xmlns:q="urn:q"><a mc:MustUnderstand="q"/></mc:Fallback></mc:AlternateContent></r>"#;
-        let mut d = crate::xml::parse(x.as_bytes()).unwrap();
-        resolve_mc(&mut d.root, 0);
-        let kid = d.root.elements().next().unwrap();
-        assert!(kid.attrs.iter().any(|(k, v)| k == "xmlns:q" && v == "urn:q"), "{:?}", kid.attrs);
-    }
-
-    #[test]
-    fn mc_branches_keep_prefixes_used_in_qname_values() {
-        let x = r#"<r xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"><mc:AlternateContent><mc:Fallback xmlns:q="urn:q" xmlns:z="urn:z"><n xmlns:xsi="urn:xsi" xsi:type="q:Item"/></mc:Fallback></mc:AlternateContent></r>"#;
-        let mut d = crate::xml::parse(x.as_bytes()).unwrap();
-        resolve_mc(&mut d.root, 0);
-        let kid = d.root.elements().next().unwrap();
-        assert!(kid.attrs.iter().any(|(k, v)| k == "xmlns:q" && v == "urn:q"), "{:?}", kid.attrs);
-        assert!(!kid.attrs.iter().any(|(k, _)| k == "xmlns:z"), "unused declarations stay out: {:?}", kid.attrs);
-    }
-
-    #[test]
-    fn kept_fragments_skip_oversized_namespace_names() {
-        let big = "u".repeat(MAX_NS_VALUE + 1);
-        let ns = vec![("xmlns:a".to_string(), "urn:a".to_string()), ("xmlns:b".to_string(), big)];
-        let d = crate::xml::parse(br#"<a:x xmlns:a="urn:a" xmlns:b="urn:b"><b:y/></a:x>"#).unwrap();
-        let mut el = d.root.clone();
-        el.attrs.clear();
-        let out = keep_xml(&el, &ns);
-        assert!(out.contains(r#"xmlns:a="urn:a""#), "{out}");
-        assert!(!out.contains("uuuu"), "the oversized declaration is not copied");
-    }
-}