Rework DnD

[amrbashir]

• Tested on all platforms changed
  • Windows
  • macOS
  • X11
• Added an entry to CHANGELOG.md 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
• Updated feature matrix, if new features were added or implemented

[kchibisov]

Could you elaborate why this is needed? Don't you get pointer motion events during DnD? It's just strange that we have 2 sources of pointer events during dnd with that.

[kchibisov]

This is also a breaking change.

[amrbashir]

Could you elaborate why this is needed? Don't you get pointer motion events during DnD? It's just strange that we have 2 sources of pointer events during dnd with that.

You get the cursor event only right before the DroppedFile event happens but not while HoveredFile, also there has been reports that it sometimes fire after the DroppedFile (Can't reproduce but see #1550 (comment)). Adding the cursor position to these events removes the need to sync with other events.

[kchibisov]

Just to ensure, have you tested the X11 part of it? (I just can't do that myself, but I guess that you can easily do that with any file manager)

[amrbashir]

I haven't tested X11 nor macOS but I can test X11 later today.

[amrbashir]

Alright, X11 should be fine now. The returned coordinates were root window coordinates so I had to translate them before dispatching the event.

[kchibisov]

I think it's a bit more clear which events.

[madsmtm]

I agree that it makes sense to have for DroppedFile, but not so sure about HoveredFile? It seems like, for that to be useful, you'd have to receive the event continuously (which you at least don't on macOS)

[kchibisov]

but not so sure about HoveredFile? It seems like, for that to be useful, you'd have to receive the event continuously (which you at least don't on macOS)

That would strange, I'd assume you can know that you're over some element while doing drag and drop? Maybe the event is sent via some side channel, so we should plumb it to hovered?

[amrbashir]

but not so sure about HoveredFile? It seems like, for that to be useful, you'd have to receive the event continuously (which you at least don't on macOS)

It is the same behavior on Windows and Linux too, the event is emitted only once upon entering the window.

[madsmtm]

macOS impl tested and approved from my side.

It is the same behavior on Windows and Linux too, the event is emitted only once upon entering the window.

HoveredFile.position still doesn't really make sense to me, perhaps we should instead just emit two events CursorMoved { position } + HoveredFile { path } (or the other way around, if that's better)?

Adding the cursor position to these events removes the need to sync with other events.

(Though I do believe you when you say it's useful, I don't use the API myself, so it's not a blocker for me).

[amrbashir]

The thing is that winit should send either CursorMoved or Hovered event, it shouldn't send both at the same time.

why not both? if the OS sends them both, I think winit should pass that through as well

[madsmtm]

do you get any cursor events when moving drag and drop over the window, don't you get CurserMoved?

Nope, you get neither WindowEvent::CursorMoved nor DeviceEvent::MouseMotion.

But it seems you can opt-in to getting updates about the current drag operation, so one could probably implement a DragHover event on macOS.

---

Should we consider postponing this PR to 0.29? It seems like there's still quite some work to do figuring out the desired design (and implementing it, and testing it).

[kchibisov]

why not both? if the OS sends them both, I think winit should pass that through as well

I guess using both will also work, I'm just afraid of the potential collisions and such, since DND mouse movements shouldn't update mouse cursor. But I guess on most systems the event data is separate already.

I'm not sure I like Hovered as a name, maybe DragMoved will be better.

[kchibisov]

Should we consider postponing this PR to 0.29? It seems like there's still quite some work to do figuring out the desired design (and implementing it, and testing it).

We may do so if it'll be the only PR left.

[amrbashir]

I'm not sure I like Hovered as a name, maybe DragMoved will be better.

I went with DragOver since it could be triggered even if the mouse hasn't moved.

[dhardy]

Use of CursorMotion sounds problematic: what happens if the event source is touch not mouse?

Ah, so it pairs with some API to set DND zones?

I much prefer the API in this PR (just send the coordinates).

---

The current API (i.e. this PR) seems fine except maybe one thing: it clones a Vec<PathBuf> on each cursor motion. Is this sensible over, maybe dyn AsRef<Path> or some such?

Also the API should be clear on whether CursorMotion, Touch etc. may still be received during DnD.

---

Edit: clarify that "current API = this PR"

[amrbashir]

The current API seems fine except maybe one thing: it clones a Vec<PathBuf> on each cursor motion. Is this sensible over, maybe dyn AsRef<Path> or some such?

Using dyn AsRef<Path> would require manual implementation for the Debug trait and would need to either remove the PartialEq derive or implement it manually so I don't think it is worth it.

I initially had DragOver only emit the drag location without paths, since DragEnter emits the paths and it will be re emitted on DragDrop but I added just to show it is possible and if it is desired behavior or not.

[dhardy]

Using dyn AsRef would require manual implementation ...

Rust doesn't support multi-trait objects (yet) but you can do this:

pub trait HasPath: Debug + PartialEq + Eq + AsRef<Path> {}

But this is somewhat irrelevant in my opinion. Do we want to lock winit into an API which copies a bunch of paths on every mouse movement? Or is it irrelevant since all platforms re-send those on each movement anyway?

[amrbashir]

Do we want to lock winit into an API which copies a bunch of paths on every mouse movement? Or is it irrelevant since all platforms re-send those on each movement anyway?

Only Windows doesn't send it on each movement, x11 and macOS do.

[madsmtm]

I think I'd prefer to just leave paths out of DragOver - if you need to do something with the paths while they're being dragged you can just store it yourself from DragEnter.

And maybe even remove it from DragDrop? Can the dragged items change while they're being dragged? (Or should we leave it in for ease-of-use?)

[amrbashir]

And maybe even remove it from DragDrop? Can the dragged items change while they're being dragged? (Or should we leave it in for ease-of-use?)

I doubt that it can change but I'd say keep it for ease-of-use

[amrbashir]

@kchibisov It is been a while, and now that I have some time to work on this and fix conflicts, do you think this rework should land soon?

[geom3trik]

Is this PR still compatible with the recent winit API changes or does it need updating?

[valadaptive]

It's very frustrating to see PRs languish and become outdated due to API reworks. This solves an issue (#1550) that has been linked to by several users of other UI frameworks and GUI apps who keep running into it again and again. I would offer to help push this over the finish line, but I have no idea if it would even be looked at or bikeshed to death again and become unmergeable due to conflicts.

Imagine my frustration when I had to go to a third-party repo (iced)'s issues list to see that this PR even existed (it's not mentioned anywhere in #1550) and has been sitting here unmerged for almost two years while its creator has diligently responded to every request made by the maintainers.

[kchibisov]

You can open a PR with the new proposed events, with impl only for one backend and see how it works with the recent pointer API rework.

I myself don't have issues with the suggested design, since it's mostly about dropping into your window, starting drag is not really possible from winit and it'll require the same work as e.g. clipboard APIs. Though, it's not that much.

[amrbashir]

I will see If I can update this PR once again when I have some time.

For starting a drag operation, there is drag-rs which can handle that, see https://github.com/crabnebula-dev/drag-rs/blob/v2/examples/winit/src/main.rs for an example with winit so this PR is mainly for events of dragging into the window.