Merge main into #1484

crates/engine/src/canvas/textpreview.rs

@@ -39,7 +39,14 @@ pub fn show(s: &mut Session, command: &str, params: &Value) -> Result<()> {
     ds.text_edit = text_edit;
     match result {
         Ok(_) if !Arc::ptr_eq(&ds.doc, &base) => {
-            normalize(ds.doc_mut(), &base);
+            // A panic while normalising shows no preview (#1085).
+            if let Err(e) = guard::run(command, || {
+                normalize(ds.doc_mut(), &base);
+                Ok(())
+            }) {
+                ds.doc = base;
+                return Err(e);
+            }
             ds.text_preview = Some(base);
             ds.revision += 1;
             Ok(())

crates/engine/src/cmd/page.rs

@@ -33,6 +33,7 @@ fn page_target(s: &Session, cmd: &str, p: &Value) -> crate::Result<NodeId> {
     Ok(target)
 }
 
+/// The page and ruler guide commands.
 pub fn specs() -> Vec<CommandSpec> {
     vec![
         spec!("page.add", "Add page", "{name?} (a name starting with a hyphen adds a divider)", true, |s, p| {
@@ -106,8 +107,10 @@ pub fn specs() -> Vec<CommandSpec> {
                 copies.extend(doc.subtree(*c).into_iter().zip(doc.subtree(*copy)));
             }
             // The copy keeps the page's colour, flows (pointing at the copied frames), prototype settings and guides.
-            if let NodeKind::Page { background, flows, prototype, guides } = doc.get(src)?.kind.clone()
-                && let NodeKind::Page { background: b, flows: f, prototype: pr, guides: g } = &mut doc.get_mut(np)?.kind
+            let guides = doc.get(src)?.guides.clone();
+            doc.get_mut(np)?.guides = guides;
+            if let NodeKind::Page { background, flows, prototype } = doc.get(src)?.kind.clone()
+                && let NodeKind::Page { background: b, flows: f, prototype: pr } = &mut doc.get_mut(np)?.kind
             {
                 *b = background;
                 *f = flows
@@ -115,7 +118,6 @@ pub fn specs() -> Vec<CommandSpec> {
                     .filter_map(|fl| copies.iter().find(|(old, _)| *old == fl.start).map(|(_, new)| Flow { start: *new, ..fl }))
                     .collect();
                 *pr = prototype;
-                *g = guides;
             }
             // Keep the new page right after the source.
             if let Some(si) = doc.pages.iter().position(|x| *x == src) {

crates/engine/src/cmd/proto.rs

@@ -161,7 +161,7 @@ pub fn specs() -> Vec<CommandSpec> {
         spec!(
             "proto.linkNotice",
             "Notice before leaving for a link",
-            "{show? (default true; false skips it)}  whether a presented prototype's Open link to a website first says that it leaves the prototype, with Open link and Cancel; kept with the preferences → {shown}",
+            "{show? (default true; false skips it)}  whether a presented prototype's Open link the viewer clicked (or fired by a key) first says that it leaves the prototype, with Open link and Cancel (a link a delay or hover fired always does); kept with the preferences → {shown}",
             false,
             |s, p| {
                 let show = boolean(p, "show").unwrap_or(true);

crates/engine/src/derived.rs

@@ -10,9 +10,28 @@ pub fn changed_nodes(doc: &Document, before: &Document) -> Vec<NodeId> {
         .collect()
 }
 
+#[cfg(test)]
+thread_local! {
+    /// Tests: normalising panics, standing for a bug in it (a command's edit is rolled back).
+    pub(crate) static PANIC: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
+}
+
+/// Tests: panic when [`PANIC`] is set, as a bug in normalising would.
+#[cfg(test)]
+fn test_panic() {
+    if PANIC.with(std::cell::Cell::get) {
+        panic!("normalize: test panic");
+    }
+}
+
+/// Shipped builds: nothing (the test hook compiles away).
+#[cfg(not(test))]
+fn test_panic() {}
+
 /// Keep derived state consistent after edits: text boxes resize to their content, groups wrap
 /// their children, instances follow their main components, auto layout re-flows.
 pub fn normalize(doc: &mut Document, before: &Document) {
+    test_panic();
     // Only what was touched since the last normalisation (edited, added or removed).
     let mut touched: BTreeSet<NodeId> = doc.take_dirty();
     // A variable or style edit changes no layer directly, but bound and linked layers must follow it.

crates/engine/src/lib.rs

@@ -346,6 +346,8 @@ mod nested_override_tests;
 #[cfg(test)]
 mod node_set_refusals_tests;
 #[cfg(test)]
+mod normalize_guard_tests;
+#[cfg(test)]
 mod online_tests;
 #[cfg(test)]
 mod open_issues_tests;

crates/engine/src/normalize_guard_tests.rs

@@ -0,0 +1,72 @@
+//! A panic while normalising what a command changed is caught like one in the command itself
+//! (#1085): the command fails with `EngineError::Internal`, the document is as it was, no undo step
+//! is pushed, and the session keeps working.
+
+use serde_json::json;
+
+use crate::{EngineError, Session, derived};
+
+/// Run `f` with normalisation panicking.
+fn panicking<T>(f: impl FnOnce() -> T) -> T {
+    derived::PANIC.with(|p| p.set(true));
+    let out = f();
+    derived::PANIC.with(|p| p.set(false));
+    out
+}
+
+/// The layer id a creating command returned.
+fn id(v: &serde_json::Value) -> u64 {
+    v["id"].as_u64().unwrap()
+}
+
+/// A command whose edit panics while normalising fails with `EngineError::Internal`, leaves the
+/// document and the undo history as they were, and the same command then runs as usual.
+#[test]
+fn a_panic_while_normalising_rolls_the_command_back() {
+    let mut s = Session::new();
+    let r = id(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap());
+    let before = s.doc().unwrap().clone();
+    let undo = s.undo_depth();
+    let result = panicking(|| s.execute("node.set", &json!({"ids": [r], "props": {"name": "Renamed"}})));
+    assert!(matches!(result, Err(EngineError::Internal { .. })), "{result:?}");
+    assert_eq!(s.doc().unwrap(), &before, "the edit is rolled back");
+    assert_eq!(s.undo_depth(), undo, "no undo step for the failed command");
+    // The next command runs as usual.
+    s.execute("node.set", &json!({"ids": [r], "props": {"name": "Renamed"}})).unwrap();
+    assert_eq!(s.doc().unwrap().node(figcraft_doc::NodeId(r)).unwrap().name, "Renamed");
+    assert_eq!(s.undo_depth().0, undo.0 + 1);
+}
+
+/// The step fails like any other failed step: the batch's group restores what it set aside for
+/// its steps (the history limit, the group depth), which a panic unwinding through it skipped.
+#[test]
+fn a_panic_while_normalising_a_batch_step_rolls_the_batch_back() {
+    let mut s = Session::new();
+    let before = s.doc().unwrap().clone();
+    let undo = s.undo_depth();
+    let limit = s.ds().unwrap().history.limit;
+    let result = panicking(|| {
+        s.execute("command.batch", &json!({"commands": [{"command": "shape.rectangle", "params": {"x": 0, "y": 0, "width": 10, "height": 10}}]}))
+    });
+    assert!(result.is_err(), "{result:?}");
+    assert_eq!(s.doc().unwrap(), &before);
+    assert_eq!(s.undo_depth(), undo);
+    assert_eq!(s.ds().unwrap().history.limit, limit, "the undo limit is back");
+    assert_eq!(s.group_depth, 0, "the group ended");
+    s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap();
+}
+
+/// A live text preview whose normalising panics fails and shows nothing: the document is as it
+/// was, and the same preview then shows.
+#[test]
+fn a_panic_while_normalising_a_text_preview_shows_none() {
+    let mut s = Session::new();
+    let t = id(&s.execute("text.create", &json!({"x": 0, "y": 0, "text": "Preview"})).unwrap());
+    let before = s.doc().unwrap().clone();
+    let result = panicking(|| s.execute("text.preview", &json!({"command": "text.style", "params": {"ids": [t], "fontSize": 32}})));
+    assert!(matches!(result, Err(EngineError::Internal { .. })), "{result:?}");
+    assert_eq!(s.doc().unwrap(), &before);
+    assert!(!crate::textpreview::active(&s), "no preview shows");
+    s.execute("text.preview", &json!({"command": "text.style", "params": {"ids": [t], "fontSize": 32}})).unwrap();
+    assert!(crate::textpreview::active(&s));
+}

crates/engine/src/prefs.rs

@@ -82,8 +82,10 @@ pub struct Prefs {
     /// The Getting started tips on the home screen were closed (Help ▸ Getting started shows them
     /// again).
     pub getting_started_dismissed: bool,
-    /// A prototype's Open link goes to a website without first telling the viewer that it leaves
-    /// the prototype (the notice's "Don't show this again"; `proto.linkNotice` shows it again).
+    /// A prototype's Open link — to a website or to the mail app — that the viewer's own click,
+    /// press, drag or key fired opens without first telling them that it leaves the prototype
+    /// (the notice's "Don't show this again"; `proto.linkNotice` shows it again). A link a delay
+    /// or the pointer passing over a layer fired still shows the notice (#1096).
     pub link_notice_skipped: bool,
     /// How each home screen section shows its files, by section id (`"recents"`): `"grid"` (cards
     /// with the picture each file carries) or `"list"` (a table). Missing: the grid.

crates/engine/src/session/exec.rs

@@ -51,7 +51,14 @@ impl Session {
             .and_then(|v| self.check_instance_structure(id, &before).map(|()| v))
             .and_then(|v| self.check_preview(id, &before).map(|()| v))
             .and_then(|v| self.check_live_access(id, &before).map(|()| v))
-            .and_then(|v| self.check_finite_geometry(id, &before).map(|()| v));
+            .and_then(|v| self.check_finite_geometry(id, &before).map(|()| v))
+            // Normalising is part of the command: a panic in it rolls the edit back too (#1085).
+            .and_then(|v| {
+                guard::run(id, || {
+                    self.normalize_edit(spec, &before);
+                    Ok(v)
+                })
+            });
         match &result {
             Ok(_) => {
                 self.after_command(spec, before);
@@ -141,7 +148,19 @@ impl Session {
             .map_err(|m| EngineError::Disabled(id.to_string(), m))
     }
 
-    /// Bookkeeping after a successful command: normalise what changed, push undo, bump revision.
+    /// Normalise what a command changed in the document it ran on (text boxes, groups, instances,
+    /// auto layout…).
+    fn normalize_edit(&mut self, spec: &CommandSpec, before: &Option<(Arc<Document>, Vec<NodeId>, NodeId, u64)>) {
+        let Some((bdoc, _, _, uid)) = before else { return };
+        let Some(ds) = self.docs.iter_mut().find(|d| d.uid == *uid) else { return };
+        // Saving may adopt the chosen filename as the document title. The encoded snapshot is
+        // already final; normalising it again would detach it from the saved baseline.
+        if !Arc::ptr_eq(&ds.doc, bdoc) && !matches!(spec.id, "file.save" | "file.saveAs") {
+            normalize(ds.doc_mut(), bdoc);
+        }
+    }
+
+    /// Bookkeeping after a successful command (normalised already): push undo, bump revision.
     fn after_command(&mut self, spec: &CommandSpec, before: Option<(Arc<Document>, Vec<NodeId>, NodeId, u64)>) {
         let Some((bdoc, bsel, bpage, uid)) = before else {
             if let Ok(ds) = self.ds_mut() {
@@ -150,13 +169,7 @@ impl Session {
             return;
         };
         let Some(ds) = self.docs.iter_mut().find(|d| d.uid == uid) else { return };
-        let changed = !Arc::ptr_eq(&ds.doc, &bdoc);
-        // Saving may adopt the chosen filename as the document title. The encoded snapshot is
-        // already final; normalising it again would detach it from the saved baseline.
-        if changed && !matches!(spec.id, "file.save" | "file.saveAs") {
-            normalize(ds.doc_mut(), &bdoc);
-        }
-        let edit = changed && ds.interaction.is_none();
+        let edit = !Arc::ptr_eq(&ds.doc, &bdoc) && ds.interaction.is_none();
         ds.clean_selection();
         ds.revision += 1;
         if edit {

crates/engine/src/tests.rs

@@ -616,13 +616,11 @@ fn document_colors_are_counted_per_page() {
     assert!(v["colors"].as_array().unwrap().is_empty());
 }
 
+/// `guide.add`, `guide.move` and `guide.remove` edit the current page's guides by index.
 #[test]
 fn guides_add_move_remove() {
     let mut s = sess();
-    let guides = |s: &Session| match &s.doc().unwrap().node(s.page().unwrap()).unwrap().kind {
-        NodeKind::Page { guides, .. } => guides.clone(),
-        _ => vec![],
-    };
+    let guides = |s: &Session| s.doc().unwrap().node(s.page().unwrap()).unwrap().guides.clone();
     let r = s.execute("guide.add", &json!({"vertical": true, "offset": 120})).unwrap();
     assert_eq!(r["index"], json!(0));
     s.execute("guide.add", &json!({"vertical": false, "offset": 40.5})).unwrap();

crates/engine/src/theme_pages_tests.rs

@@ -183,6 +183,8 @@ fn text_drawn_on_a_dark_page_is_white_and_on_a_light_page_black() {
     assert_eq!(s.doc().unwrap().node(made).unwrap().fills, vec![Paint::solid(Color::WHITE)]);
 }
 
+/// `page.duplicate` copies the page's flows (pointing at the copied frames), prototype settings
+/// and guides.
 #[test]
 fn a_copy_of_a_page_keeps_its_flows_prototype_settings_and_guides() {
     let mut s = Session::new();
@@ -195,9 +197,12 @@ fn a_copy_of_a_page_keeps_its_flows_prototype_settings_and_guides() {
     let source = s.page().unwrap();
     let copy = id_of(&ex(&mut s, "page.duplicate", json!({}))["id"]).unwrap();
     let frames = s.doc().unwrap().node(copy).unwrap().children.clone();
-    let state = |s: &Session, id: NodeId| match &s.doc().unwrap().node(id).unwrap().kind {
-        NodeKind::Page { flows, prototype, guides, .. } => (flows.clone(), prototype.clone(), guides.clone()),
-        _ => panic!("expected a page"),
+    let state = |s: &Session, id: NodeId| {
+        let n = s.doc().unwrap().node(id).unwrap();
+        match &n.kind {
+            NodeKind::Page { flows, prototype, .. } => (flows.clone(), prototype.clone(), n.guides.clone()),
+            _ => panic!("expected a page"),
+        }
     };
     let (flows, prototype, guides) = state(&s, copy);
     let (src_flows, src_prototype, src_guides) = state(&s, source);