-
Notifications
You must be signed in to change notification settings - Fork 920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wayland: support basic Drag&Drop #2429
base: master
Are you sure you want to change the base?
Conversation
I've also pushed a commit that emits all appropriate cursor events, which should address #1550 at least on Wayland. Other platforms don't do this yet, so let me know if I should leave it out. (though one issue I noticed is that some compositors send DnD events when the cursor is slightly outside the window (eg. hovering over its shadow only on KDE), which results in out-of-bounds cursor coordinates being sent to the app) |
Could you test that it doesn't blow up in alacritty for example or when using https://github.com/smithay/smithay-clipboard ? I think gnome doesn't allow to have drag and drop and clipboard by using multiple data device managers. IIRC it uses just the last one, and that's the reason I haven't implemented drag and drop inside winit, since winit should support clipboard, otherwise registering clipboard via external crates will disable drag and drop from inside the winit. The work to bring clipboard is #2156 , but I don't have that much time to work on it right now unfortunately, but I hope to go back to it soon... |
I've tested with Alacritty on KDE Plasma, and both Drag&Drop and clipboard access work fine with this patch applied. If GNOME doesn't support this properly, that does seem unfortunate, but this PR will at least make apps work on other compositors, and also make apps work everywhere that only need Drag&Drop and no clipboard access. Do you happen to know if GNOME's behavior is correct here or does the Wayland specification require compositors to support multiple data device managers? |
Would suggest to test GNOME, since it's a major compositor and we can't break it even when it's a GNOME bug. |
Alacritty built with this patch running in a nested GNOME session does not crash or break when dropping a file, but Drag&Drop doesn't do anything (so behavior matches winit master). Clipboard access still works fine. Amusingly, Konsole in the same nested GNOME session does crash when dropping a file from a Dolphin window in the same session. Additionally, closing the nested GNOME shell window made it abort with several assertion failures, so it looks like winit is miles ahead of the typical Linux-on-the-desktop experience already... sadly EDIT: also tested my drag&drop example app in the GNOME session and it works fine! |
The thing is that it'll work, but if you create another device manager the privious one (in winit here) would stop working. I do want to have drag and drop in winit Wayland, but I'm not sure whether I'll have time to look into it, given I think that clipboard issue should be resolved first, so we can have both at the same time without collapsing. |
Yeah, I understand (unless Alacritty somehow does something differently?). As far as I can tell, this PR does not regress anything even if applications do use another data manager for accessing the clipboard. Drag&Drop simply won't work, just like it does now. |
I know what alacritty does and I know why the feature wasn't in winit, since I remember the GNOME bug given that it's quite easy to repro it. Besides that it's fair to assume that we can have drag and drop that way for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the clipboard PR I've linked to you before on how to handle pipe reading with calloop. It could also give an idea how to integrate dnd given that it has data device as well.
} | ||
} | ||
DndEvent::Motion { x, y, .. } => { | ||
if let Some(window_id) = inner.window_id { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why it generates that event, could you elaborate?
.event_sink | ||
.push_window_event(WindowEvent::HoveredFileCancelled, window_id); | ||
winit_state.event_sink.push_window_event( | ||
WindowEvent::CursorLeft { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not sure how it's related, I'm pretty sure wl_pointer
will send the right events anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem to happen at least on KDE and GNOME. Only when no file is being dragged over.
winit_state.event_sink.push_window_event( | ||
WindowEvent::CursorEntered { | ||
device_id: crate::event::DeviceId( | ||
crate::platform_impl::DeviceId::Wayland(DeviceId), | ||
), | ||
}, | ||
window_id, | ||
); | ||
winit_state.event_sink.push_window_event( | ||
WindowEvent::CursorMoved { | ||
device_id: crate::event::DeviceId( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
Addressed your comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunatelly this doesn't work for me on sway and I think the reason is that serial we pass in accept
is zero
.
The issue with sway that there's no drop
event. The mime types do match, etc.
Given that it's likely broken due to sctk bug I can't merge it, unless there's a way to make it work with e.g. thunar + alacritty on sway without patching gtk. Other applications do work here, but they pass normal serial.
let window_id = make_wid(&surface); | ||
inner.window_id = Some(window_id); | ||
offer.accept(Some(MIME_TYPE.into())); | ||
let _ = parse_offer(&inner.loop_handle, offer, move |paths, winit_state| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The callback emits the HoveredFile
event though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yeah. Sure. I'm not sure why anyone would need it. but ok.
sway appears to (correctly) send CRLF-delimited URI lists, while GNOME and KDE only use |
My problem that I don't see |
Huh, right, that must be thunar's doing then. I'll check if I can repro with sway master. |
Sorry for the confusion, it reproduces even with the release of sway that I was using, the I've patched sctk to pass the correct serial to the compositor but it still does not work (Smithay/client-toolkit@bef07ea). The sway debug log is also not very helpful, it only contains this when I try to drag&drop:
|
Sway also appears to send multiple hover enter events in sequence, which end up in the application as multiple |
But it works for me with any other application, so it's confusing. |
…cenes to window
CHANGELOG.md
if knowledge of this change could be valuable to usersCloses #1881