From 13d0aef57fd99c1f36e10dedcfb95eca224be58f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Fri, 2 Oct 2026 00:16:23 -0400 Subject: [PATCH] M1.5: commands that default to the current layer validate the remembered id (reused after undo it could name a compound path: Send to Current Layer moved art into it, Delete Layer could delete it) Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/layer.rs | 6 ++--- crates/engine/src/cmd/object.rs | 4 +++- crates/engine/src/lib.rs | 4 ++++ crates/engine/src/tests.rs | 39 +++++++++++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/layer.rs b/crates/engine/src/cmd/layer.rs index c4b7f41c..26d0feb8 100644 --- a/crates/engine/src/cmd/layer.rs +++ b/crates/engine/src/cmd/layer.rs @@ -66,7 +66,7 @@ fn new_layer(s: &mut Session, p: &Value) -> Result { } fn new_sublayer(s: &mut Session, p: &Value) -> Result { - let parent = id_param(p, "parent").or(s.doc()?.active_layer).ok_or_else(|| bad("layer.newSublayer", "no parent layer"))?; + let parent = id_param(p, "parent").or(s.doc()?.current_layer()).ok_or_else(|| bad("layer.newSublayer", "no parent layer"))?; let name = str_param(p, "name").map(str::to_string); let id = s.edit("New Sublayer", |d, _| { let n = d.node(parent).and_then(|n| n.children()).map(|c| c.iter().filter(|c| c.is_layer()).count()).unwrap_or(0); @@ -80,7 +80,7 @@ fn new_sublayer(s: &mut Session, p: &Value) -> Result { } fn delete_layer(s: &mut Session, p: &Value) -> Result { - let id = id_param(p, "id").or(s.doc()?.active_layer).ok_or_else(|| bad("layer.delete", "no layer"))?; + let id = id_param(p, "id").or(s.doc()?.current_layer()).ok_or_else(|| bad("layer.delete", "no layer"))?; let st = s.doc()?; if st.doc.layers.len() == 1 && st.doc.layers[0].id == id { return Err(EngineError::Other("a document needs at least one layer".into())); @@ -98,7 +98,7 @@ fn delete_layer(s: &mut Session, p: &Value) -> Result { } fn duplicate_layer(s: &mut Session, p: &Value) -> Result { - let id = id_param(p, "id").or(s.doc()?.active_layer).ok_or_else(|| bad("layer.duplicate", "no layer"))?; + let id = id_param(p, "id").or(s.doc()?.current_layer()).ok_or_else(|| bad("layer.duplicate", "no layer"))?; let nid = s.edit("Duplicate Layer", |d, _| { let (par, idx, _) = d.position(id).ok_or(EngineError::NoNode(id))?; let n = d.node(id).cloned().ok_or(EngineError::NoNode(id))?; diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index 6a123e46..1b6d08fc 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -321,7 +321,9 @@ fn arrange(s: &mut Session, how: Arrange) -> Result { fn send_to_current_layer(s: &mut Session, _: &Value) -> Result { let ids = selected_roots(s)?; - let layer = s.doc()?.active_layer.ok_or_else(|| EngineError::Other("no current layer".into()))?; + // Ids are reused after undo: the remembered current layer must still be a layer. + let st = s.doc()?; + let layer = st.current_layer().or_else(|| st.doc.default_layer()).ok_or_else(|| EngineError::Other("no current layer".into()))?; s.edit("Send to Current Layer", |d, _| { for id in &ids { d.move_node(*id, Some(layer), usize::MAX)?; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 0aa710f1..219a32ca 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -138,6 +138,10 @@ impl DocState { .unwrap_or_else(|| self.doc.title.clone()) } /// Where new art is inserted: the isolation container, else the active layer. + /// The current layer, if the remembered id still names a layer (ids are reused after undo). + pub fn current_layer(&self) -> Option { + self.active_layer.filter(|l| self.doc.node(*l).is_some_and(|n| n.is_layer())) + } pub fn insertion_parent(&self) -> Option { // Ids are reused after undo (the id counter is part of the document), so a remembered // layer or isolated group must still be one: a pasted group must never land in a path. diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index db7dac16..81db4186 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -357,3 +357,42 @@ fn reshape_moves_the_grabbed_point_and_its_neighbourhood() { assert!(tl.y > -40.0); assert!(pts.iter().filter(|p| (p.y - 100.0).abs() < 1e-9).count() == 2); } + +#[test] +fn graph_tool_drag_creates_a_graph_and_asks_for_data() { + let mut s = session(); + let v = ViewInfo::default(); + let undo_before = s.doc().unwrap().history.undo.len(); + s.select_tool("pieGraph", v).unwrap(); + s.pointer(&PointerEvent::new(PointerKind::Down, 100.0, 100.0), v).unwrap(); + s.pointer(&PointerEvent::new(PointerKind::Drag, 300.0, 250.0), v).unwrap(); + let ui = s.pointer(&PointerEvent::new(PointerKind::Up, 300.0, 250.0), v).unwrap(); + assert!(ui.iter().any(|r| matches!(r, crate::UiRequest::Dialog(k, _) if k == "graphData")), "{ui:?}"); + let st = s.doc().unwrap(); + assert_eq!(st.history.undo.len(), undo_before + 1); + let g = st.doc.node(st.selection.objects[0]).unwrap(); + assert_eq!(g.graph.as_ref().unwrap().kind, vectorcraft_doc::GraphKind::Pie); + assert_eq!(g.graph.as_ref().unwrap().rect, vectorcraft_geom::Rect::new(100.0, 100.0, 300.0, 250.0)); +} + +#[test] +fn stale_current_layer_id_never_targets_a_reused_id() { + let mut s = session(); + let a = rect(&mut s, 0.0, 0.0, 10.0, 10.0); + let b = rect(&mut s, 5.0, 5.0, 10.0, 10.0); + let layer = NodeId(s.execute("layer.new", &json!({})).unwrap()["id"].as_u64().unwrap()); + s.execute("edit.undo", &json!({})).unwrap(); + // The undone layer's id is handed out again, here to a compound path. + s.execute("select.set", &json!({"ids": [a.0, b.0]})).unwrap(); + let c = NodeId(s.execute("object.compoundPath.make", &json!({})).unwrap()["id"].as_u64().unwrap()); + assert_eq!(c, layer, "precondition: id reused"); + let d = rect(&mut s, 50.0, 50.0, 10.0, 10.0); + s.execute("select.set", &json!({"ids": [d.0]})).unwrap(); + s.execute("object.arrange.sendToCurrentLayer", &json!({})).unwrap(); + let doc = &s.doc().unwrap().doc; + assert!(doc.node(c).unwrap().children().unwrap().iter().all(|n| n.path_data().is_some())); + assert!(doc.parent_of(d).is_some_and(|p| doc.node(p).unwrap().is_layer())); + // layer.delete without an id must not delete the compound path. + s.execute("layer.delete", &json!({})).ok(); + assert!(s.doc().unwrap().doc.node(c).is_some()); +}