Variables: a command that changes nothing adds no undo step

Squashed from #5130 onto main.

crates/engine/src/cmd/variablecmds.rs

@@ -14,7 +14,7 @@
 use std::collections::{BTreeMap, BTreeSet};
 
 use serde_json::{Value, json};
-use vectorcraft_doc::{DataSet, DataValue, NodeId, NodeKind, Variable, VariableKind, Variables};
+use vectorcraft_doc::{DataSet, DataValue, Document, NodeId, NodeKind, Selection, Variable, VariableKind, Variables};
 
 use super::edit::selected_roots;
 use super::fileio::{absolute_path, read_file_head, write_file};
@@ -44,6 +44,66 @@ fn selection_and_unlocked(s: &Session) -> std::result::Result<(), String> {
     variables_unlocked(s)
 }
 
+/// Ends a typing session as [`Session::edit`] would, before a helper below compares the document,
+/// so the typing is its own undo step whether or not the command then changes anything.
+fn settle_typing(s: &mut Session) -> Result<()> {
+    if !s.previewing && super::typing_in_progress(s) {
+        super::settle_interaction(s)?;
+    }
+    Ok(())
+}
+
+/// [`Session::edit`] for a command that changes only the variables and data sets and may change
+/// nothing, such as binding art to the variable it already has: `f` runs on a copy of them, and
+/// only a copy that differs becomes an undo step. When nothing changed, the undo and redo steps and
+/// the saved state stay as they were.
+fn edit_variables<T>(s: &mut Session, label: &str, f: impl FnOnce(&mut Variables) -> Result<T>) -> Result<T> {
+    settle_typing(s)?;
+    let st = s.doc()?;
+    let mut vars = st.doc.variables.clone();
+    let v = f(&mut vars)?;
+    if vars != st.doc.variables {
+        s.edit(label, |d, _| {
+            d.variables = vars;
+            Ok(())
+        })?;
+    }
+    Ok(v)
+}
+
+/// [`Session::edit`] for applying a data set, which may change nothing when the art already shows
+/// the active row: `f` runs on a copy of the document and the selection, and only a document that
+/// differs becomes an undo step, with the selection `f` left. When the document did not change, the
+/// undo and redo steps and the saved state stay as they were, and a selection `f` changed is set
+/// without an undo step. When the active row is applied again and the art already shows it,
+/// threaded type is compared after its story flows through its frames, as [`Session::edit`] flows
+/// it; any other change is an undo step without that extra flow.
+fn edit_if_changed<T>(s: &mut Session, label: &str, f: impl FnOnce(&mut Document, &mut Selection) -> Result<T>) -> Result<T> {
+    settle_typing(s)?;
+    let st = s.doc()?;
+    let mut doc = (*st.doc).clone();
+    let mut sel = st.selection.clone();
+    let v = f(&mut doc, &mut sel)?;
+    let vars = &st.doc.variables;
+    if doc != *st.doc
+        && !doc.text_threads.is_empty()
+        && doc.variables == *vars
+        && vars.active_dataset.as_deref().and_then(|n| vars.dataset(n)).is_some_and(|ds| ds.matches(&st.doc.captured_values()))
+    {
+        super::threads::reflow(&st.doc, &mut doc);
+    }
+    if doc != *st.doc {
+        s.edit(label, |d, selection| {
+            *d = doc;
+            *selection = sel;
+            Ok(())
+        })?;
+    } else if sel != st.selection {
+        s.select(|_, selection| *selection = sel)?;
+    }
+    Ok(v)
+}
+
 fn var_name(p: &Value, cmd: &str) -> Result<String> {
     named(str_param(p, "name"), "name", cmd)
 }
@@ -228,7 +288,7 @@ fn delete(s: &mut Session, p: &Value) -> Result<Value> {
     // Without `names`, the highlighted variable: what the panel's bin and the menu item delete.
     let highlighted = s.doc()?.variables_highlight.clone();
     let names = names_param(p, "variable.delete", highlighted, "no variable highlighted")?;
-    let n = s.edit("Delete Variables", |d, _| Ok(d.variables.prune(&names)))?;
+    let n = edit_variables(s, "Delete Variables", |v| Ok(v.prune(&names)))?;
     // A highlight of a variable that is gone highlights nothing.
     let st = s.doc_mut()?;
     st.drop_stale_variables_highlight();
@@ -277,7 +337,7 @@ fn bind(s: &mut Session, p: &Value) -> Result<Value> {
     check_bindable(doc, &ids, kind, C)?;
     // Binding again is not a second binding, and says so rather than counting as a change.
     let already: Vec<u64> = ids.iter().filter(|id| doc.variables.objects_of(&name).contains(id)).map(|id| id.0).collect();
-    let n = s.edit("Bind Variable", |d, _| Ok(ids.iter().filter(|id| d.variables.bind(**id, &name)).count()))?;
+    let n = edit_variables(s, "Bind Variable", |v| Ok(ids.iter().filter(|id| v.bind(**id, &name)).count()))?;
     Ok(json!({"variable": name, "ids": ids.iter().map(|i| i.0).collect::<Vec<_>>(), "bound": n, "already": already}))
 }
 
@@ -285,13 +345,13 @@ fn unbind(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "variable.unbind";
     let ids = checked_ids_or_selection(s, p, C)?;
     let name = defined_variable(s, p, C)?;
-    let n = s.edit("Unbind Variable", |d, _| {
+    let n = edit_variables(s, "Unbind Variable", |vars| {
         let mut n = 0;
         for id in &ids {
             // With `variable` only that one binding goes; without it, every one the object has.
             match &name {
-                Some(v) => n += usize::from(d.variables.unbind(*id, v)),
-                None => n += usize::from(d.variables.bindings.remove(id).is_some()),
+                Some(v) => n += usize::from(vars.unbind(*id, v)),
+                None => n += usize::from(vars.bindings.remove(id).is_some()),
             }
         }
         Ok(n)
@@ -344,8 +404,8 @@ fn dataset_set(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "dataset.set";
     let name = var_name(p, C)?;
     let values = values_param(&s.doc()?.doc.variables, p, C)?;
-    let (set, removed) = s.edit("Edit Data Set", |d, _| {
-        let ds = d.variables.datasets.iter_mut().find(|x| x.name == name).ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
+    let (set, removed) = edit_variables(s, "Edit Data Set", |v| {
+        let ds = v.datasets.iter_mut().find(|x| x.name == name).ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
         let removed: Vec<String> = ds.values.keys().filter(|k| !values.contains_key(*k)).cloned().collect();
         ds.values = values.clone();
         Ok((values.len(), removed))
@@ -415,8 +475,8 @@ fn dataset_update(s: &mut Session, p: &Value) -> Result<Value> {
     if values.is_empty() {
         return Err(bad(C, "bind something to a variable first"));
     }
-    let (written, left) = s.edit("Update Data Set", |d, _| {
-        let ds = d.variables.datasets.iter_mut().find(|d| d.name == name).ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
+    let (written, left) = edit_variables(s, "Update Data Set", |vars| {
+        let ds = vars.datasets.iter_mut().find(|d| d.name == name).ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
         let mut written = BTreeSet::new();
         for (var, value) in &values {
             if all || ds.values.contains_key(var) {
@@ -424,7 +484,7 @@ fn dataset_update(s: &mut Session, p: &Value) -> Result<Value> {
                 written.insert(var.as_str());
             }
         }
-        let left: Vec<String> = d.variables.variables.iter().filter(|v| !written.contains(v.name.as_str())).map(|v| v.name.clone()).collect();
+        let left: Vec<String> = vars.variables.iter().filter(|v| !written.contains(v.name.as_str())).map(|v| v.name.clone()).collect();
         Ok((written.len(), left))
     })?;
     Ok(json!({"name": name, "values": written, "left": left}))
@@ -434,14 +494,14 @@ fn dataset_delete(s: &mut Session, p: &Value) -> Result<Value> {
     // Without `names`, the active dataset: what the panel's Delete Data Set deletes.
     let active = s.doc()?.doc.variables.active_dataset.clone();
     let names = names_param(p, "dataset.delete", active, "no active data set")?;
-    let n = s.edit("Delete Data Sets", |d, _| {
-        let before = d.variables.datasets.len();
-        d.variables.datasets.retain(|d| !names.contains(&d.name));
+    let n = edit_variables(s, "Delete Data Sets", |v| {
+        let before = v.datasets.len();
+        v.datasets.retain(|d| !names.contains(&d.name));
         // The active dataset's own name is the one that goes, not a variable's.
-        if d.variables.active_dataset.as_ref().is_some_and(|a| names.contains(a)) {
-            d.variables.active_dataset = None;
+        if v.active_dataset.as_ref().is_some_and(|a| names.contains(a)) {
+            v.active_dataset = None;
         }
-        Ok(before - d.variables.datasets.len())
+        Ok(before - v.datasets.len())
     })?;
     Ok(json!({"deleted": n}))
 }
@@ -464,7 +524,7 @@ fn dataset_list(s: &mut Session, _p: &Value) -> Result<Value> {
 fn apply_dataset(s: &mut Session, name: &str) -> Result<Value> {
     const C: &str = "dataset.select";
     let doc_path = s.doc()?.path.clone();
-    let (applied, skipped, mut typed, bound) = s.edit("Apply Data Set", |d, sel| {
+    let (applied, skipped, mut typed, bound) = edit_if_changed(s, "Apply Data Set", |d, sel| {
         let ds = d.variables.dataset(name).cloned().ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
         // One object may be driven by several variables; walk them in definition order, so two
         // text variables on the same object land the later one's value and both are counted.
@@ -714,7 +774,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Select Data Set",
             ["Window", "Variables"],
             None,
-            "{name} apply a dataset in one undo step, deselecting every selected object left hidden or locked (the key object too); a text value bound to a frame of threaded text replaces the whole story → {applied, skipped, overflow?: [id…] (area type and type on a path whose text it rewrote and that no longer fits, the story's last frame for threaded text; left out when all fits), warning? (when no object is bound to a variable that the dataset has a value for, so nothing was applied)}",
+            "{name} apply a dataset in one undo step (none when it leaves the document as it was), deselecting every selected object left hidden or locked (the key object too); a text value bound to a frame of threaded text replaces the whole story → {applied, skipped, overflow?: [id…] (area type and type on a path whose text it rewrote and that no longer fits, the story's last frame for threaded text; left out when all fits), warning? (when no object is bound to a variable that the dataset has a value for, so nothing was applied)}",
             has_doc,
             dataset_select
         ),