Rework DnD redux #4079

valadaptive · 2025-01-08T16:52:50Z

This picks up from #2615, rebasing it atop the latest winit version and fixing a couple coordinate-space bugs. Part of #720.

Resolves #1550. Closes #2615.

Tested on all platforms changed
- Windows (low- and high-DPI)
- macOS (low- and high-DPI)
- Linux X11 (low- and high-DPI)
Added an entry to the changelog module if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
Created or updated an example program if it would help users understand this functionality

kchibisov

Will leave macOS re-review up to @madsmtm .

kchibisov · 2025-01-10T15:12:44Z

src/event.rs

+    },
+    /// The drag operation has been cancelled or left the window. On some platforms, this may occur
+    /// even if no other drag events have occurred.
+    DragLeave,


We should add a position: Option<PhysicalPosition<f64>> to follow the pointer api.

Added position.

kchibisov · 2025-01-10T15:13:32Z

src/changelog/unreleased.md

+
+  The `WindowEvent::DroppedFile`, `WindowEvent::HoveredFile` and `WindowEvent::HoveredFileCancelled`
+  events have been removed, and replaced with `WindowEvent::DragEnter`, `WindowEvent::DragOver`,
+  `WindowEvent::DragDrop` and `WindowEvent::DragLeave`.
+
+  The old drag-and-drop events were emitted once per file. This occurred when files were *first*
+  hovered over the window, dropped, or left the window. The new drag-and-drop events are emitted
+  once per set of files dragged, and include a list of all dragged files. They also include the
+  pointer position.
+
+  The rough correspondence is:
+  - `WindowEvent::HoveredFile` -> `WindowEvent::DragEnter`
+  - `WindowEvent::DroppedFile` -> `WindowEvent::DragDrop`
+  - `WindowEvent::HoveredFileCancelled` -> `WindowEvent::DragLeave`
+
+  The `WindowEvent::DragOver` event is entirely new, and is emitted whenever the pointer moves
+  whilst files are being dragged over the window. It doesn't contain any file paths, just the
+  pointer position.


Could you write them the way e.g. Pointer event above written?

Renamed them to DragEntered, DragDropped, DragLeft, and DragMoved.

kchibisov · 2025-01-10T15:17:03Z

src/platform_impl/linux/x11/event_processor.rs

+            // https://www.freedesktop.org/wiki/Specifications/XDND/#xdndposition
+            // "data.l[2] contains the coordinates of the mouse position relative to the root
+            // window."
+            // "data.l[2] = (x << 16) | y"


you can keep the link, but what you wrote is already written in the next paragraph.

Removed the redundant comment

kchibisov · 2025-01-10T15:18:08Z

src/platform_impl/linux/x11/event_processor.rs

+            let packed_coordinates = xev.data.get_long(2);
+            let x = (packed_coordinates >> 16) as i16;
+            let y = (packed_coordinates & 0xffff) as i16;
+            self.dnd.position = (x, y);


Maybe store translated position directly? I don't think there's a reason to use root coords?

Changed it to store the translated position.

kchibisov · 2025-01-10T15:22:13Z

src/platform_impl/linux/x11/event_processor.rs

@@ -502,21 +504,19 @@ impl EventProcessor {
            }

            self.dnd.source_window = Some(source_window);
-            if self.dnd.result.is_none() {


why this got removed?

self.dnd.result.is_none() is only true for the first DragEnter event. Keeping this check means no DragOver (aka DragMoved) events will be fired.

kchibisov · 2025-01-10T15:27:51Z

src/platform_impl/windows/drop_handler.rs

+        if drop_handler.enter_is_valid {
            drop_handler.send_event(Event::WindowEvent {
                window_id: WindowId::from_raw(drop_handler.window as usize),
-                event: HoveredFileCancelled,
+                event: DragLeave,
            });


I think it's wrong, if you had Enter you should have Leave, and you send Enter regardless. Sending Enter and then not Leave looks wrong.

See above; changed it so nothing is emitted if !enter_is_valid.

kchibisov · 2025-01-10T15:28:38Z

src/platform_impl/windows/drop_handler.rs

+            window_id: WindowId::from_raw(drop_handler.window as usize),
+            event: DragEnter { paths, position },
+        });
+        drop_handler.enter_is_valid = hdrop.is_some();


if we want to utilize it, we should use it for sedening the Enter as well?

Changed it so no events are emitted if !enter_is_valid.

X11 has the opposite issue: if there are no valid files being dragged, it won't emit any drag start/position events but will emit DragLeave. That's a pre-existing issue (in the current master branch, it will emit HoveredFileCancelled without any prior HoveredFile events), but I'll go ahead and fix that too

Can't we stash whether the enter was emitted for X11?

Yes; it's already stored as self.dnd.has_entered (I'll push those changes)

kchibisov · 2025-01-10T15:34:18Z

src/event.rs

+    /// A drag operation has entered the window.
+    DragEnter {
+        /// List of paths that are being dragged onto the window.
+        paths: Vec<PathBuf>,
+        /// Position of the drag operation. May be negative on some platforms if something is
+        /// dragged over a window's decorations (title bar, frame, etc).
+        position: PhysicalPosition<f64>,
+    },
+    /// A drag operation is moving over the window.
+    DragOver {
+        /// Position of the drag operation. May be negative on some platforms if something is
+        /// dragged over a window's decorations (title bar, frame, etc).
+        position: PhysicalPosition<f64>,
+    },
+    /// The drag operation has dropped file(s) on the window.
+    DragDrop {
+        /// List of paths that are being dragged onto the window.
+        paths: Vec<PathBuf>,
+        /// Position of the drag operation. May be negative on some platforms if something is
+        /// dragged over a window's decorations (title bar, frame, etc).
+        position: PhysicalPosition<f64>,
+    },
+    /// The drag operation has been cancelled or left the window. On some platforms, this may occur
+    /// even if no other drag events have occurred.
+    DragLeave,


Could you reword things to how they are in pointer events?

Renamed the events and reworded the documentation a bit; see above

kchibisov · 2025-01-10T15:34:55Z

src/event.rs

+        position: PhysicalPosition<f64>,
+    },
+    /// The drag operation has dropped file(s) on the window.
+    DragDrop {


Suggested change

DragDrop {

DragMoved {

so it looks like pointer.

valadaptive · 2025-01-10T17:03:10Z

src/platform_impl/apple/appkit/window_delegate.rs

@@ -426,9 +457,16 @@ declare_class!(

        /// Invoked when the dragging operation is cancelled
        #[method(draggingExited:)]
-        fn dragging_exited(&self, _sender: Option<&NSObject>) {
+        fn dragging_exited(&self, info: Option<&ProtocolObject<dyn NSDraggingInfo>>) {


For @madsmtm: is this the right way to represent a sender param that may be null? The Swift docs note that for this specific callback, this parameter is nullable.

(As a side node, kinda alarming how the Objective-C docs just don't mention its nullability at all, as far as I can tell. Any idea what's going on there?)

@valadaptive

(Original work at rust-windowing#2615; rebased by @valadaptive)

Fixes GTK drag-and-drop coords being offset by (-100, -100). The relevant spec says: > data.l[2] contains the coordinates of the mouse position relative > to the root window. https://www.freedesktop.org/wiki/Specifications/XDND/#xdndposition

We also need to use `convertPoint_fromView` for coordinate conversion-- attempting to do it manually resulted in (0, 0) being the top-left of the window *including* decorations.

It's what translate_coords takes anyway, so the extra precision is misleading if we're going to cast it to i16 everywhere it's used. We can also simplify the "unpacking" from the XdndPosition message--we can and should use the value of 16 as the shift instead of size_of::<c_short> * 2 or something like that, because the specification gives us the constant 16.

ProtocolObject is new in objc2, and lets us use the generated AppKit bindings instead of roughing it with `msg_send!`.

Not yet implemented on macOS

The Swift docs say the dragging info may be nullable here specifically: https://developer.apple.com/documentation/appkit/nsdraggingdestination/draggingexited(_:)

Keep everything in past-tense

Make it clearer that drag events are for dragged *files*, and remove the outdated bit about spurious DragLeft events.

valadaptive · 2025-01-10T20:41:59Z

Made the requested review changes.

Needed to display anything on Wayland. DnD still doesn't work on Wayland but the example should at least run.

valadaptive mentioned this pull request Jan 8, 2025

Rework DnD #2615

Open

8 tasks

valadaptive force-pushed the dnd-cursor-location-rebase branch from 008f86a to aa18119 Compare January 8, 2025 19:40

valadaptive changed the title ~~DnD changes so far (rebased)~~ Rework DnD redux Jan 8, 2025

valadaptive force-pushed the dnd-cursor-location-rebase branch from aa18119 to 4f0819f Compare January 8, 2025 19:42

valadaptive marked this pull request as ready for review January 8, 2025 19:50

valadaptive requested review from notgull, kchibisov and madsmtm as code owners January 8, 2025 19:50

valadaptive force-pushed the dnd-cursor-location-rebase branch 3 times, most recently from b6eba6b to fe7132e Compare January 9, 2025 19:26

kchibisov requested changes Jan 10, 2025

View reviewed changes

valadaptive requested a review from kchibisov January 10, 2025 16:31

valadaptive commented Jan 10, 2025

View reviewed changes

amrbashir and others added 16 commits January 10, 2025 12:26

feat: DnD changes so far (rebased)

f5383d1

(Original work at rust-windowing#2615; rebased by @valadaptive)

feat: DnD example/test

79ad9a5

fix: make DnD compile and function on macOS

612f690

We also need to use `convertPoint_fromView` for coordinate conversion-- attempting to do it manually resulted in (0, 0) being the top-left of the window *including* decorations.

fix: make DnD compile and function on Windows

ac5e69b

chore: format according to nightly rules

ddfc13c

docs: expand drag event documentation

098ff91

docs(changelog): better document DnD changes

0670cde

appkit: use ProtocolObject<dyn NSDraggingInfo>

e0bcd2e

ProtocolObject is new in objc2, and lets us use the generated AppKit bindings instead of roughing it with `msg_send!`.

windows: only emit DragEnter if enter_is_valid

69aff21

x11: store translated DnD coords

6e2ca5d

x11: don't emit DragLeave without DragEnter

b64f77e

refactor: reword drag events to match pointer ones

26b815c

feat: add position to DragLeft

672091a

Not yet implemented on macOS

macos: implement position on DragLeft

3025f4f

The Swift docs say the dragging info may be nullable here specifically: https://developer.apple.com/documentation/appkit/nsdraggingdestination/draggingexited(_:)

valadaptive added 2 commits January 10, 2025 12:26

refactor: DragDrop -> DragDropped

d714b1c

Keep everything in past-tense

docs: clarify drag events

1084930

Make it clearer that drag events are for dragged *files*, and remove the outdated bit about spurious DragLeft events.

valadaptive force-pushed the dnd-cursor-location-rebase branch from 18849af to 1084930 Compare January 10, 2025 17:28

valadaptive mentioned this pull request Jan 10, 2025

Wayland: support basic Drag&Drop #2429

Open

5 tasks

examples/dnd: handle RedrawRequested event

647ac9d

Needed to display anything on Wayland. DnD still doesn't work on Wayland but the example should at least run.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework DnD redux #4079

Rework DnD redux #4079

valadaptive commented Jan 8, 2025 •

edited

Loading

kchibisov left a comment

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

kchibisov Jan 10, 2025

valadaptive Jan 10, 2025

valadaptive Jan 10, 2025

valadaptive commented Jan 10, 2025

Rework DnD redux #4079

Are you sure you want to change the base?

Rework DnD redux #4079

Conversation

valadaptive commented Jan 8, 2025 • edited Loading

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valadaptive commented Jan 10, 2025

valadaptive commented Jan 8, 2025 •

edited

Loading