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 .

src/event.rs

src/changelog/unreleased.md

src/platform_impl/linux/x11/event_processor.rs

src/platform_impl/windows/drop_handler.rs

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)

Do we actually need to do something when we have invalid drag from our perspective? Can't we early return throughout and do nothing?

src/event.rs

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?)

Yes, this is correct, see also the docs in objc2-app-kit. In the future, I plan to make it required to have the same signature as in objc2-app-kit, but haven't gotten around to it yet.

The docs on Apple's website don't mention it, probably to be less verbose, but the header does contain a nullability attribute:

// AppKit.framework/Headers/NSDragging.h - (void)draggingExited:(nullable id <NSDraggingInfo>)sender;

Nit: Please keep the name as sender.

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

Made the requested review changes.

src/platform_impl/windows/drop_handler.rs

kchibisov · 2025-01-17T17:05:52Z

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();


Do we actually need to do something when we have invalid drag from our perspective? Can't we early return throughout and do nothing?

src/platform_impl/windows/drop_handler.rs

kchibisov · 2025-01-17T17:06:46Z

src/platform_impl/windows/drop_handler.rs

-        };
-        if let Some(hdrop) = hdrop {
-            unsafe { DragFinish(hdrop) };
+        if drop_handler.enter_is_valid {


can early return.

For some reason I can't comment on the above thread:

Do we actually need to do something when we have invalid drag from our perspective? Can't we early return throughout and do nothing?

Regarding this thread and that one, I believe we need to set pdwEffect regardless of whether the drag is valid, so we cannot early-return.

The Windows documentation says:

The IDropTarget::DragEnter method must choose one of these effects or disable the drop.

And for DragOver:

Upon return, pdwEffect is set to one of the DROPEFFECT flags.

It appears that other apps like Chromium set pdwEffect on all callbacks that pass it in.

An aside: Setting pdwEffect to DROPEFFECT_NONE doesn't work here and uses the "copy" cursor instead. I have no idea why. If you set it to any other value (e.g. DROPEFFECT_MOVE), it works. If you copy and paste this exact same drag-and-drop handler code into the windows-sys create_window example, it works properly and shows the "not allowed" cursor. I spent a few hours trying to figure this out to no avail, and this bug existed previously, so it's not a blocker.

In scenarios other than this specific one which doesn't work for unfathomable reasons, you do need to set DROPEFFECT_NONE on every frame or it'll reset to the "copy" cursor.

src/platform_impl/linux/x11/event_processor.rs

src/platform_impl/linux/x11/dnd.rs

@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.

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

It appears other apps (like Chromium) set pdwEffect on Drop too: https://github.com/chromium/chromium/blob/61a391b86bd946d6e1105412539e77ba9fb2a6b3/ui/base/dragdrop/drop_target_win.cc

valadaptive · 2025-01-18T02:25:15Z

Made the requested review changes again.

valadaptive · 2025-01-28T03:44:21Z

@madsmtm Still awaiting your review for the macOS parts.

The main changes from #2615 are:

Switched from taking &NSObject as the sender param to &ProtocolObject<dyn NSDraggingInfo> in the drag callback methods (both the existing and new ones), which means we don't need to use msg_send_id!.
Used convertPoint_fromView to convert the drag coords to window-space instead of doing the transform manually. This correctly takes into account the size of the window frame-- (0, 0) is now the top-left corner of the window contents, excluding the title bar.

Would it be possible to get this in before #4092 to avoid having to do another rebase?

madsmtm

API looks good, and AppKit impl looks good, thanks for rebasing this!

madsmtm · 2025-01-28T19:42:33Z

src/event.rs

+        /// (x,y) coordinates in pixels relative to the top-left corner of the window. May be
+        /// negative on some platforms if something is dragged over a window's decorations (title
+        /// bar, frame, etc).
+        position: PhysicalPosition<f64>,


Nit: This extra text with "may be negative" sounds confusing, and probably only relevant on Wayland given that the coordinates are actually in window coordinates, and not surface coordinates. But that's to be resolved in #3891.

By "surface coordinates", do you mean relative to the contents of the window? Because the drag position is currently in surface coordinates, to match the pointer position. Unless I'm misunderstanding.

If you run the dnd example and drag a file over the window's title bar, the y-position will be negative in macOS and Windows. On X11, no drag events are emitted unless you're dragging over the window's contents/surface.

My understanding is that "window coordinates" are relative to the top-left corner of the window including decorations, and that "surface coordinates" are relative to the top-left corner of the window's contents. Pointer events are in surface coordinates.

DnD events in Windows and X11 were also in surface coordinates in the original PR, so I changed macOS to match those platforms (EDIT: and also the pointer events).

valadaptive mentioned this pull request Jan 8, 2025

Rework DnD #2615

Closed

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

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

kchibisov requested changes Jan 17, 2025

View reviewed changes

amrbashir and others added 12 commits January 17, 2025 17:34

feat: DnD changes so far (rebased)

2594b83

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

feat: DnD example/test

b3510c2

fix: make DnD compile and function on macOS

d100b86

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

9bf7a4e

chore: format according to nightly rules

7c293f5

docs: expand drag event documentation

35a972f

docs(changelog): better document DnD changes

36438b1

appkit: use ProtocolObject<dyn NSDraggingInfo>

a2b150b

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

5bbbedd

x11: store translated DnD coords

3d3f6fb

valadaptive added 11 commits January 17, 2025 17:34

x11: don't emit DragLeave without DragEnter

bafb090

refactor: reword drag events to match pointer ones

2873e88

feat: add position to DragLeft

991b4c8

Not yet implemented on macOS

macos: implement position on DragLeft

dcfb62d

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

refactor: DragDrop -> DragDropped

6b4b84b

Keep everything in past-tense

docs: clarify drag events

a4be71e

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

examples/dnd: handle RedrawRequested event

df8e5ce

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

x11: DnD code style tweaks

f55a534

windows: import DnD events at top of file

2f589b5

windows: drop_handler.enter_is_valid -> valid

59dd7ed

windows: in DnD, always set pdwEffect

2153865

It appears other apps (like Chromium) set pdwEffect on Drop too: https://github.com/chromium/chromium/blob/61a391b86bd946d6e1105412539e77ba9fb2a6b3/ui/base/dragdrop/drop_target_win.cc

valadaptive force-pushed the dnd-cursor-location-rebase branch from 7b82760 to 2153865 Compare January 17, 2025 22:35

valadaptive requested a review from kchibisov January 17, 2025 22:42

kchibisov approved these changes Jan 27, 2025

View reviewed changes

madsmtm mentioned this pull request Jan 28, 2025

Coordinate systems #3891

Open

madsmtm approved these changes Jan 28, 2025

View reviewed changes

Don't rename protocol parameter

731a0ce

madsmtm merged commit f5dcd2a into rust-windowing:master Jan 28, 2025
58 checks passed

madsmtm added this to the Version 0.31.0 milestone Jan 28, 2025

fredizzimo mentioned this pull request Feb 1, 2025

Allow customizing drag-n-drop neovide/neovide#2991

Open

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 17, 2025

valadaptive Jan 10, 2025

madsmtm Jan 28, 2025

madsmtm Jan 28, 2025

valadaptive commented Jan 10, 2025

kchibisov Jan 17, 2025

kchibisov Jan 17, 2025

valadaptive Jan 17, 2025 •

edited

Loading

valadaptive commented Jan 18, 2025

valadaptive commented Jan 28, 2025

madsmtm left a comment

madsmtm Jan 28, 2025

valadaptive Jan 28, 2025

valadaptive Jan 28, 2025 •

edited

Loading

Rework DnD redux #4079

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

valadaptive commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valadaptive Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

valadaptive commented Jan 18, 2025

valadaptive commented Jan 28, 2025

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valadaptive Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

valadaptive commented Jan 8, 2025 •

edited

Loading

valadaptive Jan 17, 2025 •

edited

Loading

valadaptive Jan 28, 2025 •

edited

Loading