Merge pull request #5145 from dotCooCoo/fix/vars-noop-import-undo

Variables: an import or library load that changes nothing adds no undo step

crates/engine/src/cmd/variablecmds.rs

@@ -816,7 +816,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Load Variable Library…",
             ["Window", "Variables"],
             None,
-            "{path? | xml? | dataBase64?} replace the document's variables and data sets with a variable library's (XML: `variableSets` › `variableSet` › `variables` of `variable` elements with `varName` and `trait`, and `sampleDataSets` of `sampleDataSet` elements with `dataSetName`) in one undo step. Every variable of the document is deleted with its bindings and values, and every data set is deleted; then variables whose `trait` is `textcontent` are defined as text variables, unbound, and each data set takes a value for a variable from the `p` elements of the element named after it, one paragraph each. Variables of other kinds are left out and named in `skipped` and `warning`. Refused while the variables are locked when it would delete or define a variable → {variables, deleted, deletedDataSets, names, skipped, warning?}",
+            "{path? | xml? | dataBase64?} replace the document's variables and data sets with a variable library's (XML: `variableSets` › `variableSet` › `variables` of `variable` elements with `varName` and `trait`, and `sampleDataSets` of `sampleDataSet` elements with `dataSetName`) in one undo step (none when the document already holds what the library gives). Every variable of the document is deleted with its bindings and values, and every data set is deleted; then variables whose `trait` is `textcontent` are defined as text variables, unbound, and each data set takes a value for a variable from the `p` elements of the element named after it, one paragraph each. Variables of other kinds are left out and named in `skipped` and `warning`. Refused while the variables are locked when it would delete or define a variable → {variables, deleted, deletedDataSets, names, skipped, warning?}",
             has_doc,
             load_library
         ),

crates/engine/src/cmd/variablecmds/csv.rs

@@ -324,16 +324,17 @@ pub(super) fn dataset_import(s: &mut Session, p: &Value) -> Result<Value> {
     }
     let names: Vec<String> = parsed.iter().map(|d| d.name.clone()).collect();
     let new_variables: Vec<String> = defined.iter().map(|v| v.name.clone()).collect();
-    let (imported, updated) = s.edit("Import Data Sets", |d, _| {
-        d.variables.variables.extend(defined);
-        let mut imported = 0usize;
-        let mut updated = 0usize;
+    // The import is one undo step, and an import that leaves the variables and data sets as they
+    // were adds none.
+    let (imported, updated) = super::edit_variables(s, "Import Data Sets", |vars| {
+        vars.variables.extend(defined);
+        let (mut imported, mut updated) = (0usize, 0usize);
         for row in parsed {
-            if let Some(existing) = d.variables.datasets.iter_mut().find(|x| x.name == row.name) {
+            if let Some(existing) = vars.datasets.iter_mut().find(|x| x.name == row.name) {
                 *existing = row;
                 updated += 1;
             } else {
-                d.variables.datasets.push(row);
+                vars.datasets.push(row);
                 imported += 1;
             }
         }

crates/engine/src/cmd/variablecmds/library.rs

@@ -249,7 +249,8 @@ fn parse_library(xml: &str, cmd: &str) -> Result<Library> {
 }
 
 /// Load Variable Library: replace the document's variables and data sets with a variable library's
-/// ([`parse_library`]) in one undo step. Every variable of the document is deleted with its bindings
+/// ([`parse_library`]) in one undo step, or none when the document already holds what the library
+/// gives. Every variable of the document is deleted with its bindings
 /// and values, and every data set is deleted. The library's text variables are then defined,
 /// unbound, and its data sets are added. The library's variables of other kinds are left out, named
 /// in `skipped` and in a warning.
@@ -268,11 +269,12 @@ pub(super) fn load_library(s: &mut Session, p: &Value) -> Result<Value> {
     }
     let added: Vec<Variable> = lib.text.iter().map(|n| Variable { name: n.clone(), kind: VariableKind::Text }).collect();
     let names: Vec<String> = lib.datasets.iter().map(|d| d.name.clone()).collect();
-    s.edit("Load Variable Library", |d, _| {
-        d.variables.prune(&deleted);
-        d.variables.variables.extend(added);
-        d.variables.datasets = lib.datasets;
-        d.variables.active_dataset = None;
+    // A load that leaves the variables and data sets as they were adds no undo step.
+    super::edit_variables(s, "Load Variable Library", |vars| {
+        vars.prune(&deleted);
+        vars.variables.extend(added);
+        vars.datasets = lib.datasets;
+        vars.active_dataset = None;
         Ok(())
     })?;
     // A highlight of a variable that is gone highlights nothing.

crates/engine/src/tests/variables.rs

@@ -1762,6 +1762,59 @@ fn a_variable_library_replaces_every_variable_and_data_set() {
     assert_eq!(s.doc().unwrap().doc.variables, before, "one undo step brings everything back");
 }
 
+/// An import or a library load that leaves the variables and data sets as they were (the same CSV
+/// imported again, a row rewritten with its own values, the same library loaded again) records no
+/// undo step: the step to redo stays, and the document stays unmodified. An import or load that
+/// changes something is still one undo step.
+#[test]
+fn an_import_or_library_load_that_changes_nothing_records_no_undo_step() {
+    let mut s = session();
+    let t = text(&mut s, "Alice");
+    let csv = "name,Name,#Shown\nA,Ann,true\nB,Bob,false\n";
+    s.execute("dataset.import", &json!({"csv": csv})).unwrap();
+    for csv in [csv, "name\tName\t#Shown\nB\tBob\tfalse\n", "Name,#Shown\nAnn,true\n"] {
+        super::select_with_a_step_to_redo(&mut s, &[t]);
+        let before = super::history(&s);
+        assert_eq!((before.1, before.2), (1, false), "a step to redo, and nothing unsaved");
+        let r = s.execute("dataset.import", &json!({"csv": csv})).unwrap();
+        if csv.starts_with("name") {
+            assert_eq!((super::history(&s), r["imported"].clone()), (before, json!(0)), "{csv}");
+        } else {
+            // Without a name column the row is a new data set, which is a change.
+            assert_eq!((super::history(&s), r["imported"].clone()), ((before.0 + 1, 0, true), json!(1)), "{csv}");
+        }
+    }
+    super::select_with_a_step_to_redo(&mut s, &[t]);
+    let undo = super::history(&s).0;
+    s.execute("dataset.import", &json!({"csv": "name,Name\nB,Cy\n"})).unwrap();
+    assert_eq!(super::history(&s), (undo + 1, 0, true));
+
+    let xml = library(&[("Name", "textcontent")], &[("A", &[("Name", &["Ann"])]), ("B", &[("Name", &["Bob"])])]);
+    s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
+    super::select_with_a_step_to_redo(&mut s, &[t]);
+    let before = super::history(&s);
+    let r = s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
+    assert_eq!((super::history(&s), r["names"].clone()), (before, json!(["A", "B"])));
+    // Binding the variable makes the next load a change: it deletes the binding.
+    s.execute("variable.bind", &json!({"variable": "Name", "ids": [t.0]})).unwrap();
+    super::select_with_a_step_to_redo(&mut s, &[t]);
+    let undo = super::history(&s).0;
+    s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
+    assert_eq!(super::history(&s), (undo + 1, 0, true));
+
+    // While the variables are locked, a library without text variables still loads its data sets,
+    // and loading it again changes nothing.
+    let mut s = session();
+    let t = text(&mut s, "Alice");
+    s.execute("variable.lock", &json!({"locked": true})).unwrap();
+    let xml = library(&[("Shown", "other")], &[("B", &[])]);
+    s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
+    super::select_with_a_step_to_redo(&mut s, &[t]);
+    let before = super::history(&s);
+    s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
+    assert_eq!(super::history(&s), before);
+}
+
 /// A library that is refused leaves the document's variables and data sets as they were.
 #[test]
 fn a_refused_variable_library_deletes_nothing() {

docs/mcp.md

@@ -1652,7 +1652,8 @@ already walks Image Trace. Short agent entry points:
   reply's `defined` lists the variables the import defined. While the variables are
   locked, an import whose header would define a variable fails with "variables are locked".
 - Variable libraries: `variable.loadLibrary {path|xml|dataBase64}` (the Variables panel menu's Load Variable
-  Library…) replaces the document's variables and data sets with a variable library's in one undo step. The library
+  Library…) replaces the document's variables and data sets with a variable library's in one undo step. A load, like a
+  `dataset.import`, adds no undo step when it leaves the variables and data sets as they were. The library
   is XML whose `variableSets` element holds `variableSet` elements, each with a `variables` element of `variable`
   elements (`varName`, `trait`) and a `sampleDataSets` element of `sampleDataSet` elements (`dataSetName`). Every
   variable of the document is deleted with its bindings and values, and every data set is deleted. Then the library's