diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 08eb8695..cdfcd4cc 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -10,6 +10,12 @@ use crate::{EngineError, Result, Sel, Session, Snapshot}; pub static COMMANDS: &[CommandSpec] = &[ CommandSpec::new("edit.undo", "Undo", undo).icon("undo").key("Ctrl+Z").noundo(), CommandSpec::new("edit.redo", "Redo", redo).icon("redo").key("Ctrl+Y").noundo(), + CommandSpec::new("edit.checkpoint", "Checkpoint", checkpoint) + .noundo() + .params("→ {checkpoint: id}: remember the design, active sketch and undo history (restore with edit.restore_checkpoint)"), + CommandSpec::new("edit.restore_checkpoint", "Restore Checkpoint", restore_checkpoint) + .noundo() + .params("checkpoint: id; forget?: bool (true = drop it without going back). Goes back to it, undo history included"), CommandSpec::new("timeline.compute_all", "Compute All", compute_all).at("SOLID", "MODIFY").icon("compute").noundo(), CommandSpec::new("timeline.delete", "Delete", delete) .at("SOLID", "MODIFY") @@ -60,6 +66,21 @@ fn redo(s: &mut Session, _p: &Value) -> Result { Ok(json!({"redone": label})) } +fn checkpoint(s: &mut Session, _p: &Value) -> Result { + Ok(json!({"checkpoint": s.checkpoint()})) +} + +fn restore_checkpoint(s: &mut Session, p: &Value) -> Result { + let cmd = "edit.restore_checkpoint"; + let id = p.get("checkpoint").and_then(Value::as_u64).ok_or_else(|| bad(cmd, "`checkpoint` must be an id from edit.checkpoint"))?; + if bool_(p, "forget") == Some(true) { + s.drop_checkpoint(id); + return Ok(json!({"checkpoint": id, "restored": false})); + } + s.restore_checkpoint(id)?; + Ok(json!({"checkpoint": id, "restored": true})) +} + fn compute_all(s: &mut Session, _p: &Value) -> Result { s.model = solvecraft_doc::Model::new(); s.refresh(); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 2ce89495..22a5106f 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -170,6 +170,20 @@ pub struct Session { pub analysis: Option, /// What is hidden or shown in the view (`browser.visibility`). pub visibility: Visibility, + /// States to go back to (`edit.checkpoint`, `edit.restore_checkpoint`): how batches of + /// commands are undone however long the undo history is. + pub(crate) checkpoints: Vec, + next_checkpoint: u64, +} + +/// The design, active sketch and undo history at an `edit.checkpoint`. +#[derive(Clone, Debug)] +pub(crate) struct Checkpoint { + pub id: u64, + pub doc: Arc, + pub active_sketch: Option, + pub undo: Vec, + pub redo: Vec, } /// Items hidden or shown one by one (view state, not part of the design and not an undo step). @@ -204,6 +218,8 @@ pub enum SurfaceAnalysis { } const MAX_UNDO: usize = 200; +/// Checkpoints kept (the oldest go first). +const MAX_CHECKPOINTS: usize = 16; const MAX_LOG: usize = 2000; impl Default for Session { @@ -235,6 +251,8 @@ impl Session { clipboard: Vec::new(), section: None, analysis: None, + checkpoints: Vec::new(), + next_checkpoint: 1, visibility: Visibility::default(), } } @@ -356,6 +374,44 @@ impl Session { } } + /// Remember the design, active sketch and undo history; returns the checkpoint's id. + pub fn checkpoint(&mut self) -> u64 { + let id = self.next_checkpoint; + self.next_checkpoint += 1; + self.checkpoints.push(Checkpoint { + id, + doc: self.doc.clone(), + active_sketch: self.active_sketch, + undo: self.undo.clone(), + redo: self.redo.clone(), + }); + if self.checkpoints.len() > MAX_CHECKPOINTS { + self.checkpoints.remove(0); + } + id + } + + /// Go back to checkpoint `id` as if nothing since had happened (the undo history too). It + /// and later checkpoints are used up. + pub fn restore_checkpoint(&mut self, id: u64) -> Result<()> { + let i = self.checkpoints.iter().position(|c| c.id == id).ok_or_else(|| EngineError::Other(format!("no checkpoint {id}")))?; + let Some(c) = self.checkpoints.drain(i..).next() else { return Err(EngineError::Other(format!("no checkpoint {id}"))) }; + self.doc = c.doc; + self.active_sketch = c.active_sketch; + self.undo = c.undo; + self.redo = c.redo; + self.selection.clear(); + self.refresh(); + Ok(()) + } + + /// Forget checkpoint `id` (and later ones) without going back. + pub fn drop_checkpoint(&mut self, id: u64) { + if let Some(i) = self.checkpoints.iter().position(|c| c.id == id) { + self.checkpoints.truncate(i); + } + } + /// A scratch copy of the session (same design and model, no history or log) to try /// commands on without touching this one. pub fn scratch(&self) -> Session { @@ -377,6 +433,8 @@ impl Session { clipboard: self.clipboard.clone(), section: None, analysis: None, + checkpoints: Vec::new(), + next_checkpoint: 1, visibility: Visibility::default(), } } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 3a06c2e6..2495c83c 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -855,6 +855,35 @@ fn deleting_a_path_sketch_deletes_its_pipe() { assert!(s.model.results.iter().all(|r| r.error.is_none())); } +/// Regression: batches were rolled back by undoing as many steps as the undo history grew, +/// which is none once the history is full, leaving their sketches behind. +#[test] +fn checkpoint_restores_with_a_full_undo_history() { + let mut s = Session::default(); + for i in 0..MAX_UNDO + 5 { + run(&mut s, "parameters.add", json!({"name": format!("p{i}"), "expression": "1 mm"})); + } + assert_eq!(s.undo.len(), MAX_UNDO); + let labels: Vec = s.undo.iter().map(|u| u.label.clone()).collect(); + let doc = s.doc.clone(); + let id = run(&mut s, "edit.checkpoint", json!({}))["checkpoint"].as_u64().unwrap(); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "TapHole"})); + run(&mut s, "sketch.circle.center", json!({"center": [0, 0], "radius": 2})); + assert!(s.active_sketch.is_some()); + run(&mut s, "edit.restore_checkpoint", json!({"checkpoint": id})); + assert!(Arc::ptr_eq(&s.doc, &doc) && s.active_sketch.is_none()); + assert!(s.doc.find_feature("TapHole").is_none()); + assert_eq!(s.undo.iter().map(|u| u.label.clone()).collect::>(), labels); + // Used up: a second restore is an error, not a jump somewhere else. + assert!(s.execute("edit.restore_checkpoint", &json!({"checkpoint": id})).is_err()); + // Forgetting one leaves the design alone. + let id = run(&mut s, "edit.checkpoint", json!({}))["checkpoint"].as_u64().unwrap(); + run(&mut s, "sketch.create", json!({"plane": "XY", "name": "Kept"})); + run(&mut s, "edit.restore_checkpoint", json!({"checkpoint": id, "forget": true})); + assert!(s.doc.find_feature("Kept").is_some()); + assert!(s.execute("edit.restore_checkpoint", &json!({"checkpoint": id})).is_err()); +} + #[test] fn components_occurrences_and_world_placement() { let mut s = Session::default(); diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index f050cd49..64aa12f8 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -213,6 +213,35 @@ fn batch_stops_at_first_error_and_rolls_back() { assert_eq!(tool(&mut s, "measure", json!({}))["body_count"], 2); } +/// Regression: with a full undo history a failed batch reported `undone: 0` and left its +/// sketches behind (an empty "Sketch1", two sketches named "TapHole"). +#[test] +fn batch_rolls_back_with_a_full_undo_history() { + let mut s = server(); + let fill: Vec = + (0..230).map(|i| json!({"command": "parameters.add", "params": {"name": format!("p{i}"), "expression": "1 mm"}})).collect(); + tool(&mut s, "batch", json!({"commands": fill})); + let undo = tool(&mut s, "inspect_design", json!({"measure": false}))["undo"].clone(); + for _ in 0..2 { + let r = raw_tool( + &mut s, + "batch", + json!({"commands": [ + {"command": "sketch.create", "params": {"plane": "XY", "name": "TapHole"}}, + {"command": "sketch.circle.center", "params": {"center": [0, 0], "radius": 2}}, + {"command": "sketch.dimension", "params": {"entities": ["nope"], "value": 3}}, + ]}), + ); + let body: Value = serde_json::from_str(r["result"]["content"][0]["text"].as_str().unwrap()).unwrap(); + assert_eq!(body["failed"]["index"], 2, "{body}"); + assert_eq!(body["undone"], 2, "{body}"); + } + let d = tool(&mut s, "inspect_design", json!({"measure": false, "sketches": false})); + assert_eq!(d["sketches"].as_array().map(Vec::len), Some(0), "{d}"); + assert!(d["active_sketch"].is_null(), "{d}"); + assert_eq!(d["undo"], undo); +} + #[test] fn helper_sketches_can_be_hidden() { let mut s = server(); diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 0d1e993a..938e0de7 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -361,7 +361,56 @@ fn undo_depth(b: &mut dyn Backend) -> Option { b.call("document.inspect", json!({})).ok().and_then(|d| d["undo"].as_u64()) } -/// `batch`: run steps in order, stop at the first failure and (by default) undo what ran. +/// How a failed batch goes back to where it started. +enum Undo { + /// An engine checkpoint: the design, active sketch and undo history exactly as they were. + Checkpoint(u64), + /// An app without checkpoints: undo as many steps as the batch added (wrong once the undo + /// history is full, as its depth no longer grows). + Depth(u64), + None, +} + +impl Undo { + fn begin(b: &mut dyn Backend) -> Undo { + match exec(b, "edit.checkpoint", json!({})).ok().and_then(|v| v["checkpoint"].as_u64()) { + Some(id) => Undo::Checkpoint(id), + None => undo_depth(b).map_or(Undo::None, Undo::Depth), + } + } + + /// Go back; the number of steps undone. + fn rollback(self, b: &mut dyn Backend, steps: usize) -> u64 { + match self { + Undo::Checkpoint(id) => match exec(b, "edit.restore_checkpoint", json!({"checkpoint": id})) { + Ok(_) => steps as u64, + Err(_) => 0, + }, + Undo::Depth(before) => { + let after = undo_depth(b).unwrap_or(before); + let mut undone = 0u64; + for _ in before..after { + if exec(b, "edit.undo", json!({})).is_err() { + break; + } + undone += 1; + } + undone + } + Undo::None => 0, + } + } + + /// The batch went through: forget the checkpoint. + fn finish(self, b: &mut dyn Backend) { + if let Undo::Checkpoint(id) = self { + let _ = exec(b, "edit.restore_checkpoint", json!({"checkpoint": id, "forget": true})); + } + } +} + +/// `batch`: run steps in order, stop at the first failure and (by default) go back to the +/// design as it was before the batch. fn batch(b: &mut dyn Backend, a: &Map) -> Value { let Some(steps) = a.get("commands").and_then(Value::as_array) else { return text_result(&json!("`commands` must be an array of {command, params}"), true); @@ -370,7 +419,7 @@ fn batch(b: &mut dyn Backend, a: &Map) -> Value { return text_result(&json!(format!("batch too long ({} steps, at most {MAX_BATCH})", steps.len())), true); } let rollback = a.get("rollback").and_then(Value::as_bool).unwrap_or(true); - let before = if rollback { undo_depth(b) } else { None }; + let start = if rollback { Undo::begin(b) } else { Undo::None }; let mut results = Vec::with_capacity(steps.len()); for (i, step) in steps.iter().enumerate() { let command = step.get("command").or_else(|| step.get("id")).and_then(Value::as_str); @@ -381,15 +430,7 @@ fn batch(b: &mut dyn Backend, a: &Map) -> Value { match r { Ok(v) => results.push(v), Err(e) => { - let mut undone = 0u64; - if let (Some(before), Some(after)) = (before, rollback.then(|| undo_depth(b)).flatten()) { - for _ in before..after { - if exec(b, "edit.undo", json!({})).is_err() { - break; - } - undone += 1; - } - } + let undone = start.rollback(b, i); return text_result( &json!({ "ok": false, @@ -404,6 +445,7 @@ fn batch(b: &mut dyn Backend, a: &Map) -> Value { } } } + start.finish(b); text_result(&json!({"ok": true, "completed": results.len(), "results": results}), false) } diff --git a/docs/mcp.md b/docs/mcp.md index 3aec985e..63370d71 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -69,8 +69,11 @@ Errors: an unknown tool or arguments that don't match the tool's schema are JSON `-32602` errors; an engine failure (bad command parameters, a feature that fails) is a tool result with `isError: true` and the engine's message, which the model can read and correct. -`batch` rollback undoes as many steps as the batch added to the undo history; batches that -themselves call `edit.undo`, `file.new` or `doc.open` are not rolled back exactly. +`batch` rollback goes back to an engine checkpoint (`edit.checkpoint`) taken before the first +step: the design, the active sketch and the undo history are restored exactly as they were, +however long the history is. Batches that call `file.new` or `doc.open` get the design back, +not its file path. (An older app without checkpoints falls back to undoing as many steps as +the batch added to the undo history.) ## Demo: an enclosure built over MCP