From 8b1e20fd8fd2ada3c15692963b6f49e606f9e000 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Wed, 4 Nov 2026 15:40:44 +0000 Subject: [PATCH 1/2] Apply review fixes - crates/ui-egui/src/app/input.rs - crates/ui-egui/src/services.rs --- crates/ui-egui/src/app/input.rs | 1 + crates/ui-egui/src/services.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/app/input.rs b/crates/ui-egui/src/app/input.rs index 2682478c4..1972faf03 100644 --- a/crates/ui-egui/src/app/input.rs +++ b/crates/ui-egui/src/app/input.rs @@ -166,6 +166,7 @@ impl DesignApp { /// Where the pointer is in the window, asked of the system ([`crate::Services::pointer`]), /// for files dropped without pointer events: dragged in from another application, they bring /// none. `None` when the system doesn't say or the window's place isn't known. + #[cfg(not(target_arch = "wasm32"))] pub(crate) fn system_pointer(&self, ctx: &egui::Context) -> Option { let at = (self.services.pointer.as_ref()?)()?; // The system's points are unscaled; egui's (the window's rect too) are divided by the diff --git a/crates/ui-egui/src/services.rs b/crates/ui-egui/src/services.rs index 107e9e775..384a75b4b 100644 --- a/crates/ui-egui/src/services.rs +++ b/crates/ui-egui/src/services.rs @@ -47,6 +47,6 @@ pub struct Services { /// The pointer's place on the screen, asked when a file is dropped: a file dragged in from /// another application brings no pointer events with it (winit reports none during a drag on /// macOS), so without it a drop can't tell which frame it landed on. `None`: a drop without - /// pointer events loads the place cursor. + /// pointer events lands in the middle of the view. pub pointer: Option, } From f02e13318b0c870e09f4c791047e69db7681f2b1 Mon Sep 17 00:00:00 2001 From: texjer Date: Wed, 4 Nov 2026 11:02:26 -0500 Subject: [PATCH 2/2] Place: a drop without pointer events keeps the system pointer of its own input Review fixes for #2304: - Each pointer-less drop asks the system where the pointer is when its raw input arrives, and converts it with that input's window rect and the UI scale, instead of one sample taken at the later UI pass for the whole batch. Drops with pointer events keep their own pointer, and the system is still only asked when a drop brought none. - system_pointer is cfg'd off on wasm with its only caller; Services::pointer's docs say a drop without a pointer lands in the middle of the view. - pointer.rs: the AppKit flip is a pure function with a test, documenting why it flips against the main display only (winit's window positions use the same origin). - Test: a filled compound path takes a dropped image through its ring, not its hole. --- apps/designcraft/src/pointer.rs | 29 +++++++++++- crates/ui-egui/src/app.rs | 2 +- crates/ui-egui/src/app/input.rs | 46 ++++++++++--------- crates/ui-egui/src/lib.rs | 4 ++ crates/ui-egui/src/services.rs | 2 +- crates/ui-egui/src/tests.rs | 78 ++++++++++++++++++++++++++++++--- 6 files changed, 132 insertions(+), 29 deletions(-) diff --git a/apps/designcraft/src/pointer.rs b/apps/designcraft/src/pointer.rs index 14bf9599a..3725cfbaa 100644 --- a/apps/designcraft/src/pointer.rs +++ b/apps/designcraft/src/pointer.rs @@ -11,6 +11,33 @@ pub fn screen_position() -> Option { // AppKit's origin is the bottom left of the first screen, the one with the menu bar. let height = NSScreen::screens(mtm).firstObject()?.frame().size.height; let at = NSEvent::mouseLocation(); - let (x, y) = (at.x, height - at.y); + from_appkit(at.x, at.y, height) +} + +/// A point in AppKit's global coordinates (origin at the bottom left of the main display, the one +/// with the menu bar, y up) in winit's (origin at its top left, y down), given that display's +/// height. Only the main display's height: winit flips window positions the same way +/// (`flip_window_screen_coordinates`, against `CGMainDisplayID`), whatever display the window is +/// on, so a display left of or above the main one gives negative coordinates in both. +fn from_appkit(x: f64, y: f64, main_height: f64) -> Option { + let y = main_height - y; (x.is_finite() && y.is_finite()).then(|| egui::pos2(x as f32, y as f32)) } + +#[cfg(test)] +mod tests { + use super::from_appkit; + + #[test] + fn appkit_points_flip_against_the_main_display_only() { + // The main display is 1000 points high: its top left is (0, 0), its bottom left (0, 1000). + assert_eq!(from_appkit(0.0, 1000.0, 1000.0), Some(egui::pos2(0.0, 0.0))); + assert_eq!(from_appkit(200.0, 100.0, 1000.0), Some(egui::pos2(200.0, 900.0))); + // A second display above the main one, and one to its left: negative, as winit reports a + // window there. + assert_eq!(from_appkit(300.0, 1200.0, 1000.0), Some(egui::pos2(300.0, -200.0))); + assert_eq!(from_appkit(-800.0, 500.0, 1000.0), Some(egui::pos2(-800.0, 500.0))); + assert_eq!(from_appkit(f64::NAN, 0.0, 1000.0), None); + assert_eq!(from_appkit(0.0, 0.0, f64::INFINITY), None); + } +} diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index b2b433b71..55a257dd3 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -206,7 +206,7 @@ impl DesignApp { // must not replay shortcuts or dropped files from stale input. if ctx.current_pass_index() == 0 { #[cfg(not(target_arch = "wasm32"))] - self.take_pending_drops(&ctx); + self.take_pending_drops(); menus::shortcuts(self, &ctx); } let t0 = now_ms(); diff --git a/crates/ui-egui/src/app/input.rs b/crates/ui-egui/src/app/input.rs index 2682478c4..27049a1a7 100644 --- a/crates/ui-egui/src/app/input.rs +++ b/crates/ui-egui/src/app/input.rs @@ -59,10 +59,13 @@ impl DesignApp { _ => None, }); let generation = self.input_generation(); + // A drop without pointer events (dragged in from another application) asks the + // system now, while the pointer is still where the files were let go. + let system = if pointer.is_none() && !raw.dropped_files.is_empty() { self.system_pointer(ctx, raw) } else { None }; for file in std::mem::take(&mut raw.dropped_files) { let path = file.path().to_string_lossy().to_string(); if !path.is_empty() { - self.pending_drops.push(PendingDrop { path, pointer, generation }); + self.pending_drops.push(PendingDrop { path, pointer, system, generation }); } } } @@ -144,37 +147,29 @@ impl DesignApp { /// Open or place the files dropped since the last UI pass, at the pointer. Files dropped /// without pointer events (dragged in from another application) are placed where the system - /// says the pointer is, so one dropped on a frame still fills it. + /// said the pointer was when they arrived, so one dropped on a frame still fills it. #[cfg(not(target_arch = "wasm32"))] - pub(crate) fn take_pending_drops(&mut self, ctx: &egui::Context) { + pub(crate) fn take_pending_drops(&mut self) { // Sample the generation once for the whole batch: placing the first valid file must not // suppress later files that belong to the same captured batch. let current = self.input_generation(); - let drops: Vec<_> = std::mem::take(&mut self.pending_drops).into_iter().filter(|d| d.generation == current).collect(); - if drops.is_empty() { - return; - } - // Each file keeps the pointer it was dropped at; the system is asked once, for those - // that came without one. - let system = drops.iter().any(|d| d.pointer.is_none()).then(|| self.system_pointer(ctx)).flatten(); let mut frame_free = true; - for drop in drops { - let _ = self.place_dropped(&drop.path, drop.pointer.or(system), &mut frame_free); + for drop in std::mem::take(&mut self.pending_drops) { + if drop.generation == current { + let _ = self.place_dropped(&drop.path, drop.pointer.or(drop.system), &mut frame_free); + } } } /// Where the pointer is in the window, asked of the system ([`crate::Services::pointer`]), /// for files dropped without pointer events: dragged in from another application, they bring - /// none. `None` when the system doesn't say or the window's place isn't known. - pub(crate) fn system_pointer(&self, ctx: &egui::Context) -> Option { + /// none. Converted with the window's place in `raw` (that input's own viewport info, else the + /// last pass's). `None` when the system doesn't say or the window's place isn't known. + #[cfg(not(target_arch = "wasm32"))] + fn system_pointer(&self, ctx: &egui::Context, raw: &egui::RawInput) -> Option { let at = (self.services.pointer.as_ref()?)()?; - // The system's points are unscaled; egui's (the window's rect too) are divided by the - // UI scale. - let zoom = ctx.zoom_factor(); - let at = if zoom.is_finite() && zoom > 0.0 { at / zoom } else { at }; - let inner = ctx.input(|i| i.viewport().inner_rect)?; - let local = at - inner.min.to_vec2(); - (local.x.is_finite() && local.y.is_finite()).then_some(local) + let inner = raw.viewports.get(&raw.viewport_id).and_then(|v| v.inner_rect).or_else(|| ctx.input(|i| i.viewport().inner_rect))?; + screen_to_window(at, ctx.zoom_factor(), inner) } /// The current state an injected batch belongs to. @@ -224,3 +219,12 @@ impl DesignApp { self.control_input_frame.store(false, std::sync::atomic::Ordering::Relaxed); } } + +/// A point on the screen in the system's unscaled points, in the window whose content is `inner` +/// (egui's points, divided by the UI scale `zoom`). `None` when the result isn't finite. +#[cfg(not(target_arch = "wasm32"))] +fn screen_to_window(at: egui::Pos2, zoom: f32, inner: egui::Rect) -> Option { + let at = if zoom.is_finite() && zoom > 0.0 { at / zoom } else { at }; + let local = at - inner.min.to_vec2(); + (local.x.is_finite() && local.y.is_finite()).then_some(local) +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index c780d632a..531a8dbe7 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -111,6 +111,10 @@ struct InputGeneration { struct PendingDrop { path: String, pointer: Option, + /// Where the system said the pointer was in the window when this drop's raw input arrived, + /// for a drop that brought no pointer events ([`crate::Services::pointer`]). Each drop keeps + /// its own, converted with the window's place and the UI scale of that input. + system: Option, generation: InputGeneration, } diff --git a/crates/ui-egui/src/services.rs b/crates/ui-egui/src/services.rs index 107e9e775..32d7e4599 100644 --- a/crates/ui-egui/src/services.rs +++ b/crates/ui-egui/src/services.rs @@ -47,6 +47,6 @@ pub struct Services { /// The pointer's place on the screen, asked when a file is dropped: a file dragged in from /// another application brings no pointer events with it (winit reports none during a drag on /// macOS), so without it a drop can't tell which frame it landed on. `None`: a drop without - /// pointer events loads the place cursor. + /// pointer events lands in the middle of the view (`canvas::drop_point`), filling no frame. pub pointer: Option, } diff --git a/crates/ui-egui/src/tests.rs b/crates/ui-egui/src/tests.rs index e53e603a1..46c3ce3e2 100644 --- a/crates/ui-egui/src/tests.rs +++ b/crates/ui-egui/src/tests.rs @@ -899,6 +899,15 @@ fn a_graphic_dropped_on_a_frame_fills_it() { assert!(matches!(d.item(designcraft_doc::ItemId(frame)).unwrap().content, designcraft_doc::Content::Graphic(_))); } +/// The raw input of a drop without pointer events, with the window's content at `inner`. +#[cfg(not(target_arch = "wasm32"))] +fn pointerless_drop(path: &std::path::Path, inner: egui::Rect) -> egui::RawInput { + let mut raw = egui::RawInput { screen_rect: Some(egui::Rect::from_min_size(egui::Pos2::ZERO, inner.size())), ..Default::default() }; + raw.viewports.insert(egui::ViewportId::ROOT, egui::ViewportInfo { inner_rect: Some(inner), ..Default::default() }); + raw.dropped_files.push(dropped_file(path)); + raw +} + /// A file dragged in from the Finder brings no pointer events (winit sends none during a drag on /// macOS): the drop asks the system where the pointer is, so one dropped on a frame fills it. #[cfg(not(target_arch = "wasm32"))] @@ -911,13 +920,13 @@ fn a_file_dropped_without_pointer_events_fills_the_frame_under_the_system_pointe let window = egui::vec2(300.0, 200.0); app.services.pointer = Some(Box::new(move || Some(((inside + window).to_vec2() * zoom).to_pos2()))); let path = dropped_png("drop-system-pointer"); - app.pending_drops.push(crate::PendingDrop { path: path.to_string_lossy().into_owned(), pointer: None, generation: app.input_generation() }); let ctx = egui::Context::default(); ctx.set_zoom_factor(zoom); - let rect = egui::Rect::from_min_size(egui::Pos2::ZERO + window, egui::vec2(1400.0, 900.0)); - let mut raw = egui::RawInput { screen_rect: Some(egui::Rect::from_min_size(egui::Pos2::ZERO, rect.size())), ..Default::default() }; - raw.viewports.insert(egui::ViewportId::ROOT, egui::ViewportInfo { inner_rect: Some(rect), ..Default::default() }); - ctx.run_ui(raw, |ui| app.take_pending_drops(ui.ctx())).textures_delta.clear(); + // The zoom takes effect from the next pass, as it has by the time the system drops a file. + ctx.run_ui(egui::RawInput::default(), |_| {}).textures_delta.clear(); + let mut raw = pointerless_drop(&path, egui::Rect::from_min_size(egui::Pos2::ZERO + window, egui::vec2(1400.0, 900.0))); + app.raw_input_hook(&ctx, &mut raw); + ctx.run_ui(raw, |_| app.take_pending_drops()).textures_delta.clear(); let _ = std::fs::remove_file(&path); assert!(app.pending_drops.is_empty()); let d = &app.session.doc().unwrap().doc; @@ -929,6 +938,65 @@ fn a_file_dropped_without_pointer_events_fills_the_frame_under_the_system_pointe } } +/// A drop without pointer events keeps the place the system gave when its input arrived, +/// converted with that input's window: the pointer and the window moving before the UI pass that +/// places it don't move the drop. +#[cfg(not(target_arch = "wasm32"))] +#[test] +fn a_pointerless_drop_keeps_the_system_pointer_of_its_own_input() { + let (mut app, frame, inside, outside) = app_with_drop_frame(); + let at = std::rc::Rc::new(std::cell::Cell::new(inside)); + let reported = at.clone(); + app.services.pointer = Some(Box::new(move || Some(reported.get()))); + let ctx = egui::Context::default(); + let window = egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(1400.0, 900.0)); + let path = dropped_png("drop-own-input"); + let mut raw = pointerless_drop(&path, window); + app.raw_input_hook(&ctx, &mut raw); + // Before the pass that places it, the window moves and the pointer leaves the frame; a later + // input with no drop doesn't ask the system again. + let moved = window.translate(egui::vec2(50.0, 40.0)); + at.set(outside + moved.min.to_vec2()); + let mut later = egui::RawInput { screen_rect: Some(egui::Rect::from_min_size(egui::Pos2::ZERO, moved.size())), ..Default::default() }; + later.viewports.insert(egui::ViewportId::ROOT, egui::ViewportInfo { inner_rect: Some(moved), ..Default::default() }); + app.raw_input_hook(&ctx, &mut later); + ctx.run_ui(later, |_| app.take_pending_drops()).textures_delta.clear(); + let _ = std::fs::remove_file(&path); + let d = &app.session.doc().unwrap().doc; + assert_eq!(d.spreads[0].items.len(), 1, "no new object"); + let designcraft_doc::Content::Graphic(g) = &d.item(designcraft_doc::ItemId(frame)).unwrap().content else { + panic!("the file fills the frame it was dropped on") + }; + assert!(d.assets[&g.asset].name.contains("drop-own-input")); +} + +/// A filled compound path (Object > Compound Paths > Make) is a frame a dropped image fills, as +/// in InDesign, through its ring and not its hole. +#[test] +fn a_graphic_dropped_on_a_compound_path_fills_it() { + let (mut app, frame, _, _) = app_with_drop_frame(); + let hole = app.run("frame.create", json!({"rect": [66, 66, 106, 106]})).unwrap()["id"].as_u64().unwrap(); + assert_eq!(app.run("object.makeCompoundPath", json!({"ids": [frame, hole]})).unwrap()["id"].as_u64(), Some(frame)); + app.run("object.fill", json!({"ids": [frame], "swatch": "[Black]"})).unwrap(); + let (view, rect) = (app.view().unwrap(), app.canvas_rect.unwrap()); + let st = app.session.active().unwrap(); + let layout = designcraft_tools::CanvasLayout::new(&st.doc, false); + let slot = layout.slot(designcraft_doc::SpreadRef::Doc(0)).unwrap(); + let screen = |x, y| canvas::Xf::new(rect, view).to_screen(slot.to_canvas(designcraft_geom::Point::new(x, y))); + // On the ring, between the outer edge (36) and the hole (66); the hole's middle is no frame. + let ring = screen(50.0, 50.0); + assert!(canvas::drop_frame(&app, Some(screen(86.0, 86.0))).is_none(), "the hole"); + let path = dropped_png("drop-compound"); + let r = app.open_dropped_at(&path.to_string_lossy(), Some(ring)); + let _ = std::fs::remove_file(&path); + assert_eq!(r.unwrap()["id"].as_u64(), Some(frame), "the compound path under the pointer is filled"); + let d = &app.session.doc().unwrap().doc; + assert_eq!(d.spreads[0].items.len(), 1, "no new object"); + let it = d.item(designcraft_doc::ItemId(frame)).unwrap(); + assert!(matches!(it.content, designcraft_doc::Content::Graphic(_))); + assert_eq!(it.path.subpaths.len(), 2, "it keeps its hole"); +} + #[test] fn a_locked_frame_is_not_a_drop_target() { let (mut app, frame, inside, _) = app_with_drop_frame();