Merge pull request #5772 from dotCooCoo/fix/discarded-batch-cancels-pointer

MCP: pointer_gesture fails when the input sent before it is dropped

crates/ui-egui/src/app.rs

@@ -105,6 +105,7 @@ impl VectorcraftApp {
             pending_url: None,
             control_rx: None,
             deferred_control: None,
+            synthetic_dropped_before: 0,
             pending_screenshots: vec![],
             queued_screenshots: vec![],
             screenshot_token: 0,

crates/ui-egui/src/app/control.rs

@@ -10,6 +10,18 @@ impl VectorcraftApp {
         let mut handled = 0;
         while handled < control::MAX_CONTROL_REQUESTS_PER_FRAME {
             let Some(req) = self.deferred_control.take().or_else(|| rx.try_recv().ok()) else { break };
+            // Synthetic input sent before this pointer request was dropped (see
+            // `discard_stale_injected_batch`): its events would reach the tool without that input.
+            if req.method == "ui.pointer" && req.made_before(self.synthetic_dropped_before) {
+                handled += 1;
+                if req.claim() {
+                    let _ = req.reply.send(json!({
+                        "ok": false,
+                        "error": "input sent before this pointer request was dropped because the document, dialog or focus changed"
+                    }));
+                }
+                continue;
+            }
             // `ui.pointer` drives the tool at once, while `ui.key` and the other synthetic input
             // reach it in a later UI pass: a pointer request waits until the input sent before it
             // was applied, so a key pressed during a drag changes the drag before its release.

crates/ui-egui/src/app/input.rs

@@ -123,6 +123,9 @@ impl VectorcraftApp {
         self.synthetic_modifiers = false;
         self.synthetic_injected = false;
         self.synthetic_frame = false;
+        // The `ui.pointer` requests made after this input, waiting or still on the channel, would
+        // run without it: `drain_control` answers the ones made before now with an error.
+        self.synthetic_dropped_before = crate::control::ControlRequest::next_seq();
     }
 
     /// Release the pointer buttons egui still holds down when the system reports that no mouse

crates/ui-egui/src/control.rs

@@ -34,7 +34,7 @@
 //!   Background Save or Background Export on. A `wait` other than true, false or null is refused
 
 use std::sync::Arc;
-use std::sync::atomic::{AtomicU8, Ordering};
+use std::sync::atomic::{AtomicU8, AtomicU64, Ordering};
 use std::sync::mpsc::Sender;
 
 use serde_json::{Value, json};
@@ -101,8 +101,13 @@ pub struct ControlRequest {
     pub params: Value,
     pub reply: Sender<ControlResponse>,
     state: Arc<AtomicU8>,
+    /// Its place in the order requests were made ([`ControlRequest::next_seq`]).
+    seq: u64,
 }
 
+/// The sequence number of the next request made.
+static NEXT_SEQ: AtomicU64 = AtomicU64::new(0);
+
 /// A request's state: waiting to run, taken by the app ([`ControlRequest::claim`]), or given up
 /// by its caller before that ([`Ticket::give_up`]). It leaves `QUEUED` once, one way or the other.
 const QUEUED: u8 = 0;
@@ -124,7 +129,18 @@ impl Ticket {
 impl ControlRequest {
     pub fn new(method: impl Into<String>, params: Value) -> (Self, std::sync::mpsc::Receiver<ControlResponse>) {
         let (tx, rx) = std::sync::mpsc::channel();
-        (Self { method: method.into(), params, reply: tx, state: Arc::new(AtomicU8::new(QUEUED)) }, rx)
+        let seq = NEXT_SEQ.fetch_add(1, Ordering::SeqCst);
+        (Self { method: method.into(), params, reply: tx, state: Arc::new(AtomicU8::new(QUEUED)), seq }, rx)
+    }
+
+    /// The sequence number the next request made gets: every request made so far is before it.
+    pub fn next_seq() -> u64 {
+        NEXT_SEQ.load(Ordering::SeqCst)
+    }
+
+    /// Whether this request was made before the one numbered `seq`.
+    pub fn made_before(&self, seq: u64) -> bool {
+        self.seq < seq
     }
 
     /// The caller's handle on this request, to give it up with.

crates/ui-egui/src/lib.rs

@@ -154,6 +154,10 @@ pub struct VectorcraftApp {
     /// A `ui.pointer` request taken from the channel while synthetic input sent before it was not
     /// yet applied: it runs, ahead of the channel, on the first frame after that input is applied.
     deferred_control: Option<ControlRequest>,
+    /// When synthetic input was last dropped before a UI pass took it, the sequence number of the
+    /// next control request ([`ControlRequest::next_seq`]): a `ui.pointer` request made before it
+    /// came after that input and is answered with an error.
+    synthetic_dropped_before: u64,
     /// Window captures asked for and not answered yet.
     pending_screenshots: Vec<PendingScreenshot>,
     /// Admitted captures whose settled frame has not come yet (issued one at a time).

crates/ui-egui/src/tests/synthetic.rs

@@ -142,6 +142,62 @@ fn a_given_up_pointer_does_not_hold_back_later_requests() {
     assert!(up_reply.try_recv().is_err(), "the given-up pointer request did not run");
 }
 
+/// A `ui.pointer` release sent after a `ui.key` fails when the key's batch is discarded because a
+/// dialog opened before a UI pass took it, whether the release was waiting for the key
+/// (`waiting`) or still on the channel. A release made after the discard runs.
+fn a_release_after_a_discarded_key(waiting: bool) {
+    let (tx, rx) = std::sync::mpsc::channel();
+    let mut app = VectorcraftApp::new(Session::new(), Default::default()).with_control(rx);
+    app.run("file.new", json!({"width": 300, "height": 300})).unwrap();
+    app.run("tool.select", json!({"tool": "arc"})).unwrap();
+    let ctx = egui::Context::default();
+    control(&mut app, &ctx, "ui.pointer", json!({"events": [{"kind": "down", "x": 100, "y": 100}, {"kind": "drag", "x": 200, "y": 180}]}));
+    let (key, _key_reply) = ControlRequest::new("ui.key", json!({"key": "c"}));
+    let (up, up_reply) = ControlRequest::new("ui.pointer", json!({"events": [{"kind": "up", "x": 200, "y": 180}]}));
+    tx.send(key).unwrap();
+    let mut up = Some(up);
+    if waiting {
+        tx.send(up.take().unwrap()).unwrap();
+    }
+    // The first frame queues the key; the second injects it.
+    for _ in 0..2 {
+        let mut raw = egui::RawInput::default();
+        app.raw_input_hook(&ctx, &mut raw);
+        app.drain_control(&ctx);
+        assert!(up_reply.try_recv().is_err(), "the release waits for the key");
+    }
+    assert!(app.synthetic_injected, "the key is in the frame's input");
+    if let Some(up) = up {
+        tx.send(up).unwrap();
+    }
+    // A dialog opens before a UI pass takes the key, so the next frame discards it.
+    app.ui.dialog = Some(crate::state::Dialog::new(crate::dialogs::save_options::KIND, json!({})));
+    let mut raw = egui::RawInput::default();
+    app.raw_input_hook(&ctx, &mut raw);
+    // A release made after the key was discarded, queued behind the first one, runs.
+    let (later, later_reply) = ControlRequest::new("ui.pointer", json!({"events": [{"kind": "up", "x": 200, "y": 180}]}));
+    tx.send(later).unwrap();
+    app.drain_control(&ctx);
+    let r = up_reply.try_recv().expect("the release was answered");
+    assert_eq!(r["ok"], false, "{r}");
+    let r = later_reply.try_recv().expect("the later release was answered");
+    assert_eq!(r["ok"], true, "{r}");
+    let document = app.session.active().unwrap();
+    let arc = *document.selection.objects.last().expect("the arc drawn on the later release");
+    let closed = document.doc.node(arc).unwrap().path_data().unwrap().subpaths[0].closed;
+    assert!(!closed, "the discarded C did not reach the Arc tool");
+}
+
+#[test]
+fn a_waiting_pointer_release_fails_when_the_key_before_it_is_discarded() {
+    a_release_after_a_discarded_key(true);
+}
+
+#[test]
+fn a_queued_pointer_release_fails_when_the_key_before_it_is_discarded() {
+    a_release_after_a_discarded_key(false);
+}
+
 #[test]
 fn a_drag_holds_its_modifiers_from_the_press_to_the_release() {
     let (mut app, _) = one_square();

docs/mcp.md

@@ -427,7 +427,7 @@ opens the picker through `run_command` with the remote backend.
 | `inspect_ui` | `{}` | UI state. Remote mode only. |
 | `select_tool` | `{tool}` | `selection`, `directSelection`, `pen`, `rectangle`, `ellipse`, `polygon`, `star`, `lineSegment`, … |
 | `pointer_gesture` | `{events:[{kind,x,y,mods?,backquote?,pressure?,tiltX?,tiltY?,rotation?,holdMs?}], tool?, mods?, backquote?}` | `kind` is one of `down`, `drag`, `up`, `move`, `doubleclick`. `backquote` is the held state for each event; the top-level value supplies its default, and a `backquote` that is not a boolean is an error. Events go through the same path as the mouse. Headless, a `move` replies after the tool's highlight under the pointer is ready, including the Shape Builder's and the Live Paint Bucket's regions of a large selection, which the desktop app finds off the UI thread and highlights after the reply. |
-| `pointer_gesture` | `{events:[{kind,x,y,mods?,backquote?,pressure?,tiltX?,tiltY?,rotation?,holdMs?}], tool?, mods?, backquote?}` | `kind` is one of `down`, `drag`, `up`, `move`, `doubleclick`. `backquote` is the held state for each event; the top-level value supplies its default. Events go through the same path as the mouse. Headless, a `move` replies after the tool's highlight under the pointer is ready, including the Shape Builder's and the Live Paint Bucket's regions of a large selection, which the desktop app finds off the UI thread and highlights after the reply. Remote, the events wait until the keys and other input sent before them (`press_key`, `type_text`) have reached the app, so a key pressed between two calls of one drag applies before the release. |
+| `pointer_gesture` | `{events:[{kind,x,y,mods?,backquote?,pressure?,tiltX?,tiltY?,rotation?,holdMs?}], tool?, mods?, backquote?}` | `kind` is one of `down`, `drag`, `up`, `move`, `doubleclick`. `backquote` is the held state for each event; the top-level value supplies its default. Events go through the same path as the mouse. Headless, a `move` replies after the tool's highlight under the pointer is ready, including the Shape Builder's and the Live Paint Bucket's regions of a large selection, which the desktop app finds off the UI thread and highlights after the reply. Remote, the events wait until the keys and other input sent before them (`press_key`, `type_text`) have reached the app, so a key pressed between two calls of one drag applies before the release. When that input is dropped because the document, a dialog or the focus changed first, the call fails and its events are not sent. |
 | `draw_path` | `{points \| d, closed?, fill?, stroke?, strokeWidth?, dash?, cap?, join?, opacity?, name?, effects?}` | `points` is `[[x,y],…]` or `[{x,y,in?,out?,smooth?},…]`. `d` is SVG path data. The options are below. |
 | `draw_shape` | `{shape, …geometry, fill?, stroke?, strokeWidth?, dash?, cap?, join?, opacity?, name?, effects?}` | `rectangle`/`ellipse`: `x,y,width,height` (plus `radius` for corners). `polygon`: `cx,cy,radius,sides`. `star`: `cx,cy,radius1,radius2,points`. `line`: `x1,y1,x2,y2`. |
 | `set_paint` | `{fill?, stroke?, strokeWidth?, ids?}` | Applies to the selection (or `ids`) and becomes the default for new art. Returns `{applied, objects}`: `objects` is the number of objects painted, as the `objects` of the `paint.setFill`, `paint.setStroke` and `stroke.set` replies (the largest of them): a selected group counts the objects inside it. When it is 0, a `note` says that only the paint for new art changed. |