From b3887c226f93813d16873c2f706ae91086c36adc Mon Sep 17 00:00:00 2001 From: Kristian Myram Date: Thu, 29 Oct 2026 21:58:14 +0100 Subject: [PATCH] Graph Editor: keep close focus and locked selections consistent (#1342) --- crates/engine/src/cmd/anim.rs | 15 +++++++-- crates/engine/src/tests/delete_keys.rs | 28 ++++++++++++++++ crates/ui-egui/src/dock_ui.rs | 2 +- crates/ui-egui/src/menus/entries.rs | 2 +- crates/ui-egui/src/menus/invoke.rs | 3 +- crates/ui-egui/src/menus/tests.rs | 44 ++++++++++++++++++++++++++ 6 files changed, 88 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/cmd/anim.rs b/crates/engine/src/cmd/anim.rs index 073262d0f..e2bbab62b 100644 --- a/crates/engine/src/cmd/anim.rs +++ b/crates/engine/src/cmd/anim.rs @@ -55,6 +55,10 @@ 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 { + selected.iter().filter(|k| comp.layer(k.layer).is_some_and(|l| l.switches.locked)).copied().collect() +} + /// Paste copied keyframes at the current time into the target layers' properties. fn paste(s: &mut Session, p: &Value) -> Result { let clip = s.state.key_clipboard.clone(); @@ -117,7 +121,7 @@ fn paste(s: &mut Session, p: &Value) -> Result { } s.edit("Paste Keyframes", merge_p(p), |proj, st| { let comp = proj.comp_mut(cid).ok_or(EngineError::NoComp)?; - let mut sel: Vec = st.selected_keys.iter().filter(|k| comp.layer(k.layer).is_some_and(|l| l.switches.locked)).copied().collect(); + let mut sel = locked_selection(comp, &st.selected_keys); let retained = sel.len(); for (lid, uid, ci) in &plan { let Some(l) = comp.layer_mut(*lid) else { continue }; @@ -210,6 +214,9 @@ pub(super) fn paste_path(s: &mut Session, p: &Value, (cid, lid, uid): (ItemId, L }) .collect(); s.edit("Paste Keyframes", merge_p(p), |proj, st| { + let comp = proj.comp(cid).ok_or(EngineError::NoComp)?; + let mut selected = locked_selection(comp, &st.selected_keys); + let retained = selected.len(); let l = super::layer_mut(proj, cid, lid)?; let pr = l.props.find_mut(uid).ok_or_else(|| bad("keys.paste", "no property"))?; let mut sel = vec![]; @@ -224,8 +231,10 @@ pub(super) fn paste_path(s: &mut Session, p: &Value, (cid, lid, uid): (ItemId, L // Re-timing keeps the keys' order, so the pasted keys are found again by index. let pasted: Vec = pr.keys.iter().enumerate().filter(|(_, k)| sel.contains(&k.time)).map(|(i, _)| i).collect(); retime_roving(&mut pr.keys, pr.spatial); - st.selected_keys = pasted.iter().filter_map(|&i| pr.keys.get(i)).map(|k| KeyRef { layer: lid, prop: uid, time: k.time }).collect(); - Ok(json!(st.selected_keys.len())) + selected.extend(pasted.iter().filter_map(|&i| pr.keys.get(i)).map(|k| KeyRef { layer: lid, prop: uid, time: k.time })); + let count = selected.len().saturating_sub(retained); + st.selected_keys = selected; + Ok(json!(count)) }) } diff --git a/crates/engine/src/tests/delete_keys.rs b/crates/engine/src/tests/delete_keys.rs index a6ca08bed..ec6ec888e 100644 --- a/crates/engine/src/tests/delete_keys.rs +++ b/crates/engine/src/tests/delete_keys.rs @@ -181,6 +181,34 @@ fn key_paste_skips_locked_targets_without_changing_layer_pairing() { assert_eq!(s.project.to_json(), pasted); } +#[test] +fn motion_path_paste_preserves_locked_source_selection() { + for explicit in [false, true] { + let (mut s, source, _) = setup(); + s.execute("mask.new", json!({"layer": source, "vertices": [[0, 0], [20, 10], [40, 0]], "closed": false})).unwrap(); + s.execute("prop.toggleAnimation", json!({"layer": source, "path": "masks/#1/path"})).unwrap(); + let uid = s.active_comp().unwrap().layer(LayerId(source)).unwrap().props.prop("masks/#1/path").unwrap().uid; + s.execute("keys.select", json!({"keys": [{"layer": source, "prop": uid, "time": 0}]})).unwrap(); + s.execute("keys.copy", json!({})).unwrap(); + let locked_keys = s.state.selected_keys.clone(); + let target = s.execute("layer.newSolid", json!({"name": "Target", "color": "#fff"})).unwrap()["layer"].as_u64().unwrap(); + s.execute("prop.select", json!({"layer": target, "path": "transform/position"})).unwrap(); + s.execute("layer.setSwitch", json!({"layers": [source], "switch": "locked", "value": true})).unwrap(); + s.state.selected_keys = locked_keys.clone(); + let params = if explicit { json!({"layer": target, "path": "transform/position"}) } else { json!({}) }; + let before = s.project.to_json(); + assert_eq!(s.execute("keys.paste", params).unwrap(), json!(3)); + assert!(locked_keys.iter().all(|k| s.state.selected_keys.contains(k))); + assert_eq!(s.state.selected_keys.len(), 4); + assert_eq!(s.state.selected_keys.iter().filter(|k| k.layer == LayerId(target)).count(), 3); + let pasted = s.project.to_json(); + s.execute("edit.undo", json!({})).unwrap(); + assert_eq!(s.project.to_json(), before); + s.execute("edit.redo", json!({})).unwrap(); + assert_eq!(s.project.to_json(), pasted); + } +} + #[test] fn key_transform_preserves_locked_keys_in_a_mixed_selection() { let (mut s, locked, locked_prop) = setup(); diff --git a/crates/ui-egui/src/dock_ui.rs b/crates/ui-egui/src/dock_ui.rs index 30710392a..750764b84 100644 --- a/crates/ui-egui/src/dock_ui.rs +++ b/crates/ui-egui/src/dock_ui.rs @@ -398,7 +398,7 @@ impl EffectcraftApp { /// A press in a panel (its body, tab or group header) focuses it; another Composition viewer /// also becomes the active one. - fn focus_panel(&mut self, ctx: &egui::Context, p: PanelKind) { + pub(crate) fn focus_panel(&mut self, ctx: &egui::Context, p: PanelKind) { self.ui.focused = p; panels::viewer_text::panel_clicked(self, ctx, p); if let Some(id) = panels::viewers::id_of(p) { diff --git a/crates/ui-egui/src/menus/entries.rs b/crates/ui-egui/src/menus/entries.rs index e9a99522e..cb3a0042b 100644 --- a/crates/ui-egui/src/menus/entries.rs +++ b/crates/ui-egui/src/menus/entries.rs @@ -125,7 +125,7 @@ pub(crate) fn entry_enabled(app: &EffectcraftApp, e: &MenuEntry) -> bool { return !app.session.is_rendering() && !app.session.project.render_queue.is_empty(); } if e.command == "file.close" && super::invoke::no_params(&e.params) && app.ui.focused == PanelKind::RenderQueue { - return true; + return !app.ui.dock.locked; } if e.command == "window.panel" { return true; diff --git a/crates/ui-egui/src/menus/invoke.rs b/crates/ui-egui/src/menus/invoke.rs index fbd4fa1fe..b3a29d66a 100644 --- a/crates/ui-egui/src/menus/invoke.rs +++ b/crates/ui-egui/src/menus/invoke.rs @@ -217,12 +217,13 @@ pub fn invoke(app: &mut EffectcraftApp, ctx: &egui::Context, id: &str, params: V // Close follows panel focus too: with the Render Queue active, Cmd+W closes that panel and // leaves the active composition open (it used to close the composition next to it). if id == "file.close" && no_params(¶ms) && app.ui.focused == PanelKind::RenderQueue { - app.close_panel(PanelKind::RenderQueue); + app.apply_dock(craft_ui::docking::Action::Close { panel: PanelKind::RenderQueue })?; // The focus goes to a panel that is still there, so the next Close acts on the composition // instead of on the queue that is gone. let next = [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); 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 a043f67d1..83ba78dc1 100644 --- a/crates/ui-egui/src/menus/tests.rs +++ b/crates/ui-egui/src/menus/tests.rs @@ -473,6 +473,50 @@ fn closing_the_render_queue_reveals_the_panel_receiving_focus() { } } +#[test] +fn queue_close_preserves_state_when_workspace_is_locked() { + let ctx = egui::Context::default(); + let mut app = EffectcraftApp::new(effectcraft_engine::Session::default()); + app.session.execute("comp.new", json!({"name": "A", "duration": 2})).unwrap(); + app.show_panel(PanelKind::RenderQueue); + app.ui.dock.locked = true; + let workspace = app.ui.dock.clone(); + let comp = app.session.active_comp_id(); + for _ in 0..2 { + assert!(invoke(&mut app, &ctx, "file.close", Value::Null).is_err()); + assert_eq!(app.ui.dock, workspace); + assert_eq!(app.ui.focused, PanelKind::RenderQueue); + assert_eq!(app.session.active_comp_id(), comp); + } + assert!(!menu_items(&app).iter().find(|item| item.id == "file.close").unwrap().enabled); + app.ui.dock.locked = false; + invoke(&mut app, &ctx, "file.close", Value::Null).unwrap(); + assert!(!app.ui.dock.is_open(PanelKind::RenderQueue)); +} + +#[test] +fn queue_close_preserves_viewer_command_target() { + let ctx = egui::Context::default(); + let mut app = EffectcraftApp::new(effectcraft_engine::Session::default()); + let a = app.session.execute("comp.new", json!({"name": "A", "duration": 2})).unwrap()["comp"].as_u64().unwrap(); + let b = app.session.execute("comp.new", json!({"name": "B", "duration": 2})).unwrap()["comp"].as_u64().unwrap(); + 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.close_panel(PanelKind::Timeline); + app.show_panel(PanelKind::Composition); + app.show_panel(PanelKind::RenderQueue); + assert_eq!(app.session.active_comp_id().map(|id| id.0), Some(b)); + invoke(&mut app, &ctx, "file.close", json!({})).unwrap(); + 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)); + invoke(&mut 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)); +} + #[test] fn menu_items_cover_the_tree() { assert_eq!(menus().last(), Some(&"Help"));