From b7482dd392aeedae8ff51543668aefb8340066b9 Mon Sep 17 00:00:00 2001 From: marshiyar <57055687+marshiyar@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:13:05 -0700 Subject: [PATCH 1/2] engine: animation.add and animation.set take the points of a custom motion path What was wrong: animation.add ignored a `path` parameter, so a customPath animation always got the default straight path of a quarter of the slide height, and no command could set or change the points of one. What changed: animation.add, animation.set and animation.options accept `path` for a motion path: either points [[x, y], ...] in slide points, as offsets from where the shape sits (converted to the stored slide-fraction path; `closed` returns to the first point), or the OOXML path string animation.get answers. At most 1000 points, finite numbers within 1e6 points, and strings of at most 16 KiB made of path commands and numbers are accepted; anything else is refused before the deck changes, as is `path` on an animation that is not a motion path. animation.add answers the stored path. How it was tested: three new tests in cmd/animation.rs: points become the expected path (add, set with `closed`, back to the default without `path`), a raw string is kept and animation.options replaces it, and malformed paths are refused with nothing added. The animation tests pass, and clippy is clean with -D warnings. Fixes #1576 --- crates/engine/src/cmd/animation.rs | 158 +++++++++++++++++++++++++++-- 1 file changed, 149 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs index b5d00bba..e69d97b6 100644 --- a/crates/engine/src/cmd/animation.rs +++ b/crates/engine/src/cmd/animation.rs @@ -17,7 +17,7 @@ pub fn specs() -> Vec { "Animation Styles", ["Animations", "Animation"], None, - "{effect: fade|fly|zoom|wipe|appear|spin|pulse|fadeOut|lines|… (animation.list), class?: entrance|emphasis|exit|path, option?, ids?} — replaces the shapes' animations", + "{effect: fade|fly|zoom|wipe|appear|spin|pulse|fadeOut|lines|… (animation.list), class?: entrance|emphasis|exit|path, option?, path?: [[x,y],…] or OOXML string, closed?: bool, ids?} — replaces the shapes' animations. path (motion paths): points in slide points, as offsets from where the shape sits, or the OOXML path string (slide fractions) animation.get answers", has_selection, set ), @@ -26,7 +26,7 @@ pub fn specs() -> Vec { "Add Animation", ["Animations", "Advanced Animation"], None, - "{effect, class?, option?, start?: onClick|withPrevious|afterPrevious, duration?: ms, delay?: ms, ids?}", + "{effect, class?, option?, path?: [[x,y],…] or OOXML string, closed?: bool, start?: onClick|withPrevious|afterPrevious, duration?: ms, delay?: ms, ids?} → {count, path?}", has_selection, add ), @@ -46,7 +46,7 @@ pub fn specs() -> Vec { "Effect Options", ["Animations", "Animation"], None, - "{option?, textBuild?: asOne|byParagraph|byWord|byLetter, index?}", + "{option?, textBuild?: asOne|byParagraph|byWord|byLetter, path?: [[x,y],…] or OOXML string, closed?: bool, index?}", has_slide, options ), @@ -129,7 +129,7 @@ fn resolve_option(allowed: &[&str], given: &str) -> Option { cands.into_iter().find(|c| allowed.contains(&c.as_str())) } -fn build(p: &Value) -> Result { +fn build(p: &Value, slide: (f64, f64)) -> Result { let effect = str_param(p, "effect").ok_or_else(|| bad("animation", "missing `effect`"))?; let class = class_of(str_param(p, "class")); let info = match class { @@ -155,7 +155,9 @@ fn build(p: &Value) -> Result { a.start = start_of(st); } if a.class == AnimClass::Path { - a.path = Some(default_path(&a.effect, &a.option)); + a.path = Some(path_param(p, slide)?.unwrap_or_else(|| default_path(&a.effect, &a.option))); + } else if p.get("path").is_some() { + return Err(bad("animation", "`path` only fits a motion path (class `path`)")); } if a.effect == "spin" { a.amount = Some(360.0); @@ -166,6 +168,60 @@ fn build(p: &Value) -> Result { Ok(a) } +/// Most points of a motion path given as an array, and longest path string given as text. +const MAX_PATH_POINTS: usize = 1_000; +const MAX_PATH_TEXT: usize = 16 * 1024; +/// Largest offset of a point, in slide points; anything beyond is not a place on any slide. +const MAX_PATH_OFFSET: f64 = 1.0e6; + +/// A number of a path string: slide fractions, rounded to 6 places. +fn path_num(v: f64) -> String { + let t = format!("{:.6}", v); + let t = t.trim_end_matches('0').trim_end_matches('.'); + if t == "-0" || t.is_empty() { "0".into() } else { t.into() } +} + +/// The motion path `path` asks for, as the OOXML path string animations store: either points +/// (`[[x, y], …]`, slide points as offsets from where the shape sits, joined by lines, `closed?` +/// to return to the first) or that string itself. +fn path_param(p: &Value, slide: (f64, f64)) -> Result> { + let Some(v) = p.get("path") else { return Ok(None) }; + let refuse = |why: &str| bad("animation", format!("`path` {why}")); + match v { + Value::String(text) => { + let ok = text.len() <= MAX_PATH_TEXT + && text.contains(|c: char| c.is_ascii_digit()) + && text.chars().all(|c| c.is_ascii_digit() || " .+-eE,MmLlCcQqHhVvZz".contains(c)); + if !ok { + return Err(refuse("string must be an OOXML motion path such as `M 0 0 L 0.25 0 E` (at most 16 KiB)")); + } + Ok(Some(text.clone())) + } + Value::Array(points) => { + if !(2..=MAX_PATH_POINTS).contains(&points.len()) { + return Err(refuse(&format!("needs 2 to {MAX_PATH_POINTS} points"))); + } + if !(slide.0.is_finite() && slide.1.is_finite() && slide.0 > 0.0 && slide.1 > 0.0) { + return Err(refuse("can't be placed: the slide has no size")); + } + let mut out = String::new(); + for (k, pt) in points.iter().enumerate() { + let xy = pt.as_array().filter(|a| a.len() == 2).and_then(|a| Some((a.first()?.as_f64()?, a.get(1)?.as_f64()?))); + let Some((x, y)) = xy.filter(|(x, y)| x.abs() <= MAX_PATH_OFFSET && y.abs() <= MAX_PATH_OFFSET) else { + return Err(refuse(&format!("point {k} must be [x, y], two finite numbers within ±{MAX_PATH_OFFSET} points"))); + }; + out.push_str(&format!("{} {} {} ", if k == 0 { "M" } else { "L" }, path_num(x / slide.0), path_num(y / slide.1))); + } + if p.get("closed").and_then(Value::as_bool) == Some(true) { + out.push_str("Z "); + } + out.push('E'); + Ok(Some(out)) + } + _ => Err(refuse("must be [[x, y], …] or an OOXML path string")), + } +} + /// Default motion paths, in slide-fraction units relative to the shape's position. pub fn default_path(effect: &str, option: &str) -> String { let (dx, dy) = match option { @@ -185,7 +241,7 @@ pub fn default_path(effect: &str, option: &str) -> String { fn set(s: &mut Session, p: &Value) -> Result { let ids = targets(s, p)?; - let a = build(p)?; + let a = build(p, slide_extent(s)?)?; s.edit(|doc, sel| { let sl = doc.slides.get_mut(sel.slide).ok_or_else(|| bad("animation.set", "no slide"))?; let sl = Arc::make_mut(sl); @@ -215,9 +271,16 @@ fn set(s: &mut Session, p: &Value) -> Result { }) } +/// The slide's width and height in points. +fn slide_extent(s: &mut Session) -> Result<(f64, f64)> { + let size = &s.doc()?.doc.slide_size; + Ok((size.width, size.height)) +} + fn add(s: &mut Session, p: &Value) -> Result { let ids = targets(s, p)?; - let a = build(p)?; + let a = build(p, slide_extent(s)?)?; + let path = a.path.clone(); s.edit(|doc, sel| { let sl = Arc::make_mut(doc.slides.get_mut(sel.slide).ok_or_else(|| bad("animation.add", "no slide"))?); for (k, id) in ids.iter().enumerate() { @@ -227,7 +290,10 @@ fn add(s: &mut Session, p: &Value) -> Result { } sl.animations.push(x); } - Ok(json!({"count": sl.animations.len()})) + Ok(match &path { + Some(path) => json!({"count": sl.animations.len(), "path": path}), + None => json!({"count": sl.animations.len()}), + }) }) } @@ -320,13 +386,17 @@ fn options(s: &mut Session, p: &Value) -> Result { "byLetter" => TextBuild::ByLetter, _ => TextBuild::AsOne, }); + let path = path_param(p, slide_extent(s)?)?; each_target(s, p, |a| { + if let (Some(path), AnimClass::Path) = (&path, a.class) { + a.path = Some(path.clone()); + } if let Some(o) = &option { // Each target takes the option in its own effect's terms; one it has no such // option for keeps its own. let allowed = animation_info(&a.effect, a.class).map(|i| i.5).unwrap_or(&[]); if let Some(o) = resolve_option(allowed, o) { - if a.class == AnimClass::Path { + if a.class == AnimClass::Path && path.is_none() { a.path = Some(default_path(&a.effect, &o)); } a.option = o; @@ -606,4 +676,74 @@ mod tests { assert_eq!(s.execute("animation.get", &json!({})).unwrap().as_array().unwrap().len(), 1); } } + + fn path_of(s: &mut Session) -> Option { + let all = s.execute("animation.get", &json!({})).unwrap(); + all.as_array().unwrap().last().and_then(|a| a.get("path")).and_then(Value::as_str).map(String::from) + } + + fn shape(s: &mut Session) -> u64 { + s.execute("shape.insert", &json!({"preset": "ellipse", "rect": [100.0, 400.0, 20.0, 20.0]})).unwrap().get("id").unwrap().as_u64().unwrap() + } + + /// #1576: points in slide points, as offsets from the shape, become the slide-fraction path. + #[test] + fn a_custom_path_takes_its_points() { + let mut s = Session::with_new(); + let id = shape(&mut s); + let w = s.doc().unwrap().doc.slide_size.width; + let h = s.doc().unwrap().doc.slide_size.height; + let r = s + .execute( + "animation.add", + &json!({"effect": "customPath", "class": "path", "path": [[0, 0], [w / 2.0, -h / 4.0], [w, -h / 2.0]], "ids": [id]}), + ) + .unwrap(); + assert_eq!(r["path"], "M 0 0 L 0.5 -0.25 L 1 -0.5 E"); + assert_eq!(path_of(&mut s).as_deref(), Some("M 0 0 L 0.5 -0.25 L 1 -0.5 E")); + // closed shapes end back at the start; set replaces; no path keeps the default + s.execute("animation.set", &json!({"effect": "customPath", "path": [[0, 0], [w / 4.0, 0], [w / 4.0, h / 4.0]], "closed": true, "ids": [id]})) + .unwrap(); + assert_eq!(path_of(&mut s).as_deref(), Some("M 0 0 L 0.25 0 L 0.25 0.25 Z E")); + s.execute("animation.set", &json!({"effect": "customPath", "ids": [id]})).unwrap(); + assert_eq!(path_of(&mut s).as_deref(), Some("M 0 0 L 0 0.25 E")); + } + + #[test] + fn a_raw_path_string_is_kept_and_options_edits_it() { + let mut s = Session::with_new(); + let id = shape(&mut s); + s.execute("animation.add", &json!({"effect": "customPath", "path": "M 0 0 C 0.1 0 0.2 0.1 0.3 0.1 E", "ids": [id]})).unwrap(); + assert_eq!(path_of(&mut s).as_deref(), Some("M 0 0 C 0.1 0 0.2 0.1 0.3 0.1 E")); + s.execute("animation.options", &json!({"index": 0, "path": [[0, 0], [96, 54]]})).unwrap(); + assert_eq!(path_of(&mut s).as_deref(), Some("M 0 0 L 0.1 0.1 E")); + } + + #[test] + fn a_bad_path_is_refused_and_changes_nothing() { + let mut s = Session::with_new(); + let id = shape(&mut s); + let many: Vec = (0..2_000).map(|i| json!([i, i])).collect(); + for bad_path in [ + json!([[0, 0]]), + json!([]), + json!([[0, 0], [1]]), + json!([[0, 0], ["a", 1]]), + json!([[0, 0], [1e300, 1]]), + json!([[0, 0], [null, 1]]), + json!("M 0 0 L