Merge pull request #6741 from dotCooCoo/fix/edit-colors-param-types

Edit Colors: a switch or number of the wrong type is an error instead of the default

crates/engine/src/cmd/colorcmds.rs

@@ -19,8 +19,8 @@ use vectorcraft_doc::{Document, ImageBlob, Node, NodeId, NodeKind};
 
 use super::edit::selected_roots;
 use super::{
-    CommandSpec, EngineError, Result, Session, always, bad, bool_or, check_numbers, cmd, color_value, f64_or, f64_req, has_selection, ids_param,
-    str_param, unique_name,
+    CommandSpec, EngineError, Result, Session, always, bad, check_numbers, cmd, color_value, f64_or, f64_req, has_selection, ids_param, switch_or,
+    text_param, unique_name,
 };
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -166,13 +166,15 @@ pub(crate) struct Scope {
 }
 
 impl Scope {
-    pub(crate) fn of(p: &Value) -> Self {
-        Self {
-            fill: bool_or(p, "fill", true),
-            stroke: bool_or(p, "stroke", true),
-            images: bool_or(p, "includeImages", true),
-            patterns: bool_or(p, "includePatterns", true),
-        }
+    /// The scope `p` gives; each switch left out (or null) is on. A switch that is not true or false,
+    /// such as `"false"`, is an error that names `cmd`.
+    pub(crate) fn of(p: &Value, cmd: &str) -> Result<Self> {
+        Ok(Self {
+            fill: switch_or(p, "fill", true, cmd)?,
+            stroke: switch_or(p, "stroke", true, cmd)?,
+            images: switch_or(p, "includeImages", true, cmd)?,
+            patterns: switch_or(p, "includePatterns", true, cmd)?,
+        })
     }
 }
 
@@ -414,18 +416,18 @@ impl ColorVisitor for Visit<'_, '_> {
 }
 
 pub(crate) fn recolor(s: &mut Session, p: &Value, cmd: &str, label: &str, f: &dyn Fn(Color) -> Color) -> Result<Value> {
-    run_pass(s, p, cmd, label, Recolor::new(Scope::of(p), f))
+    run_pass(s, p, cmd, label, Recolor::new(Scope::of(p, cmd)?, f))
 }
 
 /// [`recolor`] where `f` gives gray colors, so embedded images are stored with one gray channel.
 fn recolor_gray(s: &mut Session, p: &Value, cmd: &str, label: &str, f: &dyn Fn(Color) -> Color) -> Result<Value> {
-    run_pass(s, p, cmd, label, Recolor { gray_images: true, ..Recolor::new(Scope::of(p), f) })
+    run_pass(s, p, cmd, label, Recolor { gray_images: true, ..Recolor::new(Scope::of(p, cmd)?, f) })
 }
 
 /// [`recolor`] with solid colours and gradient stops mapped through `links`, with their swatch
 /// link and tint.
 pub(crate) fn recolor_links(s: &mut Session, p: &Value, cmd: &str, label: &str, f: &dyn Fn(Color) -> Color, links: &LinkMap) -> Result<Value> {
-    run_pass(s, p, cmd, label, Recolor::with_links(Scope::of(p), f, links))
+    run_pass(s, p, cmd, label, Recolor::with_links(Scope::of(p, cmd)?, f, links))
 }
 
 /// Run `pass` on the `ids` param's objects (default: the selection) as one undo step named `label`;
@@ -548,8 +550,9 @@ fn saturate(s: &mut Session, p: &Value) -> Result<Value> {
 fn adjust_balance(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "edit.colors.adjustBalance";
     check_numbers(p, &["r", "g", "b", "c", "m", "y", "k", "gray", "tint"], C)?;
+    let convert = switch_or(p, "convert", false, C)?;
     let given = |keys: &[&str]| keys.iter().any(|k| p.get(*k).is_some());
-    let mode = match str_param(p, "mode") {
+    let mode = match text_param(p, "mode", C)? {
         Some(m) => m.to_ascii_lowercase(),
         None if given(&["c", "m", "y", "k"]) => "cmyk".into(),
         None if given(&["gray"]) => "gray".into(),
@@ -592,11 +595,10 @@ fn adjust_balance(s: &mut Session, p: &Value) -> Result<Value> {
                 (*tint, *c) = (t, base.tinted(t));
                 true
             };
-            return run_pass(s, p, C, "Adjust Colors", Recolor::links(Scope::of(p), &shift));
+            return run_pass(s, p, C, "Adjust Colors", Recolor::links(Scope::of(p, C)?, &shift));
         }
         other => return Err(bad(C, format!("unknown mode `{other}` (rgb|cmyk|gray|global)"))),
     };
-    let convert = bool_or(p, "convert", false);
     let f = move |c| {
         let out = adjust(c);
         if convert { out } else { keep_model(c, out) }
@@ -703,7 +705,7 @@ fn blend(s: &mut Session, order: BlendOrder) -> Result<Value> {
 fn harmony(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "color.harmony";
     let base = p.get("color").and_then(color_value).ok_or_else(|| bad(C, "missing or invalid `color`"))?;
-    let rule = str_param(p, "rule").ok_or_else(|| bad(C, "missing `rule`"))?;
+    let rule = text_param(p, "rule", C)?.ok_or_else(|| bad(C, "missing `rule`"))?;
     let rule = Harmony::parse(rule).ok_or_else(|| {
         let ids: Vec<String> = Harmony::ALL.iter().map(|h| h.id()).collect();
         bad(C, format!("unknown rule `{rule}` ({})", ids.join("|")))
@@ -713,7 +715,7 @@ fn harmony(s: &mut Session, p: &Value) -> Result<Value> {
     if let Some(n) = p.get("steps").and_then(Value::as_u64) {
         opts.steps = n.min(u32::MAX as u64) as u32;
     }
-    if let Some(v) = str_param(p, "variation") {
+    if let Some(v) = text_param(p, "variation", C)? {
         opts.variation = Variation::parse(v).ok_or_else(|| bad(C, format!("unknown variation `{v}` (tintsShades|warmCool|vividMuted)")))?;
     }
     if let Some(a) = p.get("amount").and_then(Value::as_f64) {

crates/engine/src/cmd/overprint.rs

@@ -12,7 +12,7 @@ use vectorcraft_geom::FillRule;
 
 use super::appearance::{ItemTarget, item_target};
 use super::edit::selected_roots;
-use super::{CommandSpec, Result, Session, bad, bool_or, cmd, f64_or, has_doc, ids_param, targets};
+use super::{CommandSpec, Result, Session, bad, cmd, has_doc, ids_param, number_or, switch_or, targets};
 
 pub fn specs() -> Vec<CommandSpec> {
     vec![
@@ -221,15 +221,16 @@ fn overprint_black_tree(n: &mut Node, op: &OverprintBlack, spots: &[String]) ->
 }
 
 pub(crate) fn overprint_black(s: &mut Session, p: &Value) -> Result<Value> {
+    const C: &str = "edit.colors.overprintBlack";
     let op = OverprintBlack {
-        remove: bool_or(p, "remove", false),
-        min_k: (f64_or(p, "percentage", 100.0) / 100.0).clamp(0.0, 1.0) as f32,
-        fill: bool_or(p, "fill", true),
-        stroke: bool_or(p, "stroke", true),
-        rich: bool_or(p, "includeCmyBlacks", false),
-        spot: bool_or(p, "includeSpotBlacks", false),
+        remove: switch_or(p, "remove", false, C)?,
+        min_k: (number_or(p, "percentage", 100.0, C)? / 100.0).clamp(0.0, 1.0) as f32,
+        fill: switch_or(p, "fill", true, C)?,
+        stroke: switch_or(p, "stroke", true, C)?,
+        rich: switch_or(p, "includeCmyBlacks", false, C)?,
+        spot: switch_or(p, "includeSpotBlacks", false, C)?,
     };
-    let ids = match ids_param(p, "ids", "edit.colors.overprintBlack")? {
+    let ids = match ids_param(p, "ids", C)? {
         Some(v) => v,
         None => selected_roots(s)?,
     };

crates/engine/src/cmd/recolor.rs

@@ -13,9 +13,7 @@ use vectorcraft_doc::swatches::{node_colors, node_paints, override_paints};
 use vectorcraft_doc::{Document, Node, NodeId, NodeKind};
 
 use super::colorcmds::{NotChanged, Recolor, Scope, left_out};
-use super::{
-    CommandSpec, Result, Session, always, bad, check_switches, checked_ids_param, cmd, color_value, has_doc, index, str_param, switch_or, text_param,
-};
+use super::{CommandSpec, Result, Session, always, bad, checked_ids_param, cmd, color_value, has_doc, index, str_param, switch_or, text_param};
 
 pub fn specs() -> Vec<CommandSpec> {
     vec![
@@ -566,8 +564,8 @@ fn apply(s: &mut Session, p: &Value) -> Result<Value> {
     if ids.is_empty() && group.is_none() {
         return Err(bad(C, "select artwork to recolour, or give a colour `group`"));
     }
-    check_switches(p, &["includeImages", "includePatterns"], C)?;
-    let scope = Scope { fill: true, stroke: true, ..Scope::of(p) };
+    let scope =
+        Scope { fill: true, stroke: true, images: switch_or(p, "includeImages", true, C)?, patterns: switch_or(p, "includePatterns", true, C)? };
     let label = if ids.is_empty() { "Edit Color Group" } else { "Recolor Artwork" };
     // `model`: keep each colour's model (default), take the new colour's own, or the document's.
     // A given `model` must be a string; only a missing one means "keep". A row's own `model`

crates/engine/src/tests/colorguide.rs

@@ -60,6 +60,18 @@ fn harmony_refuses_steps_or_amount_that_is_not_a_number() {
     assert_eq!(r["grid"][0].as_array().unwrap().len(), 9, "a null is read as a key left out");
 }
 
+/// The Color Guide refuses a `rule` or `variation` that is not a string; a `variation` such as 5
+/// would otherwise be dropped and the default variation used.
+#[test]
+fn harmony_refuses_rule_or_variation_that_is_not_a_string() {
+    let mut s = Session::new();
+    for p in [json!({"color": "#ff0000", "rule": 5}), json!({"color": "#ff0000", "rule": "triad", "variation": 5})] {
+        let e = s.execute("color.harmony", &p).unwrap_err().to_string();
+        assert!(e.contains("must be a string"), "{p}: {e}");
+    }
+    s.execute("color.harmony", &json!({"color": "#ff0000", "rule": "triad", "variation": null})).unwrap();
+}
+
 #[test]
 fn new_swatch_saves_several_colours_in_one_step() {
     let mut s = session();

crates/engine/src/tests/editcolors.rs

@@ -124,6 +124,35 @@ fn adjust_balance_refuses_a_channel_that_is_not_a_number() {
     assert_eq!(fill_of(&s, a).to_hex(), "#b38080");
 }
 
+/// The Edit Colors commands refuse a scope switch, `convert` or `mode` of the wrong type, which they
+/// would otherwise replace with the default: `{"fill": "false"}` recolored the fills.
+#[test]
+fn edit_colors_refuse_a_param_of_the_wrong_type() {
+    let mut s = session();
+    let a = rect(&mut s, json!("#ff0000"));
+    let undo = s.doc().unwrap().history.undo.len();
+    let cases = [
+        ("edit.colors.invert", json!({"fill": "false"}), "`fill` must be true or false"),
+        ("edit.colors.toCMYK", json!({"stroke": 0}), "`stroke` must be true or false"),
+        ("edit.colors.toGrayscale", json!({"fill": "false"}), "`fill` must be true or false"),
+        ("edit.colors.toRGB", json!({"includeImages": "no"}), "`includeImages` must be true or false"),
+        ("edit.colors.saturate", json!({"intensity": 50, "includePatterns": 1}), "`includePatterns` must be true or false"),
+        ("edit.colors.adjustBalance", json!({"r": 20, "fill": "false"}), "`fill` must be true or false"),
+        ("edit.colors.adjustBalance", json!({"tint": 20, "convert": "true"}), "`convert` must be true or false"),
+        ("edit.colors.adjustBalance", json!({"tint": 20, "fill": "false"}), "`fill` must be true or false"),
+        ("edit.colors.adjustBalance", json!({"mode": 1, "r": 20}), "`mode` must be a string"),
+    ];
+    for (id, p, want) in cases {
+        let e = s.execute(id, &p).unwrap_err().to_string();
+        assert!(e.contains(want), "{id} {p}: {e}");
+    }
+    assert_eq!(fill_of(&s, a).to_hex(), "#ff0000");
+    assert_eq!(s.doc().unwrap().history.undo.len(), undo, "no undo step");
+    // A null is read as a switch left out.
+    s.execute("edit.colors.invert", &json!({"fill": null, "stroke": null})).unwrap();
+    assert_eq!(fill_of(&s, a).to_hex(), "#00ffff");
+}
+
 // ---------- meshes, images and patterns ----------
 
 fn mesh_colors(s: &Session, id: NodeId) -> Vec<Color> {

crates/engine/src/tests/overprint.rs

@@ -198,6 +198,30 @@ fn overprints(s: &Session, id: NodeId) -> (bool, bool) {
     (a.fill().unwrap().overprint, a.stroke().unwrap().overprint)
 }
 
+/// Overprint Black refuses a switch or `percentage` of the wrong type, which it would otherwise
+/// replace with the default: `{"fill": "false"}` marked the fills.
+#[test]
+fn overprint_black_refuses_a_param_of_the_wrong_type() {
+    let mut s = session();
+    let k = rect(&mut s, [0.0, 0.0, 10.0, 10.0], black_ink(), Some(black_ink()), 1.0);
+    let before = undo_len(&s);
+    for (p, want) in [
+        (json!({"ids": [k.0], "fill": "false"}), "`fill` must be true or false"),
+        (json!({"ids": [k.0], "stroke": 0}), "`stroke` must be true or false"),
+        (json!({"ids": [k.0], "remove": "true"}), "`remove` must be true or false"),
+        (json!({"ids": [k.0], "includeCmyBlacks": "yes"}), "`includeCmyBlacks` must be true or false"),
+        (json!({"ids": [k.0], "includeSpotBlacks": 1}), "`includeSpotBlacks` must be true or false"),
+        (json!({"ids": [k.0], "percentage": "75"}), "`percentage` must be a number"),
+    ] {
+        let e = s.execute("edit.colors.overprintBlack", &p).unwrap_err().to_string();
+        assert!(e.contains(want), "{p}: {e}");
+    }
+    assert_eq!((overprints(&s, k), undo_len(&s)), ((false, false), before), "nothing changed, no undo step");
+    // A null is read as a key left out.
+    s.execute("edit.colors.overprintBlack", &json!({"ids": [k.0], "fill": null, "percentage": null})).unwrap();
+    assert_eq!(overprints(&s, k), (true, true));
+}
+
 #[test]
 fn overprint_black_options() {
     let mut s = session();

docs/mcp.md

@@ -3044,6 +3044,11 @@ with one gray channel, so a PDF writes it in gray.
 `includePatterns: false` leaves patterns out. The Blend commands also grade gradient meshes, which keep their shading.
 `edit.colors.saturate` needs `intensity`, a number from -100 to 100 (a number outside the range counts as its nearest
 end). Without one, or with a value that is not a number, it answers `` missing number `intensity` `` and changes nothing.
+The `edit.colors.*` commands, `edit.colors.overprintBlack` and `addon.color.filter` answer a `fill`, `stroke`,
+`includeImages`, `includePatterns`, `convert`, `remove`, `includeCmyBlacks` or `includeSpotBlacks` that is not true or
+false with `` `fill` must be true or false ``, naming the key, a `percentage` that is not a number with
+`` `percentage` must be a number ``, and a `mode` that is not a string with `` `mode` must be a string ``. They change
+nothing and record no undo step. A null counts as a key left out.
 
 The reply of `recolor.apply` and of every command above except the Blend commands has `notChanged: {locked, hidden}`.
 When the selection is exactly what `select.all` takes (in isolation mode, what it takes inside the isolated group), it