Merge pull request #2250 from dotCooCoo/fix/variable-binding-errors

Variables: binding errors name the command and say when a layer is hidden

crates/engine/src/cmd/variablecmds.rs

@@ -55,7 +55,7 @@ fn check_bindable(doc: &vectorcraft_doc::Document, ids: &[NodeId], kind: Variabl
     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(_) if !doc.variable_target(*id) => return Err(bad(cmd, not_a_target(doc, *id))),
             Some(n) if kind == VariableKind::Text && !matches!(n.kind, NodeKind::Text(_)) => {
                 return Err(bad(cmd, format!("node {} is not type", id.0)));
             }
@@ -65,6 +65,20 @@ fn check_bindable(doc: &vectorcraft_doc::Document, ids: &[NodeId], kind: Variabl
     Ok(())
 }
 
+/// Why `variable_target` refuses `id`: the object is locked, a group or layer that holds it is
+/// locked, or one that holds it is hidden, checked in that order.
+fn not_a_target(doc: &vectorcraft_doc::Document, id: NodeId) -> String {
+    let holders: Vec<&vectorcraft_doc::Node> =
+        doc.ancestry(id).unwrap_or_default().into_iter().filter(|i| *i != id).filter_map(|i| doc.node(i)).collect();
+    if doc.node(id).is_some_and(|n| n.locked) {
+        format!("node {} is locked", id.0)
+    } else if holders.iter().any(|n| n.locked) {
+        format!("node {} is in a locked group or layer", id.0)
+    } else {
+        format!("node {} is in a hidden group or layer", id.0)
+    }
+}
+
 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())),
@@ -136,13 +150,14 @@ fn rename(s: &mut Session, p: &Value) -> Result<Value> {
 /// Make Text Dynamic / Make Visibility Dynamic: define a variable of that kind and bind the
 /// selection to it in one step, naming it after what it drives — the panel's buttons.
 fn make_dynamic(s: &mut Session, kind: VariableKind, stem: &str) -> Result<Value> {
-    const C: &str = "variable.makeDynamic";
+    // Errors name the command that ran: `variable.make{stem}Dynamic`.
+    let c = &format!("variable.make{stem}Dynamic");
     let ids = selected_roots(s)?;
     if ids.is_empty() {
-        return Err(bad(C, "select the objects to bind"));
+        return Err(bad(c, "select the objects to bind"));
     }
     let doc = &s.doc()?.doc;
-    check_bindable(doc, &ids, kind, C)?;
+    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 shown = ids.first().and_then(|id| doc.node(*id)).map(|n| n.display_name()).unwrap_or_default();

crates/engine/src/tests_variables.rs

@@ -253,6 +253,40 @@ fn locked_art_is_neither_bound_nor_rewritten() {
     assert!(!s.doc().unwrap().doc.node(r).unwrap().visible, "the unlocked rectangle still applied");
 }
 
+/// Binding fails for locked art and for art in a hidden or locked group or layer, and the
+/// error names which of the three applies.
+#[test]
+fn binding_errors_say_whether_a_layer_or_group_is_hidden_or_locked() {
+    let mut s = session();
+    s.execute("variable.define", &json!({"name": "Name", "kind": "text"})).unwrap();
+    let layer = NodeId(s.execute("layer.new", &json!({})).unwrap()["id"].as_u64().unwrap());
+    let t = text(&mut s, "x");
+    let r = rect(&mut s);
+    let bind = |s: &mut Session| s.execute("variable.bind", &json!({"variable": "Name", "ids": [t.0]})).unwrap_err().to_string();
+
+    s.execute("layer.setProps", &json!({"id": layer.0, "visible": false})).unwrap();
+    let e = bind(&mut s);
+    assert!(e.contains(&format!("node {} is in a hidden group or layer", t.0)), "{e}");
+    s.execute("layer.setProps", &json!({"id": layer.0, "visible": true})).unwrap();
+
+    s.execute("select.set", &json!({"ids": [t.0, r.0]})).unwrap();
+    s.execute("object.group", &json!({})).unwrap();
+    s.execute("object.hide", &json!({})).unwrap();
+    let e = bind(&mut s);
+    assert!(e.contains(&format!("node {} is in a hidden group or layer", t.0)), "{e}");
+    s.execute("object.showAll", &json!({})).unwrap();
+
+    s.execute("layer.setProps", &json!({"id": layer.0, "locked": true})).unwrap();
+    let e = bind(&mut s);
+    assert!(e.contains(&format!("node {} is in a locked group or layer", t.0)), "{e}");
+    s.execute("layer.setProps", &json!({"id": layer.0, "locked": false})).unwrap();
+
+    s.execute("select.set", &json!({"ids": [t.0]})).unwrap();
+    s.execute("object.lock", &json!({})).unwrap();
+    let e = bind(&mut s);
+    assert!(e.contains(&format!("node {} is locked", t.0)), "{e}");
+}
+
 /// Make Text Dynamic / Make Visibility Dynamic: what the panel's buttons run. One step from a
 /// selection to a bound variable, named after what it drives.
 #[test]
@@ -284,6 +318,18 @@ fn making_a_selection_dynamic_names_and_binds_it() {
     assert!(s.execute("variable.makeTextDynamic", &json!({})).is_err(), "a rectangle is not type");
 }
 
+/// Make Text Dynamic reports its errors under its own command id.
+#[test]
+fn make_dynamic_errors_name_the_command() {
+    let mut s = session();
+    let t = text(&mut s, "x");
+    let r = rect(&mut s);
+    s.execute("select.set", &json!({"ids": [t.0, r.0]})).unwrap();
+    s.execute("object.group", &json!({})).unwrap();
+    let e = s.execute("variable.makeTextDynamic", &json!({})).unwrap_err().to_string();
+    assert!(e.contains("`variable.makeTextDynamic`") && e.contains("is not type"), "{e}");
+}
+
 /// Capture Data Set records what the art holds now, which is how rows are made — the art is
 /// edited and captured, rather than the values typed.
 #[test]