diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index b439766b..57ddb369 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -265,7 +265,7 @@ fn undo_group_end(s: &mut Session, p: &Value) -> Result { /// under an interaction that would then preview on, or cancel to, the document from before (#1719). /// A typing session also ends in the Type tool, whose caret and marked text (IME) must follow the /// document. -fn settle_interaction(s: &mut Session) -> Result<()> { +pub(crate) fn settle_interaction(s: &mut Session) -> Result<()> { let typing = typing_in_progress(s); s.commit_interaction()?; if typing { diff --git a/crates/engine/src/cmd/fileio/batch.rs b/crates/engine/src/cmd/fileio/batch.rs index dd836846..99d16667 100644 --- a/crates/engine/src/cmd/fileio/batch.rs +++ b/crates/engine/src/cmd/fileio/batch.rs @@ -13,14 +13,22 @@ pub(super) fn batch(s: &mut Session, p: &Value) -> Result { // What an error rolls back besides the documents: session-level state a step may change // (paint defaults, drawing mode, clipboard, the Untitled-N count). let saved = (s.paint.clone(), s.fill_active, s.draw_mode, s.draw_inside, s.clipboard.clone(), s.untitled_counter); - let (start, active): (Vec, _) = (s.docs.iter().map(|d| d.uid).collect(), s.active); + // A drag or typing still in progress is a step of its own, as before Undo: the batch's steps + // would otherwise run inside it and take its baseline as theirs (#1697). + if s.active().is_some() { + super::super::edit::settle_interaction(s)?; + } + // The documents as they are now, whole: what a failed step brings back. Their art, history, + // file, format and saved state share their data with the open ones until a step changes them. + let start = s.docs.clone(); + let active = s.active; s.batch_stash = Some(vec![]); let r = run_steps(s, &label, artboard, &mut cmds); let stash = s.batch_stash.take().unwrap_or_default(); let results = match r { Ok(results) => results, Err(e) => { - roll_back(s, &start, stash); + roll_back(s, start, &stash); s.active = active; (s.paint, s.fill_active, s.draw_mode, s.draw_inside, s.clipboard, s.untitled_counter) = saved; s.pending_paint = None; @@ -81,22 +89,17 @@ fn run_steps(s: &mut Session, label: &str, artboard: Option, cmds: &mut [Va Ok(results) } -/// The documents as they were before the batch (`start`: their uids in order): the ones it opened -/// close, the ones it closed or replaced (`stash`) come back, and each one's interaction is undone. -fn roll_back(s: &mut Session, start: &[u64], mut stash: Vec) { - let mut open = std::mem::take(&mut s.docs); - for &uid in start { - // The newest revision the document reached, so the restored one redraws. - let newest = open.iter().chain(&stash).filter(|d| d.uid == uid).map(|d| d.revision).max().unwrap_or_default(); - // Its state before the batch: the first one a step closed or replaced, else the open one. - let st = match stash.iter().position(|d| d.uid == uid) { - Some(i) => Some(stash.remove(i)), - None => open.iter().position(|d| d.uid == uid).map(|i| open.remove(i)), - }; - if let Some(mut st) = st { - st.undo_interaction(); - st.revision = newest.saturating_add(1); - s.docs.push(st); - } +/// The documents as they were before the batch (`start`), whatever the steps did to them: opened +/// ones close, closed or replaced ones (`stash`) come back, and each has its art, selection, +/// history, file, format and saved state again (#1697: undoing the batch's interaction alone left +/// a Save As step's new file and an Undo step's history in place). Files the steps wrote stay +/// written. +fn roll_back(s: &mut Session, start: Vec, stash: &[DocState]) { + let open = std::mem::take(&mut s.docs); + s.docs = start; + for st in &mut s.docs { + // Past the newest revision the document reached, so the restored one redraws. + let newest = open.iter().chain(stash).filter(|d| d.uid == st.uid).map(|d| d.revision).max().unwrap_or(st.revision); + st.revision = newest.saturating_add(1); } } diff --git a/crates/engine/tests/model_based.rs b/crates/engine/tests/model_based.rs index af91b249..13a21032 100644 --- a/crates/engine/tests/model_based.rs +++ b/crates/engine/tests/model_based.rs @@ -121,6 +121,44 @@ fn batch_is_one_undo_step_and_rolls_back_on_error() { assert_eq!(s.doc().unwrap().doc.node_count(), 1); } +/// #1697: a failed batch brings back the whole document, not only its art: the file a Save As +/// step named and the history an Undo step moved; and a batch leaves a drag in progress its own +/// step. +#[test] +fn a_failed_batch_restores_the_file_the_history_and_a_drag_in_progress() { + let mut s = fixtures::session(); + let rect = |x: i32| json!({"command": "shape.rectangle", "params": {"x": x, "y": 0, "width": 10, "height": 10}}); + let fail = json!({"command": "no.such.command", "params": {}}); + exec(&mut s, "shape.rectangle", json!({"x": 50, "y": 0, "width": 10, "height": 10})); + let before = (doc_json(&s.doc().unwrap().doc), s.doc().unwrap().history.undo.len(), s.doc().unwrap().history.redo.len()); + let state = |s: &vectorcraft_engine::Session| { + let st = s.doc().unwrap(); + (doc_json(&st.doc), st.history.undo.len(), st.history.redo.len()) + }; + // Save As, then a failing step: the document keeps no file and stays modified. + let path = vectorcraft_testkit::temp_dir("batch-rollback").join("named.vectorcraft").to_string_lossy().to_string(); + let save = json!({"command": "file.saveAs", "params": {"path": path}}); + assert!(s.execute("command.batch", &json!({"commands": [rect(0), save, fail]})).is_err()); + assert_eq!(state(&s), before); + assert_eq!((s.doc().unwrap().path.as_deref(), s.doc().unwrap().is_dirty()), (None, true)); + // Undo, then a failing step: the art and both history stacks are as before. + let undo = json!({"command": "edit.undo", "params": {}}); + assert!(s.execute("command.batch", &json!({"commands": [undo, fail]})).is_err()); + assert_eq!(state(&s), before); + // A batch during a drag: the drag is kept as a step, the batch is the next one. + exec(&mut s, "select.all", json!({})); + s.begin_interaction("Move").unwrap(); + s.preview("object.move", &json!({"dx": 5, "dy": 0})).unwrap(); + let moved = doc_json(&s.doc().unwrap().doc); + exec(&mut s, "command.batch", json!({"commands": [rect(100)]})); + assert!(!s.in_interaction()); + assert_eq!(s.doc().unwrap().history.undo.len(), before.1 + 2); + exec(&mut s, "edit.undo", json!({})); + assert_eq!(doc_json(&s.doc().unwrap().doc), moved, "undoing the batch leaves the drag's move"); + exec(&mut s, "edit.undo", json!({})); + assert_eq!(doc_json(&s.doc().unwrap().doc), before.0); +} + #[test] fn empty_batch_is_harmless() { let mut s = fixtures::session();