diff --git a/crates/engine/src/cmd/graph/data.rs b/crates/engine/src/cmd/graph/data.rs index 0e69e0240..913d52522 100644 --- a/crates/engine/src/cmd/graph/data.rs +++ b/crates/engine/src/cmd/graph/data.rs @@ -87,16 +87,20 @@ pub(super) fn to_csv(g: &GraphSpec) -> String { } /// Apply CSV graph data to the graph root `id` and rebuild its art (Variables data merge), as `graph.setData` with -/// `csv` does. +/// `csv` does. Data the graph already holds leaves the graph as it is. pub(crate) fn apply_csv(d: &mut Document, id: NodeId, csv: &str) -> Result<()> { let Some(node) = d.node(id) else { return Err(EngineError::NoNode(id)); }; - let Some(mut spec) = node.extras().graph.as_deref().cloned() else { + let Some(current) = node.extras().graph.as_deref() else { return Err(bad("graph.applyCsv", format!("node {} is not a graph", id.0))); }; + let mut spec = current.clone(); apply_data(&mut spec, &json!({ "csv": csv })); keep_right_series(&mut spec); + if spec == *current { + return Ok(()); + } regenerate(d, id, spec) } diff --git a/crates/engine/src/cmd/variablecmds.rs b/crates/engine/src/cmd/variablecmds.rs index 03636d11f..873a694e2 100644 --- a/crates/engine/src/cmd/variablecmds.rs +++ b/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(s: &mut Session, label: &str, f: impl FnOnce(&mut Variables) -> Result) -> Result { + 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(s: &mut Session, label: &str, f: impl FnOnce(&mut Document, &mut Selection) -> Result) -> Result { + 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 { named(str_param(p, "name"), "name", cmd) } @@ -228,7 +288,7 @@ fn delete(s: &mut Session, p: &Value) -> Result { // 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 { 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 = 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::>(), "bound": n, "already": already})) } @@ -285,13 +345,13 @@ fn unbind(s: &mut Session, p: &Value) -> Result { 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 { 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 = 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 { 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 { written.insert(var.as_str()); } } - let left: Vec = d.variables.variables.iter().filter(|v| !written.contains(v.name.as_str())).map(|v| v.name.clone()).collect(); + let left: Vec = 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 { // 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 { fn apply_dataset(s: &mut Session, name: &str) -> Result { 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 { "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 ), diff --git a/crates/engine/src/tests/variables.rs b/crates/engine/src/tests/variables.rs index 776cf1978..f470242de 100644 --- a/crates/engine/src/tests/variables.rs +++ b/crates/engine/src/tests/variables.rs @@ -1936,3 +1936,133 @@ fn graph_variable_applies_csv_and_captures() { let r = rect(&mut s); assert!(s.execute("variable.bind", &json!({"variable": name, "ids": [r.0]})).is_err()); } + +/// A Variables command that leaves the document as it was (binding art to the variable it already +/// has, unbinding art that has none, deleting a variable or data set that does not exist, writing a +/// row's own values back, applying the active data set again) records no undo step: the step to +/// redo stays, and the document stays unmodified. +#[test] +fn a_variables_command_that_changes_nothing_records_no_undo_step() { + let dir = Folder::new("vars-noop"); + let red_path = dir.file("red.png"); + write(&red_path, &png(8, 8, RED)); + let mut s = session(); + let t = text(&mut s, "Alice"); + let r = rect(&mut s); + let img = place(&mut s, &red_path); + let other = rect(&mut s); + s.execute("variable.define", &json!({"name": "Name", "kind": "text"})).unwrap(); + s.execute("variable.define", &json!({"name": "Show", "kind": "visibility"})).unwrap(); + s.execute("variable.define", &json!({"name": "Photo", "kind": "image"})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Name", "ids": [t.0]})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Show", "ids": [r.0]})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Photo", "ids": [img.0]})).unwrap(); + s.execute("dataset.new", &json!({"name": "A", "values": {"Name": "Bob", "Show": false, "Photo": red_path}})).unwrap(); + s.execute("dataset.select", &json!({"name": "A"})).unwrap(); + let a = json!({"name": "A", "values": {"Name": "Bob", "Show": false, "Photo": red_path}}); + let calls = [ + ("variable.bind", json!({"variable": "Show", "ids": [r.0]})), + ("variable.unbind", json!({"ids": [other.0]})), + ("variable.unbind", json!({"variable": "Name", "ids": [r.0]})), + ("variable.delete", json!({"names": ["Nope"]})), + ("dataset.delete", json!({"names": ["Nope"]})), + ("dataset.set", a), + ("dataset.update", json!({})), + ("dataset.update", json!({"all": true})), + ("dataset.select", json!({"name": "A"})), + ("dataset.next", json!({})), + ("dataset.prev", json!({})), + ]; + for (cmd, p) in calls { + super::select_with_a_step_to_redo(&mut s, &[other]); + let before = super::history(&s); + assert_eq!((before.1, before.2), (1, false), "a step to redo, and nothing unsaved"); + s.execute(cmd, &p).unwrap(); + assert_eq!(super::history(&s), before, "{cmd} {p}"); + } + assert_eq!(plain(&s, t), "Bob"); + // Applying the row again still deselects the art it leaves hidden, without an undo step. + s.execute("select.set", &json!({"ids": [r.0, other.0]})).unwrap(); + assert!(s.doc().unwrap().selection.objects.contains(&r), "the hidden rectangle is selected"); + let before = super::history(&s); + s.execute("dataset.select", &json!({"name": "A"})).unwrap(); + assert_eq!((super::history(&s), s.doc().unwrap().selection.objects.clone()), (before, vec![other])); + // A command that changes something is still one undo step, and it clears Redo. + super::select_with_a_step_to_redo(&mut s, &[other]); + let undo = super::history(&s).0; + s.execute("variable.bind", &json!({"variable": "Show", "ids": [other.0]})).unwrap(); + assert_eq!(super::history(&s), (undo + 1, 0, true)); + s.execute("dataset.new", &json!({"name": "B", "values": {"Name": "Cy"}})).unwrap(); + s.doc_mut().unwrap().mark_saved(); + let undo = super::history(&s).0; + s.execute("dataset.next", &json!({})).unwrap(); + assert_eq!((super::history(&s), plain(&s, t)), ((undo + 1, 0, true), "Cy".to_string())); +} + +/// A Variables command that changes nothing, run while the Type tool is typing, still ends the +/// typing session as one that changes something does: the typing is its own undo step, and the +/// next keystroke starts a new one. +#[test] +fn a_variables_command_that_changes_nothing_ends_the_typing() { + use vectorcraft_tools::{PointerEvent, PointerKind}; + let mut s = session(); + let v = ViewInfo::default(); + let t = text(&mut s, "Hello"); + let r = s.execute("shape.rectangle", &json!({"x": 200, "y": 200, "width": 50, "height": 40})).unwrap(); + let r = NodeId(r["id"].as_u64().unwrap()); + s.execute("variable.define", &json!({"name": "Show", "kind": "visibility"})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Show", "ids": [r.0]})).unwrap(); + s.execute("dataset.new", &json!({"name": "A", "values": {"Show": true}})).unwrap(); + s.execute("dataset.select", &json!({"name": "A"})).unwrap(); + for (cmd, p) in [("variable.bind", json!({"variable": "Show", "ids": [r.0]})), ("dataset.select", json!({"name": "A"}))] { + s.select_tool("type", v).unwrap(); + let b = s.doc().unwrap().doc.bounds_of(&[t], false).unwrap(); + for kind in [PointerKind::Down, PointerKind::Up] { + s.pointer(&PointerEvent::new(kind, b.x1 - 1.0, b.center().y), v).unwrap(); + } + s.tool_text("!", v).unwrap(); + assert!(crate::cmd::typing_in_progress(&s), "{cmd}"); + let steps = s.doc().unwrap().history.undo.len(); + s.execute(cmd, &p).unwrap(); + assert!(!crate::cmd::typing_in_progress(&s), "{cmd}"); + let labels: Vec = s.doc().unwrap().history.undo.iter().map(|e| e.label.clone()).collect(); + assert_eq!((labels.len(), labels.last().map(String::as_str)), (steps + 1, Some("Typing")), "{cmd}"); + } +} + +/// Applying the active data set again to threaded type that already holds the row's story records +/// no undo step. +#[test] +fn applying_a_story_row_again_records_no_undo_step() { + let (mut s, a, b, _) = threaded(200); + s.execute("variable.define", &json!({"name": "Name", "kind": "text"})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Name", "ids": [a.0]})).unwrap(); + s.execute("dataset.capture", &json!({"name": "Orig"})).unwrap(); + s.execute("dataset.select", &json!({"name": "Orig"})).unwrap(); + assert!(!plain(&s, b).is_empty()); + super::select_with_a_step_to_redo(&mut s, &[a]); + let before = super::history(&s); + s.execute("dataset.select", &json!({"name": "Orig"})).unwrap(); + assert_eq!(super::history(&s), before); +} + +/// Applying the active data set again to a graph whose data already matches the row records no +/// undo step, and a row with other data still rebuilds the graph in one. +#[test] +fn applying_a_graph_row_again_records_no_undo_step() { + let mut s = session(); + let graph = json!({"type": "column", "x": 10, "y": 10, "width": 120, "height": 80, "csv": ",Sales\nA,1\nB,2"}); + let id = s.execute("graph.create", &graph).unwrap()["id"].as_u64().unwrap(); + s.execute("select.set", &json!({"ids": [id]})).unwrap(); + s.execute("variable.makeGraphDynamic", &json!({})).unwrap(); + let name = s.execute("variable.list", &json!({})).unwrap()["variables"][0]["name"].as_str().unwrap().to_string(); + s.execute("dataset.new", &json!({"name": "A", "values": {name.clone(): ",Sales\nA,10\nB,20"}})).unwrap(); + s.execute("dataset.new", &json!({"name": "B", "values": {name: ",Sales\nA,3\nB,4"}})).unwrap(); + s.execute("dataset.select", &json!({"name": "A"})).unwrap(); + super::select_with_a_step_to_redo(&mut s, &[NodeId(id)]); + let before = super::history(&s); + let v = s.execute("dataset.select", &json!({"name": "A"})).unwrap(); + assert_eq!((v["applied"].as_u64(), super::history(&s)), (Some(1), before)); + s.execute("dataset.select", &json!({"name": "B"})).unwrap(); + assert_eq!(super::history(&s), (before.0 + 1, 0, true)); +} diff --git a/docs/mcp.md b/docs/mcp.md index 77a079b4b..7b9c59a14 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1743,6 +1743,11 @@ Brushes panel's New Brush button, its menu's New Brush… and art dropped on the Brush Options for the new brush; for a scatter, art or pattern brush they preview the art from `brush.selectionArt`. Their OK runs `brush.new {type, name, params, ids?}`, and Cancel adds no brush. +`variable.bind`, `variable.unbind`, `variable.delete`, `dataset.set`, `dataset.update`, `dataset.delete`, +`dataset.select`, `dataset.next` and `dataset.prev` add no undo step when they leave the document as it was, such as +binding art to a variable it already has or applying the active data set again: the steps to redo stay, and a saved +document stays unmodified. + `dataset.select`, `dataset.next` and `dataset.prev` reply `{name, applied, skipped}`. They add `overflow: [id…]` when area type or type on a path whose text they rewrote doesn't fit, as `text.setText` does (see [Fitting area type](#fitting-area-type-auto-size-shrink-text-to-fit-overflow)). They skip an object that is