Review fix: properties.set counts each object once and refuses missing ones first

`targets` keeps a handle listed twice, so it was counted twice as left
out. During REFEDIT, a handle that names no object counted as outside the
working set instead of being refused. properties.set now takes each
object once, and refuses a handle that names no object before anything
changes; before, the objects ahead of it in the list had already been
changed when the error came.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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<Handle>) -> Result<(Vec<Handle>, 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.

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));
 }