Engine: a command that selects its ids first puts the selection back when it fails

Review follow-up (#3785, CodeRabbit): commands in SELECTS_GIVEN_IDS run select.set with the given ids before their body; when the body then returned an ordinary error, the ids stayed selected (and their layer current), while a panic restored the selection and refused ids never changed it. The dispatcher now keeps the active document's selection and current layer from before select.set and restores them when the body errors. This affects every command in SELECTS_GIVEN_IDS (Duplicate, Arrange, Clipping Mask, Compound Path, Group/Ungroup, Hide, Lock, Move, the Pathfinder panel commands including Trap, and the transforms). Tests: Trap with ids whose edges don't meet, and Intersect with ids of shapes that don't overlap, leave the selection as it was.

crates/engine/src/lib.rs

@@ -1717,13 +1717,25 @@ impl Session {
             self.note_depth = 1;
             self.batch_stash = None;
         }
-        // A selection-only command given `ids` selects them first, in the same undo step (#2130).
+        // A selection-only command given `ids` selects them first, in the same undo step (#2130);
+        // when the command then fails, the selection (and the current layer selecting them made
+        // current) goes back to how it was, as when the ids are refused.
         let select_first = SELECTS_GIVEN_IDS.contains(&id).then(|| params.get("ids")).flatten();
+        let selected_before = select_first.and(self.active()).map(|d| (d.uid, d.selection.clone(), d.active_layer));
         let run = |s: &mut Session| {
             if let Some(ids) = select_first {
                 s.execute("select.set", &serde_json::json!({ "ids": ids }))?;
             }
-            (spec.run)(s, params)
+            let r = (spec.run)(s, params);
+            if r.is_err()
+                && let Some((uid, selection, layer)) = selected_before
+                && let Some(st) = s.docs.iter_mut().find(|d| d.uid == uid)
+            {
+                st.selection = selection;
+                st.active_layer = layer;
+                st.revision += 1;
+            }
+            r
         };
         // The entries the command journals belong to the undo groups of the documents it edits, and to
         // the group of the document it starts in when it ends there too.

crates/engine/src/tests_select_ids_first.rs

@@ -143,6 +143,24 @@ fn bad_ids_are_refused_and_change_nothing() {
     }
 }
 
+#[test]
+fn a_command_that_fails_after_selecting_its_ids_puts_the_selection_back() {
+    let (mut s, ids) = doc();
+    let far = s.execute("shape.rectangle", &json!({"x": 300, "y": 200, "width": 20, "height": 20})).unwrap()["id"].as_u64().unwrap();
+    s.execute("select.set", &json!({"ids": [ids[3]]})).unwrap();
+    let (before, layer) = (s.doc().unwrap().doc.clone(), s.doc().unwrap().active_layer);
+    // The ids are fine, so they are selected; the shapes don't overlap, so Intersect then fails.
+    let e = s.execute("object.pathfinder.intersect", &json!({"ids": [ids[0], far]})).unwrap_err().to_string();
+    assert!(e.contains("empty result"), "{e}");
+    assert!(s.doc().unwrap().doc == before);
+    assert_eq!(s.doc().unwrap().selection.objects, vec![NodeId(ids[3])], "the selection stays");
+    assert_eq!(s.doc().unwrap().active_layer, layer);
+    // Without ids the failing command leaves the selection it ran on, as before.
+    s.execute("select.set", &json!({"ids": [ids[0], far]})).unwrap();
+    assert!(s.execute("object.pathfinder.intersect", &json!({})).is_err());
+    assert_eq!(s.doc().unwrap().selection.objects, vec![NodeId(ids[0]), NodeId(far)]);
+}
+
 #[test]
 fn without_ids_the_selection_is_used_as_before() {
     let (mut s, ids) = doc();

crates/engine/src/tests_trap.rs

@@ -125,6 +125,16 @@ fn no_adjoining_edges_is_a_clear_error_that_changes_nothing() {
     s.execute("select.set", &json!({"ids": [a.0]})).unwrap();
     let e = s.execute("object.pathfinder.trap", &json!({})).unwrap_err().to_string();
     assert!(e.contains("two or more objects"), "{e}");
+    // Given as ids, the objects are selected for the command, and its failure puts the
+    // selection back.
+    s.execute("select.none", &json!({})).unwrap();
+    let e = s.execute("object.pathfinder.trap", &json!({"ids": [a.0, b.0]})).unwrap_err().to_string();
+    assert!(e.contains("no edges of different colours meet"), "{e}");
+    assert!(s.doc().unwrap().selection.objects.is_empty(), "{:?}", s.doc().unwrap().selection.objects);
+    s.execute("select.set", &json!({"ids": [a.0]})).unwrap();
+    assert!(s.execute("object.pathfinder.trap", &json!({"ids": [a.0, b.0]})).is_err());
+    assert_eq!(s.doc().unwrap().selection.objects, vec![a]);
+    assert_eq!(s.doc().unwrap().doc, before);
 }
 
 #[test]