diff --git a/crates/engine/src/cmd/colormgmt.rs b/crates/engine/src/cmd/colormgmt.rs index 9842cfbe9..ccc5d4ece 100644 --- a/crates/engine/src/cmd/colormgmt.rs +++ b/crates/engine/src/cmd/colormgmt.rs @@ -19,7 +19,7 @@ use vectorcraft_render::proof; #[cfg(not(target_arch = "wasm32"))] use super::EngineError; -use super::{CommandSpec, Result, Session, always, bad, bool_or, cmd, color_value, has_doc, str_param, targets}; +use super::{CommandSpec, Result, Session, always, bad, cmd, color_value, has_doc, switch_or, targets, text_param}; /// `Document.unknown` key under which files before format v3 kept the assigned profiles (migrated /// to `Document::color_profiles` on load). @@ -100,7 +100,7 @@ pub fn specs() -> Vec { proof_setup ), cmd!(query "view.proofColors", "Proof Colors", ["View"], None, "{on?: bool (default: toggle)} → proof state", always, |s, p| { - toggle_view(s, p, |v, on| v.proof_colors = on.unwrap_or(!v.proof_colors)) + toggle_view(s, p, "view.proofColors", |v, on| v.proof_colors = on.unwrap_or(!v.proof_colors)) }), cmd!( query "view.overprintPreview", @@ -109,7 +109,7 @@ pub fn specs() -> Vec { Some("Cmd+Alt+Shift+Y"), "{on?: bool (default: toggle)} → proof state", always, - |s, p| toggle_view(s, p, |v, on| v.overprint = on.unwrap_or(!v.overprint)) + |s, p| toggle_view(s, p, "view.overprintPreview", |v, on| v.overprint = on.unwrap_or(!v.overprint)) ), cmd!( query "view.separationsPreview", @@ -143,12 +143,21 @@ pub fn specs() -> Vec { } fn intent_param(p: &Value, cmd: &str) -> Result> { - match str_param(p, "intent") { + match text_param(p, "intent", cmd)? { None => Ok(None), Some(i) => Intent::parse(i).map(Some).ok_or_else(|| bad(cmd, format!("unknown intent `{i}`"))), } } +/// The true or false `p` gives for `key`, or `None` when `p` leaves it out or gives null. Any other +/// value is an error, as [`switch_or`] reads it. +fn on_param(p: &Value, key: &str, cmd: &str) -> Result> { + if p.get(key).is_none_or(Value::is_null) { + return Ok(None); + } + switch_or(p, key, false, cmd).map(Some) +} + fn settings_json(st: &ColorSettings) -> Value { json!({ "rgb": st.rgb, @@ -171,16 +180,16 @@ fn color_settings(s: &mut Session, p: &Value) -> Result { const C: &str = "edit.colorSettings"; let mut st = cms::active_settings(); let before = st.clone(); - if let Some(v) = str_param(p, "rgb") { + if let Some(v) = text_param(p, "rgb", C)? { st.rgb = cms::canonical_name(v).to_string(); } - if let Some(v) = str_param(p, "cmyk") { + if let Some(v) = text_param(p, "cmyk", C)? { st.cmyk = cms::canonical_name(v).to_string(); } if let Some(i) = intent_param(p, C)? { st.intent = i; } - st.bpc = bool_or(p, "bpc", st.bpc); + st.bpc = switch_or(p, "bpc", st.bpc, C)?; if st != before { cms::set_active(&st).map_err(|e| bad(C, e.to_string()))?; touch_all(s); @@ -191,7 +200,7 @@ fn color_settings(s: &mut Session, p: &Value) -> Result { #[cfg(not(target_arch = "wasm32"))] fn load_profile(s: &mut Session, p: &Value) -> Result { const C: &str = "color.loadProfile"; - let path = str_param(p, "path").ok_or_else(|| bad(C, "missing `path`"))?; + let path = text_param(p, "path", C)?.ok_or_else(|| bad(C, "missing `path`"))?; crate::file_access::check_read(path).map_err(EngineError::Other)?; let bytes = crate::store::read_capped(std::path::Path::new(path), crate::store::MAX_PROFILE_FILE).map_err(|e| bad(C, format!("{path}: {e}")))?; let info = cms::register_icc(&bytes, None).map_err(|e| bad(C, e.to_string()))?; @@ -209,7 +218,7 @@ fn load_profile(s: &mut Session, p: &Value) -> Result { /// Forget a profile `color.loadProfile` loaded, and the copy that is kept of it. fn remove_profile(s: &mut Session, p: &Value) -> Result { const C: &str = "color.removeProfile"; - let name = str_param(p, "name").ok_or_else(|| bad(C, "missing `name`"))?; + let name = text_param(p, "name", C)?.ok_or_else(|| bad(C, "missing `name`"))?; let info = cms::profiles().into_iter().find(|i| i.name == name).ok_or_else(|| bad(C, format!("no profile named `{name}`")))?; if info.builtin { return Err(bad(C, format!("`{name}` is a built-in profile and stays"))); @@ -289,7 +298,7 @@ pub enum Grays { impl Grays { /// The `grays` param of `cmd`: `"profile"` (default) or `"black"`. pub(crate) fn param(cmd: &str, p: &Value) -> Result { - match str_param(p, "grays").map(str::to_ascii_lowercase).as_deref() { + match text_param(p, "grays", cmd)?.map(str::to_ascii_lowercase).as_deref() { None | Some("profile") => Ok(Grays::Profile), Some("black") => Ok(Grays::Black), Some(g) => Err(bad(cmd, format!("grays must be \"profile\" or \"black\", not `{g}`"))), @@ -302,18 +311,18 @@ impl Grays { /// colour through the colour settings. pub(crate) fn convert_mode(s: &mut Session, p: &Value) -> Result { const C: &str = "file.documentColorMode"; - let mode = match str_param(p, "mode").map(str::to_ascii_lowercase).as_deref() { + let mode = match p.get("mode").and_then(Value::as_str).map(str::to_ascii_lowercase).as_deref() { Some("cmyk") => ColorMode::Cmyk, Some("rgb") => ColorMode::Rgb, _ => return Err(bad(C, "mode must be \"cmyk\" or \"rgb\"")), }; let intent = intent_param(p, C)?; let grays = Grays::param(C, p)?; + let convert = switch_or(p, "convert", true, C)?; // Choosing the mode the document already has (the checked menu item) is not an edit. if s.doc()?.doc.color_mode == mode { return Ok(json!({ "changed": 0 })); } - let convert = bool_or(p, "convert", true); let mut changed = 0usize; s.edit("Document Color Mode", |d, _| { changed = set_color_mode(d, mode, convert, intent, grays); @@ -654,7 +663,7 @@ fn convert_color(_: &mut Session, p: &Value) -> Result { let c = cms::active(); let intent = intent_param(p, C)?.unwrap_or(c.settings().intent); let lab = c.lab(&col); - let (model, vals) = match str_param(p, "to").unwrap_or("rgb") { + let (model, vals) = match text_param(p, "to", C)?.unwrap_or("rgb") { "rgb" => ("rgb", values(&c.convert(&col, Model::Rgb, intent))), "cmyk" => ("cmyk", values(&c.convert(&col, Model::Cmyk, intent))), "gray" | "grayscale" => ("gray", values(&c.convert(&col, Model::Gray, intent))), @@ -728,7 +737,7 @@ fn view_json(v: &proof::ProofView) -> Value { fn proof_setup(s: &mut Session, p: &Value) -> Result { const C: &str = "view.proofSetup"; let mut v = proof::view(); - if let Some(t) = str_param(p, "target") { + if let Some(t) = text_param(p, "target", C)? { let t = ProofTarget::parse(t).ok_or_else(|| bad(C, format!("unknown proof target `{t}`")))?; if let ProofTarget::Cmyk(name) = &t && !cms::profile(name).is_some_and(|k| k.kind == cms::ProfileKind::Cmyk) @@ -740,8 +749,8 @@ fn proof_setup(s: &mut Session, p: &Value) -> Result { if let Some(i) = intent_param(p, C)? { v.setup.intent = i; } - v.setup.simulate_paper = bool_or(p, "simulatePaper", v.setup.simulate_paper); - if let Some(on) = p.get("proof").and_then(Value::as_bool) { + v.setup.simulate_paper = switch_or(p, "simulatePaper", v.setup.simulate_paper, C)?; + if let Some(on) = on_param(p, "proof", C)? { v.proof_colors = on; } proof::set_view(v.clone()); @@ -749,9 +758,10 @@ fn proof_setup(s: &mut Session, p: &Value) -> Result { Ok(view_json(&v)) } -fn toggle_view(s: &mut Session, p: &Value, f: impl FnOnce(&mut proof::ProofView, Option)) -> Result { +fn toggle_view(s: &mut Session, p: &Value, cmd: &str, f: impl FnOnce(&mut proof::ProofView, Option)) -> Result { + let on = on_param(p, "on", cmd)?; let mut v = proof::view(); - f(&mut v, p.get("on").and_then(Value::as_bool)); + f(&mut v, on); proof::set_view(v.clone()); touch_all(s); Ok(view_json(&v)) @@ -779,16 +789,23 @@ fn separations(s: &mut Session, p: &Value) -> Result { .map(|d| proof::plates(&d.doc).into_iter().map(|p| p.name).collect()) .unwrap_or_else(|| cms::PROCESS_PLATES.iter().map(|s| s.to_string()).collect()); let check = |n: &str| if all.iter().any(|a| a == n) { Ok(n.to_string()) } else { Err(bad(C, format!("unknown plate `{n}`"))) }; - if let Some(on) = p.get("on").and_then(Value::as_bool) { + if let Some(on) = on_param(p, "on", C)? { v.separations = if on { Some(v.separations.take().unwrap_or_else(|| all.clone())) } else { None }; } - if let Some(a) = p.get("plates").and_then(Value::as_array) { - v.separations = Some(a.iter().filter_map(Value::as_str).map(check).collect::>>()?); + match p.get("plates") { + None | Some(Value::Null) => {} + Some(Value::Array(a)) if a.iter().all(Value::is_string) => { + v.separations = Some(a.iter().filter_map(Value::as_str).map(check).collect::>>()?); + } + Some(_) => { + let key = "plates"; + return Err(bad(C, format!("`{key}` must be an array of these names: {}", all.join(", ")))); + } } - if let Some(n) = str_param(p, "only") { + if let Some(n) = text_param(p, "only", C)? { v.separations = Some(vec![check(n)?]); } - if let Some(n) = str_param(p, "toggle") { + if let Some(n) = text_param(p, "toggle", C)? { let n = check(n)?; let vis = v.separations.get_or_insert_with(|| all.clone()); if let Some(i) = vis.iter().position(|x| *x == n) { @@ -804,8 +821,8 @@ fn separations(s: &mut Session, p: &Value) -> Result { fn set_spot(s: &mut Session, p: &Value) -> Result { const C: &str = "swatch.setSpot"; - let name = str_param(p, "name").ok_or_else(|| bad(C, "missing `name`"))?.to_string(); - let spot = bool_or(p, "spot", true); + let name = text_param(p, "name", C)?.ok_or_else(|| bad(C, "missing `name`"))?.to_string(); + let spot = switch_or(p, "spot", true, C)?; let sw = s.doc()?.doc.swatch(&name).ok_or_else(|| bad(C, format!("no swatch `{name}`")))?; if !matches!(sw.paint, Paint::Solid { .. }) { return Err(bad(C, "only solid-colour swatches can be spot colours")); diff --git a/crates/engine/src/tests/colormgmt.rs b/crates/engine/src/tests/colormgmt.rs index cb6ca47c5..1ca956794 100644 --- a/crates/engine/src/tests/colormgmt.rs +++ b/crates/engine/src/tests/colormgmt.rs @@ -542,3 +542,92 @@ fn intents_reach_the_document_conversion() { assert_ne!(first(&a), first(&b)); let _ = Intent::Perceptual; } + +/// The error `cmd` gives for `params`: it must name `msg`. +fn refused(s: &mut Session, cmd: &str, params: Value, msg: &str) { + let e = match s.execute(cmd, ¶ms) { + Ok(r) => panic!("{cmd} {params} gave {r}"), + Err(e) => e.to_string(), + }; + assert!(e.contains(msg), "{cmd} {params}: {e}"); +} + +#[test] +fn color_settings_refuse_a_param_of_the_wrong_type() { + let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let mut s = session(); + let before = cms::active_settings(); + let rev = s.doc().unwrap().revision; + let c = "edit.colorSettings"; + refused(&mut s, c, json!({"bpc": "no"}), "`bpc` must be true or false"); + refused(&mut s, c, json!({"rgb": 5}), "`rgb` must be a string"); + refused(&mut s, c, json!({"cmyk": true}), "`cmyk` must be a string"); + refused(&mut s, c, json!({"intent": 2}), "`intent` must be a string"); + assert_eq!(cms::active_settings(), before, "the settings are unchanged"); + assert_eq!(s.doc().unwrap().revision, rev, "nothing redraws"); + // Omitted and null params keep the settings. + s.execute(c, &json!({"bpc": null, "rgb": null})).unwrap(); + assert_eq!(cms::active_settings(), before); + refused(&mut s, "color.convert", json!({"color": "#808080", "to": 1}), "`to` must be a string"); + refused(&mut s, "color.convert", json!({"color": "#808080", "intent": ["relative"]}), "`intent` must be a string"); + assert_eq!(s.execute("color.convert", &json!({"color": "#808080", "to": null})).unwrap()["model"], "rgb"); + refused(&mut s, "color.loadProfile", json!({"path": 7}), "`path` must be a string"); + refused(&mut s, "color.removeProfile", json!({"name": 7}), "`name` must be a string"); +} + +#[test] +fn document_color_mode_refuses_a_param_of_the_wrong_type() { + let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let mut s = session(); + let a = rect(&mut s, 0.0, 0.0, 10.0, 10.0, json!("#ff0000")); + let fill = fill_of(&s, a); + let undo = s.doc().unwrap().history.undo.len(); + for c in ["file.documentColorMode", "object.convertDocumentColorMode"] { + refused(&mut s, c, json!({"mode": "cmyk", "convert": "no"}), "`convert` must be true or false"); + refused(&mut s, c, json!({"mode": "rgb", "convert": 0}), "`convert` must be true or false"); + refused(&mut s, c, json!({"mode": "cmyk", "grays": 1}), "`grays` must be a string"); + refused(&mut s, c, json!({"mode": "cmyk", "intent": false}), "`intent` must be a string"); + } + let st = s.doc().unwrap(); + assert_eq!(st.doc.color_mode, ColorMode::Rgb); + assert_eq!(st.history.undo.len(), undo, "no undo step"); + assert_eq!(fill_of(&s, a), fill, "the colors are unchanged"); +} + +#[test] +fn proof_views_refuse_a_param_of_the_wrong_type() { + let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let mut s = session(); + let view = vectorcraft_render::proof::view(); + let rev = s.doc().unwrap().revision; + refused(&mut s, "view.proofSetup", json!({"simulatePaper": "yes"}), "`simulatePaper` must be true or false"); + refused(&mut s, "view.proofSetup", json!({"proof": 1}), "`proof` must be true or false"); + refused(&mut s, "view.proofSetup", json!({"target": 3}), "`target` must be a string"); + refused(&mut s, "view.proofColors", json!({"on": "true"}), "`on` must be true or false"); + refused(&mut s, "view.overprintPreview", json!({"on": 1}), "`on` must be true or false"); + refused(&mut s, "view.separationsPreview", json!({"on": "false"}), "`on` must be true or false"); + refused(&mut s, "view.separationsPreview", json!({"plates": "Cyan"}), "`plates` must be an array of these names: Cyan, Magenta"); + refused(&mut s, "view.separationsPreview", json!({"plates": ["Cyan", 2]}), "`plates` must be an array of these names"); + refused(&mut s, "view.separationsPreview", json!({"only": 0}), "`only` must be a string"); + refused(&mut s, "view.separationsPreview", json!({"toggle": ["Cyan"]}), "`toggle` must be a string"); + assert_eq!(vectorcraft_render::proof::view(), view, "the proof view is unchanged"); + assert_eq!(s.doc().unwrap().revision, rev, "nothing redraws"); + // A null `on` toggles, as an omitted one does. + let r = s.execute("view.proofColors", &json!({"on": null})).unwrap(); + assert_eq!(r["proofColors"], !view.proof_colors); + s.execute("view.proofColors", &json!({"on": view.proof_colors})).unwrap(); +} + +#[test] +fn set_spot_refuses_a_param_of_the_wrong_type() { + let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); + let mut s = session(); + s.execute("swatch.new", &json!({"name": "Ink", "color": {"c": 0, "m": 60, "y": 100, "k": 0}, "global": true})).unwrap(); + let undo = s.doc().unwrap().history.undo.len(); + refused(&mut s, "swatch.setSpot", json!({"name": "Ink", "spot": "true"}), "`spot` must be true or false"); + refused(&mut s, "swatch.setSpot", json!({"name": 1}), "`name` must be a string"); + assert!(!s.doc().unwrap().doc.swatch("Ink").unwrap().spot, "the swatch stays a process color"); + assert_eq!(s.doc().unwrap().history.undo.len(), undo, "no undo step"); + s.execute("swatch.setSpot", &json!({"name": "Ink", "spot": null})).unwrap(); + assert!(s.doc().unwrap().doc.swatch("Ink").unwrap().spot, "a null `spot` is the default, true"); +} diff --git a/docs/mcp.md b/docs/mcp.md index db1d43ae8..c978252f4 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -662,6 +662,10 @@ them. `grays: "black"` puts RGB greys with R = G = B on the black plate instead, Convert to Grayscale before the switch would give); other colors convert as usual. The reply's `changed` counts the colors that changed in the art, symbols, graph designs and pattern tiles; it does not count the colors of graphic styles and brushes. +`edit.colorSettings`, `file.documentColorMode`, `color.convert`, `color.loadProfile`, `color.removeProfile`, +`view.proofSetup`, `view.proofColors`, `view.overprintPreview`, `view.separationsPreview` and `swatch.setSpot` reject a +param of the wrong type, such as `"bpc": "no"`, `"on": 1` or `"to": 5`, and change nothing. Omitting a param or giving +null keeps its default. SVG import keeps what the canvas edits live. `` becomes a pattern swatch. A `` with `` becomes a symbol (named after its `data-name`, else its id) with an instance per ``; a `` that shows it differently