From d7011a36b8dbae2c9d926d70b2cdbf28fe2ed8ee Mon Sep 17 00:00:00 2001 From: arnav-hiwarkar Date: Thu, 12 Nov 2026 18:22:21 +0530 Subject: [PATCH] #2817: Closed falls back to the layer's paths when the selection names none Selecting a non-path property (Position) or a stale selection disabled Closed, unlike RotoBezier and the other Mask and Shape Path commands, which fall back to every mask of the selected layers. A selected mask property (Feather, Opacity, Expansion) now selects its mask. A selected locked mask still counts as a match, so it never falls back to its unlocked siblings. --- crates/engine/src/cmd/paths/closed.rs | 51 ++++++++++++++++----------- crates/engine/src/tests/viewer.rs | 28 ++++++++++----- 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/crates/engine/src/cmd/paths/closed.rs b/crates/engine/src/cmd/paths/closed.rs index 268e68843..12ccb10ac 100644 --- a/crates/engine/src/cmd/paths/closed.rs +++ b/crates/engine/src/cmd/paths/closed.rs @@ -1,7 +1,7 @@ //! The selection-aware Closed menu action. Explicit edits retain `mask.setClosed`'s API. use effectcraft_keyframe::Value as KV; -use effectcraft_project::{GroupKind, Layer, LayerId, PropGroup, Uid}; +use effectcraft_project::{GroupKind, Layer, LayerId, Node, PropGroup, Uid}; use effectcraft_time::Tick; use serde_json::{Value, json}; @@ -14,6 +14,7 @@ struct Target { layer: LayerId, route: Vec, closed: bool, + editable: bool, } struct Selection { @@ -30,58 +31,66 @@ fn group_at_mut<'a>(mut group: &'a mut PropGroup, route: &[usize]) -> Option<&'a Some(group) } +/// The selected paths on the selected layers, else every path of those layers (as the other +/// Mask and Shape Path commands do). Locked layers and masks count as matches, so selecting one +/// never falls back to its siblings, and are then left out. fn targets(s: &Session) -> Result> { let comp = s.active_comp().ok_or(EngineError::NoComp)?; + let on_selected = |lid: &LayerId| s.state.selected_layers.contains(lid); // Layer navigation and insertion can retain point/property selections on other layers. let selection = Selection { - vertices: s.state.selected_vertices.iter().filter(|v| s.state.selected_layers.contains(&v.layer)).copied().collect(), - props: s.state.selected_props.iter().filter(|(lid, _)| s.state.selected_layers.contains(lid)).copied().collect(), + vertices: s.state.selected_vertices.iter().filter(|v| on_selected(&v.layer)).copied().collect(), + props: s.state.selected_props.iter().filter(|(lid, _)| on_selected(lid)).copied().collect(), time: s.time(), }; - let whole_layers = selection.vertices.is_empty() && selection.props.is_empty(); let mut targets = Vec::new(); - for layer in comp.layers.iter() { - let selected_layer = if whole_layers { - s.state.selected_layers.contains(&layer.id) - } else { - selection.vertices.iter().any(|v| v.layer == layer.id) || selection.props.iter().any(|(lid, _)| *lid == layer.id) - }; - if !selected_layer || layer.switches.locked { - continue; + for whole_layers in [false, true] { + for layer in comp.layers.iter().filter(|l| on_selected(&l.id)) { + collect(&selection, layer, &layer.props, &mut Vec::new(), whole_layers, layer.switches.locked, &mut targets)?; + } + if !targets.is_empty() { + break; } - collect(&selection, layer, &layer.props, &mut Vec::new(), whole_layers, &mut targets)?; } + targets.retain(|target| target.editable); if targets.is_empty() { return Err(bad("path.toggleClosed", "select an unlocked mask or Bezier shape path")); } Ok(targets) } -fn collect(selection: &Selection, layer: &Layer, group: &PropGroup, route: &mut Vec, inherited: bool, targets: &mut Vec) -> Result<()> { +fn collect( + selection: &Selection, + layer: &Layer, + group: &PropGroup, + route: &mut Vec, + inherited: bool, + locked: bool, + targets: &mut Vec, +) -> Result<()> { if route.len() > MAX_DEPTH { return Err(bad("path.toggleClosed", "path group nesting exceeds 128 levels")); } - if matches!(group.kind, GroupKind::Mask { locked: true, .. }) { - return Ok(()); - } + let locked = locked || matches!(group.kind, GroupKind::Mask { locked: true, .. }); let selected = inherited || selection.props.contains(&(layer.id, group.uid)); if super::super::mask::is_path_group(group) && let Some(prop) = group.get("path") && let KV::Path(path) = prop.value_at(layer.layer_time(selection.time)) { - // Property selection also selects its points; both describe the selected paths. + // Selecting any of a path group's properties (Mask Path, Feather, Opacity…) or its points + // selects the path. let selected = selected - || selection.props.contains(&(layer.id, prop.uid)) + || group.children.iter().any(|c| matches!(c, Node::Prop(p) if selection.props.contains(&(layer.id, p.uid)))) || selection.vertices.iter().any(|v| v.layer == layer.id && v.mask == group.uid && v.index < path.vertices.len()); if selected { - targets.push(Target { layer: layer.id, route: route.clone(), closed: path.closed }); + targets.push(Target { layer: layer.id, route: route.clone(), closed: path.closed, editable: !locked }); } return Ok(()); } for (index, child) in group.children.iter().enumerate() { let Some(child) = child.as_group() else { continue }; route.push(index); - collect(selection, layer, child, route, selected, targets)?; + collect(selection, layer, child, route, selected, locked, targets)?; route.pop(); } Ok(()) diff --git a/crates/engine/src/tests/viewer.rs b/crates/engine/src/tests/viewer.rs index 36fa846e4..45517e3cd 100644 --- a/crates/engine/src/tests/viewer.rs +++ b/crates/engine/src/tests/viewer.rs @@ -434,7 +434,10 @@ fn toggle_closed_ignores_selections_retained_on_other_layers() { s.execute("layer.selectPrevious", json!({})).unwrap(); assert_eq!(s.state.selected_layers, vec![LayerId(b_layer)]); s.execute("prop.select", json!({"layer": b_layer, "path": "transform/position"})).unwrap(); - assert!(!s.is_enabled("path.toggleClosed"), "a non-path property does not select sibling paths"); + assert_eq!(crate::menus::checked(&s, "path.toggleClosed", &json!({})), Some(true), "a non-path property falls back to the layer's paths"); + s.execute("path.toggleClosed", json!({})).unwrap(); + assert!(!shape_path(&s, b_layer, b_path).closed); + s.execute("edit.undo", json!({})).unwrap(); s.execute("layer.selectNext", json!({})).unwrap(); s.execute("path.toggleClosed", json!({})).unwrap(); assert!(shape_path(&s, a_layer, a_path).closed, "a property retained on another layer cannot block this layer"); @@ -442,20 +445,22 @@ fn toggle_closed_ignores_selections_retained_on_other_layers() { } #[test] -fn toggle_closed_rejects_locked_stale_and_nonpath_selection() { +fn toggle_closed_falls_back_from_stale_selection_and_rejects_locked_or_pathless_layers() { let mut s = comp(); let r = s.execute("shape.newPath", json!({"vertices": [[0, 0], [100, 0], [100, 100]], "closed": true})).unwrap(); let (lid, uid) = (r["layer"].as_u64().unwrap(), r["path"].as_u64().unwrap()); let before = s.project.clone(); s.state.selected_vertices.clear(); s.state.selected_props = vec![(LayerId(lid), u64::MAX)]; - assert!(!s.is_enabled("path.toggleClosed")); - assert!(s.execute("path.toggleClosed", json!({})).is_err()); + s.execute("path.toggleClosed", json!({})).unwrap(); + assert!(!shape_path(&s, lid, uid).closed, "a stale property selection falls back to the layer's paths"); + s.execute("edit.undo", json!({})).unwrap(); assert!(s.project.same_content(&before)); s.state.selected_props.clear(); s.state.selected_vertices = vec![crate::VertexRef { layer: LayerId(lid), mask: uid, index: usize::MAX }]; - assert!(!s.is_enabled("path.toggleClosed")); - assert!(s.execute("path.toggleClosed", json!({})).is_err()); + s.execute("path.toggleClosed", json!({})).unwrap(); + assert!(!shape_path(&s, lid, uid).closed, "a stale vertex selection falls back to the layer's paths"); + s.execute("edit.undo", json!({})).unwrap(); s.state.selected_vertices.clear(); let cid = s.active_comp_id().unwrap(); std::sync::Arc::make_mut(&mut s.project).comp_mut(cid).unwrap().layer_mut(LayerId(lid)).unwrap().switches.locked = true; @@ -475,10 +480,16 @@ fn toggle_closed_handles_masks_and_preserves_explicit_set_closed() { let uid = s.execute("mask.new", json!({"layer": lid, "vertices": [[0, 0], [100, 0], [100, 100]], "closed": true})).unwrap()["mask"].as_u64().unwrap(); let path = |s: &Session| layer(s, lid).props.find_group(uid).unwrap().get("path").unwrap().value.as_path().unwrap().clone(); let original = path(&s); - s.state.selected_props = vec![(LayerId(lid), uid)]; + let other = s.execute("mask.new", json!({"layer": lid, "vertices": [[10, 10], [90, 10], [90, 90]], "closed": true})).unwrap()["mask"].as_u64().unwrap(); + let other_closed = |s: &Session| layer(s, lid).props.find_group(other).unwrap().get("path").unwrap().value.as_path().unwrap().closed; + let feather = layer(&s, lid).props.find_group(uid).unwrap().get("feather").unwrap().uid; + s.state.selected_vertices.clear(); + s.state.selected_props = vec![(LayerId(lid), feather)]; s.execute("path.toggleClosed", json!({})).unwrap(); - assert!(!path(&s).closed); + assert!(!path(&s).closed, "selecting Mask Feather selects its mask"); + assert!(other_closed(&s), "an unselected mask stays closed"); assert!(layer(&s, lid).props.find_group(uid).unwrap().get("path").unwrap().keys.is_empty(), "a static path gains no keyframe"); + s.state.selected_props = vec![(LayerId(lid), uid)]; s.execute("mask.setClosed", json!({"layer": lid, "mask": uid})).unwrap(); assert_eq!(path(&s), original, "explicit API still defaults to closed"); s.execute("mask.setClosed", json!({"layer": lid, "mask": uid, "closed": false})).unwrap(); @@ -493,6 +504,7 @@ fn toggle_closed_handles_masks_and_preserves_explicit_set_closed() { assert!(!s.is_enabled("path.toggleClosed")); assert!(s.execute("path.toggleClosed", json!({})).is_err()); assert_eq!(path(&s), original); + assert!(other_closed(&s), "a selected locked mask does not fall back to its unlocked siblings"); } #[test]