Merge PR #454 from MarekSpodBiedry: Engine: a command that fails after editing puts the presentation back (#405)

crates/engine/src/guard.rs

@@ -88,4 +88,32 @@ mod tests {
         // The session keeps working.
         s.execute("shape.insert", &json!({"preset": "ellipse", "rect": [36, 236, 200, 400]})).unwrap();
     }
+
+    #[test]
+    fn a_command_that_fails_after_editing_leaves_the_document_alone() {
+        let mut s = Session::new();
+        s.execute("file.new", &json!({})).unwrap();
+        let a = s.execute("shape.insert", &json!({"preset": "rect", "rect": [36, 36, 100, 100]})).unwrap()["id"].as_u64().unwrap();
+        let b = s.execute("shape.insert", &json!({"preset": "rect", "rect": [236, 36, 100, 100]})).unwrap()["id"].as_u64().unwrap();
+        let before = s.active().unwrap().doc.clone();
+        let undo_len = s.active().unwrap().history.undo.len();
+        // `shape.connect` inserts the requested preset first, then rejects it because it is not a line.
+        let r = s.execute("shape.connect", &json!({"from": a, "to": b, "preset": "rect"}));
+        assert!(r.is_err());
+        let st = s.active().unwrap();
+        assert!(std::sync::Arc::ptr_eq(&st.doc, &before), "the stray shape is gone");
+        assert_eq!(st.history.undo.len(), undo_len);
+    }
+
+    #[test]
+    fn a_failed_picture_change_adds_no_media() {
+        let mut s = Session::new();
+        s.execute("file.new", &json!({})).unwrap();
+        s.execute("shape.insert", &json!({"preset": "rect", "rect": [36, 36, 100, 100]})).unwrap();
+        let before = s.active().unwrap().doc.clone();
+        // `picture.change` stores the image first, then finds no shape with this id.
+        let r = s.execute("picture.change", &json!({"ids": [9999], "name": "a.png", "data": crate::cmd::base64_encode(b"not a picture")}));
+        assert!(r.is_err());
+        assert!(std::sync::Arc::ptr_eq(&s.active().unwrap().doc, &before), "the stored image is gone");
+    }
 }

crates/engine/src/lib.rs

@@ -452,7 +452,23 @@ impl Session {
         self.depth += 1;
         let r = (spec.run)(self, params);
         self.depth -= 1;
-        let r = r?;
+        let r = match r {
+            Ok(r) => r,
+            Err(e) => {
+                // A command made of several edits can fail after some of them went through; the
+                // outermost one puts the presentation back, so a failed command changes nothing.
+                if outer
+                    && let (Some((uid, old, sel)), Some(st)) = (before, self.active_mut())
+                    && st.uid == uid
+                    && !Arc::ptr_eq(&old, &st.doc)
+                {
+                    st.doc = old;
+                    st.selection = sel;
+                    st.revision += 1;
+                }
+                return Err(e);
+            }
+        };
         if let Some(st) = self.active_mut() {
             clamp_selection(st);
         }