Slice Options and Divide Slice are one undo step, and a rejected one changes nothing (#910, #911, #912)

An auto slice used to be promoted in its own history step before the command
validated its options, so a rejected slice.set still promoted (and an off-canvas
rect was committed before the error), slice.divide took two undos, and
slice.set folded into any earlier explicit Promote. The promotion now happens
inside the command's own edit, which is all-or-nothing.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

crates/engine/src/slice_cmds.rs

@@ -11,7 +11,7 @@
 use std::sync::Arc;
 
 use photocraft_doc::slices::{self, ResolvedSlice, Slice, SliceKind, SliceOrigin};
-use photocraft_doc::{Document, Layer, LayerContent, LayerId};
+use photocraft_doc::{Document, Layer, LayerContent};
 use photocraft_geom::Rect;
 use serde_json::{Value, json};
 
@@ -194,27 +194,46 @@ fn apply_options(sl: &mut Slice, p: &Value, cmd: &str) -> Result<()> {
     Ok(())
 }
 
-/// The stored slice addressed by `"slice"` (id) or `"number"`; an auto slice's number promotes
-/// it to a user slice first when `promote` is set.
-fn target(s: &mut Session, p: &Value, cmd: &str, promote: bool) -> Result<u32> {
+/// What `"slice"` (id) or `"number"` addresses: a stored slice, or an auto slice that isn't
+/// stored yet (its rectangle).
+#[derive(Clone, Copy)]
+enum Target {
+    Stored(u32),
+    Auto(Rect),
+}
+
+fn find(s: &Session, p: &Value, cmd: &str) -> Result<Target> {
+    let d = s.active().ok_or(EngineError::NoDocument)?;
     if let Some(id) = crate::commands::int(p, "slice").filter(|v| *v >= 0).map(|v| v as u64) {
-        let d = s.active().ok_or(EngineError::NoDocument)?;
-        return d.doc.slices.get(id as u32).map(|sl| sl.id).ok_or_else(|| bad(cmd, format!("no slice with id {id}")));
+        return d.doc.slices.get(id as u32).map(|sl| Target::Stored(sl.id)).ok_or_else(|| bad(cmd, format!("no slice with id {id}")));
     }
     let n = crate::commands::int(p, "number").filter(|v| *v > 0).ok_or_else(|| bad(cmd, "give \"slice\" (id) or \"number\""))? as usize;
-    let d = s.active().ok_or(EngineError::NoDocument)?;
     let r = slices::resolve(&d.doc).into_iter().find(|r| r.number == n).ok_or_else(|| bad(cmd, format!("no slice number {n}")))?;
-    match r.id {
-        Some(id) => Ok(id),
-        None if promote => {
-            let rect = r.rect;
-            s.edit("Promote to User Slice", |doc, _| {
-                let id = doc.slices.next_id().ok_or_else(exhausted_id)?;
-                doc.slices.list.push(Slice { id, rect, ..Default::default() });
-                Ok(id)
-            })
+    Ok(match r.id {
+        Some(id) => Target::Stored(id),
+        None => Target::Auto(r.rect),
+    })
+}
+
+/// The stored slice's id; an auto slice is promoted to a user slice in `doc` first. Call it inside
+/// the command's own edit, so promoting and the rest of the command are one history step and a
+/// rejected command changes nothing.
+fn store(doc: &mut Document, t: Target) -> Result<u32> {
+    match t {
+        Target::Stored(id) => Ok(id),
+        Target::Auto(rect) => {
+            let id = doc.slices.next_id().ok_or_else(exhausted_id)?;
+            doc.slices.list.push(Slice { id, rect, ..Default::default() });
+            Ok(id)
         }
-        None => Err(bad(cmd, format!("slice {n} is an auto slice"))),
+    }
+}
+
+/// The stored slice addressed by `p`; an auto slice is an error.
+fn stored(s: &Session, p: &Value, cmd: &str) -> Result<u32> {
+    match find(s, p, cmd)? {
+        Target::Stored(id) => Ok(id),
+        Target::Auto(_) => Err(bad(cmd, "that is an auto slice")),
     }
 }
 
@@ -297,26 +316,18 @@ fn set_slice(s: &mut Session, p: &Value) -> Result<Value> {
     if ["rect", "x", "y", "width", "height"].iter().any(|key| p.get(key).is_some()) && rect_param(p).is_none() {
         return Err(bad(cmd, "give a finite rectangle with positive dimensions and representable corners"));
     }
-    let id = target(s, p, cmd, true)?;
-    let coalesce_into_promote = s.active().is_some_and(|d| d.history.undo_label() == Some("Promote to User Slice")) && p.get("number").is_some();
-    let label = "Slice Options";
-    let run = |doc: &mut Document, _: &mut Option<LayerId>| -> Result<()> {
+    let t = find(s, p, cmd)?;
+    // Promoting an auto slice and its options are one user action (one history step), and
+    // nothing is committed unless every option is valid and the slice stays on the canvas.
+    let id = s.edit("Slice Options", |doc, _| {
+        let id = store(doc, t)?;
         let sl = doc.slices.get_mut(id).ok_or_else(|| bad(cmd, "slice vanished"))?;
         apply_options(sl, p, cmd)?;
-        Ok(())
-    };
-    if coalesce_into_promote {
-        // Promote + options are one user action: fold the options into the promote step.
-        let st = s.active_mut().ok_or(EngineError::NoDocument)?;
-        let mut doc = (*st.doc).clone();
-        let mut a = st.active_layer;
-        run(&mut doc, &mut a)?;
-        st.doc = Arc::new(doc);
-        st.revision += 1;
-        st.last_damage = None;
-    } else {
-        s.edit(label, run)?;
-    }
+        if !slices::resolve(doc).iter().any(|r| r.id == Some(id)) {
+            return Err(bad(cmd, "slice is off the canvas"));
+        }
+        Ok(id)
+    })?;
     let d = s.active().ok_or(EngineError::NoDocument)?;
     let r = slices::resolve(&d.doc).into_iter().find(|r| r.id == Some(id)).ok_or_else(|| bad(cmd, "slice is off the canvas"))?;
     Ok(resolved_json(&d.doc, &r))
@@ -324,18 +335,23 @@ fn set_slice(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn promote(s: &mut Session, p: &Value) -> Result<Value> {
     let cmd = "slice.promote";
-    let id = target(s, p, cmd, true)?;
+    let t = find(s, p, cmd)?;
     // Layer-based slices promote too: they stop following their layer.
-    let is_layer = s.active().and_then(|d| d.doc.slices.get(id)).is_some_and(|sl| sl.origin == SliceOrigin::Layer);
-    if is_layer {
-        s.edit("Promote to User Slice", |doc, _| {
+    let promotes = match t {
+        Target::Auto(_) => true,
+        Target::Stored(id) => s.active().and_then(|d| d.doc.slices.get(id)).is_some_and(|sl| sl.origin == SliceOrigin::Layer),
+    };
+    let id = match t {
+        Target::Stored(id) if !promotes => id,
+        _ => s.edit("Promote to User Slice", |doc, _| {
+            let id = store(doc, t)?;
             if let Some(sl) = doc.slices.get_mut(id) {
                 sl.origin = SliceOrigin::User;
                 sl.layer = None;
             }
-            Ok(())
-        })?;
-    }
+            Ok(id)
+        })?,
+    };
     Ok(json!({"slice": id}))
 }
 
@@ -348,7 +364,7 @@ fn ids_param(s: &mut Session, p: &Value, cmd: &str) -> Result<Vec<u32>> {
         }
         return Ok(ids);
     }
-    Ok(vec![target(s, p, cmd, false)?])
+    Ok(vec![stored(s, p, cmd)?])
 }
 
 fn delete(s: &mut Session, p: &Value) -> Result<Value> {
@@ -363,20 +379,24 @@ fn delete(s: &mut Session, p: &Value) -> Result<Value> {
 
 fn divide(s: &mut Session, p: &Value) -> Result<Value> {
     let cmd = "slice.divide";
-    let id = target(s, p, cmd, true)?;
+    let t = find(s, p, cmd)?;
     let down = crate::commands::int(p, "horizontal").unwrap_or(1).clamp(1, 1000) as i32;
     let across = crate::commands::int(p, "vertical").unwrap_or(1).clamp(1, 1000) as i32;
     if down == 1 && across == 1 {
         return Err(bad(cmd, "give \"horizontal\" (slices down) and/or \"vertical\" (slices across) > 1"));
     }
-    let d = s.active().ok_or(EngineError::NoDocument)?;
-    let src = d.doc.slices.get(id).cloned().ok_or_else(|| bad(cmd, "no such slice"))?;
-    let r = src.rect;
+    let r = match t {
+        Target::Auto(r) => r,
+        Target::Stored(id) => s.active().and_then(|d| d.doc.slices.get(id)).map(|sl| sl.rect).ok_or_else(|| bad(cmd, "no such slice"))?,
+    };
     if (r.width() as i32) < across || (r.height() as i32) < down {
         return Err(bad(cmd, "the slice is too small to divide that many times"));
     }
     let n = (down * across) as usize;
+    // An auto slice is promoted inside the same step: one undo restores it.
     let ids = s.edit("Divide Slice", |doc, _| {
+        let id = store(doc, t)?;
+        let src = doc.slices.get(id).cloned().ok_or_else(|| bad(cmd, "no such slice"))?;
         let pos = doc.slices.list.iter().position(|sl| sl.id == id).unwrap_or(0);
         doc.slices.list.retain(|sl| sl.id != id);
         let mut ids = Vec::new();

crates/engine/src/slice_cmds/tests.rs

@@ -1,4 +1,5 @@
 use super::*;
+use photocraft_doc::LayerId;
 use photocraft_doc::effects::Effect;
 
 fn session(depth: u32) -> Session {
@@ -196,3 +197,49 @@ fn exhausted_slice_ids_fail_without_mutating_document_or_history() {
         assert_eq!(s.active().unwrap().revision, revision, "{command}");
     }
 }
+
+#[test]
+fn rejected_slice_options_change_nothing() {
+    // #910: an invalid option on an auto slice used to promote it before failing.
+    let mut s = session(8);
+    let past = s.active().unwrap().history.past_len();
+    assert!(s.execute("slice.set", json!({"number": 1, "kind": "bogus"})).is_err());
+    assert!(doc(&s).slices.is_empty());
+    assert_eq!(s.active().unwrap().history.past_len(), past);
+    // A user slice moved off the canvas is rejected without committing the move.
+    let id = s.execute("slice.new", json!({"rect": [0, 0, 10, 10]})).unwrap()["slice"].as_u64().unwrap() as u32;
+    let past = s.active().unwrap().history.past_len();
+    assert!(s.execute("slice.set", json!({"slice": id, "rect": [500, 500, 10, 10]})).is_err());
+    assert_eq!(doc(&s).slices.get(id).unwrap().rect, Rect::new(0, 0, 10, 10));
+    assert_eq!(s.active().unwrap().history.past_len(), past);
+}
+
+#[test]
+fn dividing_an_auto_slice_is_one_step() {
+    // #911: one undo restores the original auto slice.
+    let mut s = session(8);
+    let past = s.active().unwrap().history.past_len();
+    let r = s.execute("slice.divide", json!({"number": 1, "vertical": 2})).unwrap();
+    assert_eq!(r["count"], 2);
+    assert_eq!(doc(&s).slices.list.len(), 2);
+    assert_eq!(s.active().unwrap().history.past_len(), past + 1);
+    assert!(s.undo());
+    assert!(doc(&s).slices.is_empty());
+    // A rejected division doesn't promote either.
+    assert!(s.execute("slice.divide", json!({"number": 1, "vertical": 1000})).is_err());
+    assert!(doc(&s).slices.is_empty());
+    assert_eq!(s.active().unwrap().history.past_len(), past);
+}
+
+#[test]
+fn slice_options_after_an_explicit_promote_is_its_own_step() {
+    // #912: Slice Options used to fold into an earlier, separate Promote.
+    let mut s = session(8);
+    s.execute("slice.promote", json!({"number": 1})).unwrap();
+    s.execute("slice.set", json!({"number": 1, "name": "renamed"})).unwrap();
+    assert!(s.undo());
+    assert_eq!(doc(&s).slices.list.len(), 1, "the promotion survives undoing the rename");
+    assert_eq!(doc(&s).slices.list[0].name, "");
+    assert!(s.undo());
+    assert!(doc(&s).slices.is_empty());
+}