diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index 00e914ddb..0377cd5ae 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -56,6 +56,24 @@ fn s<'a>(p: &'a Value, k: &str) -> Option<&'a str> { p.get(k).and_then(Value::as_str) } +/// An optional param read with `read`: none when it is omitted or null, else an error naming the +/// key when `read` gets nothing from its value. +fn opt(p: &Value, k: &str, read: impl Fn(&Value) -> Option, what: &str) -> Result, String> { + match p.get(k) { + None | Some(Value::Null) => Ok(None), + Some(v) => read(v).map(Some).ok_or_else(|| format!("`{k}` must be {what}")), + } +} + +fn opt_bool(p: &Value, k: &str) -> Result, String> { + opt(p, k, Value::as_bool, "true or false") +} + +/// A pointer packet's `mods`: none when omitted or null, else an object of booleans. +fn packet_mods(p: &Value) -> Result, String> { + opt(p, "mods", |m| serde_json::from_value(m.clone()).ok(), "an object of true or false values") +} + pub(crate) fn pointer_kind(k: &str) -> Option { Some(match k { "down" => PointerKind::Down, @@ -210,12 +228,13 @@ impl Headless { fn pointer(&mut self, p: &Value) -> Result { let events = p.get("events").and_then(Value::as_array).ok_or("missing `events`")?; - let base: Mods = p.get("mods").and_then(|m| serde_json::from_value(m.clone()).ok()).unwrap_or_default(); + let base = packet_mods(p)?.unwrap_or_default(); let default = PointerEvent::json_backquote(p, false)?; let held = events.iter().map(|e| PointerEvent::json_backquote(e, default)).collect::, _>>()?; + let mods = events.iter().map(|e| Ok(packet_mods(e)?.unwrap_or(base))).collect::, String>>()?; self.ensure_document(); let mut requests = vec![]; - for (e, backquote) in events.iter().zip(held) { + for ((e, backquote), mods) in events.iter().zip(held).zip(mods) { let kind = s(e, "kind").unwrap_or(""); let kind = pointer_kind(kind).ok_or_else(|| format!("unknown pointer kind `{kind}` (down|drag|up|move|doubleclick)"))?; if s(e, "space") == Some("screen") { @@ -223,7 +242,6 @@ impl Headless { } let x = e.get("x").and_then(Value::as_f64).ok_or("pointer event needs numeric `x`")?; let y = e.get("y").and_then(Value::as_f64).ok_or("pointer event needs numeric `y`")?; - let mods = e.get("mods").and_then(|m| serde_json::from_value(m.clone()).ok()).unwrap_or(base); let ev = PointerEvent { kind, pos: Point::new(x, y), @@ -253,8 +271,8 @@ impl Headless { fn key(&mut self, p: &Value) -> Result { let key = s(p, "key").ok_or("missing `key`")?; - let b = |n: &str| p.get(n).and_then(Value::as_bool).unwrap_or(false); - let mods = Mods { shift: b("shift"), alt: b("alt"), cmd: b("cmd"), ctrl: b("ctrl"), ..Mods::default() }; + let b = |n: &str| opt_bool(p, n).map(|on| on.unwrap_or(false)); + let mods = Mods { shift: b("shift")?, alt: b("alt")?, cmd: b("cmd")?, ctrl: b("ctrl")?, ..Mods::default() }; // A letter reaches the tool only when the tool claims it without Cmd, Ctrl or Alt (E while // a selection marquee is dragged); otherwise it is a shortcut. let mut tk = tool_key(key); @@ -296,16 +314,18 @@ impl Headless { } fn render(&mut self, p: &Value) -> Result { + let idx = opt(p, "artboard", |v| v.as_u64().and_then(|i| usize::try_from(i).ok()), "a whole number of 0 or more")?.unwrap_or(0); + let scale = opt(p, "scale", Value::as_f64, "a number")?.unwrap_or(1.0).clamp(0.01, 16.0); + let trace_views = opt_bool(p, "traceViews")? == Some(true); + let data = opt_bool(p, "data")? == Some(true); let st = self.session.active().ok_or("no document")?; let doc = st.doc.clone(); - let idx = p.get("artboard").and_then(Value::as_u64).unwrap_or(0) as usize; let r = doc.artboards.get(idx).map(|a| a.rect).ok_or("no such artboard")?; - let scale = p.get("scale").and_then(Value::as_f64).unwrap_or(1.0).clamp(0.01, 16.0); vectorcraft_render::raster_size(r, scale)?; // Rendered with the options of a PNG export on white, so its pixels are the exported file's. // `traceViews: true` draws Image Trace objects as their View shows them, as the canvas does. let mut opts = RasterExportOptions { background: Some([255; 3]), ..Default::default() }.render_options(RasterFormat::Png); - opts.trace_views = p.get("traceViews").and_then(Value::as_bool) == Some(true); + opts.trace_views = trace_views; let img = self.renderer.render_region_with(&doc, r, scale, &opts); // A cancelled render stops drawing: the picture is not whole and is not written. vectorcraft_engine::cancel::check().map_err(|e| e.to_string())?; @@ -317,7 +337,7 @@ impl Headless { vectorcraft_format::write_atomic(std::path::Path::new(path), &png).map_err(|e| format!("write {path}: {e}"))?; out["path"] = json!(path); // `data: true` asks for the image as well as the file. - if p.get("data").and_then(Value::as_bool) == Some(true) { + if data { out["pngBase64"] = json!(vectorcraft_format::base64_encode(&png)); } } diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index d7de8dd7f..dbe429a28 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -1452,6 +1452,34 @@ fn headless_backend_methods() { assert!(h.call("ui.resize", json!({})).is_err()); } +/// The headless key, pointer and screenshot calls refuse a value of the wrong type and change +/// nothing; omitting it or giving null keeps its default. +#[test] +fn headless_methods_refuse_params_of_the_wrong_type() { + let mut h = Headless::new(); + h.call("engine.execute", json!({"command": "file.new", "params": {"width": 300, "height": 200}})).unwrap(); + h.call("ui.tool.select", json!({"tool": "rectangle"})).unwrap(); + let before = h.call("document.json", json!({})).unwrap(); + let drag = |mods: Value| json!({"events": [{"kind": "down", "x": 10, "y": 10}, {"kind": "drag", "x": 60, "y": 60, "mods": mods}, {"kind": "up", "x": 60, "y": 60}]}); + for (method, params, key) in [ + ("ui.render", json!({"artboard": "0"}), "artboard"), + ("ui.render", json!({"scale": "2"}), "scale"), + ("ui.render", json!({"traceViews": 1}), "traceViews"), + ("ui.screenshot", json!({"data": "yes", "path": "x.png"}), "data"), + ("ui.key", json!({"key": "Delete", "shift": "true"}), "shift"), + ("ui.pointer", drag(json!({"shift": "yes"})), "mods"), + ("ui.pointer", json!({"mods": 1, "events": []}), "mods"), + ] { + let e = h.call(method, params.clone()).unwrap_err(); + assert!(e.contains(&format!("`{key}`")), "{method} {params}: {e}"); + } + assert_eq!(h.call("document.json", json!({})).unwrap(), before, "a refused call changes nothing"); + let r = h.call("ui.render", json!({"artboard": null, "scale": null, "traceViews": null})).unwrap(); + assert_eq!((r["width"].as_u64(), r["height"].as_u64()), (Some(300), Some(200))); + h.call("ui.pointer", drag(Value::Null)).unwrap(); + assert_ne!(h.call("document.json", json!({})).unwrap(), before, "null mods keep the packet's"); +} + // ---------- workflow tools ---------- fn id_of(result: &Value) -> u64 { diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index b02c3b6e1..77e931bdc 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -494,9 +494,23 @@ fn key_from(name: &str) -> Option { /// The modifier keys a request holds down (`cmd`, `ctrl`, `alt`, `shift`; `cmd` is Command on /// macOS, Ctrl elsewhere). -fn modifiers(p: &Value) -> egui::Modifiers { - let b = |n: &str| p.get(n).and_then(Value::as_bool).unwrap_or(false); - egui::Modifiers { alt: b("alt"), ctrl: b("ctrl"), shift: b("shift"), mac_cmd: b("cmd") && cfg!(target_os = "macos"), command: b("cmd") } +fn modifiers(p: &Value) -> Result { + let b = |n: &str| crate::menus::opt_bool(p, n).map(|on| on.unwrap_or(false)); + let cmd = b("cmd")?; + Ok(egui::Modifiers { alt: b("alt")?, ctrl: b("ctrl")?, shift: b("shift")?, mac_cmd: cmd && cfg!(target_os = "macos"), command: cmd }) +} + +/// An optional number param of a control request as `f32`, `default` when it is omitted or null. +fn coord(p: &Value, key: &str, default: f32) -> Result { + crate::menus::opt_number(p, key).map(|n| n.map_or(default, |n| n as f32)) +} + +/// A pointer packet's `mods`: none when omitted or null, else an object of booleans. +fn packet_mods(value: Option<&Value>) -> Result, String> { + match value { + None | Some(Value::Null) => Ok(None), + Some(m) => serde_json::from_value(m.clone()).map(Some).map_err(|e| format!("`mods` must be an object of true or false values: {e}")), + } } pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlRequest) -> Outcome { @@ -532,10 +546,13 @@ pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlReques "ui.pointer" => pointer_events(app, ctx, req), "ui.key" => { let Some(k) = s("key").and_then(key_from) else { return err("unknown or missing `key`") }; + let m = match modifiers(p) { + Ok(m) => m, + Err(e) => return err(e), + }; if let Err(e) = reserve_synthetic(app, 2 + usize::from(s("text").is_some())) { return err(e); } - let m = modifiers(p); app.synthetic.push(egui::Event::Key { key: k, physical_key: None, pressed: true, repeat: false, modifiers: m }); // Typed text comes with the press, so it shares the key's frame and modifiers. if let Some(t) = s("text") { @@ -546,31 +563,39 @@ pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlReques ok(Value::Null) } "ui.move" => { + let pos = match coord(p, "x", 0.0).and_then(|x| Ok(egui::pos2(x, coord(p, "y", 0.0)?))) { + Ok(pos) => pos, + Err(e) => return err(e), + }; if let Err(e) = reserve_synthetic(app, 1) { return err(e); } - let pos = egui::pos2(p.get("x").and_then(Value::as_f64).unwrap_or(0.0) as f32, p.get("y").and_then(Value::as_f64).unwrap_or(0.0) as f32); app.synthetic.push(egui::Event::PointerMoved(pos)); ctx.request_repaint(); ok(Value::Null) } "ui.click" | "ui.drag" => synthetic_click(app, ctx, req), "ui.wheel" => { + // Mouse wheel notches over screen point (x, y): `dy` up (+) or down, `dx` sideways + // (`unit: "point"` for trackpad-like points), with modifiers as `ui.click` takes them. + let given = + (|| Ok::<_, String>(([coord(p, "dx", 0.0)?, coord(p, "dy", 0.0)?, coord(p, "x", 0.0)?, coord(p, "y", 0.0)?], modifiers(p)?)))(); + let ([dx, dy, x, y], modifiers) = match given { + Ok(given) => given, + Err(e) => return err(e), + }; if let Err(e) = reserve_synthetic(app, 2) { return err(e); } - // Mouse wheel notches over screen point (x, y): `dy` up (+) or down, `dx` sideways - // (`unit: "point"` for trackpad-like points), with modifiers as `ui.click` takes them. - let f = |k: &str| p.get(k).and_then(Value::as_f64).unwrap_or(0.0) as f32; let unit = if s("unit") == Some("point") { egui::MouseWheelUnit::Point } else { egui::MouseWheelUnit::Line }; // Notches, not a flood: a turn stays within what a real wheel sends. - let delta = egui::vec2(f("dx"), f("dy")).clamp(egui::Vec2::splat(-100.0), egui::Vec2::splat(100.0)); - app.synthetic.push(egui::Event::PointerMoved(egui::pos2(f("x"), f("y")))); + let delta = egui::vec2(dx, dy).clamp(egui::Vec2::splat(-100.0), egui::Vec2::splat(100.0)); + app.synthetic.push(egui::Event::PointerMoved(egui::pos2(x, y))); app.synthetic.push(egui::Event::MouseWheel { unit, delta, phase: egui::TouchPhase::Move, - modifiers: modifiers(p), + modifiers, source: egui::MouseWheelSource::Unknown, }); ctx.request_repaint(); @@ -635,8 +660,10 @@ pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlReques ok(Value::Null) } "ui.resize" => { - let w = p.get("width").and_then(Value::as_f64).unwrap_or(1440.0) as f32; - let h = p.get("height").and_then(Value::as_f64).unwrap_or(900.0) as f32; + let (w, h) = match coord(p, "width", 1440.0).and_then(|w| Ok((w, coord(p, "height", 900.0)?))) { + Ok(size) => size, + Err(e) => return err(e), + }; ctx.send_viewport_cmd(egui::ViewportCommand::InnerSize(egui::vec2(w, h))); ok(Value::Null) } @@ -649,32 +676,48 @@ pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlReques if let Some(Err(e)) = s("path").map(vectorcraft_engine::file_access::check_write) { return err(e); } - if p.get("focus").and_then(Value::as_bool).unwrap_or(true) { + let flags = crate::menus::opt_bool(p, "focus").and_then(|focus| Ok((focus, crate::menus::opt_bool(p, "data")?))); + let (focus, data) = match flags { + Ok(flags) => flags, + Err(e) => return err(e), + }; + if focus.unwrap_or(true) { ctx.send_viewport_cmd(egui::ViewportCommand::Focus); } ctx.request_repaint(); - Outcome::Screenshot { path: s("path").map(str::to_string), data: p.get("data").and_then(Value::as_bool) == Some(true) } + Outcome::Screenshot { path: s("path").map(str::to_string), data: data == Some(true) } } "ui.render" => { + let given = (|| { + Ok::<_, String>(( + crate::menus::opt_index(p, "artboard")?, + crate::menus::opt_number(p, "scale")?, + crate::menus::opt_bool(p, "traceViews")?, + crate::menus::opt_bool(p, "data")?, + )) + })(); + let (index, scale, trace_views, data) = match given { + Ok(given) => given, + Err(e) => return err(e), + }; let Some(st) = app.session.active() else { return err("no document") }; let doc = st.doc.clone(); // The artboard asked for (0-based, as headless), else the first. - let index = p.get("artboard").and_then(Value::as_u64).map_or(Some(0), |i| usize::try_from(i).ok()); - let Some(r) = index.and_then(|i| doc.artboards.get(i)).map(|a| a.rect) else { return err("no such artboard") }; - let scale = p.get("scale").and_then(Value::as_f64).unwrap_or(1.0); + let Some(r) = doc.artboards.get(index.unwrap_or(0)).map(|a| a.rect) else { return err("no such artboard") }; + let scale = scale.unwrap_or(1.0); if let Err(e) = vectorcraft_render::raster_size(r, scale) { return err(e); } // Rendered with the options of a PNG export on white, so its pixels are the exported file's. // `traceViews: true` draws Image Trace objects as their View shows them, as the canvas does. let mut opts = RasterExportOptions { background: Some([255; 3]), ..Default::default() }.render_options(RasterFormat::Png); - opts.trace_views = p.get("traceViews").and_then(Value::as_bool) == Some(true); + opts.trace_views = trace_views == Some(true); let img = app.canvas.renderer.render_region_with(&doc, r, scale, &opts); let png = match img.to_png() { Ok(png) => png, Err(e) => return err(e), }; - let data = p.get("data").and_then(Value::as_bool) == Some(true); + let data = data == Some(true); match s("path") { Some(path) => match app.services.write.as_mut() { Some(w) => wrap( @@ -926,7 +969,21 @@ fn pointer_events_inner(app: &mut VectorcraftApp, ctx: &egui::Context, req: &Con if rotating && let Err(error) = validate_rotate_view_events(p, events) { return err(error); } - let base_mods: Mods = p.get("mods").and_then(|m| serde_json::from_value(m.clone()).ok()).unwrap_or_default(); + // Every event is read before any is sent, so a malformed packet sends none of them. + let read = packet_mods(p.get("mods")).and_then(|base| { + let base = base.unwrap_or_default(); + events + .iter() + .map(|e| { + let at = |k: &str| crate::menus::opt_number(e, k).map(|n| n.unwrap_or(0.0)); + Ok((at("x")?, at("y")?, packet_mods(e.get("mods"))?.unwrap_or(base))) + }) + .collect::, String>>() + }); + let read = match read { + Ok(read) => read, + Err(e) => return err(e), + }; let view = app.view_info(); let xf = app.canvas_rect.zip(app.view().copied()).map(|(rect, v)| Xf::new(rect, &v)); let from = app.session.input_count(); @@ -936,7 +993,7 @@ fn pointer_events_inner(app: &mut VectorcraftApp, ctx: &egui::Context, req: &Con Ok(held) => held, Err(e) => return err(e), }; - for (e, backquote) in events.iter().zip(held) { + for ((e, backquote), (x, y, mods)) in events.iter().zip(held).zip(read) { let kind = match e.get("kind").and_then(Value::as_str).unwrap_or("") { "down" => PointerKind::Down, "drag" => PointerKind::Drag, @@ -945,10 +1002,7 @@ fn pointer_events_inner(app: &mut VectorcraftApp, ctx: &egui::Context, req: &Con "doubleclick" | "dblclick" => PointerKind::DoubleClick, other => return err(format!("unknown pointer kind `{other}`")), }; - let x = e.get("x").and_then(Value::as_f64).unwrap_or(0.0); - let y = e.get("y").and_then(Value::as_f64).unwrap_or(0.0); let screen = e.get("space").and_then(Value::as_str) == Some("screen"); - let mods = e.get("mods").and_then(|m| serde_json::from_value(m.clone()).ok()).unwrap_or(base_mods); if rotating { if let Err(e) = rotate_view_pointer(app, kind, vectorcraft_geom::Point::new(x, y), screen, mods.shift) { return err(e); @@ -1007,18 +1061,24 @@ fn synthetic_click(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlR let Some(events) = count.checked_mul(2).and_then(|n| n.checked_add(steps)).and_then(|n| n.checked_add(1)) else { return err("synthetic input event count is out of range"); }; + // Screen-space (egui points) pointer input through egui itself: reaches every widget. + let drag = req.method == "ui.drag"; + let given = (|| { + let a = egui::pos2(coord(p, "x", 0.0)?, coord(p, "y", 0.0)?); + let end = if drag { egui::pos2(coord(p, "toX", 0.0)?, coord(p, "toY", 0.0)?) } else { a }; + Ok::<_, String>((a, end, modifiers(p)?)) + })(); + let (a, end, modifiers) = match given { + Ok(given) => given, + Err(e) => return err(e), + }; if let Err(e) = reserve_synthetic(app, events) { return err(e); } - // Screen-space (egui points) pointer input through egui itself: reaches every widget. - let f = |k: &str| p.get(k).and_then(Value::as_f64).unwrap_or(0.0) as f32; let button = match s("button") { Some("right") | Some("secondary") => egui::PointerButton::Secondary, _ => egui::PointerButton::Primary, }; - let modifiers = modifiers(p); - let a = egui::pos2(f("x"), f("y")); - let end = if req.method == "ui.drag" { egui::pos2(f("toX"), f("toY")) } else { a }; app.synthetic.push(egui::Event::PointerMoved(a)); app.synthetic.push(egui::Event::PointerButton { pos: a, button, pressed: true, modifiers }); if req.method == "ui.drag" { @@ -1040,6 +1100,11 @@ fn synthetic_click(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlR fn set_ui(app: &mut VectorcraftApp, req: &ControlRequest) -> Outcome { let p = &req.params; let s = |k: &str| p.get(k).and_then(Value::as_str); + // A flag of the wrong type is refused before anything is set. + let flags = ["rulers", "outline", "grid", "smartGuides", "boundingBox", "controlBar", "imageTracePreview"]; + if let Some(e) = flags.iter().find_map(|k| crate::menus::opt_bool(p, k).err()) { + return err(e); + } // Outline is a setting of the active document's view: without one, nothing is set. if p.get("outline").is_some_and(Value::is_boolean) && app.session.active().is_none() { return err("no document"); diff --git a/crates/ui-egui/src/control/tests.rs b/crates/ui-egui/src/control/tests.rs index 0a9c042a1..2cdf13a4d 100644 --- a/crates/ui-egui/src/control/tests.rs +++ b/crates/ui-egui/src/control/tests.rs @@ -314,6 +314,92 @@ fn render_takes_the_artboard_asked_for() { assert_eq!(got, size.map(Some), "{params}: {r}"); } assert!(request("ui.render", json!({"artboard": 2}))["error"].is_string()); + for (params, key) in [ + (json!({"artboard": "1"}), "artboard"), + (json!({"artboard": -1}), "artboard"), + (json!({"scale": "0.5"}), "scale"), + (json!({"traceViews": 1}), "traceViews"), + (json!({"data": "yes"}), "data"), + ] { + let r = request("ui.render", params.clone()); + assert!(r["error"].as_str().is_some_and(|e| e.contains(&format!("`{key}`"))), "{params}: {r}"); + } + let r = request("ui.render", json!({"artboard": null, "scale": null})); + assert_eq!([&r["result"]["width"], &r["result"]["height"]].map(Value::as_u64), [Some(64), Some(64)], "null keeps the default: {r}"); +} + +/// The control channel's input methods refuse a number or switch of the wrong type before they +/// queue any event; omitting it keeps its default. +#[test] +fn control_input_refuses_params_of_the_wrong_type() { + let move_to = |mods: Value| json!({"events": [{"kind": "move", "x": 5, "y": 5}, {"kind": "move", "x": 6, "y": 6, "mods": mods}]}); + for (method, params, key) in [ + ("ui.move", json!({"x": "10"}), "x"), + ("ui.move", json!({"y": true}), "y"), + ("ui.wheel", json!({"dy": "1"}), "dy"), + ("ui.wheel", json!({"dy": 1, "shift": "yes"}), "shift"), + ("ui.click", json!({"x": [1]}), "x"), + ("ui.click", json!({"cmd": 1}), "cmd"), + ("ui.drag", json!({"toX": "5"}), "toX"), + ("ui.key", json!({"key": "Enter", "alt": "true"}), "alt"), + ("ui.resize", json!({"width": "800"}), "width"), + ("ui.screenshot", json!({"focus": "no"}), "focus"), + ("ui.screenshot", json!({"data": 1}), "data"), + ("ui.set", json!({"rulers": "true"}), "rulers"), + ("ui.set", json!({"grid": true, "controlBar": 0}), "controlBar"), + ("ui.pointer", json!({"events": [{"kind": "move", "x": "5", "y": 0}]}), "x"), + ("ui.pointer", move_to(json!({"shift": "yes"})), "mods"), + ("ui.pointer", json!({"mods": "shift", "events": [{"kind": "move", "x": 1, "y": 1}]}), "mods"), + ] { + let mut app = VectorcraftApp::new(Session::new(), Services::default()); + app.synthetic.push(egui::Event::Text("pending".into())); + let before = (format!("{:?}", app.synthetic), app.ui.view.rulers, app.ui.view.grid, app.ui.control_bar); + let r = input_request(&mut app, method, params.clone()); + assert_eq!(r["ok"], false, "{method} {params}: {r}"); + assert!(r["error"].as_str().is_some_and(|e| e.contains(&format!("`{key}`"))), "{method} {params}: {r}"); + let after = (format!("{:?}", app.synthetic), app.ui.view.rulers, app.ui.view.grid, app.ui.control_bar); + assert_eq!(after, before, "{method} {params}: nothing is queued or set"); + } + let mut app = VectorcraftApp::new(Session::new(), Services::default()); + assert_eq!(input_request(&mut app, "ui.move", json!({"x": null, "y": 4}))["ok"], true); + assert!(matches!(app.synthetic.first(), Some(egui::Event::PointerMoved(p)) if *p == egui::pos2(0.0, 4.0))); +} + +/// The app's own commands refuse a param of the wrong type and change nothing; omitting it or +/// giving null keeps its default. +#[test] +fn ui_commands_refuse_params_of_the_wrong_type() { + let mut app = VectorcraftApp::new(Session::new(), Services::default()); + app.run("file.new", json!({"width": 200, "height": 200})).unwrap(); + let id = app.run("shape.rectangle", json!({"x": 10, "y": 10, "width": 40, "height": 40})).unwrap()["id"].as_u64().unwrap(); + app.run("select.set", json!({"ids": [id]})).unwrap(); + app.run("view.setZoom", json!({"zoom": 300, "center": [5, 5]})).unwrap(); + let view = |app: &VectorcraftApp| app.view().map(|v| (v.zoom, v.center.x, v.center.y)); + let before = (view(&app), app.ui.screen_mode, app.session.documents().len(), app.ui.dialog.is_some(), app.ui.open_panel.clone()); + for (id, params, key) in [ + ("view.setZoom", json!({"zoom": "200"}), "zoom"), + ("view.setZoom", json!({"zoom": 200, "center": [1]}), "center"), + ("view.setZoom", json!({"zoom": 200, "center": "0,0"}), "center"), + ("view.screenMode", json!({"mode": "2"}), "mode"), + ("view.screenMode", json!({"mode": 1.5}), "mode"), + ("window.panel", json!({"panel": "layers", "toggle": "true"}), "toggle"), + ("window.panel", json!({"panel": "layers", "toggle": true, "toggleGroup": 1}), "toggleGroup"), + ("file.revert", json!({"confirmed": "yes"}), "confirmed"), + ("file.close", json!({"index": "0"}), "index"), + ("ui.newSwatch", json!({"spot": "true"}), "spot"), + ("ui.newSwatch", json!({"global": 1}), "global"), + ("ui.symbolOptions", json!({"edit": 1}), "edit"), + ("ui.batchDialog", json!({"set": "1"}), "set"), + ("ui.batchDialog", json!({"action": -1}), "action"), + ("file.exportSelection", json!({"multiple": "true"}), "multiple"), + ] { + let e = app.run(id, params.clone()).unwrap_err(); + assert!(e.contains(&format!("`{key}`")), "{id} {params}: {e}"); + let after = (view(&app), app.ui.screen_mode, app.session.documents().len(), app.ui.dialog.is_some(), app.ui.open_panel.clone()); + assert_eq!(after, before, "{id} {params}: nothing changes"); + } + app.run("view.setZoom", json!({"zoom": null, "center": null})).unwrap(); + assert_eq!(app.view().map(|v| v.zoom), Some(crate::state::zoom_from_display_percent(100.0, app.session.prefs.display_print_size))); } #[test] diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 3d3f2a8c0..7326a3157 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -75,7 +75,7 @@ pub(crate) use labels::lists_plugin; use labels::shows_a_name; pub use labels::{CONTEXT_LABELS, DYNAMIC_LABELS, display_label, dynamic_label, item_shortcut, menu_strings}; pub use run::{check_params, run_ui_command}; -pub(crate) use run::{normalize_panel, opt_bool}; +pub(crate) use run::{normalize_panel, opt_bool, opt_index, opt_number}; #[cfg(test)] use slots::RECENT_FONT_IDS; pub use state::{checked, enabled, plugin_revision, shortcut_of, shown_state}; diff --git a/crates/ui-egui/src/menus/run.rs b/crates/ui-egui/src/menus/run.rs index 1223e1e42..febb8313e 100644 --- a/crates/ui-egui/src/menus/run.rs +++ b/crates/ui-egui/src/menus/run.rs @@ -25,6 +25,34 @@ pub(crate) fn opt_bool(p: &Value, key: &str) -> Result, String> { } } +/// An optional `{key?: number}` param: none when it is omitted or null. +pub(crate) fn opt_number(p: &Value, key: &str) -> Result, String> { + match p.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_f64().map(Some).ok_or_else(|| format!("`{key}` must be a number")), + } +} + +/// An optional `{key?: index}` param, a whole number of 0 or more: none when it is omitted or null. +pub(crate) fn opt_index(p: &Value, key: &str) -> Result, String> { + match p.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().and_then(|i| usize::try_from(i).ok()).map(Some).ok_or_else(|| format!("`{key}` must be a whole number of 0 or more")), + } +} + +/// An optional `{key?: [x, y]}` param: none when it is omitted or null. +fn opt_point(p: &Value, key: &str) -> Result, String> { + match p.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => match v.as_array().map(Vec::as_slice) { + Some([x, y]) => x.as_f64().zip(y.as_f64()).map(|(x, y)| Some(vectorcraft_geom::Point::new(x, y))), + _ => None, + } + .ok_or_else(|| format!("`{key}` must be [x, y] numbers")), + } +} + /// Handle a UI command. `None` = not a UI command (the engine handles it). /// `params` has only keys the params description of engine command `id` names, or the parameter /// error naming the one it doesn't (#1442). The UI's own commands aren't checked yet: their @@ -48,13 +76,18 @@ pub fn check_params(session: &vectorcraft_engine::Session, id: &str, params: &Va pub fn run_ui_command(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option> { // Window menu items and the named panel function keys toggle against the checked state; // other commands that open panels show them. - if id == "window.panel" && p.get("toggle").and_then(Value::as_bool) == Some(true) && checked(app, id, p) == Some(true) { + let toggles = if id == "window.panel" { opt_bool(p, "toggle").and_then(|t| Ok((t, opt_bool(p, "toggleGroup")?))) } else { Ok((None, None)) }; + let (toggle, toggle_group) = match toggles { + Ok(flags) => flags, + Err(e) => return Some(Err(e)), + }; + if toggle == Some(true) && checked(app, id, p) == Some(true) { let panel = p.get("panel").and_then(Value::as_str).and_then(normalize_panel).unwrap_or_default(); if app.ui.open_panel.as_deref() == Some(panel) { app.ui.open_panel = None; return Some(Ok(json!({"open": null}))); } - if p.get("toggleGroup").and_then(Value::as_bool) == Some(true) { + if toggle_group == Some(true) { return Some(crate::panel_docking::hide_group(app, panel)); } return Some(app.run("window.panel.close", json!({"panel": panel}))); @@ -131,7 +164,7 @@ fn run_ui_part1(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option io::new_from_template(app, s("path")), - "file.revert" if p.get("confirmed").and_then(Value::as_bool) != Some(true) => io::ask_revert(app), + "file.revert" if opt_bool(p, "confirmed") != Ok(Some(true)) => opt_bool(p, "confirmed").and_then(|_| io::ask_revert(app)), "file.reveal" => io::reveal(app), "file.cancelExport" => Ok(io::cancel_exports(app)), "file.exportInBackground" => Ok(io::hide_exports(app)), @@ -386,9 +419,11 @@ fn run_ui_part5(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option { let print = app.session.prefs.display_print_size; - let z = crate::state::zoom_from_display_percent(p.get("zoom").and_then(Value::as_f64).unwrap_or(100.0), print); - let center = - p.get("center").and_then(Value::as_array).and_then(|a| Some(vectorcraft_geom::Point::new(a.first()?.as_f64()?, a.get(1)?.as_f64()?))); + let (zoom, center) = match opt_number(p, "zoom").and_then(|z| Ok((z, opt_point(p, "center")?))) { + Ok(given) => given, + Err(e) => return Some(Err(e)), + }; + let z = crate::state::zoom_from_display_percent(zoom.unwrap_or(100.0), print); match app.view_mut() { Some(v) => { v.zoom = z.clamp(0.0313, 640.0); @@ -417,7 +452,11 @@ fn run_ui_part5(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option { - app.ui.screen_mode = match p.get("mode").and_then(Value::as_u64) { + let mode = match opt_index(p, "mode") { + Ok(mode) => mode, + Err(e) => return Some(Err(e)), + }; + app.ui.screen_mode = match mode { Some(m) => m.min(3) as u8, // F cycles the three screen modes. Without a mode, the command also goes from // Presentation Mode (not in the cycle) back to Normal. @@ -664,9 +703,10 @@ fn run_ui_part7(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option match p.get("index").and_then(Value::as_u64).map(|i| i as usize) { - Some(i) => crate::unsaved::close(app, i), - None => { + "file.close" => match opt_index(p, "index") { + Err(e) => Err(e), + Ok(Some(i)) => crate::unsaved::close(app, i), + Ok(None) => { app.session.active_index()?; crate::doc_windows::close_current(app) } @@ -678,8 +718,10 @@ fn run_ui_part7(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option Err("missing `name`".into()), }, "ui.newSwatch" => { - let on = |k: &str| p.get(k).and_then(Value::as_bool).unwrap_or(false); - crate::dialogs::new_swatch::open(app, on("spot"), on("global"), s("group").as_deref()) + let on = |k: &str| opt_bool(p, k).map(|on| on.unwrap_or(false)); + on("spot") + .and_then(|spot| Ok((spot, on("global")?))) + .and_then(|(spot, global)| crate::dialogs::new_swatch::open(app, spot, global, s("group").as_deref())) } "ui.newColorGroup" => { let names = p.get("swatches").and_then(Value::as_array).map(|a| a.iter().filter_map(Value::as_str).map(str::to_string).collect()); @@ -697,7 +739,10 @@ fn run_ui_part7(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option crate::panels::text_styles::open_new(app, crate::panels::text_styles::Kind::Para), "ui.symbolOptions" => { let name = s("name"); - let edit = p.get("edit").and_then(Value::as_bool).unwrap_or(false); + let edit = match opt_bool(p, "edit") { + Ok(edit) => edit.unwrap_or(false), + Err(e) => return Some(Err(e)), + }; if edit || name.is_some() { crate::dialogs::symbol_options::open_edit(app, name.as_deref()) } else { @@ -899,9 +944,8 @@ fn run_ui_part11(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option crate::panels::actions::continue_playback(app, p), crate::panels::actions::DELETE_STEP => crate::panels::actions::delete_step(app, p), "ui.batchDialog" => { - let set = p.get("set").and_then(Value::as_u64).unwrap_or(0) as usize; - let action = p.get("action").and_then(Value::as_u64).unwrap_or(0) as usize; - crate::dialogs::action_batch::open(app, set, action) + let index = |k: &str| opt_index(p, k).map(|i| i.unwrap_or(0)); + index("set").and_then(|set| Ok((set, index("action")?))).and_then(|(set, action)| crate::dialogs::action_batch::open(app, set, action)) } crate::panels::actions::SAVE_SET => crate::panels::actions::save_set(app, p), crate::panels::actions::LOAD_SET => crate::panels::actions::load_set(app, p), @@ -934,7 +978,11 @@ fn run_ui_part12(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option { - app.run("assets.add", json!({ "multiple": p.get("multiple").and_then(Value::as_bool).unwrap_or(false) })).map(|r| { + let multiple = match opt_bool(p, "multiple") { + Ok(multiple) => multiple.unwrap_or(false), + Err(e) => return Some(Err(e)), + }; + app.run("assets.add", json!({ "multiple": multiple })).map(|r| { let ids: Vec = r["assets"].as_array().into_iter().flatten().filter_map(Value::as_u64).collect(); crate::dialogs::open_export_for_screens_assets(app, Some(&ids)); json!({ "assets": ids }) diff --git a/docs/mcp.md b/docs/mcp.md index 5826c5c8c..7ced9eb15 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1265,6 +1265,19 @@ fails with no change. A description with a `…` entry takes more keys than it l object can carry several formats' options). A command that takes its own reply back (`document.setup`, `text.areaOptions`) says so the same way. +The app's own commands are not checked against their descriptions, but those listed here refuse a value of the wrong +type and change nothing: `view.setZoom` (`zoom`, `center`), `view.screenMode` (`mode`), `window.panel` (`toggle`, +`toggleGroup`), `file.revert` (`confirmed`), `file.close` (`index`), `ui.newSwatch` (`spot`, `global`), +`ui.symbolOptions` (`edit`), `ui.batchDialog` (`set`, `action`) and `file.exportSelection` (`multiple`). So do the +control channel's `ui.key`, `ui.move`, `ui.wheel`, `ui.click`, `ui.drag`, `ui.pointer`, `ui.resize`, `ui.screenshot`, +`ui.render` and `ui.set` for their numbers, switches and `mods`, and the headless session's key, pointer and screenshot +calls. Omitting a param or giving null keeps its default: + +```json +{"id": "view.setZoom", "params": {"zoom": "200"}} +→ `zoom` must be a number +``` + Saved actions aren't checked this way. When the Actions panel plays an action (Play at any speed, an action's function key, the Batch dialog or `ui.action.play`), a key a command no longer reads, recorded by an older version, is ignored and the action still plays. `Session::replay` plays a journal the same way. The command still refuses an effect