diff --git a/crates/doc/src/variables.rs b/crates/doc/src/variables.rs index fb4a3c8f..45e3a37a 100644 --- a/crates/doc/src/variables.rs +++ b/crates/doc/src/variables.rs @@ -70,6 +70,15 @@ pub struct DataSet { pub values: BTreeMap, } +impl DataSet { + /// Whether the art, as [`Document::captured_values`] read it, still holds what this row + /// says. A value the art can't report (its object is locked or gone) isn't drift: the row + /// keeps it, and applying it again would skip it too. + pub fn matches(&self, current: &BTreeMap) -> bool { + self.values.iter().all(|(k, v)| current.get(k).is_none_or(|c| c == v)) + } +} + /// The variable state of a document: definitions, datasets, bindings /// (object id → the variables bound to it) and which dataset is active. /// @@ -128,13 +137,15 @@ impl Variables { /// them. Datasets named here are left alone: their names are their own namespace (the /// active one is dropped by whoever deletes the dataset). pub fn prune(&mut self, names: &[String]) -> usize { + // A set, so a long list of names costs a lookup per value rather than a scan. + let names: BTreeSet<&str> = names.iter().map(String::as_str).collect(); let before = self.variables.len(); - self.variables.retain(|v| !names.contains(&v.name)); + self.variables.retain(|v| !names.contains(v.name.as_str())); for dataset in &mut self.datasets { - dataset.values.retain(|k, _| !names.contains(k)); + dataset.values.retain(|k, _| !names.contains(k.as_str())); } for bound in self.bindings.values_mut() { - bound.retain(|v| !names.contains(v)); + bound.retain(|v| !names.contains(v.as_str())); } self.bindings.retain(|_, v| !v.is_empty()); before - self.variables.len() @@ -154,11 +165,19 @@ impl Document { self.variables.bindings = bindings; } + /// Whether a variable may read and rewrite the object `id`: it is in the document, neither + /// it nor anything above it is locked, and nothing above it is hidden. Its own visibility + /// doesn't count, as that is what a visibility variable drives: a row that hid it must be + /// able to show it again, and a capture must record it hidden. + pub fn variable_target(&self, id: NodeId) -> bool { + self.ancestry(id).is_some_and(|a| a.iter().all(|i| self.node(*i).is_some_and(|n| !n.locked && (n.visible || *i == id)))) + } + /// What the object `id` holds for the variable of `kind`: its characters, or whether it - /// shows. `None` when the object is gone, is not of that kind, or is on a locked layer - /// (the document's value is whatever it was left at). + /// shows. `None` when the object is gone, is not of that kind, or is locked or on a hidden + /// or locked layer (the document's value is whatever it was left at). pub fn variable_value(&self, id: NodeId, kind: VariableKind) -> Option { - if !self.is_editable(id) { + if !self.variable_target(id) { return None; } let node = self.node(id)?; @@ -185,14 +204,4 @@ impl Document { } out } - - /// Whether the bound objects still hold what `name`'s row says they do. False when there - /// is no such row, or when an edit has moved the art away from it. - pub fn matches_dataset(&self, name: &str) -> bool { - let Some(ds) = self.variables.dataset(name) else { return false }; - let current = self.captured_values(); - // A value the art can't report (its object is locked or gone) isn't drift: the row - // keeps it, and applying it again would skip it too. - ds.values.iter().all(|(k, v)| current.get(k).is_none_or(|c| c == v)) - } } diff --git a/crates/engine/src/cmd/variablecmds.rs b/crates/engine/src/cmd/variablecmds.rs index eae00c9b..a4755170 100644 --- a/crates/engine/src/cmd/variablecmds.rs +++ b/crates/engine/src/cmd/variablecmds.rs @@ -14,34 +14,62 @@ use super::edit::selected_roots; use super::textedit::set_plain_text; use super::*; +/// The longest variable or dataset name, in characters (names arrive from MCP and files). +const MAX_NAME: usize = 255; + fn var_name(p: &Value, cmd: &str) -> Result { named(str_param(p, "name"), "name", cmd) } -/// `key`'s value, as a non-empty name. +/// `key`'s value, as a name: trimmed, not empty and at most [`MAX_NAME`] characters. fn named(v: Option<&str>, key: &str, cmd: &str) -> Result { - match v.filter(|v| !v.is_empty()) { + match v.map(str::trim).filter(|v| !v.is_empty()) { + Some(v) if v.chars().count() > MAX_NAME => Err(bad(cmd, format!("`{key}` is longer than {MAX_NAME} characters"))), Some(v) => Ok(v.to_string()), None => Err(bad(cmd, format!("missing `{key}`"))), } } -fn names_param(p: &Value, cmd: &str) -> Result> { - match p.get("names").and_then(Value::as_array) { - Some(names) if !names.is_empty() => { - let mut out = Vec::with_capacity(names.len()); - for n in names { - match n.as_str().filter(|v| !v.is_empty()) { - Some(v) => out.push(v.to_string()), - None => return Err(bad(cmd, "`names` must be non-empty strings")), - } - } - Ok(out) - } - _ => Err(bad(cmd, "missing `names`")), +/// `key`'s value as a name, or `None` when it is missing or empty. +fn optional_name(p: &Value, key: &str, cmd: &str) -> Result> { + match str_param(p, key).map(str::trim).filter(|v| !v.is_empty()) { + Some(v) => named(Some(v), key, cmd).map(Some), + None => Ok(None), } } +/// The `names` of a delete, or without them `fallback` (what the panel acts on) as the one name. +fn names_param(p: &Value, cmd: &str, fallback: Option, nothing: &str) -> Result> { + let Some(given) = p.get("names") else { + return fallback.map(|n| vec![n]).ok_or_else(|| bad(cmd, nothing.to_string())); + }; + match given.as_array() { + Some(names) if !names.is_empty() => names.iter().map(|n| named(n.as_str(), "names", cmd)).collect(), + _ => Err(bad(cmd, "`names` must be a list of names")), + } +} + +/// The objects a bind acts on: `ids` when given (every one checked), else the selection. +fn target_ids(s: &Session, p: &Value, cmd: &str) -> Result> { + if p.get("ids").is_some() { checked_ids_param(s, p, "ids", cmd) } else { selected_roots(s) } +} + +/// Refuse what a variable of `kind` can't drive: art that is locked or on a hidden or locked +/// layer is not the document's to rewrite, and a text variable only drives type. +fn check_bindable(doc: &vectorcraft_doc::Document, ids: &[NodeId], kind: VariableKind, cmd: &str) -> Result<()> { + for id in ids { + match doc.node(*id) { + None => return Err(EngineError::NoNode(*id)), + Some(_) if !doc.variable_target(*id) => return Err(bad(cmd, format!("node {} is locked", id.0))), + Some(n) if kind == VariableKind::Text && !matches!(n.kind, NodeKind::Text(_)) => { + return Err(bad(cmd, format!("node {} is not type", id.0))); + } + Some(_) => {} + } + } + Ok(()) +} + fn dataset_value(cmd: &str, var: &str, kind: VariableKind, v: &Value) -> Result { match kind { VariableKind::Text => v.as_str().map(|t| DataValue::Text(t.to_string())), @@ -99,9 +127,6 @@ fn rename(s: &mut Session, p: &Value) -> Result { ds.values.insert(to.clone(), value); } } - if d.variables.active_dataset.as_deref() == Some(from.as_str()) { - d.variables.active_dataset = Some(to.clone()); - } Ok(()) })?; // The highlight names a variable, so it follows the rename rather than going stale. @@ -122,21 +147,15 @@ fn make_dynamic(s: &mut Session, kind: VariableKind, stem: &str) -> Result return Err(EngineError::NoNode(*id)), - Some(_) if !doc.is_editable(*id) => return Err(bad(C, format!("node {} is locked", id.0))), - Some(n) if kind == VariableKind::Text && !matches!(n.kind, NodeKind::Text(_)) => { - return Err(bad(C, format!("node {} is not type", id.0))); - } - Some(_) => {} - } - } + check_bindable(doc, &ids, kind, C)?; // Named after the first object it drives, as the reference panel does; a second variable // over the same art takes a number rather than colliding. - let base = doc.node(ids[0]).map(|n| n.display_name()).unwrap_or_default(); - let base = base.trim(); - let mut name = if base.is_empty() { stem.to_string() } else { base.to_string() }; + let shown = ids.first().and_then(|id| doc.node(*id)).map(|n| n.display_name()).unwrap_or_default(); + let base: String = match shown.trim() { + "" => stem.to_string(), + b => b.chars().take(MAX_NAME - 8).collect(), + }; + let mut name = base.clone(); let mut n = 2; while doc.variables.variable(&name).is_some() { name = format!("{base} {n}"); @@ -153,7 +172,9 @@ fn make_dynamic(s: &mut Session, kind: VariableKind, stem: &str) -> Result Result { - let names = names_param(p, "variable.delete")?; + // 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)))?; // A highlight of a variable that is gone highlights nothing. let st = s.doc_mut()?; @@ -182,27 +203,13 @@ fn list(s: &mut Session, _p: &Value) -> Result { fn bind(s: &mut Session, p: &Value) -> Result { const C: &str = "variable.bind"; let name = named(str_param(p, "variable"), "variable", C)?; - let ids = match ids_param(p, "ids") { - Some(v) => v, - None => selected_roots(s)?, - }; + let ids = target_ids(s, p, C)?; if ids.is_empty() { return Err(bad(C, "nothing to bind")); } let doc = &s.doc()?.doc; let kind = doc.variables.variable(&name).map(|v| v.kind).ok_or_else(|| bad(C, format!("no variable named `{name}`")))?; - for id in &ids { - match doc.node(*id) { - None => return Err(EngineError::NoNode(*id)), - // Art on a locked or hidden layer is not the document's to rewrite, and a text - // variable only drives type. - Some(_) if !doc.is_editable(*id) => return Err(bad(C, format!("node {} is locked", id.0))), - Some(n) if kind == VariableKind::Text && !matches!(n.kind, NodeKind::Text(_)) => { - return Err(bad(C, format!("node {} is not type", id.0))); - } - Some(_) => {} - } - } + 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()))?; @@ -211,20 +218,8 @@ fn bind(s: &mut Session, p: &Value) -> Result { fn unbind(s: &mut Session, p: &Value) -> Result { const C: &str = "variable.unbind"; - let ids = match ids_param(p, "ids") { - Some(v) => v, - None => selected_roots(s)?, - }; - let name = match str_param(p, "variable").filter(|v| !v.is_empty()) { - Some(v) => { - let v = v.to_string(); - if s.doc()?.doc.variables.variable(&v).is_none() { - return Err(bad(C, format!("no variable named `{v}`"))); - } - Some(v) - } - None => None, - }; + let ids = target_ids(s, p, C)?; + let name = defined_variable(s, p, C)?; let n = s.edit("Unbind Variable", |d, _| { let mut n = 0; for id in &ids { @@ -239,21 +234,22 @@ fn unbind(s: &mut Session, p: &Value) -> Result { Ok(json!({"unbound": n})) } +/// The `variable` a command names, which must be defined; `None` when it names none. +fn defined_variable(s: &Session, p: &Value, cmd: &str) -> Result> { + let name = optional_name(p, "variable", cmd)?; + if let Some(v) = &name + && s.doc()?.doc.variables.variable(v).is_none() + { + return Err(bad(cmd, format!("no variable named `{v}`"))); + } + Ok(name) +} + /// Highlight one Variables panel row: what the panel's Delete, Options… and Select Bound /// Object act on. Panel state, not art selection: not saved, not an undo step (an empty /// or missing `variable` clears it), like `layer.highlight`. fn highlight(s: &mut Session, p: &Value) -> Result { - const C: &str = "variable.highlight"; - let name = match str_param(p, "variable").filter(|v| !v.is_empty()) { - Some(v) => { - let v = v.to_string(); - if s.doc()?.doc.variables.variable(&v).is_none() { - return Err(bad(C, format!("no variable named `{v}`"))); - } - Some(v) - } - None => None, - }; + let name = defined_variable(s, p, "variable.highlight")?; let st = s.doc_mut()?; st.variables_highlight = name.clone(); st.revision += 1; @@ -320,8 +316,8 @@ fn dataset_capture(s: &mut Session, p: &Value) -> Result { if values.is_empty() { return Err(bad(C, "bind something to a variable first")); } - let name = match str_param(p, "name").filter(|v| !v.is_empty()) { - Some(n) => n.to_string(), + let name = match optional_name(p, "name", C)? { + Some(n) => n, // "Data Set 1", the first free number, as the reference panel's rows are named. None => (1..).map(|i| format!("Data Set {i}")).find(|n| st.doc.variables.dataset(n).is_none()).unwrap_or_default(), }; @@ -356,7 +352,9 @@ fn dataset_update(s: &mut Session, _p: &Value) -> Result { } fn dataset_delete(s: &mut Session, p: &Value) -> Result { - let names = names_param(p, "dataset.delete")?; + // 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)); @@ -371,6 +369,7 @@ fn dataset_delete(s: &mut Session, p: &Value) -> Result { fn dataset_list(s: &mut Session, _p: &Value) -> Result { let st = s.doc()?; + let current = st.doc.captured_values(); let datasets: Vec = st .doc .variables @@ -378,7 +377,7 @@ fn dataset_list(s: &mut Session, _p: &Value) -> Result { .iter() // `matches` says whether the art still holds what the row says, which is what the // panel shows in italics. - .map(|d| json!({"name": d.name, "values": d.values, "matches": st.doc.matches_dataset(&d.name)})) + .map(|d| json!({"name": d.name, "values": d.values, "matches": d.matches(¤t)})) .collect(); Ok(json!({"datasets": datasets, "active": st.doc.variables.active_dataset})) } @@ -406,9 +405,10 @@ fn apply_dataset(s: &mut Session, name: &str) -> Result { continue; } }; - // Art on a locked or hidden layer, and text whose object is gone, can't be - // rewritten: skipped and counted, like the rest. - if !d.is_editable(id) { + // Locked art, art on a locked or hidden layer, and text whose object is gone, can't + // be rewritten: skipped and counted, like the rest. A hidden object is rewritten: a + // row that hid it shows it again. + if !d.variable_target(id) { skipped += 1; continue; } @@ -463,7 +463,7 @@ pub fn specs() -> Vec { "Delete Variable", ["Window", "Variables"], None, - "{names} delete variables with their bindings and values", + "{names?} delete variables with their bindings and values (default: the highlighted one)", has_doc, delete ), @@ -551,7 +551,15 @@ pub fn specs() -> Vec { has_doc, dataset_update ), - cmd!("dataset.delete", "Delete Data Set", ["Window", "Variables"], None, "{names} delete datasets", has_doc, dataset_delete), + cmd!( + "dataset.delete", + "Delete Data Set", + ["Window", "Variables"], + None, + "{names?} delete datasets (default: the active one)", + has_doc, + dataset_delete + ), cmd!( query "dataset.list", "List Data Sets", diff --git a/crates/engine/src/tests_variables.rs b/crates/engine/src/tests_variables.rs index a0bcf270..350905c2 100644 --- a/crates/engine/src/tests_variables.rs +++ b/crates/engine/src/tests_variables.rs @@ -489,3 +489,103 @@ fn highlight_follows_rename_and_clears_with_delete() { s.execute("variable.delete", &json!({"names": ["Title"]})).unwrap(); assert!(s.doc().unwrap().variables_highlight.is_none(), "a highlight of a deleted variable highlights nothing"); } + +/// A row that hides an object must be able to show it again, and hiding it on the artboard +/// (as the Layers panel does) is what a capture records: a visibility variable reads and +/// writes its object's own visibility, which therefore never makes the object off limits. +#[test] +fn a_hidden_object_comes_back_and_is_captured_hidden() { + let mut s = session(); + let t = text(&mut s, "Alice"); + let r = 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.bind", &json!({"variable": "Name", "ids": [t.0]})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Show", "ids": [r.0, t.0]})).unwrap(); + s.execute("dataset.new", &json!({"name": "Shown", "values": {"Name": "Alice", "Show": true}})).unwrap(); + s.execute("dataset.new", &json!({"name": "Hidden", "values": {"Name": "Bob", "Show": false}})).unwrap(); + + s.execute("dataset.select", &json!({"name": "Hidden"})).unwrap(); + assert!(!s.doc().unwrap().doc.node(r).unwrap().visible); + let v = s.execute("dataset.select", &json!({"name": "Shown"})).unwrap(); + assert_eq!(v["skipped"], json!(0), "{v}"); + assert!(s.doc().unwrap().doc.node(r).unwrap().visible, "the row shows it again"); + assert_eq!(plain(&s, t), "Alice", "and rewrites the text of the object it had hidden"); + + // Hidden on the artboard, then captured: the row says hidden, and it matches the art. + s.execute("select.set", &json!({"ids": [t.0, r.0]})).unwrap(); + s.execute("object.hide", &json!({})).unwrap(); + s.execute("dataset.capture", &json!({"name": "Captured"})).unwrap(); + let sets = s.execute("dataset.list", &json!({})).unwrap(); + let captured = sets["datasets"].as_array().unwrap().iter().find(|d| d["name"] == "Captured").unwrap().clone(); + assert_eq!(captured["values"]["Show"], json!({"visible": false}), "{captured}"); + assert_eq!(captured["matches"], json!(true)); + + // A hidden object can still be bound by id. + s.execute("variable.define", &json!({"name": "More", "kind": "visibility"})).unwrap(); + s.execute("variable.bind", &json!({"variable": "More", "ids": [r.0]})).unwrap(); +} + +/// Variables and datasets are separate namespaces: renaming a variable leaves a dataset of +/// the same name, and which dataset is active, alone. +#[test] +fn renaming_a_variable_leaves_a_dataset_of_the_same_name_active() { + let mut s = session(); + let t = text(&mut s, "Alice"); + s.execute("variable.define", &json!({"name": "Title", "kind": "text"})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Title", "ids": [t.0]})).unwrap(); + s.execute("dataset.capture", &json!({"name": "Title"})).unwrap(); + s.execute("variable.rename", &json!({"name": "Title", "newName": "Heading"})).unwrap(); + let sets = s.execute("dataset.list", &json!({})).unwrap(); + assert_eq!((sets["active"].clone(), sets["datasets"][0]["name"].clone()), (json!("Title"), json!("Title"))); + s.execute("dataset.update", &json!({})).unwrap(); +} + +/// Without `names`, the deletes act on what the panel shows as current: the highlighted +/// variable and the active dataset (Window › Variables runs them with no params). +#[test] +fn deletes_without_names_take_the_highlight_and_the_active_dataset() { + let mut s = session(); + let t = text(&mut s, "Alice"); + for name in ["A", "B"] { + s.execute("variable.define", &json!({"name": name, "kind": "text"})).unwrap(); + } + s.execute("variable.bind", &json!({"variable": "A", "ids": [t.0]})).unwrap(); + s.execute("dataset.capture", &json!({})).unwrap(); + s.execute("dataset.capture", &json!({})).unwrap(); + assert!(s.execute("variable.delete", &json!({})).is_err(), "nothing highlighted"); + + s.execute("variable.highlight", &json!({"variable": "B"})).unwrap(); + assert_eq!(s.execute("variable.delete", &json!({})).unwrap()["deleted"], json!(1)); + let vars = s.execute("variable.list", &json!({})).unwrap(); + assert_eq!(vars["variables"].as_array().map(Vec::len), Some(1)); + assert_eq!(vars["variables"][0]["name"], "A"); + + assert_eq!(s.execute("dataset.delete", &json!({})).unwrap()["deleted"], json!(1)); + let sets = s.execute("dataset.list", &json!({})).unwrap(); + assert_eq!(sets["datasets"].as_array().map(Vec::len), Some(1)); + assert_eq!(sets["datasets"][0]["name"], "Data Set 1"); + assert!(s.execute("dataset.delete", &json!({})).is_err(), "no active dataset left"); +} + +/// Ids and names from an agent are checked, never reinterpreted: an `ids` that isn't a list +/// of objects is an error rather than the selection, and names are trimmed and capped. +#[test] +fn ids_and_names_from_an_agent_are_checked() { + let mut s = session(); + let t = text(&mut s, "Alice"); + s.execute("variable.define", &json!({"name": " Name ", "kind": "text"})).unwrap(); + assert_eq!(s.execute("variable.list", &json!({})).unwrap()["variables"][0]["name"], "Name"); + s.execute("select.set", &json!({"ids": [t.0]})).unwrap(); + for params in + [json!({"variable": "Name", "ids": 5}), json!({"variable": "Name", "ids": [t.0, "x"]}), json!({"variable": "Name", "ids": [999_999]})] + { + assert!(s.execute("variable.bind", ¶ms).is_err(), "{params}"); + } + assert!(s.doc().unwrap().doc.variables.objects_of("Name").is_empty(), "nothing was bound"); + assert!(s.execute("variable.unbind", &json!({"ids": "all"})).is_err()); + assert!(s.execute("variable.define", &json!({"name": "x".repeat(256), "kind": "text"})).is_err()); + s.execute("variable.define", &json!({"name": "x".repeat(255), "kind": "text"})).unwrap(); + assert!(s.execute("variable.delete", &json!({"names": []})).is_err()); + assert!(s.execute("variable.delete", &json!({"names": [5]})).is_err()); +}