From ef25dffe013d0a5c4f7dd4b71e5425ef6b70eb01 Mon Sep 17 00:00:00 2001 From: Christian Lappin <36496308+christianlappin@users.noreply.github.com> Date: Mon, 9 Nov 2026 15:31:35 -0500 Subject: [PATCH] fix(mcp): validate and cancel Rotate View gestures (#6322) --- crates/mcp/src/tests.rs | 54 ++++++++++++ crates/ui-egui/src/app.rs | 1 + crates/ui-egui/src/app/commands.rs | 4 + crates/ui-egui/src/control.rs | 109 ++++++++++++++++++------- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/tests/rotateview.rs | 87 +++++++++++++++++++- docs/mcp.md | 2 + 7 files changed, 228 insertions(+), 31 deletions(-) diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index e986deda0..b12f44680 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -1463,6 +1463,60 @@ fn pointer_gesture_forwards_backquote_on_each_event() { assert_eq!(rect["bounds"]["x"], 100.0, "{doc}"); } +/// The documented Rotate View calls reach the desktop control methods, preserving document +/// coordinates and modifiers, and a successful null reset remains a successful MCP response. +#[test] +fn rotate_view_calls_forward_to_desktop_control() { + use std::cell::RefCell; + use std::rc::Rc; + + struct Recording(Rc>>); + impl Backend for Recording { + fn call(&mut self, method: &str, params: Value) -> Result { + self.0.borrow_mut().push((method.into(), params)); + Ok(Value::Null) + } + fn has_ui(&self) -> bool { + true + } + fn describe(&self) -> String { + "desktop control contract".into() + } + } + + let seen = Rc::new(RefCell::new(Vec::new())); + let mut s = Server::new(Box::new(Recording(seen.clone()))); + let selected = call(&mut s, 1, "select_tool", json!({"tool": "rotateView"})); + let turned = call( + &mut s, + 2, + "pointer_gesture", + json!({"events": [ + {"kind": "down", "x": 300, "y": 150}, {"kind": "drag", "x": 200, "y": 250}, {"kind": "up", "x": 200, "y": 250} + ], "mods": {"shift": true}}), + ); + let reset = call(&mut s, 3, "command_run", json!({"id": "view.rotateReset", "params": {}})); + for reply in [&selected, &turned, &reset] { + assert_eq!(reply["isError"], false, "{reply}"); + } + assert_eq!(text_of(&reset), "null"); + assert_eq!( + *seen.borrow(), + vec![ + ("ui.tool.select".into(), json!({"tool": "rotateView"})), + ( + "ui.pointer".into(), + json!({"events": [ + {"kind": "down", "x": 300, "y": 150, "space": "doc"}, + {"kind": "drag", "x": 200, "y": 250, "space": "doc"}, + {"kind": "up", "x": 200, "y": 250, "space": "doc"} + ], "mods": {"shift": true}}) + ), + ("engine.execute".into(), json!({"command": "view.rotateReset", "params": {}})) + ] + ); +} + /// A `backquote` that is not a boolean is an error, and none of the gesture's events runs. #[test] fn pointer_gesture_rejects_a_backquote_that_is_not_a_boolean() { diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index d02c59728..32e020523 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -123,6 +123,7 @@ impl VectorcraftApp { updates_rt: Default::default(), frame_rate: Default::default(), canvas_rect: None, + control_rotate_press: None, hover_doc: None, titlebar_from_env: false, custom_titlebar: false, diff --git a/crates/ui-egui/src/app/commands.rs b/crates/ui-egui/src/app/commands.rs index d5ca8a196..9e698b1bf 100644 --- a/crates/ui-egui/src/app/commands.rs +++ b/crates/ui-egui/src/app/commands.rs @@ -404,6 +404,10 @@ impl VectorcraftApp { if let Err(e) = switched { self.ui.status = e.to_string(); } + if before != self.session.tool_id() { + // A control gesture cannot resume after another tool has owned the pointer. + self.control_rotate_press = None; + } // A newly chosen Artboard tool starts on the active artboard (the view's). if before != "artboard" && self.session.tool_id() == "artboard" diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index bb2918d13..4b2b6f867 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -610,9 +610,9 @@ mod tests; /// The window and coordinate mapping where a control-channel Rotate View drag began. #[derive(Clone, Copy)] -struct RotateViewPress { +pub(crate) struct RotateViewPress { window: (u64, u32), - tool_resets: u64, + document_resets: u64, xf: Xf, start_angle: f64, start_rotation: f64, @@ -622,53 +622,102 @@ struct RotateViewPress { /// across requests so document-coordinate drags do not move again as the view turns underneath. fn rotate_view_pointer( app: &mut VectorcraftApp, - ctx: &egui::Context, kind: PointerKind, position: vectorcraft_geom::Point, screen: bool, shift: bool, ) -> Result<(), String> { - let id = egui::Id::unique("control.rotateView"); - match kind { - PointerKind::Up => { - ctx.data_mut(|d| d.remove::(id)); - return Ok(()); - } - PointerKind::Move | PointerKind::DoubleClick => return Ok(()), - _ => {} - } let window = app.session.active().zip(crate::doc_windows::current(app)).map(|(d, w)| (d.uid, w.1)).ok_or("no document")?; - let tool_resets = app.session.tool_resets(); - let mut press = if kind == PointerKind::Down { - let rect = app.canvas_rect.ok_or("no canvas yet")?; - let view = app.view().ok_or("no document")?; - RotateViewPress { window, tool_resets, xf: Xf::new(rect, view), start_angle: 0.0, start_rotation: view.rotation } - } else { - ctx.data(|d| d.get_temp::(id)).ok_or("Rotate View needs a down event before a drag")? + let document_resets = app.session.tool_resets(); + let stored = if kind == PointerKind::Down { None } else { app.control_rotate_press }; + if kind == PointerKind::Drag && stored.is_none() { + return Err("Rotate View needs a down event before a drag".into()); + } + let mut press = match stored { + Some(press) => press, + None => { + let rect = app.canvas_rect.ok_or("no canvas yet")?; + let view = app.view().ok_or("no document")?; + RotateViewPress { window, document_resets, xf: Xf::new(rect, view), start_angle: 0.0, start_rotation: view.rotation } + } }; - // A gesture cannot turn a different document/view or resume after another tool was chosen. - if (press.window, press.tool_resets) != (window, tool_resets) { - ctx.data_mut(|d| d.remove::(id)); - return Err("the Rotate View gesture's window or tool changed".into()); + // A gesture cannot turn a different document/view or resume after its document was reset. + if (press.window, press.document_resets) != (window, document_resets) { + return Err("the Rotate View gesture's window or document changed".into()); } let point = if screen { egui::pos2(position.x as f32, position.y as f32) } else { press.xf.to_screen(position) }; let center = press.xf.rect.center(); if !point.is_finite() || !center.is_finite() { return Err("Rotate View needs finite canvas coordinates".into()); } - if kind == PointerKind::Down { - press.start_angle = (point.y - center.y).atan2(point.x - center.x) as f64; - ctx.data_mut(|d| d.insert_temp(id, press)); - } else { - app.view_mut().ok_or("no document")?.rotation = crate::canvas::drag_rotation(center, point, press.start_angle, press.start_rotation, shift); + match kind { + PointerKind::Down => { + press.start_angle = (point.y - center.y).atan2(point.x - center.x) as f64; + app.control_rotate_press = Some(press); + } + PointerKind::Drag => { + app.view_mut().ok_or("no document")?.rotation = + crate::canvas::drag_rotation(center, point, press.start_angle, press.start_rotation, shift); + } + PointerKind::Up => { + app.control_rotate_press = None; + } + PointerKind::Move | PointerKind::DoubleClick => {} + } + Ok(()) +} + +/// A Rotate View packet accepts only the documented boolean modifier fields. +fn validate_rotate_view_mods(value: Option<&Value>) -> Result<(), String> { + let Some(value) = value else { return Ok(()) }; + let Some(fields) = value.as_object() else { return Err("Rotate View `mods` must be an object of booleans".into()) }; + if fields.iter().any(|(key, value)| !matches!(key.as_str(), "shift" | "alt" | "cmd" | "ctrl" | "space") || !value.is_boolean()) { + return Err("Rotate View `mods` accepts only boolean shift, alt, cmd, ctrl and space".into()); + } + Ok(()) +} + +/// Reject malformed coordinates and modifiers before any event can change the angle or press. +fn validate_rotate_view_events(params: &Value, events: &[Value]) -> Result<(), String> { + validate_rotate_view_mods(params.get("mods"))?; + for event in events { + for axis in ["x", "y"] { + if event.get(axis).and_then(Value::as_f64).filter(|value| value.is_finite()).is_none() { + return Err(format!("Rotate View `{axis}` must be a finite number")); + } + } + validate_rotate_view_mods(event.get("mods"))?; + if event.get("space").is_some_and(|space| !matches!(space.as_str(), Some("doc" | "screen"))) { + return Err("Rotate View `space` must be doc or screen".into()); + } } Ok(()) } /// `ui.pointer`: pointer events through the tools. fn pointer_events(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlRequest) -> Outcome { + let rotating = app.session.tool_id() == "rotateView"; + let before = if rotating { app.view().map(|view| view.rotation) } else { None }; + let outcome = pointer_events_inner(app, ctx, req); + if rotating && matches!(&outcome, Outcome::Done(reply) if reply.get("ok").and_then(Value::as_bool) == Some(false)) { + // A failed packet must neither leave a partial turn nor allow a later drag to resume it. + app.control_rotate_press = None; + if let (Some(before), Some(view)) = (before, app.view_mut()) { + view.rotation = before; + } + ctx.request_repaint(); + } + outcome +} + +/// Apply a pointer packet after Rotate View validation; other tools retain their existing routing. +fn pointer_events_inner(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlRequest) -> Outcome { let p = &req.params; let Some(events) = p.get("events").and_then(Value::as_array) else { return err("missing `events`") }; + let rotating = app.session.tool_id() == "rotateView"; + 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(); let view = app.view_info(); let xf = app.canvas_rect.zip(app.view().copied()).map(|(rect, v)| Xf::new(rect, &v)); @@ -692,8 +741,8 @@ fn pointer_events(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlRe 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 app.session.tool_id() == "rotateView" { - if let Err(e) = rotate_view_pointer(app, ctx, kind, vectorcraft_geom::Point::new(x, y), screen, mods.shift) { + if rotating { + if let Err(e) = rotate_view_pointer(app, kind, vectorcraft_geom::Point::new(x, y), screen, mods.shift) { return err(e); } continue; diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 03acb7597..52ad116ba 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -186,6 +186,8 @@ pub struct VectorcraftApp { frame_rate: frame_rate::FrameRate, /// Canvas rect of the last frame (screen points), for control-channel coordinate mapping. pub canvas_rect: Option, + /// An unfinished control-channel Rotate View gesture, kept only for this app instance. + pub(crate) control_rotate_press: Option, /// Hover position in document coordinates. pub hover_doc: Option, /// System clipboard: SVG to publish next frame, the last SVG we published (so pasting it back diff --git a/crates/ui-egui/src/tests/rotateview.rs b/crates/ui-egui/src/tests/rotateview.rs index a709a767e..296ab7674 100644 --- a/crates/ui-egui/src/tests/rotateview.rs +++ b/crates/ui-egui/src/tests/rotateview.rs @@ -30,6 +30,13 @@ fn with_document() -> VectorcraftApp { app } +/// Send a control request and keep its success/error value for protocol assertions. +fn control_response(app: &mut VectorcraftApp, ctx: &egui::Context, method: &str, params: Value) -> Value { + let (request, _) = crate::control::ControlRequest::new(method, params); + let crate::control::Outcome::Done(reply) = crate::control::handle(app, ctx, &request) else { panic!("not done") }; + reply +} + /// Returns the labels of the View › Rotate View items that are checked. fn checked(app: &VectorcraftApp) -> Vec<&'static str> { ANGLES.iter().filter(|(_, a)| menus::checked(app, "view.rotate", &json!({ "angle": a })) == Some(true)).map(|(l, _)| *l).collect() @@ -69,6 +76,8 @@ fn control_rotate_view_gesture_turns_the_canvas_and_reset_restores_it() { let send = super::synthetic::control; send(&mut app, &ctx, "ui.tool.select", json!({"tool": "rotateView"})); send(&mut app, &ctx, "ui.pointer", json!({"events": [{"kind": "down", "x": 300, "y": 150}]})); + let mut frame = ctx.run_ui(Default::default(), |_| {}); + frame.textures_delta.clear(); send( &mut app, &ctx, @@ -83,10 +92,13 @@ fn control_rotate_view_gesture_turns_the_canvas_and_reset_restores_it() { assert_eq!(view.center, vectorcraft_geom::Point::new(200.0, 150.0)); assert_eq!(view.zoom, 1.0); assert_eq!(app.session.active().unwrap().revision, revision, "navigation does not edit the document"); - send(&mut app, &ctx, "engine.execute", json!({"command": "view.rotateReset", "params": {}})); + let reset = control_response(&mut app, &ctx, "engine.execute", json!({"command": "view.rotateReset", "params": {}})); + assert_eq!(reset["ok"], true); + assert!(reset["result"].is_null()); assert_eq!(app.view().unwrap().rotation, 0.0); } +/// Repeated document points stay fixed across requests, including Shift snapping from a turned view. #[test] fn control_rotate_view_keeps_press_coordinates_and_shift_snapping_across_requests() { let mut app = with_document(); @@ -109,6 +121,7 @@ fn control_rotate_view_keeps_press_coordinates_and_shift_snapping_across_request assert_eq!(app.view().unwrap().rotation, 45.0); } +/// Invalid coordinates and document changes cannot turn a different view. #[test] fn control_rotate_view_rejects_nonfinite_positions_and_a_changed_document() { let mut app = with_document(); @@ -128,6 +141,78 @@ fn control_rotate_view_rejects_nonfinite_positions_and_a_changed_document() { assert_eq!(app.view().unwrap().rotation, 0.0); } +/// An invalid packet leaves the angle unchanged and requires a fresh press, including errors +/// after an otherwise valid drag and malformed releases or hovers. +#[test] +fn control_rotate_view_rejects_malformed_packets_without_leaving_a_press() { + let drag = json!({"kind": "drag", "x": 200, "y": 250}); + for packet in [ + json!({"events": [drag.clone(), {"kind": "drag", "y": 250}]}), + json!({"events": [drag.clone(), {"kind": "drag", "x": "200", "y": 250}]}), + json!({"events": [drag.clone(), {"kind": "drag", "x": 200, "y": 250, "mods": {"shift": "yes"}}]}), + json!({"mods": "shift", "events": [drag.clone()]}), + json!({"events": [drag.clone(), {"kind": "drag", "x": 200, "y": 250, "mods": {"unknown": true}}]}), + json!({"events": [drag.clone(), {"kind": "drag", "x": 1e300, "y": 250}]}), + json!({"events": [drag.clone(), {"kind": "up", "x": 1e300, "y": 250}]}), + json!({"events": [drag.clone(), {"kind": "move", "x": 1e300, "y": 250}]}), + json!({"events": [drag.clone(), {"kind": "doubleclick", "x": 1e300, "y": 250}]}), + json!({"events": [drag.clone(), {"kind": "wiggle", "x": 200, "y": 250}]}), + json!({"events": [drag.clone(), {"kind": "drag", "x": 200, "y": 250, "space": "window"}]}), + json!({"events": null}), + ] { + let mut app = with_document(); + let ctx = egui::Context::default(); + app.canvas_rect = Some(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(800.0, 600.0))); + let view = app.view_mut().unwrap(); + view.center = vectorcraft_geom::Point::new(200.0, 150.0); + view.zoom = 1.0; + super::synthetic::control(&mut app, &ctx, "ui.tool.select", json!({"tool": "rotateView"})); + super::synthetic::control(&mut app, &ctx, "ui.pointer", json!({"events": [{"kind": "down", "x": 300, "y": 150}]})); + let reply = control_response(&mut app, &ctx, "ui.pointer", packet.clone()); + assert_eq!(reply["ok"], false, "{packet}: {reply}"); + assert_eq!(app.view().unwrap().rotation, 0.0, "{packet}"); + let resume = control_response(&mut app, &ctx, "ui.pointer", json!({"events": [drag.clone()]})); + assert_eq!(resume["ok"], false, "a failed packet cancels its press: {packet}"); + assert_eq!(app.view().unwrap().rotation, 0.0); + } +} + +/// Screen-space gestures survive frames, stop on release or a tool change, and start afresh +/// from the current angle. A press at the pivot uses the same finite convention as the mouse. +#[test] +fn control_rotate_view_screen_events_require_a_live_press() { + let mut app = with_document(); + let ctx = egui::Context::default(); + app.canvas_rect = Some(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(800.0, 600.0))); + let view = app.view_mut().unwrap(); + view.center = vectorcraft_geom::Point::new(1200.0, 900.0); + view.zoom = 2.0; + let send = super::synthetic::control; + send(&mut app, &ctx, "ui.tool.select", json!({"tool": "rotateView"})); + let down = json!({"events": [{"kind": "down", "x": 500, "y": 300, "space": "screen"}]}); + let drag = json!({"events": [{"kind": "drag", "x": 400, "y": 400, "space": "screen"}]}); + assert_eq!(control_response(&mut app, &ctx, "ui.pointer", drag.clone())["ok"], false); + send(&mut app, &ctx, "ui.pointer", down.clone()); + let mut frame = ctx.run_ui(Default::default(), |_| {}); + frame.textures_delta.clear(); + send(&mut app, &ctx, "ui.pointer", drag.clone()); + assert!((app.view().unwrap().rotation - 90.0).abs() < 1e-3); + send(&mut app, &ctx, "ui.pointer", json!({"events": [{"kind": "up", "x": 400, "y": 400, "space": "screen"}]})); + assert_eq!(control_response(&mut app, &ctx, "ui.pointer", drag.clone())["ok"], false, "release ends the press"); + send(&mut app, &ctx, "ui.pointer", down.clone()); + send(&mut app, &ctx, "ui.tool.select", json!({"tool": "selection"})); + send(&mut app, &ctx, "ui.tool.select", json!({"tool": "rotateView"})); + assert_eq!(control_response(&mut app, &ctx, "ui.pointer", drag.clone())["ok"], false, "a tool change ends the press"); + send(&mut app, &ctx, "ui.pointer", down); + send(&mut app, &ctx, "ui.pointer", drag); + assert!((app.view().unwrap().rotation.abs() - 180.0).abs() < 1e-3, "a new press starts at the current angle"); + let before = app.view().unwrap().rotation; + send(&mut app, &ctx, "ui.pointer", json!({"events": [{"kind": "down", "x": 400, "y": 300, "space": "screen"}]})); + send(&mut app, &ctx, "ui.pointer", json!({"events": [{"kind": "drag", "x": 500, "y": 300, "space": "screen"}]})); + assert!(app.view().unwrap().rotation.is_finite()); + assert!((app.view().unwrap().rotation - before).abs() < 1e-3); +} + #[test] fn the_view_menu_lists_the_status_bar_angles_and_checks_the_current_one() { let mut app = with_document(); diff --git a/docs/mcp.md b/docs/mcp.md index d8a5f3759..28c62b279 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -4014,6 +4014,8 @@ angle it set, normalized to (-180, 180], and `view.rotateReset` turns the view b Select the Rotate View Tool with `select_tool` and the tool id `rotateView`. Its `pointer_gesture` drags turn the canvas about its center; Shift snaps to 15°. Document coordinates for a rotation gesture use the view at the initial down event, including when the gesture spans several calls. +The control channel rejects malformed rotation packets without changing the angle and cancels +the current press. Start a new down event after an error. Reset Rotate View is the command `view.rotateReset`: ```json