Transparency: opacity mask commands reject a param of the wrong type

transparency.makeOpacityMask read `clip` and `invert` with bool_or,
transparency.setOpacityMask read `clip`, `invert`, `disabled` and
`linked` with as_bool, and the New Opacity Masks and page toggles read
`value` with bool_or, so a value of the wrong type was dropped:
"clip": "false" made a clipping mask, "invert": "yes" next to another
flag was ignored, and "value": "on" toggled the page knockout group.
They are now read with switch_or, and such a value is an error that
names the param, before anything changes. Omitting a param or giving
null keeps its default.

crates/engine/src/cmd/opacitymask.rs

@@ -14,7 +14,7 @@ use vectorcraft_doc::{Document, Knockout, Node, NodeId, NodeKind, OpacityMask, S
 
 use super::maskedit::{begin, finish, mask_group, mask_parts, show_editing};
 use super::pathops::{Beside, separate};
-use super::{CommandSpec, Result, Session, always, bad, bool_or, cmd, has_doc, ids_param, ok, targets};
+use super::{CommandSpec, Result, Session, always, bad, cmd, has_doc, ids_param, ok, switch_or, targets};
 use crate::EngineError;
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -246,8 +246,8 @@ fn make(s: &mut Session, p: &Value) -> Result<Value> {
     {
         return Err(bad(C, why));
     }
-    let clip = bool_or(p, "clip", !s.menu.new_masks_unclipped);
-    let invert = bool_or(p, "invert", s.menu.new_masks_inverted);
+    let clip = switch_or(p, "clip", !s.menu.new_masks_unclipped, C)?;
+    let invert = switch_or(p, "invert", s.menu.new_masks_inverted, C)?;
     // Several objects: the top one is the mask. One: an empty mask, drawn in mask editing.
     let (art, mask_id) = if below.is_empty() { (&ids[..], None) } else { (below, Some(top)) };
     // Several objects go into a new group, and a layer can't: the group takes a layer's art.
@@ -330,8 +330,13 @@ fn release(s: &mut Session, p: &Value) -> Result<Value> {
 
 /// Set the mask flags given in `p` on the masked objects among `target`'s targets.
 fn set_flags(s: &mut Session, target: &Value, label: &str, p: &Value) -> Result<Value> {
-    let get = |k: &str| p.get(k).and_then(Value::as_bool);
-    let (clip, invert, disabled, linked) = (get("clip"), get("invert"), get("disabled"), get("linked"));
+    let get = |k: &str| -> Result<Option<bool>> {
+        match p.get(k) {
+            None | Some(Value::Null) => Ok(None),
+            Some(_) => switch_or(p, k, false, "transparency.setOpacityMask").map(Some),
+        }
+    };
+    let (clip, invert, disabled, linked) = (get("clip")?, get("invert")?, get("disabled")?, get("linked")?);
     if clip.is_none() && invert.is_none() && disabled.is_none() && linked.is_none() {
         return Err(bad("transparency.setOpacityMask", "give at least one of clip, invert, disabled, linked"));
     }
@@ -363,7 +368,7 @@ fn set_flags(s: &mut Session, target: &Value, label: &str, p: &Value) -> Result<
 }
 
 fn toggle_new_clip(s: &mut Session, p: &Value) -> Result<Value> {
-    let v = bool_or(p, "value", s.menu.new_masks_unclipped);
+    let v = switch_or(p, "value", s.menu.new_masks_unclipped, "transparency.toggleNewMasksClipping")?;
     s.menu.new_masks_unclipped = !v;
     Ok(json!({ "value": v }))
 }
@@ -376,7 +381,7 @@ fn toggle_page(s: &mut Session, p: &Value, knockout: bool) -> Result<Value> {
     }
     let d = &s.doc()?.doc;
     let cur = if knockout { d.page_knockout } else { d.page_isolate };
-    let v = bool_or(p, "value", !cur);
+    let v = switch_or(p, "value", !cur, if knockout { "transparency.togglePageKnockoutGroup" } else { "transparency.togglePageIsolatedBlending" })?;
     if v != cur {
         s.edit(if knockout { "Page Knockout Group" } else { "Page Isolated Blending" }, |d, _| {
             *flag(d, knockout) = v;
@@ -387,7 +392,7 @@ fn toggle_page(s: &mut Session, p: &Value, knockout: bool) -> Result<Value> {
 }
 
 fn toggle_new_invert(s: &mut Session, p: &Value) -> Result<Value> {
-    let v = bool_or(p, "value", !s.menu.new_masks_inverted);
+    let v = switch_or(p, "value", !s.menu.new_masks_inverted, "transparency.toggleNewMasksInverted")?;
     s.menu.new_masks_inverted = v;
     Ok(json!({ "value": v }))
 }

crates/engine/src/tests/opacitymask.rs

@@ -151,6 +151,59 @@ fn mask_options_already_set_record_no_undo_step() {
     assert!(s.execute("transparency.setOpacityMask", &json!({"id": a.0})).is_err(), "no flag given");
 }
 
+/// Make Opacity Mask, Opacity Mask Options and the New Opacity Masks and page toggles refuse a
+/// param of the wrong type with an error naming it, and leave the document, the selection, the
+/// undo history and the defaults for new masks as they were. Null counts as a param left out,
+/// which keeps its default.
+#[test]
+fn mask_commands_refuse_a_param_of_the_wrong_type() {
+    let mut s = session();
+    let (a, m) = (rect(&mut s, 0.0, 100.0), rect(&mut s, 0.0, 50.0));
+    s.execute("select.set", &json!({"ids": [a.0, m.0]})).unwrap();
+    s.execute("transparency.makeOpacityMask", &json!({})).unwrap();
+    let (b, c) = (rect(&mut s, 150.0, 100.0), rect(&mut s, 150.0, 50.0));
+    s.execute("select.set", &json!({"ids": [b.0, c.0]})).unwrap();
+    let state = |s: &Session| {
+        let st = s.doc().unwrap();
+        (st.doc.clone(), st.revision, history(s), st.selection.objects.clone(), s.new_mask_defaults())
+    };
+    let before = state(&s);
+    let switch = |k: &str| format!("`{k}` must be true or false");
+    for (cmd, p, want) in [
+        ("transparency.makeOpacityMask", json!({"clip": "false"}), switch("clip")),
+        ("transparency.makeOpacityMask", json!({"clip": 0}), switch("clip")),
+        ("transparency.makeOpacityMask", json!({"invert": "true"}), switch("invert")),
+        ("transparency.makeOpacityMask", json!({"invert": [true]}), switch("invert")),
+        ("transparency.setOpacityMask", json!({"id": a.0, "clip": "no"}), switch("clip")),
+        ("transparency.setOpacityMask", json!({"id": a.0, "clip": false, "invert": "yes"}), switch("invert")),
+        ("transparency.setOpacityMask", json!({"id": a.0, "disabled": 1}), switch("disabled")),
+        ("transparency.setOpacityMask", json!({"id": a.0, "linked": "false"}), switch("linked")),
+        ("transparency.toggleNewMasksClipping", json!({"value": "true"}), switch("value")),
+        ("transparency.toggleNewMasksInverted", json!({"value": 1}), switch("value")),
+        ("transparency.togglePageKnockoutGroup", json!({"value": "on"}), switch("value")),
+        ("transparency.togglePageIsolatedBlending", json!({"value": 0}), switch("value")),
+    ] {
+        let r = s.execute(cmd, &p);
+        assert!(matches!(&r, Err(EngineError::BadParams { msg, .. }) if *msg == want), "{cmd} {p}: {r:?}");
+        assert!(state(&s) == before, "{cmd} {p}: nothing changes");
+    }
+    // Null params take their defaults: a new mask clips and is not inverted, a null flag stays as
+    // it is, and a null value toggles.
+    let id = s.execute("transparency.makeOpacityMask", &json!({"clip": null, "invert": null})).unwrap()["id"].as_u64().unwrap();
+    let made = mask(&s, NodeId(id)).unwrap();
+    assert!(made.clip && !made.invert);
+    s.execute("transparency.setOpacityMask", &json!({"id": a.0, "clip": null, "invert": true, "disabled": null, "linked": null})).unwrap();
+    let set = mask(&s, a).unwrap();
+    assert!(set.clip && set.invert && !set.disabled && set.linked);
+    assert!(s.execute("transparency.setOpacityMask", &json!({"id": a.0, "clip": null})).is_err(), "no flag given");
+    assert_eq!(s.execute("transparency.toggleNewMasksClipping", &json!({"value": null})).unwrap(), json!({"value": false}));
+    assert_eq!(s.execute("transparency.toggleNewMasksInverted", &json!({"value": null})).unwrap(), json!({"value": true}));
+    assert_eq!(s.new_mask_defaults(), (false, true));
+    assert_eq!(s.execute("transparency.togglePageKnockoutGroup", &json!({"value": null})).unwrap(), json!({"value": true}));
+    assert_eq!(s.execute("transparency.togglePageIsolatedBlending", &json!({"value": null})).unwrap(), json!({"value": true}));
+    assert!(doc(&s).page_knockout && doc(&s).page_isolate);
+}
+
 /// #4538: while a mask is edited the Layers panel lists only the mask, so New Layer and New
 /// Sublayer are refused instead of adding layers no row shows; they work again afterwards.
 #[test]

docs/mcp.md

@@ -1608,6 +1608,12 @@ the editing layer. With gradient mesh points picked, `opacity` goes to those poi
 {"name":"run_command","arguments":{"command":"transparency.info","params":{"ids":[12,20]}}}
 ```
 
+`transparency.makeOpacityMask` rejects a `clip` or `invert` value that is not a boolean, such as `"clip": "false"`,
+and `transparency.setOpacityMask` does the same for `clip`, `invert`, `disabled` and `linked`. The toggles
+`transparency.toggleNewMasksClipping`, `transparency.toggleNewMasksInverted`, `transparency.togglePageKnockoutGroup`
+and `transparency.togglePageIsolatedBlending` reject a `value` that is not a boolean. A rejected call changes nothing.
+Omitting a param or giving null keeps its default.
+
 Knockout Group has three states: `transparency.set {knockout: "on"|"off"|"neutral"}` (`true` = on, `false` =
 neutral, the default). In a knockout group each child hides what it covers of the children below it; neutral groups
 pass the enclosing group's setting to their children, off groups never knock out. `knockoutShape: true` makes the