Variables: Update Data Set keeps a row's values when every bound object is locked

Squashed from #5126 onto main.

crates/engine/src/cmd/variablecmds.rs

@@ -442,6 +442,9 @@ fn dataset_capture(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
     let values = st.doc.captured_values();
     if values.is_empty() {
+        if anything_bound(&st.doc) {
+            return Err(bad(C, "no bound object reports a value (locked objects and embedded images report none)"));
+        }
         return Err(bad(C, "bind something to a variable first"));
     }
     let name = match optional_name(p, "name", C)? {
@@ -464,32 +467,36 @@ fn dataset_capture(s: &mut Session, p: &Value) -> Result<Value> {
 /// variable the row holds. A variable whose objects report no value (locked, inside a locked
 /// layer or group, or gone) keeps the row's value. With `all`, every other variable the
 /// objects report a value for is added to the row. Without an active row there is nothing to
-/// update, so say so rather than picking one.
+/// update, so say so rather than picking one. With every bound object locked, no value is
+/// written; an update that writes no value adds no undo step.
 fn dataset_update(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "dataset.update";
     let all = bool_or(p, "all", false);
-    let values = s.doc()?.doc.captured_values();
-    let Some(name) = s.doc()?.doc.variables.active_dataset.clone() else {
+    let st = s.doc()?;
+    let Some(name) = st.doc.variables.active_dataset.clone() else {
         return Err(bad(C, "no active data set"));
     };
-    if values.is_empty() {
+    if !anything_bound(&st.doc) {
         return Err(bad(C, "bind something to a variable first"));
     }
-    let (written, left) = edit_variables(s, "Update Data Set", |vars| {
+    let ds = st.doc.variables.dataset(&name).ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?;
+    let writes: BTreeMap<String, DataValue> = st.doc.captured_values().into_iter().filter(|(var, _)| all || ds.values.contains_key(var)).collect();
+    let left: Vec<String> = st.doc.variables.variables.iter().filter(|v| !writes.contains_key(&v.name)).map(|v| v.name.clone()).collect();
+    let written = writes.len();
+    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) {
-                ds.values.insert(var.clone(), value.clone());
-                written.insert(var.as_str());
-            }
-        }
-        let left: Vec<String> = vars.variables.iter().filter(|v| !written.contains(v.name.as_str())).map(|v| v.name.clone()).collect();
-        Ok((written.len(), left))
+        ds.values.extend(writes);
+        Ok(())
     })?;
     Ok(json!({"name": name, "values": written, "left": left}))
 }
 
+/// Whether any object in the document is bound to a variable. A locked one counts: it is
+/// bound, though it reports no value.
+fn anything_bound(doc: &Document) -> bool {
+    doc.variables.bindings.iter().any(|(id, names)| !names.is_empty() && doc.node(*id).is_some())
+}
+
 fn dataset_delete(s: &mut Session, p: &Value) -> Result<Value> {
     // Without `names`, the active dataset: what the panel's Delete Data Set deletes.
     let active = s.doc()?.doc.variables.active_dataset.clone();

crates/engine/src/tests/variables.rs

@@ -709,6 +709,40 @@ fn updating_keeps_a_value_the_art_does_not_report() {
     }
 }
 
+/// With every bound object locked, Update keeps the whole row and adds no undo step, and
+/// Capture says the bound objects report no value rather than asking for a binding.
+#[test]
+fn updating_with_every_bound_object_locked_keeps_the_row() {
+    let mut s = session();
+    let t = text(&mut s, "First");
+    s.execute("variable.define", &json!({"name": "Label", "kind": "text"})).unwrap();
+    s.execute("variable.bind", &json!({"variable": "Label", "ids": [t.0]})).unwrap();
+    s.execute("dataset.new", &json!({"name": "Row", "values": {"Label": "Second"}})).unwrap();
+    s.execute("dataset.select", &json!({"name": "Row"})).unwrap();
+    s.execute("object.setProps", &json!({"ids": [t.0], "locked": true})).unwrap();
+
+    let steps = |s: &Session| s.doc().unwrap().history.undo.len();
+    let before = steps(&s);
+    for params in [json!({}), json!({"all": true})] {
+        let v = s.execute("dataset.update", &params).unwrap();
+        assert_eq!((v["values"].clone(), v["left"].clone()), (json!(0), json!(["Label"])), "{params}: {v}");
+        let sets = s.execute("dataset.list", &json!({})).unwrap();
+        assert_eq!(sets["datasets"][0]["values"], json!({"Label": {"text": "Second"}}), "{params}: {sets}");
+    }
+    assert_eq!(steps(&s), before, "an update that writes nothing is not a step");
+
+    let e = s.execute("dataset.capture", &json!({})).unwrap_err().to_string();
+    assert!(e.contains("no bound object reports a value"), "{e}");
+    assert_eq!(s.execute("dataset.list", &json!({})).unwrap()["datasets"].as_array().unwrap().len(), 1);
+
+    // With nothing bound at all, both still ask for a binding.
+    s.execute("variable.unbind", &json!({"ids": [t.0]})).unwrap();
+    for cmd in ["dataset.update", "dataset.capture"] {
+        let e = s.execute(cmd, &json!({})).unwrap_err().to_string();
+        assert!(e.contains("bind something to a variable first"), "{cmd}: {e}");
+    }
+}
+
 /// A variable defined after a row was captured stays out of that row on Update, and joins it
 /// with `all: true`.
 #[test]