Merge pull request #1755 from tylerjenningsw/fix/timeline-shift-select-groups

Puppet: Shift-select several pins in the Timeline and edit them together

crates/engine/src/cmd/prop.rs

@@ -247,7 +247,8 @@ fn property_reply(p: &Value, layer: &Layer, pr: &Property, time: Tick, value: Va
 /// property (by match path, `transform/position`, `effects/<effect>#n/<param>`) of every other
 /// selected, unlocked layer of the comp whose value is of the same kind, as (layer, uid). Empty
 /// when `selected` is not set or the edited layer isn't selected. After Effects applies a value
-/// edit, the stopwatch and the keyframe button to all selected layers (#355, #491).
+/// edit, the stopwatch and the keyframe button to all selected layers (#355, #491). A parameter
+/// of a selected Puppet pin also reaches the same parameter of the other selected pins (#1751).
 fn selected_twins(s: &Session, p: &Value, cmd: &str) -> Result<Vec<(LayerId, Uid)>> {
     if b_p(p, "selected") != Some(true) {
         return Ok(vec![]);
@@ -260,14 +261,31 @@ fn selected_twins(s: &Session, p: &Value, cmd: &str) -> Result<Vec<(LayerId, Uid
         return Ok(vec![]);
     }
     let kind = (pr.value.kind_name(), pr.value.components().len());
-    Ok(s.state
+    let same_kind = |q: &&Property| (q.value.kind_name(), q.value.components().len()) == kind;
+    let mut twins: Vec<(LayerId, Uid)> = s
+        .state
         .selected_layers
         .iter()
         .filter(|o| **o != lid)
         .filter_map(|o| comp.layer(*o))
         .filter(|l| !l.switches.locked)
-        .filter_map(|l| l.props.prop(&path).filter(|q| (q.value.kind_name(), q.value.components().len()) == kind).map(|q| (l.id, q.uid)))
-        .collect())
+        .filter_map(|l| l.props.prop(&path).filter(same_kind).map(|q| (l.id, q.uid)))
+        .collect();
+    // A parameter of a selected Puppet pin: the same parameter of every other selected pin, on
+    // this layer too, so several pins move or key together (#1751).
+    let pin = super::puppet::layer_pins(layer).find(|g| g.props().any(|q| q.uid == uid)).map(|g| g.uid);
+    if let Some(pin) = pin
+        && s.state.selected_props.contains(&(lid, pin))
+    {
+        for (ol, ou) in &s.state.selected_props {
+            let Some(other) = comp.layer(*ol).filter(|l| !l.switches.locked) else { continue };
+            let Some(q) = super::puppet::layer_pins(other).find(|g| g.uid == *ou).and_then(|g| g.get(&pr.match_id)).filter(same_kind) else { continue };
+            if (*ol, q.uid) != (lid, uid) && !twins.contains(&(*ol, q.uid)) {
+                twins.push((*ol, q.uid));
+            }
+        }
+    }
+    Ok(twins)
 }
 
 /// `p` aimed at property `uid` of layer `lid` instead.

crates/engine/src/tests/paint.rs

@@ -308,6 +308,45 @@ fn puppet_select_all_takes_the_pin_kind_and_several_pins_record_together() {
     assert!(s.execute("puppet.recordPin", json!({"layer": id, "pin": a, "pins": [st], "samples": samples})).is_err());
 }
 
+/// Editing a parameter of one of several selected pins edits the same parameter of the others,
+/// in one undo step; unselected pins keep theirs (#1751).
+#[test]
+fn editing_one_selected_pin_edits_the_other_selected_pins() {
+    let (mut s, id) = setup();
+    let pin = |s: &mut Session, x: i64| s.execute("puppet.addPin", json!({"layer": id, "position": [x, 30]})).unwrap()["pin"].as_u64().unwrap();
+    let (a, b, c) = (pin(&mut s, 10), pin(&mut s, 50), pin(&mut s, 90));
+    let position = |s: &Session, pin: u64| layer(s, id).props.find_group(pin).unwrap().get("position").unwrap().clone();
+    let (pa, pb) = (position(&s, a).uid, position(&s, b).uid);
+    let at = |s: &Session, pin: u64| position(s, pin).value_at(Tick::ZERO).components();
+    s.execute("prop.select", json!({"layer": id, "prop": a, "selectKeys": false})).unwrap();
+    s.execute("prop.select", json!({"layer": id, "prop": b, "selectKeys": false, "add": true})).unwrap();
+
+    // A typed value lands on both selected pins, and one Undo takes it back.
+    let steps = s.history.undo.len();
+    s.execute("prop.set", json!({"layer": id, "prop": pa, "value": [20, 40], "selected": true})).unwrap();
+    assert_eq!((at(&s, a), at(&s, b), at(&s, c)), (vec![20.0, 40.0], vec![20.0, 40.0], vec![90.0, 30.0]));
+    assert_eq!(s.history.undo.len(), steps + 1, "one undo step");
+    s.undo();
+    assert_eq!((at(&s, a), at(&s, b)), (vec![10.0, 30.0], vec![50.0, 30.0]));
+
+    // A scrub moves each selected pin by the same change from its own value.
+    s.execute("prop.set", json!({"layer": id, "prop": pb, "value": [55, 25], "selected": true, "offset": true})).unwrap();
+    assert_eq!((at(&s, a), at(&s, b), at(&s, c)), (vec![15.0, 25.0], vec![55.0, 25.0], vec![90.0, 30.0]));
+
+    // The stopwatch follows on both (a new pin starts keyed, so the toggle turns it off).
+    let animated = |s: &Session| [a, b, c].map(|p| position(s, p).is_animated());
+    assert_eq!(animated(&s), [true; 3]);
+    s.execute("prop.toggleAnimation", json!({"layer": id, "prop": pa, "selected": true})).unwrap();
+    assert_eq!(animated(&s), [false, false, true]);
+
+    // Without `selected`, or with only one pin selected, the edit stays on its own pin.
+    s.execute("prop.set", json!({"layer": id, "prop": pa, "value": [0, 0]})).unwrap();
+    assert_eq!(at(&s, b), vec![55.0, 25.0]);
+    s.execute("prop.select", json!({"layer": id, "prop": a, "selectKeys": false})).unwrap();
+    s.execute("prop.set", json!({"layer": id, "prop": pa, "value": [5, 5], "selected": true})).unwrap();
+    assert_eq!(at(&s, b), vec![55.0, 25.0]);
+}
+
 fn expr_of(s: &Session, layer: u64, uid: u64) -> String {
     let l = layer_of(s, layer);
     l.props.find(uid).and_then(|p| p.expr.as_ref()).map(|e| e.text.clone()).unwrap_or_default()

crates/ui-egui/tests/it/main.rs

@@ -78,6 +78,7 @@ mod ui_theme_favorites;
 mod ui_time_zoom_controls;
 mod ui_timeline_audio_levels;
 mod ui_timeline_effect_label;
+mod ui_timeline_group_select;
 mod ui_timeline_pan;
 mod ui_timeline_shapes;
 mod ui_timeline_switches;

crates/ui-egui/tests/it/ui_timeline_group_select.rs

@@ -0,0 +1,83 @@
+//! Shift-clicking a group row in the Timeline adds it to the selection, as on a property row, so
+//! several Puppet pins can be selected and deleted together (#1751).
+
+use effectcraft_engine::Session;
+use effectcraft_engine::project::{LayerId, PropGroup};
+use effectcraft_ui_egui::EffectcraftApp;
+use effectcraft_ui_egui::dock::PanelKind;
+use egui::{Modifiers, Pos2, vec2};
+use egui_kittest::Harness;
+use serde_json::json;
+
+fn harness() -> (Harness<'static, EffectcraftApp>, u64, [u64; 3]) {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "Main", "width": 200, "height": 100, "frameRate": 30, "duration": 2})).unwrap();
+    let layer = s.execute("layer.newSolid", json!({"width": 100, "height": 60, "color": [0, 0, 1]})).unwrap()["layer"].as_u64().unwrap();
+    let pin = |s: &mut Session, x: i64| s.execute("puppet.addPin", json!({"layer": layer, "position": [x, 30]})).unwrap()["pin"].as_u64().unwrap();
+    let pins = [pin(&mut s, 10), pin(&mut s, 50), pin(&mut s, 90)];
+    let mut app = EffectcraftApp::new(s);
+    app.show_panel(PanelKind::Timeline);
+    app.ui.timeline.open_layers.insert(layer);
+    // Twirl open every group down to the pins.
+    fn all(g: &PropGroup, out: &mut Vec<u64>) {
+        for c in g.groups() {
+            out.push(c.uid);
+            all(c, out);
+        }
+    }
+    let mut open = vec![];
+    if let Some(l) = app.session.active_comp().and_then(|c| c.layer(LayerId(layer))) {
+        all(&l.props, &mut open);
+    }
+    app.ui.timeline.open_groups.extend(open);
+    let mut h = Harness::builder().with_size(vec2(1600.0, 1100.0)).build_eframe(|_| app);
+    h.run_steps(4);
+    (h, layer, pins)
+}
+
+fn row(h: &Harness<'_, EffectcraftApp>, uid: u64) -> Pos2 {
+    let id = format!("timeline.group.{uid}.name");
+    let e = h.state().auto.find(&id).unwrap_or_else(|| panic!("no {id}"));
+    egui::pos2(e.rect[0] + e.rect[2].min(40.0) / 2.0, e.rect[1] + e.rect[3] / 2.0)
+}
+
+fn click(h: &mut Harness<'_, EffectcraftApp>, uid: u64, modifiers: Modifiers) {
+    let at = row(h, uid);
+    h.input_mut().events.push(egui::Event::ModifiersChanged(modifiers));
+    h.input_mut().events.push(egui::Event::PointerMoved(at));
+    h.input_mut().events.push(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed: true, modifiers });
+    h.input_mut().events.push(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed: false, modifiers });
+    h.run_steps(3);
+    h.input_mut().events.push(egui::Event::ModifiersChanged(Modifiers::NONE));
+    h.step();
+}
+
+fn selected(h: &Harness<'_, EffectcraftApp>, pins: [u64; 3]) -> Vec<u64> {
+    h.state().session.state.selected_props.iter().map(|(_, u)| *u).filter(|u| pins.contains(u)).collect()
+}
+
+fn pin_count(h: &Harness<'_, EffectcraftApp>, layer: u64) -> usize {
+    let l = h.state().session.active_comp().unwrap().layer(LayerId(layer)).unwrap();
+    l.effects().unwrap().groups().map(effectcraft_engine::effects::puppet::pin_count).sum()
+}
+
+#[test]
+fn shift_click_adds_puppet_pins_and_delete_removes_them_together() {
+    let (mut h, layer, pins) = harness();
+    let [a, b, c] = pins;
+    click(&mut h, a, Modifiers::NONE);
+    assert_eq!(selected(&h, pins), [a]);
+    click(&mut h, b, Modifiers::SHIFT);
+    assert_eq!(selected(&h, pins), [a, b], "Shift-click keeps the first pin");
+    click(&mut h, c, Modifiers::NONE);
+    assert_eq!(selected(&h, pins), [c], "a plain click selects only the clicked pin");
+
+    click(&mut h, a, Modifiers::NONE);
+    click(&mut h, b, Modifiers::SHIFT);
+    h.state_mut().session.execute("edit.clear", json!({})).unwrap();
+    h.run_steps(3);
+    assert_eq!(pin_count(&h, layer), 1, "Delete removes both selected pins");
+    assert!(h.state().session.active_comp().unwrap().layer(LayerId(layer)).is_some(), "the layer stays");
+    h.state_mut().session.undo();
+    assert_eq!(pin_count(&h, layer), 3, "one Undo restores both pins");
+}