aleleba/vectorcraft
1
0
Fork 0
mirror of https://github.com/storytold/vectorcraft.git synced 2026-10-27 19:27:21 -06:00
Code Issues Packages Projects Releases Wiki Activity

variables: fix binding lifetime, dataset editing and the missing UI to define

Browse Source 
Review of #761 turned up three real defects and a feature that was visible
but not usable. All of it is fixed here, each defect with a regression test
that fails without the fix.

- A binding outlived the object it named. Deleting art left its entry in
  `variables.bindings` forever: it counted as a skip in every dataset
  application, `variable.list` counted it, and the saved file grew with every
  delete. `Document::prune_variable_bindings` now lets them go after each
  edit, as `prune_assets` does for Asset Export's.
- `Variables::prune` cleared `active_dataset` by comparing it against the
  *variable* names being deleted. A dataset named like a variable lost its
  active state when the variable went. The two are separate namespaces; the
  block is gone (dataset.delete already did it right).
- `variable.delete` reported the number of names asked for, not the number
  deleted, so deleting a name that was not there still said `deleted: 1`.

- `dataset.set` (new): a dataset's values could only be set when it was
  created, so changing one meant deleting and rebuilding the row — losing its
  place in the list and the active one. This replaces the row's values and
  reports what it dropped.
- Window › Variables, and dialogs for the "…" commands. Every command had an
  empty menu path and the command palette runs those with `{}`, so nothing
  could be defined from the UI at all. The commands now carry their menu
  path, the "…" ones open a Variables dialog (a name and a kind; a name and a
  field per variable, its kind's editor), and `dataset.set` opens with the
  row's own values.
- Locked art is neither bound nor rewritten by a dataset, and says so.
- `set_plain_text` told a node that existed but was not type that it was
  missing.
- The panel had Document Info's icon, so the two were indistinguishable in
  the collapsed column; it gets its own, plus New/Delete buttons, and its
  rows say what their count is.

Format round trips now cover variables (the testkit fixture defines both
kinds, binds them and applies a row), and ROADMAP/README no longer list the
feature as missing.

cargo xtask ci: all 7 steps pass.
This commit is contained in:
Fahri Meral
2026-10-10 23:34:21 +07:00
committed by bflatastic
parent c89906a13a
commit 202894372a
24 changed files with 894 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/engine/src/cmd/variablecmds.rs
+125 -74
View File
@@ -5,17 +5,24 @@
//! dataset, and `dataset.select` swaps the whole document to that row in one undo
//! step. Agents reach every command through `run_command`, like all engine commands.
use std::collections::BTreeMap;
use serde_json::{Value, json};
use vectorcraft_doc::{DataValue, NodeId, NodeKind, VariableKind};
use vectorcraft_doc::{DataSet, DataValue, NodeId, NodeKind, Variable, VariableKind, Variables};
use super::edit::selected_roots;
use super::textedit::set_plain_text;
use super::*;
fn var_name(p: &Value, cmd: &str) -> Result<String> {
match str_param(p, "name").filter(|v| !v.is_empty()) {
named(str_param(p, "name"), "name", cmd)
}
/// `key`'s value, as a non-empty name.
fn named(v: Option<&str>, key: &str, cmd: &str) -> Result<String> {
match v.filter(|v| !v.is_empty()) {
Some(v) => Ok(v.to_string()),
None => Err(bad(cmd, "missing `name`")),
None => Err(bad(cmd, format!("missing `{key}`"))),
}
}
@@ -43,12 +50,15 @@ fn dataset_value(cmd: &str, var: &str, kind: VariableKind, v: &Value) -> Result<
.ok_or_else(|| bad(cmd, format!("value for `{var}` must match its variable kind")))
}
fn values_param(s: &Session, p: &Value, cmd: &str) -> Result<std::collections::BTreeMap<String, DataValue>> {
let mut out = std::collections::BTreeMap::new();
/// The `values` object of `dataset.new` / `dataset.set`, each entry checked against its
/// variable's kind. Every name must be a variable the document defines: a value nothing
/// reads is a typo, not a value to keep.
fn values_param(vars: &Variables, p: &Value, cmd: &str) -> Result<BTreeMap<String, DataValue>> {
let mut out = BTreeMap::new();
let Some(given) = p.get("values") else { return Ok(out) };
let Some(obj) = given.as_object() else { return Err(bad(cmd, "`values` must be an object")) };
for (var, v) in obj {
let kind = s.doc()?.doc.variables.variable(var).map(|d| d.kind).ok_or_else(|| bad(cmd, format!("no variable named `{var}`")))?;
let kind = vars.variable(var).map(|d| d.kind).ok_or_else(|| bad(cmd, format!("no variable named `{var}`")))?;
out.insert(var.clone(), dataset_value(cmd, var, kind, v)?);
}
Ok(out)
@@ -62,19 +72,15 @@ fn define(s: &mut Session, p: &Value) -> Result<Value> {
return Err(bad(C, format!("variable `{name}` is already defined")));
}
s.edit("New Variable", |d, _| {
d.variables.variables.push(vectorcraft_doc::Variable { name: name.clone(), kind });
d.variables.variables.push(Variable { name: name.clone(), kind });
Ok(())
})?;
Ok(json!({"name": name, "kind": kind.label()}))
}
fn delete(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "variable.delete";
let names = names_param(p, C)?;
let n = s.edit("Delete Variables", |d, _| {
d.variables.prune(&names);
Ok(names.len())
})?;
let names = names_param(p, "variable.delete")?;
let n = s.edit("Delete Variables", |d, _| Ok(d.variables.prune(&names)))?;
Ok(json!({"deleted": n}))
}
@@ -95,7 +101,7 @@ 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 = str_param(p, "variable").filter(|v| !v.is_empty()).map(str::to_string).ok_or_else(|| bad(C, "missing `variable`"))?;
let name = named(str_param(p, "variable"), "variable", C)?;
let ids = match ids_param(p, "ids") {
Some(v) => v,
None => selected_roots(s)?,
@@ -103,30 +109,29 @@ fn bind(s: &mut Session, p: &Value) -> Result<Value> {
if ids.is_empty() {
return Err(bad(C, "nothing to bind"));
}
let kind = s.doc()?.doc.variables.variable(&name).map(|v| v.kind).ok_or_else(|| bad(C, format!("no variable named `{name}`")))?;
// A text variable only drives type; visibility drives anything that exists.
if kind == VariableKind::Text {
for id in &ids {
match s.doc()?.doc.node(*id).map(|n| &n.kind) {
Some(NodeKind::Text(_)) => {}
Some(_) => return Err(bad(C, format!("node {} is not type", id.0))),
None => return Err(EngineError::NoNode(*id)),
}
}
} else {
for id in &ids {
if s.doc()?.doc.node(*id).is_none() {
return Err(EngineError::NoNode(*id));
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(_) => {}
}
}
// An object holds one binding, so binding it again replaces what it had; say which.
let replaced: Vec<u64> = ids.iter().filter(|id| doc.variables.bindings.get(id).is_some_and(|b| *b != name)).map(|id| id.0).collect();
s.edit("Bind Variable", |d, _| {
for id in &ids {
d.variables.bindings.insert(*id, name.clone());
}
Ok(())
})?;
Ok(json!({"variable": name, "ids": ids.iter().map(|i| i.0).collect::<Vec<_>>()}))
Ok(json!({"variable": name, "ids": ids.iter().map(|i| i.0).collect::<Vec<_>>(), "replaced": replaced}))
}
fn unbind(s: &mut Session, p: &Value) -> Result<Value> {
@@ -149,23 +154,41 @@ fn unbind(s: &mut Session, p: &Value) -> Result<Value> {
fn dataset_new(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "dataset.new";
let name = var_name(p, C)?;
let values = values_param(s, p, C)?;
if s.doc()?.doc.variables.datasets.iter().any(|d| d.name == name) {
let st = s.doc()?;
// A dataset and a variable are separate namespaces, so a dataset may share a variable's
// name; it is its own row of values either way.
let values = values_param(&st.doc.variables, p, C)?;
if st.doc.variables.dataset(&name).is_some() {
return Err(bad(C, format!("dataset `{name}` already exists")));
}
s.edit("New Data Set", |d, _| {
d.variables.datasets.push(vectorcraft_doc::DataSet { name: name.clone(), values: values.clone() });
d.variables.datasets.push(DataSet { name: name.clone(), values: values.clone() });
Ok(())
})?;
Ok(json!({"name": name}))
}
/// Replace a dataset's values. A variable left out of `values` loses its value in this row,
/// so the dataset ends up holding exactly what the call asked for.
fn dataset_set(s: &mut Session, p: &Value) -> Result<Value> {
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 removed: Vec<String> = ds.values.keys().filter(|k| !values.contains_key(*k)).cloned().collect();
ds.values = values.clone();
Ok((values.len(), removed))
})?;
Ok(json!({"name": name, "values": set, "removed": removed}))
}
fn dataset_delete(s: &mut Session, p: &Value) -> Result<Value> {
const C: &str = "dataset.delete";
let names = names_param(p, C)?;
let names = names_param(p, "dataset.delete")?;
let n = s.edit("Delete Data Sets", |d, _| {
let before = d.variables.datasets.len();
d.variables.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;
}
@@ -182,12 +205,15 @@ fn dataset_list(s: &mut Session, _p: &Value) -> Result<Value> {
fn apply_dataset(s: &mut Session, name: &str) -> Result<Value> {
const C: &str = "dataset.select";
let (applied, skipped) = s.edit("Data Set", |d, _| {
let ds = d.variables.datasets.iter().find(|x| x.name == name).cloned().ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
let (applied, skipped) = s.edit("Apply Data Set", |d, _| {
let ds = d.variables.dataset(name).cloned().ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
let bindings: Vec<(NodeId, String)> = d.variables.bindings.iter().map(|(id, v)| (*id, v.clone())).collect();
let mut applied = 0usize;
let mut skipped = 0usize;
for (id, var) in bindings {
// A binding whose variable is gone, whose value this row doesn't carry, or whose
// value doesn't fit the variable's kind, is left alone and counted, never
// silently reinterpreted.
let (kind, value) = match (d.variables.variable(&var).map(|v| v.kind), ds.values.get(&var)) {
(Some(kind), Some(value)) if value.fits(kind) => (kind, value.clone()),
_ => {
@@ -195,22 +221,20 @@ 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) {
skipped += 1;
continue;
}
let done = match (kind, &value) {
(VariableKind::Text, DataValue::Text(text)) => {
if d.node(id).is_some_and(|n| matches!(n.kind, NodeKind::Text(_))) {
set_plain_text(d, id, text).is_ok()
} else {
false
}
}
(VariableKind::Visibility, DataValue::Visible(show)) => {
if let Some(n) = d.node_mut(id) {
n.visible = *show;
true
} else {
false
}
}
// The Type tool's own whole-range replace, so a dataset's text is styled and
// laid out exactly as typing it would be.
(VariableKind::Text, DataValue::Text(text)) => set_plain_text(d, id, text).is_ok(),
(VariableKind::Visibility, DataValue::Visible(show)) => d.node_mut(id).is_some_and(|n| {
n.visible = *show;
true
}),
_ => false,
};
if done {
@@ -226,21 +250,21 @@ fn apply_dataset(s: &mut Session, name: &str) -> Result<Value> {
}
fn dataset_select(s: &mut Session, p: &Value) -> Result<Value> {
let name = var_name(p, "dataset.select")?;
apply_dataset(s, &name)
apply_dataset(s, &var_name(p, "dataset.select")?)
}
fn dataset_step(s: &mut Session, cmd: &str, forward: bool) -> Result<Value> {
let st = s.doc()?;
if st.doc.variables.datasets.is_empty() {
let n = st.doc.variables.datasets.len();
if n == 0 {
return Err(bad(cmd, "no datasets"));
}
let at = st.doc.variables.active_dataset.as_ref().and_then(|a| st.doc.variables.datasets.iter().position(|d| &d.name == a));
let next = match at {
Some(i) if forward => (i + 1) % st.doc.variables.datasets.len(),
Some(i) => (i + st.doc.variables.datasets.len() - 1) % st.doc.variables.datasets.len(),
None if forward => 0,
None => st.doc.variables.datasets.len() - 1,
let at = st.doc.variables.active_dataset.as_deref().and_then(|a| st.doc.variables.datasets.iter().position(|d| d.name == a));
let next = match (at, forward) {
(Some(i), true) => (i + 1) % n,
(Some(i), false) => (i + n - 1) % n,
(None, true) => 0,
(None, false) => n - 1,
};
let name = st.doc.variables.datasets.get(next).map(|d| d.name.clone()).ok_or_else(|| bad(cmd, "no datasets"))?;
apply_dataset(s, &name)
@@ -248,28 +272,55 @@ fn dataset_step(s: &mut Session, cmd: &str, forward: bool) -> Result<Value> {
pub fn specs() -> Vec<CommandSpec> {
vec![
cmd!("variable.define", "New Variable", [], None, "{name, kind: text|visibility} define a variable", has_doc, define),
cmd!("variable.delete", "Delete Variable", [], None, "{names} delete variables with their bindings and values", has_doc, delete),
cmd!("variable.list", "List Variables", [], None, "{} → variables with kinds and binding counts", has_doc, list),
cmd!("variable.define", "New Variable…", ["Window", "Variables"], None, "{name, kind: text|visibility} define a variable", has_doc, define),
cmd!(
"variable.delete",
"Delete Variable",
["Window", "Variables"],
None,
"{names} delete variables with their bindings and values",
has_doc,
delete
),
cmd!(query "variable.list", "List Variables", [], None, "{} → variables with kinds and binding counts", has_doc, list),
cmd!(
"variable.bind",
"Bind Variable",
[],
["Window", "Variables"],
None,
"{variable, ids?} bind objects (default: the selection) to a variable; text needs type",
"{variable, ids?} bind objects (default: the selection) to a variable; text needs type, locked art is refused. An object holds one binding, so this replaces what it had → {ids, replaced}",
has_doc,
bind
),
cmd!("variable.unbind", "Unbind Variable", [], None, "{ids?} drop bindings (default: the selection)", has_doc, unbind),
cmd!("dataset.new", "New Data Set", [], None, "{name, values?: {variable: value}} add a dataset", has_doc, dataset_new),
cmd!("dataset.delete", "Delete Data Set", [], None, "{names} delete datasets", has_doc, dataset_delete),
cmd!("dataset.list", "List Data Sets", [], None, "{} → datasets with values and the active one", has_doc, dataset_list),
cmd!("dataset.select", "Select Data Set", [], None, "{name} apply a dataset in one undo step → {applied, skipped}", has_doc, dataset_select),
cmd!("dataset.next", "Next Data Set", [], None, "{} apply the next dataset, wrapping", has_doc, |s, _| dataset_step(s, "dataset.next", true)),
cmd!("dataset.prev", "Previous Data Set", [], None, "{} apply the previous dataset, wrapping", has_doc, |s, _| dataset_step(
cmd!("variable.unbind", "Unbind Variable", ["Window", "Variables"], None, "{ids?} drop bindings (default: the selection)", has_doc, unbind),
cmd!("dataset.new", "New Data Set…", ["Window", "Variables"], None, "{name, values?: {variable: value}} add a dataset", has_doc, dataset_new),
cmd!(
"dataset.set",
"Edit Data Set…",
["Window", "Variables"],
None,
"{name, values?: {variable: value}} replace a dataset's values; a variable left out loses its value in this row → {values, removed}",
has_doc,
dataset_set
),
cmd!("dataset.delete", "Delete Data Set", ["Window", "Variables"], None, "{names} delete datasets", has_doc, dataset_delete),
cmd!(query "dataset.list", "List Data Sets", [], None, "{} → datasets with values and the active one", has_doc, dataset_list),
cmd!(
"dataset.select",
"Select Data Set",
["Window", "Variables"],
None,
"{name} apply a dataset in one undo step → {applied, skipped}",
has_doc,
dataset_select
),
cmd!("dataset.next", "Next Data Set", ["Window", "Variables"], None, "{} apply the next dataset, wrapping", has_doc, |s, _| dataset_step(
s,
"dataset.prev",
false
"dataset.next",
true
)),
cmd!("dataset.prev", "Previous Data Set", ["Window", "Variables"], None, "{} apply the previous dataset, wrapping", has_doc, |s, _| {
dataset_step(s, "dataset.prev", false)
}),
]
}