From a6ce22da5cc9c32680b0b68a8bdbdb89d9ae3cdb Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 1 Nov 2026 10:02:06 -0800 Subject: [PATCH] Variables: an import or library load that changes nothing adds no undo step dataset.import and variable.loadLibrary added an undo step, cleared Redo and marked a saved document modified even when the variables and data sets came out as they were, such as the same CSV or library loaded again. Both now build the new variables on a copy and record a step only when it differs. variable.loadLibrary's params text and docs/mcp.md say so. --- crates/engine/src/cmd/variablecmds.rs | 2 +- crates/engine/src/cmd/variablecmds/csv.rs | 33 +++++++----- crates/engine/src/cmd/variablecmds/library.rs | 22 +++++--- crates/engine/src/tests/variables.rs | 53 +++++++++++++++++++ docs/mcp.md | 3 +- 5 files changed, 89 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/cmd/variablecmds.rs b/crates/engine/src/cmd/variablecmds.rs index 70a5af938..66948e692 100644 --- a/crates/engine/src/cmd/variablecmds.rs +++ b/crates/engine/src/cmd/variablecmds.rs @@ -816,7 +816,7 @@ pub fn specs() -> Vec { "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 ), diff --git a/crates/engine/src/cmd/variablecmds/csv.rs b/crates/engine/src/cmd/variablecmds/csv.rs index 8f23772a3..f9e170c59 100644 --- a/crates/engine/src/cmd/variablecmds/csv.rs +++ b/crates/engine/src/cmd/variablecmds/csv.rs @@ -324,21 +324,26 @@ pub(super) fn dataset_import(s: &mut Session, p: &Value) -> Result { } let names: Vec = parsed.iter().map(|d| d.name.clone()).collect(); let new_variables: Vec = 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; - for row in parsed { - if let Some(existing) = d.variables.datasets.iter_mut().find(|x| x.name == row.name) { - *existing = row; - updated += 1; - } else { - d.variables.datasets.push(row); - imported += 1; - } + let mut vars = s.doc()?.doc.variables.clone(); + vars.variables.extend(defined); + let (mut imported, mut updated) = (0usize, 0usize); + for row in parsed { + if let Some(existing) = vars.datasets.iter_mut().find(|x| x.name == row.name) { + *existing = row; + updated += 1; + } else { + vars.datasets.push(row); + imported += 1; } - Ok((imported, updated)) - })?; + } + // The import is one undo step, and an import that leaves the variables and data sets as they + // were adds none. + if vars != s.doc()?.doc.variables { + s.edit("Import Data Sets", |d, _| { + d.variables = vars; + Ok(()) + })?; + } Ok(json!({"imported": imported, "updated": updated, "names": names, "defined": new_variables})) } diff --git a/crates/engine/src/cmd/variablecmds/library.rs b/crates/engine/src/cmd/variablecmds/library.rs index b410af042..cdd2ba4d0 100644 --- a/crates/engine/src/cmd/variablecmds/library.rs +++ b/crates/engine/src/cmd/variablecmds/library.rs @@ -249,7 +249,8 @@ fn parse_library(xml: &str, cmd: &str) -> Result { } /// 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,13 +269,18 @@ pub(super) fn load_library(s: &mut Session, p: &Value) -> Result { } let added: Vec = lib.text.iter().map(|n| Variable { name: n.clone(), kind: VariableKind::Text }).collect(); let names: Vec = 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; - Ok(()) - })?; + let mut vars = s.doc()?.doc.variables.clone(); + vars.prune(&deleted); + vars.variables.extend(added); + vars.datasets = lib.datasets; + vars.active_dataset = None; + // A load that leaves the variables and data sets as they were adds no undo step. + if vars != s.doc()?.doc.variables { + s.edit("Load Variable Library", |d, _| { + d.variables = vars; + Ok(()) + })?; + } // A highlight of a variable that is gone highlights nothing. let st = s.doc_mut()?; st.drop_stale_variables_highlight(); diff --git a/crates/engine/src/tests/variables.rs b/crates/engine/src/tests/variables.rs index 84a277977..1844d9f20 100644 --- a/crates/engine/src/tests/variables.rs +++ b/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() { diff --git a/docs/mcp.md b/docs/mcp.md index ca9b00379..f98195ed3 100644 --- a/docs/mcp.md +++ b/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