From 5643f39769ab561b04fc43acab4321d8d5c06581 Mon Sep 17 00:00:00 2001 From: JonRC Date: Mon, 2 Nov 2026 20:22:13 -0300 Subject: [PATCH 1/2] Fix #2907: say that text animator indexes count from 1 layer.addTextAnimatorProperty, layer.addTextSelector and text.animatorFontAxes take `animator: uid|index`, counted from 1 as the code and `#n` references do, but the parameters didn't say so and `animator: 0` failed with only "no animator 0". The parameters now say 1-based, and the error says what the value can be. --- crates/engine/src/cmd/text_anim.rs | 12 ++++++++---- crates/engine/src/tests/text.rs | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/text_anim.rs b/crates/engine/src/cmd/text_anim.rs index 296dc4c37..397e697c4 100644 --- a/crates/engine/src/cmd/text_anim.rs +++ b/crates/engine/src/cmd/text_anim.rs @@ -59,7 +59,11 @@ fn animator_uid(s: &Session, l: &Layer, p: &Value, cmd: &str) -> Result { if let Some(g) = list.iter().find(|g| g.uid == n) { return Ok(g.uid); } - return list.get((n as usize).wrapping_sub(1)).map(|g| g.uid).ok_or_else(|| bad(cmd, format!("no animator {n}"))); + let count = if list.len() == 1 { "1 animator".to_string() } else { format!("{} animators", list.len()) }; + return list + .get((n as usize).wrapping_sub(1)) + .map(|g| g.uid) + .ok_or_else(|| bad(cmd, format!("no animator {n}: `animator` is a uid or a 1-based index, and the layer has {count}"))); } for (lid, uid) in &s.state.selected_props { if *lid != l.id { @@ -454,7 +458,7 @@ pub fn specs() -> Vec { "Add Property", [], None, - "{layer?, animator?: uid|index, property: (see layer.addTextAnimator)}", + "{layer?, animator?: uid|index (1-based), property: (see layer.addTextAnimator)}", has_animator, add_animator_prop ), @@ -463,7 +467,7 @@ pub fn specs() -> Vec { "Add Text Selector", [], None, - "{layer?, animator?: uid|index, kind: range|wiggly|expression}", + "{layer?, animator?: uid|index (1-based), kind: range|wiggly|expression}", has_animator, add_selector ), @@ -482,7 +486,7 @@ pub fn specs() -> Vec { "Variable Font Axes", ["Animation", "Animate Text"], None, - "{layer?, axis?: tag|name (omit to list the font's axes), animator?: uid|index}", + "{layer?, axis?: tag|name (omit to list the font's axes), animator?: uid|index (1-based)}", has_text_layer, font_axes ), diff --git a/crates/engine/src/tests/text.rs b/crates/engine/src/tests/text.rs index 3980c7bb5..132bfc416 100644 --- a/crates/engine/src/tests/text.rs +++ b/crates/engine/src/tests/text.rs @@ -49,6 +49,27 @@ fn animate_text_adds_animators_with_companion_properties() { let _ = s.render(cid, s.time(), Default::default()); } +/// `animator` counts from 1, as `#n` references do: the commands that take it say so, and 0 fails +/// with an error that says so (agents reading `uid|index` tried 0). +#[test] +fn animator_indices_count_from_1_and_say_so() { + let (mut s, t) = session(); + let uid = s.execute("layer.addTextAnimator", json!({"layer": t, "property": "position"})).unwrap()["animator"].as_u64().unwrap(); + for id in ["layer.addTextAnimatorProperty", "layer.addTextSelector", "text.animatorFontAxes"] { + let spec = crate::cmd::command_specs().iter().find(|c| c.id == id).unwrap(); + assert!(spec.params.contains("animator?: uid|index (1-based)"), "{id}: {}", spec.params); + } + for (id, p) in [ + ("layer.addTextAnimatorProperty", json!({"layer": t, "animator": 0, "property": "fillHue"})), + ("layer.addTextSelector", json!({"layer": t, "animator": 0, "kind": "wiggly"})), + ] { + let e = s.execute(id, p).unwrap_err().to_string(); + assert!(e.contains("no animator 0") && e.contains("1-based"), "{id}: {e}"); + } + let r = s.execute("layer.addTextSelector", json!({"layer": t, "animator": 1, "kind": "wiggly"})).unwrap(); + assert_eq!(r["animator"].as_u64(), Some(uid)); +} + #[test] fn add_property_and_selectors_with_undo_redo() { let (mut s, t) = session(); From 0b59d2b8c4fc66a53c81d3850092ddcd082f8c07 Mon Sep 17 00:00:00 2001 From: JonRC Date: Mon, 2 Nov 2026 21:14:22 -0300 Subject: [PATCH 2/2] Follow-up to #2907: say that a uid wins over an index for `animator` animator_uid matches a uid before an index, so an animator whose uid equals an index is picked over the one at that position. The four commands that take `animator` (layer.addTextAnimatorProperty, layer.addTextSelector, text.addSelector, text.animatorFontAxes) now say so; text.addSelector also said only `uid|index` before. The test gives a second animator uid 1 and checks that `animator: 1` picks it. --- crates/engine/src/cmd/animation.rs | 10 +++++++++- crates/engine/src/cmd/text_anim.rs | 11 ++++++----- crates/engine/src/tests/text.rs | 19 +++++++++++++++---- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs index 2c2f23f1a..4cfaa26ed 100644 --- a/crates/engine/src/cmd/animation.rs +++ b/crates/engine/src/cmd/animation.rs @@ -555,7 +555,15 @@ pub fn specs() -> Vec { has_keys, exponential_scale ), - cmd!("text.addSelector", "Add Text Selector", [], None, "{layer?, animator?: uid|index, kind: range|wiggly|expression}", has_text_layer, add_selector), + cmd!( + "text.addSelector", + "Add Text Selector", + [], + None, + "{layer?, animator?: uid|index (1-based; a uid match wins over an index), kind: range|wiggly|expression}", + has_text_layer, + add_selector + ), cmd!("text.removeAllAnimators", "Remove All Text Animators", ["Animation"], None, "{layers?}", has_text_layer, remove_all_animators), cmd!("anim.reveal", "Reveal Properties", [], None, "{kind: keyframes|animation|modified}", has_comp, reveal), ] diff --git a/crates/engine/src/cmd/text_anim.rs b/crates/engine/src/cmd/text_anim.rs index 397e697c4..ce9c1fe46 100644 --- a/crates/engine/src/cmd/text_anim.rs +++ b/crates/engine/src/cmd/text_anim.rs @@ -49,8 +49,9 @@ fn per_char_on(l: &Layer) -> bool { l.props.prop("text/perChar3d").is_some_and(|p| p.value.as_bool()) } -/// The animator a command targets: `animator` (uid or 1-based index), else the animator holding -/// a selected property, else the last one. +/// The animator a command targets: `animator` (uid or 1-based index; a uid match wins, so an +/// animator whose uid equals an index is picked over the one at that position), else the animator +/// holding a selected property, else the last one. fn animator_uid(s: &Session, l: &Layer, p: &Value, cmd: &str) -> Result { let anims = l.props.group("text/animators").ok_or_else(|| bad(cmd, "not a text layer"))?; let list: Vec<&PropGroup> = anims.groups().collect(); @@ -458,7 +459,7 @@ pub fn specs() -> Vec { "Add Property", [], None, - "{layer?, animator?: uid|index (1-based), property: (see layer.addTextAnimator)}", + "{layer?, animator?: uid|index (1-based; a uid match wins over an index), property: (see layer.addTextAnimator)}", has_animator, add_animator_prop ), @@ -467,7 +468,7 @@ pub fn specs() -> Vec { "Add Text Selector", [], None, - "{layer?, animator?: uid|index (1-based), kind: range|wiggly|expression}", + "{layer?, animator?: uid|index (1-based; a uid match wins over an index), kind: range|wiggly|expression}", has_animator, add_selector ), @@ -486,7 +487,7 @@ pub fn specs() -> Vec { "Variable Font Axes", ["Animation", "Animate Text"], None, - "{layer?, axis?: tag|name (omit to list the font's axes), animator?: uid|index (1-based)}", + "{layer?, axis?: tag|name (omit to list the font's axes), animator?: uid|index (1-based; a uid match wins over an index)}", has_text_layer, font_axes ), diff --git a/crates/engine/src/tests/text.rs b/crates/engine/src/tests/text.rs index 132bfc416..d24a09240 100644 --- a/crates/engine/src/tests/text.rs +++ b/crates/engine/src/tests/text.rs @@ -49,15 +49,16 @@ fn animate_text_adds_animators_with_companion_properties() { let _ = s.render(cid, s.time(), Default::default()); } -/// `animator` counts from 1, as `#n` references do: the commands that take it say so, and 0 fails -/// with an error that says so (agents reading `uid|index` tried 0). +/// `animator` counts from 1, as `#n` references do, and a uid match wins over an index: every +/// command that takes it says so, and 0 fails with an error that says so (agents reading +/// `uid|index` tried 0). #[test] fn animator_indices_count_from_1_and_say_so() { let (mut s, t) = session(); let uid = s.execute("layer.addTextAnimator", json!({"layer": t, "property": "position"})).unwrap()["animator"].as_u64().unwrap(); - for id in ["layer.addTextAnimatorProperty", "layer.addTextSelector", "text.animatorFontAxes"] { + for id in ["layer.addTextAnimatorProperty", "layer.addTextSelector", "text.addSelector", "text.animatorFontAxes"] { let spec = crate::cmd::command_specs().iter().find(|c| c.id == id).unwrap(); - assert!(spec.params.contains("animator?: uid|index (1-based)"), "{id}: {}", spec.params); + assert!(spec.params.contains("animator?: uid|index (1-based; a uid match wins over an index)"), "{id}: {}", spec.params); } for (id, p) in [ ("layer.addTextAnimatorProperty", json!({"layer": t, "animator": 0, "property": "fillHue"})), @@ -68,6 +69,16 @@ fn animator_indices_count_from_1_and_say_so() { } let r = s.execute("layer.addTextSelector", json!({"layer": t, "animator": 1, "kind": "wiggly"})).unwrap(); assert_eq!(r["animator"].as_u64(), Some(uid)); + // A second animator whose uid is 1: `animator: 1` is that uid, not the first animator, and + // index 2 reaches the same animator. + s.execute("layer.addTextAnimator", json!({"layer": t, "property": "opacity"})).unwrap(); + let cid = s.active_comp_id().unwrap(); + let layer = std::sync::Arc::make_mut(&mut s.project).comp_mut(cid).unwrap().layer_mut(LayerId(t)).unwrap(); + layer.props.group_mut("text/animators").unwrap().children[1].as_group_mut().unwrap().uid = 1; + for (id, animator) in [("layer.addTextSelector", 1), ("text.addSelector", 1), ("layer.addTextSelector", 2)] { + let r = s.execute(id, json!({"layer": t, "animator": animator, "kind": "wiggly"})).unwrap(); + assert_eq!(r["animator"].as_u64(), Some(1), "{id} animator {animator}: {r}"); + } } #[test]