diff --git a/crates/engine/src/cmd/props.rs b/crates/engine/src/cmd/props.rs index 7297afad9..8f8aa454a 100644 --- a/crates/engine/src/cmd/props.rs +++ b/crates/engine/src/cmd/props.rs @@ -205,24 +205,29 @@ struct LeftOut { } impl LeftOut { - /// `hs` without the objects that can't be edited, and how many those were. + /// `hs` without the objects that can't be edited, each object once, and how many those were. A + /// handle that names no object is an error, before anything changes. fn split(s: &Session, hs: Vec) -> Result<(Vec, LeftOut)> { let d = s.doc()?; + let mut seen = std::collections::HashSet::new(); let mut out = LeftOut::default(); - let hs = hs - .into_iter() - .filter(|h| { - if super::blocks::refedit::locked(d, *h) { - out.outside += 1; - } else if super::curves::is_locked(s, *h) { - out.locked += 1; - } else { - return true; - } - false - }) - .collect(); - Ok((hs, out)) + let mut free = Vec::new(); + for h in hs { + if d.entity(h).is_none() { + return Err(cadcraft_doc::DocError::NoEntity(h).into()); + } + if !seen.insert(h) { + continue; + } + if super::blocks::refedit::locked(d, h) { + out.outside += 1; + } else if super::curves::is_locked(s, h) { + out.locked += 1; + } else { + free.push(h); + } + } + Ok((free, out)) } /// `{changed, locked}`, and a message saying how many were left out when any were. diff --git a/crates/engine/tests/it/properties_set_locked.rs b/crates/engine/tests/it/properties_set_locked.rs index 267727a4f..258294d79 100644 --- a/crates/engine/tests/it/properties_set_locked.rs +++ b/crates/engine/tests/it/properties_set_locked.rs @@ -52,6 +52,20 @@ fn objects_on_locked_layers_are_left_out() { assert_eq!(counts(&r), (Some(1), Some(2)), "{r}"); assert_eq!(r["message"], "2 objects were on a locked layer."); assert_eq!(layers(&s), ["L2", "L1", "L1"]); + // An object listed twice counts once. + let r = s.execute("properties.set", &json!({ "handles": [a.hex(), a.hex(), b.hex(), b.hex()], "color": 6 })).unwrap(); + assert_eq!(counts(&r), (Some(1), Some(1)), "{r}"); + assert_eq!(r["message"], "1 object was on a locked layer."); +} + +/// A handle that names no object is refused before anything changes. +#[test] +fn a_missing_object_is_refused_before_any_change() { + let (mut s, [a, b, _]) = locked_drawing(); + let undo = s.state().unwrap().undo.len(); + assert!(s.execute("properties.set", &json!({ "handles": [a.hex(), b.hex(), "FFFFF"], "color": 7 })).is_err()); + assert_eq!(colors(&s), [Color::ByLayer; 3]); + assert_eq!(s.state().unwrap().undo.len(), undo, "no undo step"); } #[test] @@ -111,4 +125,7 @@ fn during_refedit_objects_outside_the_working_set_are_left_out() { assert_eq!(d.entity(copy).unwrap().common.color, Color::Index(5)); assert_eq!(d.entity(other).unwrap().common.color, Color::ByLayer); assert!(d.refedit.is_some(), "the edit goes on"); + // A handle that names no object isn't counted as outside the working set: it's refused. + assert!(s.execute("properties.set", &json!({ "handles": [copy.hex(), "FFFFF"], "color": 6 })).is_err()); + assert_eq!(s.doc().unwrap().entity(copy).unwrap().common.color, Color::Index(5)); }