From 68b9f6305c7bee2c86150d27a50c18ef4454fb83 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Tue, 10 Nov 2026 14:14:31 -0800 Subject: [PATCH] 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 | 21 ++++++---- crates/engine/src/tests/opacitymask.rs | 53 ++++++++++++++++++++++++++ docs/mcp.md | 6 +++ 3 files changed, 72 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/opacitymask.rs b/crates/engine/src/cmd/opacitymask.rs index 9d81576b0..75d5852ea 100644 --- a/crates/engine/src/cmd/opacitymask.rs +++ b/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 { @@ -246,8 +246,8 @@ fn make(s: &mut Session, p: &Value) -> Result { { 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 { /// 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 { - 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> { + 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 { - 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 { } 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 { } fn toggle_new_invert(s: &mut Session, p: &Value) -> Result { - 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 })) } diff --git a/crates/engine/src/tests/opacitymask.rs b/crates/engine/src/tests/opacitymask.rs index 206ef7e66..9327bdc02 100644 --- a/crates/engine/src/tests/opacitymask.rs +++ b/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] diff --git a/docs/mcp.md b/docs/mcp.md index 972731ccc..c99bcbd91 100644 --- a/docs/mcp.md +++ b/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