From f93d336f9d7bc0dbd61822a4651c7dfd903f3b7c Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Thu, 29 Oct 2026 22:38:53 +0100 Subject: [PATCH] Graph Editor: protect selection tools and restore queue-close focus --- crates/engine/src/cmd/anim.rs | 2 +- crates/engine/src/cmd/anim_tools.rs | 5 +- crates/engine/src/cmd/animation.rs | 3 +- crates/engine/src/cmd/keys_more.rs | 2 + crates/engine/src/cmd/mask_interp.rs | 12 +++-- crates/engine/src/cmd/prop.rs | 9 ++++ crates/engine/src/tests/anim_tools.rs | 74 +++++++++++++++++++++++++++ crates/engine/src/tests/mask_warp.rs | 45 ++++++++++++++++ crates/ui-egui/src/app/frame.rs | 2 +- crates/ui-egui/src/menus/invoke.rs | 4 ++ crates/ui-egui/src/menus/tests.rs | 46 +++++++++++++++++ 11 files changed, 194 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/cmd/anim.rs b/crates/engine/src/cmd/anim.rs index e2bbab62b..1064ba021 100644 --- a/crates/engine/src/cmd/anim.rs +++ b/crates/engine/src/cmd/anim.rs @@ -55,7 +55,7 @@ fn compatible(a: &Property, b: &effectcraft_keyframe::Value) -> bool { a.value.kind_name() == b.kind_name() && a.value.components().len() == b.components().len() } -fn locked_selection(comp: &effectcraft_project::Comp, selected: &[KeyRef]) -> Vec { +pub(crate) fn locked_selection(comp: &effectcraft_project::Comp, selected: &[KeyRef]) -> Vec { selected.iter().filter(|k| comp.layer(k.layer).is_some_and(|l| l.switches.locked)).copied().collect() } diff --git a/crates/engine/src/cmd/anim_tools.rs b/crates/engine/src/cmd/anim_tools.rs index 06e0d341b..33ee39ae2 100644 --- a/crates/engine/src/cmd/anim_tools.rs +++ b/crates/engine/src/cmd/anim_tools.rs @@ -213,7 +213,8 @@ fn sample_curve( fn grouped_selection(s: &Session) -> std::collections::BTreeMap<(LayerId, Uid), Vec> { let mut g: std::collections::BTreeMap<(LayerId, Uid), Vec> = Default::default(); - for k in &s.state.selected_keys { + let Some(comp) = s.active_comp() else { return g }; + for k in super::prop::editable_keys(comp, &s.state.selected_keys) { g.entry((k.layer, k.prop)).or_default().push(k.time); } for v in g.values_mut() { @@ -273,7 +274,7 @@ fn run_on_selection( s.edit(label, None, |proj, st| { let comp = proj.comp_mut(cid).ok_or(EngineError::NoComp)?; let fr = comp.frame_rate; - let mut sel = vec![]; + let mut sel = super::anim::locked_selection(comp, &st.selected_keys); for ((lid, uid), times) in groups { let Some(l) = comp.layer_mut(lid) else { continue }; let lc = l.clone(); diff --git a/crates/engine/src/cmd/animation.rs b/crates/engine/src/cmd/animation.rs index 1bdf3fed7..c781e9da1 100644 --- a/crates/engine/src/cmd/animation.rs +++ b/crates/engine/src/cmd/animation.rs @@ -209,7 +209,8 @@ fn add_keyframe(s: &mut Session, _: &Value) -> Result { /// Selected keys grouped by (layer, property). fn keys_by_prop(s: &Session) -> BTreeMap<(LayerId, Uid), Vec> { let mut m: BTreeMap<(LayerId, Uid), Vec> = BTreeMap::new(); - for k in &s.state.selected_keys { + let Some(comp) = s.active_comp() else { return m }; + for k in super::prop::editable_keys(comp, &s.state.selected_keys) { m.entry((k.layer, k.prop)).or_default().push(k.time); } m diff --git a/crates/engine/src/cmd/keys_more.rs b/crates/engine/src/cmd/keys_more.rs index c7785d08c..4edef885a 100644 --- a/crates/engine/src/cmd/keys_more.rs +++ b/crates/engine/src/cmd/keys_more.rs @@ -37,6 +37,8 @@ pub(crate) fn label_keys(s: &mut Session, p: &Value) -> Result { return Err(bad("keys.label", "select keyframes first")); } let cid = comp_id(s, p)?; + let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?; + let keys = super::prop::editable_keys(comp, &keys); let n = s.edit("Keyframe Label", None, |proj, _| { let mut n = 0; for k in &keys { diff --git a/crates/engine/src/cmd/mask_interp.rs b/crates/engine/src/cmd/mask_interp.rs index 982468c8e..bbcc9f99d 100644 --- a/crates/engine/src/cmd/mask_interp.rs +++ b/crates/engine/src/cmd/mask_interp.rs @@ -437,20 +437,22 @@ fn interpolate(s: &mut Session, p: &Value) -> Result { // The layer: given, else the layer of selected Mask Path keys, else the selection. let cid = super::comp_id(s, p)?; let comp = s.project.comp(cid).ok_or(EngineError::NoComp)?; + let selected = super::prop::editable_keys(comp, &s.state.selected_keys); let lid = match p.get("layer") { Some(v) => super::resolve_layer(comp, v).ok_or_else(|| bad(CMD, format!("no layer {v}")))?, - None => match s.state.selected_keys.iter().find(|k| comp.layer(k.layer).is_some_and(|l| l.masks().is_some_and(|m| m.find(k.prop).is_some()))) { + None => match selected.iter().find(|k| comp.layer(k.layer).is_some_and(|l| l.masks().is_some_and(|m| m.find(k.prop).is_some()))) { Some(k) => k.layer, None => layer_p(s, p, CMD)?.1, }, }; let layer = comp.layer(lid).ok_or(EngineError::NoComp)?; + if !super::prop::key_layer_editable(comp, lid) { + return Err(bad(CMD, "the layer is locked")); + } let masks = layer.masks().ok_or_else(|| bad(CMD, "the layer has no masks"))?; let uid = match p.get("mask") { Some(_) => mask_uid(s, layer, p), - None => s - .state - .selected_keys + None => selected .iter() .filter(|k| k.layer == lid) .find_map(|k| masks.groups().find(|g| g.get("path").is_some_and(|pr| pr.uid == k.prop)).map(|g| g.uid)) @@ -465,7 +467,7 @@ fn interpolate(s: &mut Session, p: &Value) -> Result { let ts: Vec = a.iter().filter_map(Value::as_f64).map(|t| layer.layer_time(comp.frame_rate.snap_nearest(Tick::from_seconds_f64(t)))).collect(); ts.into_iter().filter_map(|t| path_prop.keys.iter().find(|k| (k.time.0 - t.0).abs() <= comp.frame_duration().0 / 2).map(|k| k.time)).collect() } - None => s.state.selected_keys.iter().filter(|k| k.layer == lid && k.prop == path_prop.uid).map(|k| k.time).collect(), + None => selected.iter().filter(|k| k.layer == lid && k.prop == path_prop.uid).map(|k| k.time).collect(), }; times.sort(); times.dedup(); diff --git a/crates/engine/src/cmd/prop.rs b/crates/engine/src/cmd/prop.rs index 0eff1dd09..b08021b66 100644 --- a/crates/engine/src/cmd/prop.rs +++ b/crates/engine/src/cmd/prop.rs @@ -883,6 +883,15 @@ fn select_keys(s: &mut Session, p: &Value) -> Result { Ok(out) } +/// References that can be edited, including explicit key lists supplied by commands. +pub(crate) fn editable_keys(comp: &effectcraft_project::Comp, selected: &[KeyRef]) -> Vec { + selected.iter().filter(|key| key_layer_editable(comp, key.layer)).copied().collect() +} + +pub(crate) fn key_layer_editable(comp: &effectcraft_project::Comp, layer: LayerId) -> bool { + comp.layer(layer).is_some_and(|layer| !layer.switches.locked) +} + /// Apply `f` to every selected key; keeps the selection pointing at moved keys. pub(crate) fn edit_keys(s: &mut Session, label: &str, merge: Option<&str>, f: impl Fn(&mut Vec, usize, &mut Tick) -> bool) -> Result { let cid = s.active_comp_id().ok_or(EngineError::NoComp)?; diff --git a/crates/engine/src/tests/anim_tools.rs b/crates/engine/src/tests/anim_tools.rs index 06d194001..96b861972 100644 --- a/crates/engine/src/tests/anim_tools.rs +++ b/crates/engine/src/tests/anim_tools.rs @@ -9,6 +9,80 @@ fn select(s: &mut crate::Session, layer: u64, path: &str) { s.execute("prop.select", json!({"layer": layer, "path": path})).unwrap(); } +fn selection_command_skips_locked_keys(command: &str, path: &str, values: [serde_json::Value; 3], mut params: serde_json::Value) { + let (mut s, locked) = setup(); + let unlocked = s.execute("layer.newSolid", json!({"name": "Editable", "width": 100, "height": 100})).unwrap()["layer"].as_u64().unwrap(); + for layer in [locked, unlocked] { + animate(&mut s, layer, path, &[(0.0, values[0].clone()), (0.5, values[1].clone()), (1.0, values[2].clone())]); + } + let before = prop(&s, locked, path).keys; + let selected: Vec<_> = [locked, unlocked] + .into_iter() + .flat_map(|layer| { + let pr = prop(&s, layer, path); + pr.keys.into_iter().map(move |key| crate::KeyRef { layer: effectcraft_project::LayerId(layer), prop: pr.uid, time: key.time }) + }) + .collect(); + s.execute("layer.setSwitch", json!({"layers": [locked], "switch": "locked", "value": true})).unwrap(); + s.state.selected_keys = selected.clone(); + if params.get("keys").is_some() { + params["keys"] = serde_json::to_value(&selected).unwrap(); + } + s.execute_checked(command, params.clone()).unwrap(); + assert_eq!(prop(&s, locked, path).keys, before, "{command} changed a locked layer"); + let after = prop(&s, unlocked, path).keys; + assert_ne!(after, before, "{command} must still edit unlocked keys"); + let after_selection = s.state.selected_keys.clone(); + assert!(selected.iter().filter(|k| k.layer.0 == locked).all(|key| after_selection.contains(key))); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(prop(&s, unlocked, path).keys, before); + assert!(selected.iter().filter(|key| key.layer.0 == locked).all(|key| s.state.selected_keys.contains(key))); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(prop(&s, unlocked, path).keys, after); + assert_eq!(s.state.selected_keys, after_selection); + // All-locked input must preserve the selection and document as well. + s.state.selected_keys.retain(|k| k.layer.0 == locked); + if params.get("keys").is_some() { + params["keys"] = serde_json::to_value(&s.state.selected_keys).unwrap(); + } + let locked_selection = s.state.selected_keys.clone(); + let project = serde_json::to_value(&*s.project).unwrap(); + let _ = s.execute_checked(command, params); + assert_eq!(serde_json::to_value(&*s.project).unwrap(), project); + assert_eq!(s.state.selected_keys, locked_selection); +} + +#[test] +fn wiggler_preserves_locked_keys_in_mixed_selection() { + selection_command_skips_locked_keys( + "keys.wiggle", + "transform/opacity", + [json!(10), json!(90), json!(20)], + json!({"apply": "temporal", "frequency": 8, "magnitude": 10}), + ); +} + +#[test] +fn smoother_preserves_locked_keys_in_mixed_selection() { + selection_command_skips_locked_keys("keys.smooth", "transform/opacity", [json!(10), json!(50), json!(90)], json!({"tolerance": 1})); +} + +#[test] +fn exponential_scale_preserves_locked_keys_in_mixed_selection() { + selection_command_skips_locked_keys( + "keys.exponentialScale", + "transform/scale", + [json!([100, 100, 100]), json!([150, 150, 150]), json!([400, 400, 400])], + json!({}), + ); +} + +#[test] +fn edit_label_preserves_locked_keys_in_mixed_selection() { + selection_command_skips_locked_keys("edit.label", "transform/opacity", [json!(10), json!(50), json!(90)], json!({"label": "Red"})); + selection_command_skips_locked_keys("edit.label", "transform/opacity", [json!(10), json!(50), json!(90)], json!({"label": "Red", "keys": true})); +} + #[test] fn wiggle_adds_keys_at_frequency_within_magnitude() { let (mut s, l) = setup(); diff --git a/crates/engine/src/tests/mask_warp.rs b/crates/engine/src/tests/mask_warp.rs index adcea44be..5387fe562 100644 --- a/crates/engine/src/tests/mask_warp.rs +++ b/crates/engine/src/tests/mask_warp.rs @@ -184,6 +184,51 @@ fn mask_interpolation_creates_matching_keys() { assert!(s.execute_checked("mask.interpolate", json!({"matchingMethod": "magic", "times": [0, 0.8], "layer": clip.0})).is_err()); } +#[test] +fn mask_interpolation_skips_locked_selection_and_rejects_explicit_locked_targets() { + let (mut s, locked, _) = setup(patch_frames(rotating_pose)); + let unlocked = effectcraft_project::LayerId(s.execute("layer.newSolid", json!({"width": 100, "height": 100})).unwrap()["layer"].as_u64().unwrap()); + let a = ShapePath::rect([20.0, 20.0], 10.0, 10.0); + let b = ShapePath::rect([40.0, 30.0], 20.0, 20.0); + let cid = s.active_comp_id().unwrap(); + let mut selected = vec![]; + let mut masks = vec![]; + for layer in [locked, unlocked] { + let uid = new_mask(&mut s, layer, &a); + masks.push(uid); + s.edit("keys", None, |project, _| { + let pr = project.comp_mut(cid).unwrap().layer_mut(layer).unwrap().props.find_group_mut(uid).unwrap().get_mut("path").unwrap(); + pr.keys = vec![Keyframe::new(frame_time(0), KV::Path(a.clone())), Keyframe::new(frame_time(20), KV::Path(b.clone()))]; + selected.extend(pr.keys.iter().map(|key| crate::KeyRef { layer, prop: pr.uid, time: key.time })); + Ok(()) + }) + .unwrap(); + } + s.execute("layer.setSwitch", json!({"layers": [locked.0], "switch": "locked", "value": true})).unwrap(); + s.state.selected_keys = selected.clone(); + let before = mask_path(&s, locked, masks[0]).keys; + let options = serde_json::to_value(&s.state.mask_interp).unwrap(); + let project = serde_json::to_value(&*s.project).unwrap(); + let error = s.execute_checked("mask.interpolate", json!({"layer": locked.0, "mask": masks[0], "times": [0.0, 0.8], "quality": 75})).unwrap_err(); + assert!(error.to_string().contains("locked"), "{error}"); + assert_eq!(serde_json::to_value(&*s.project).unwrap(), project); + assert_eq!(serde_json::to_value(&s.state.mask_interp).unwrap(), options); + assert_eq!(s.state.selected_keys, selected); + s.execute_checked("mask.interpolate", json!({})).unwrap(); + assert_eq!(mask_path(&s, locked, masks[0]).keys, before); + assert_eq!(mask_path(&s, unlocked, masks[1]).keys.len(), 21); + assert_eq!(s.state.selected_keys, selected); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(mask_path(&s, unlocked, masks[1]).keys.len(), 2); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(mask_path(&s, unlocked, masks[1]).keys.len(), 21); + s.state.selected_keys.retain(|key| key.layer == locked); + let selection = s.state.selected_keys.clone(); + assert!(s.execute_checked("mask.interpolate", json!({})).is_err()); + assert_eq!(mask_path(&s, locked, masks[0]).keys, before); + assert_eq!(s.state.selected_keys, selection); +} + // ---------------------------------------------------------------- Warp Stabilizer /// Camera offset on frame `f`: a slow pan plus jitter. diff --git a/crates/ui-egui/src/app/frame.rs b/crates/ui-egui/src/app/frame.rs index d41e0a9d0..a76e7dce1 100644 --- a/crates/ui-egui/src/app/frame.rs +++ b/crates/ui-egui/src/app/frame.rs @@ -90,7 +90,7 @@ impl EffectcraftApp { /// Apply the engine events drained this tick: open a comp, show a toast, open a URL, run a frontend command, /// purge frame caches or copy an image; command and clipboard failures go to `app.ui.status`. - pub(super) fn handle_events(&mut self, ctx: &egui::Context) { + pub(crate) fn handle_events(&mut self, ctx: &egui::Context) { for ev in self.session.drain_events() { match ev { effectcraft_engine::Event::OpenComp(c) => { diff --git a/crates/ui-egui/src/menus/invoke.rs b/crates/ui-egui/src/menus/invoke.rs index b3a29d66a..446513f05 100644 --- a/crates/ui-egui/src/menus/invoke.rs +++ b/crates/ui-egui/src/menus/invoke.rs @@ -224,6 +224,10 @@ pub fn invoke(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, params: V [PanelKind::Timeline, PanelKind::Composition, PanelKind::Project].into_iter().find(|p| app.ui.dock.is_visible(*p)).unwrap_or(PanelKind::Project); app.show_panel(next); app.focus_panel(ctx, next); + // Activating a passive viewer emits OpenComp, which raises Timeline. Finish that routing + // before restoring the destination panel, including when they share the same tab group. + app.handle_events(ctx); + app.show_panel(next); return Ok(Value::Null); } // Edit ▸ Paste with SVG markup on the system clipboard pastes it as a shape layer (After diff --git a/crates/ui-egui/src/menus/tests.rs b/crates/ui-egui/src/menus/tests.rs index 83ba78dc1..63fac9879 100644 --- a/crates/ui-egui/src/menus/tests.rs +++ b/crates/ui-egui/src/menus/tests.rs @@ -504,6 +504,12 @@ fn queue_close_preserves_viewer_command_target() { app.ui.viewers.insert(0, Some(a)); app.ui.viewers.insert(1, Some(b)); app.ui.active_viewer = 1; + app.apply_dock(craft_ui::docking::Action::Move { + panel: PanelKind::Timeline, + anchor: PanelKind::Composition, + placement: craft_ui::docking::Placement::Tab { before: None }, + }) + .unwrap(); app.close_panel(PanelKind::Timeline); app.show_panel(PanelKind::Composition); app.show_panel(PanelKind::RenderQueue); @@ -517,6 +523,46 @@ fn queue_close_preserves_viewer_command_target() { assert!(app.session.state.open_comps.iter().any(|id| id.0 == b)); } +#[test] +fn queue_close_keeps_passive_viewer_focus_across_frames() { + let mut session = effectcraft_engine::Session::default(); + let a = session.execute("comp.new", json!({"name": "A", "width": 32, "height": 32, "duration": 2})).unwrap()["comp"].as_u64().unwrap(); + let b = session.execute("comp.new", json!({"name": "B", "width": 32, "height": 32, "duration": 2})).unwrap()["comp"].as_u64().unwrap(); + let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_eframe(|_| EffectcraftApp::new(session)); + h.run_steps(3); + let ctx = h.ctx.clone(); + let app = h.state_mut(); + app.show_panel(PanelKind::Viewer(1)); + app.ui.viewers.insert(0, Some(a)); + app.ui.viewers.insert(1, Some(b)); + app.ui.active_viewer = 1; + app.apply_dock(craft_ui::docking::Action::Move { + panel: PanelKind::Timeline, + anchor: PanelKind::Composition, + placement: craft_ui::docking::Placement::Tab { before: None }, + }) + .unwrap(); + app.close_panel(PanelKind::Timeline); + app.show_panel(PanelKind::Composition); + app.show_panel(PanelKind::RenderQueue); + invoke(app, &ctx, "file.close", Value::Null).unwrap(); + h.run_steps(4); + let app = h.state_mut(); + assert_eq!(app.ui.focused, PanelKind::Composition); + assert_eq!(app.ui.active_viewer, 0); + assert_eq!(app.session.active_comp_id().map(|id| id.0), Some(a)); + assert!(app.ui.dock.is_visible(PanelKind::Composition)); + assert!(!app.ui.dock.is_open(PanelKind::RenderQueue)); + invoke(app, &ctx, "file.close", json!({})).unwrap(); + assert!(!app.session.state.open_comps.iter().any(|id| id.0 == a)); + assert!(app.session.state.open_comps.iter().any(|id| id.0 == b)); + // A later explicit Open still routes normally, rather than inheriting a deferred focus override. + invoke(app, &ctx, "comp.open", json!({"comp": b})).unwrap(); + h.run_steps(3); + assert!(h.state().ui.dock.is_visible(PanelKind::Timeline)); + assert_eq!(h.state().session.active_comp_id().map(|id| id.0), Some(b)); +} + #[test] fn menu_items_cover_the_tree() { assert_eq!(menus().last(), Some(&"Help"));