shifathosn
/
vectorcraft
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

variables: review fixes for hidden objects, namespaces and agent params 

- A visibility variable could hide an object but never show it again: dataset
  application, capture and binding refused any object that wasn't visible
  (`is_editable`), so a row that hid an object left it hidden for good and a
  capture after hiding it in the Layers panel didn't record it. They now use
  `Document::variable_target`, which ignores the object's own visibility
  (locked art and art on hidden or locked layers are still skipped).
- Renaming a variable renamed the active dataset when the two shared a name,
  leaving it pointing at a dataset that doesn't exist.
- `variable.delete` and `dataset.delete` without `names` act on the
  highlighted variable and the active dataset, so Window > Variables > Delete
  Variable / Delete Data Set no longer fail with "missing `names`".
- `ids` is checked like other commands' explicit targets (a non-list no longer
  falls back to the selection, bad ids are errors), names are trimmed and
  capped at 255 characters, and the bind checks are shared by Bind and Make
  Dynamic.
- Make Dynamic's fallback name no longer becomes " 2" for unnamed art.
- `dataset.list` reads the art once for every row's drift check, and
  `Variables::prune` looks names up in a set.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
bflatastic 2026-10-10 22:15:38 +07:00
parent f2252b0d9c
commit b3fa281282
3 changed files with 215 additions and 98 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
crates/doc/src/variables.rs
View File

@ -70,6 +70,15 @@ pub struct DataSet {
pub values: BTreeMap<String, DataValue>,
}
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<String, DataValue>) -> 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<DataValue> {
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))
}
}

172
crates/engine/src/cmd/variablecmds.rs
View File

@ -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<String> {
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<String> {
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<Vec<String>> {
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<Option<String>> {
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<String>, nothing: &str) -> Result<Vec<String>> {
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<Vec<NodeId>> {
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<DataValue> {
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<Value> {
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<Value
return Err(bad(C, "select the objects to bind"));
}
let doc = &s.doc()?.doc;
for id in &ids {
match doc.node(*id) {
None => 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<Value
}
fn delete(s: &mut Session, p: &Value) -> Result<Value> {
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<Value> {
fn bind(s: &mut Session, p: &Value) -> Result<Value> {
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<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()))?;
@ -211,20 +218,8 @@ fn bind(s: &mut Session, p: &Value) -> Result<Value> {
fn unbind(s: &mut Session, p: &Value) -> Result<Value> {
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<Value> {
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<Option<String>> {
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<Value> {
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<Value> {
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<Value> {
}
fn dataset_delete(s: &mut Session, p: &Value) -> Result<Value> {
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<Value> {
fn dataset_list(s: &mut Session, _p: &Value) -> Result<Value> {
let st = s.doc()?;
let current = st.doc.captured_values();
let datasets: Vec<Value> = st
.doc
.variables
@ -378,7 +377,7 @@ fn dataset_list(s: &mut Session, _p: &Value) -> Result<Value> {
.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(&current)}))
.collect();
Ok(json!({"datasets": datasets, "active": st.doc.variables.active_dataset}))
}
@ -406,9 +405,10 @@ fn apply_dataset(s: &mut Session, name: &str) -> Result<Value> {
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<CommandSpec> {
"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<CommandSpec> {
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",

100
crates/engine/src/tests_variables.rs
View File

@ -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", &params).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());
}