diff --git a/crates/engine/src/cmd/colorcmds.rs b/crates/engine/src/cmd/colorcmds.rs index 65f105689..bbc56dbc1 100644 --- a/crates/engine/src/cmd/colorcmds.rs +++ b/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 { @@ -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 { + 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 { - 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 { - 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 { - 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 { fn adjust_balance(s: &mut Session, p: &Value) -> Result { 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 { (*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 { fn harmony(s: &mut Session, p: &Value) -> Result { 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 = 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 { 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) { diff --git a/crates/engine/src/cmd/overprint.rs b/crates/engine/src/cmd/overprint.rs index 1e8d9ced6..4cc1fa24c 100644 --- a/crates/engine/src/cmd/overprint.rs +++ b/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 { 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 { + 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)?, }; diff --git a/crates/engine/src/cmd/recolor.rs b/crates/engine/src/cmd/recolor.rs index 6dcdc9dc4..74347caa0 100644 --- a/crates/engine/src/cmd/recolor.rs +++ b/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 { vec![ @@ -566,8 +564,8 @@ fn apply(s: &mut Session, p: &Value) -> Result { 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` diff --git a/crates/engine/src/tests/colorguide.rs b/crates/engine/src/tests/colorguide.rs index 1033edf67..6213b202c 100644 --- a/crates/engine/src/tests/colorguide.rs +++ b/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(); diff --git a/crates/engine/src/tests/editcolors.rs b/crates/engine/src/tests/editcolors.rs index 49307845f..8f41ed3a7 100644 --- a/crates/engine/src/tests/editcolors.rs +++ b/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 { diff --git a/crates/engine/src/tests/overprint.rs b/crates/engine/src/tests/overprint.rs index cade55db9..15fbb721e 100644 --- a/crates/engine/src/tests/overprint.rs +++ b/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(); diff --git a/docs/mcp.md b/docs/mcp.md index 2c12e496b..dcf200446 100644 --- a/docs/mcp.md +++ b/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