GNOME Bugzilla – Bug 738312
Add X11/wayland clipboard interoperation
Last modified: 2015-05-30 11:21:02 UTC
I'm attaching some patches that allow selections to work between X11 and wayland clients. Roughly based on the equivalent code in weston.
Created attachment 288237 [details] [review] wayland: Add methods to fetch the mimetypes in the current data source This will be useful on X selection interoperation, as the other peer won't be a regular wl_resource.
Created attachment 288238 [details] [review] wayland: Add API to unset the current data source This will be necessary for the X clipboard to claim ownership on the selection.
Created attachment 288239 [details] [review] wayland: Add API to start a data offer internally This will be useful for X clipboard interoperation, and can also be useful for selections started within the compositor.
Created attachment 288240 [details] [review] wayland: Add API to start an internal data petition to a wl_data_source This will be useful for X clipboard interoperation, and can also be useful for other selection requests from the compositor.
Created attachment 288241 [details] [review] wayland: Add X11/wayland selection interoperation This piece of code hooks in both wl_data_device and the relevant X selection events, an X11 Window is set up so it can act as the clipboard owner when any wayland client owns the selection, reacting to SelectionRequest events, and returning the data from the wayland client FD to any X11 requestor through X properties. In the opposite direction, SelectionNotify messages are received, which results in the property contents being converted then written into the wayland requestor's FD. This code also takes care of the handling incremental transfers through the INCR property type, reading/writing data chunk by chunk.
Some notes on the patches: - gnome-shell currently attempts to use the X11 clipboard, even on Wayland. Furthermore gnome-shell crashes when receiving SelectionRequest events that weren't actually meant for it. I've filed bug #738314. - From my testing INCR messages work everywhere just nice, except when trying to paste a large amount of text (eg. gtkwidget.c) on emacs, a bunch of garbage seems to end up after the pasted content then. This does not happen on any X11 GTK+ app, libreoffice nor webkit. Firefox crashes on startup though, so I couldn't test that one.
Oh, one more thing: - These patches do nothing about PRIMARY, only X11 clients ever set that, which maybe makes it rather confusing...
we should probably disable middle-click-sets-primary under wayland, via xsettings
*** Bug 744103 has been marked as a duplicate of this bug. ***
Review of attachment 288239 [details] [review]: ::: src/wayland/meta-wayland-data-device.h @@ +51,3 @@ +gboolean meta_wayland_data_device_start_offer_internal (MetaWaylandDataDevice *data_device, + const gchar *mime_type, + int fd); These names are a bit unfortunate, as I started reading these as internal to MetaWaylandDataDevice which was very confusing. Also, wouldn't it be better to just pass a MetaWaylandDataSource, letting meta-xwayland-selection.c set it up?
Review of attachment 288240 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +52,2 @@ gboolean has_target; + gboolean internal; I think that instead of adding this "internal" state we should just put those places where sources need special handling behind vfuncs and then have a Wayland source and another Xwayland source (more similar to how weston does it). What the state really does is invoke special code paths in certain places, and it'd be much easier to understand if it was more clear that so was the case. @@ +95,3 @@ + * fd, we don't close here + */ + offer->source->write_func (fd, mime_type, offer->source->func_data); Shouldn't we close(fd) here if offer->source == NULL && offer->source->internal == 1? @@ +146,3 @@ + + if (!source->internal) + wl_resource_add_destroy_listener (source->resource, &offer->source_destroy_listener); Why not adding a destroy listener here? ::: src/wayland/meta-wayland-data-device.h @@ +57,3 @@ + guint32 serial, + MetaSelectionTransferFunc write_func, + gpointer func_data); Same comment as previous patch about choosing the name "internal", and whether just to pass a MetaWaylandDataSource instead.
(In reply to Carlos Garnacho from comment #6) > - From my testing INCR messages work everywhere just nice, except when > trying to paste a large amount of text (eg. gtkwidget.c) on emacs, a bunch > of garbage seems to end up after the pasted content then. This does not > happen on any X11 GTK+ app, libreoffice nor webkit. Firefox crashes on > startup though, so I couldn't test that one. I've investigated this again, and seems like a bug in emacs emacs itself. http://www.x.org/releases/X11R7.7/doc/xorg-docs/icccm/icccm.html#INCR_Properties says: " The contents of the INCR property will be an integer, which represents a lower bound on the number of bytes of data in the selection." Weston (and these mutter patches) stick to that and set this to the maximum number of bytes in an INCR chunk, Emacs will however append garbage unless it represents the *exact* number of bytes, which is what GTK+ happens to do. IMO we're doing things by the book, and making mutter buffer the whole selection just to bypass this is a no go. (In reply to Matthias Clasen from comment #8) > we should probably disable middle-click-sets-primary under wayland, via > xsettings Right, indeed :)
Created attachment 300438 [details] [review] wayland: refactor MetaWaylandDataSource Expose it partly (in internal headers anyway), and pass a vtable for the data source functions, the wayland vfuncs just delegate operations on the wl_data_source resource. The resource has been also made optional, although it'll be present on all data sources from wayland clients. This allows the creation of custom/proxy data sources, which will turn out useful for X11 selection interoperation.
Created attachment 300439 [details] [review] wayland: Add X11/wayland selection interoperation This piece of code hooks in both wl_data_device and the relevant X selection events, an X11 Window is set up so it can act as the clipboard owner when any wayland client owns the selection, reacting to SelectionRequest events, and returning the data from the wayland client FD to any X11 requestor through X properties. In the opposite direction, SelectionNotify messages are received, which results in the property contents being converted then written into the wayland requestor's FD. This code also takes care of the handling incremental transfers through the INCR property type, reading/writing data chunk by chunk.
Created attachment 300456 [details] [review] wayland: Add X11/wayland selection interoperation This piece of code hooks in both wl_data_device and the relevant X selection events, an X11 Window is set up so it can act as the clipboard owner when any wayland client owns the selection, reacting to SelectionRequest events, and returning the data from the wayland client FD to any X11 requestor through X properties. In the opposite direction, SelectionNotify messages are received, which results in the property contents being converted then written into the wayland requestor's FD. This code also takes care of the handling incremental transfers through the INCR property type, reading/writing data chunk by chunk.
Review of attachment 300456 [details] [review]: ::: src/core/main.c @@ +81,3 @@ #ifdef HAVE_WAYLAND #include "wayland/meta-wayland.h" +#include "wayland/meta-xwayland-private.h" This is no longer needed right? ::: src/wayland/meta-wayland-data-device.c @@ +509,3 @@ { + /* Update X11 selection on any change from the wayland side */ + meta_xwayland_selection_update (meta_wayland_compositor_get_default ()); This should rather be a signal outside of the Wayland client data source if we want to have any other source type than these two, like a clipboard manager or something. But I suppose its good enough for now. Also shouldn't we also update xwayland selection on source destruction?
Review of attachment 300438 [details] [review]: I haven't looked through all of it yet, but have some comments on the parts I have looked through. ::: src/wayland/meta-wayland-data-device.c @@ +129,3 @@ + offer->source_destroy_listener.notify = destroy_offer_data_source; + wl_resource_add_destroy_listener (source->resource, &offer->source_destroy_listener); + } Do we really need the destroy listener any more when the offer owns a reference to the source? Hmm, also, now that the offer keeps the source from destroying itself, when a Wayland client destroys its source, it'll only unref the MetaWaylandDataSource, but the offer will still have a reference so it won't be destroyed. The problem is that the offer will only unref its offer->source if it receives the destroy signal from the source resource, which it won't since it has a reference to it. Another thought about this. If we want to use some different object lifetime, there might be some other things than needs adoption, for example destroy_selection_data_source() will remove any previous selection, even if there was a new selection set after the selection source resource was destroyed by the client (but held alive by some reference holder). Maybe its easier to add a separate destroy signal to MetaWaylandDataSource. This way we can also get notified when Xwayland destroys its source.
(In reply to Jonas Ådahl from comment #16) > Review of attachment 300456 [details] [review] [review]: > > ::: src/core/main.c > @@ +81,3 @@ > #ifdef HAVE_WAYLAND > #include "wayland/meta-wayland.h" > +#include "wayland/meta-xwayland-private.h" > > This is no longer needed right? > > ::: src/wayland/meta-wayland-data-device.c > @@ +509,3 @@ > { > + /* Update X11 selection on any change from the wayland side */ > + meta_xwayland_selection_update (meta_wayland_compositor_get_default ()); > > This should rather be a signal outside of the Wayland client data source if > we want to have any other source type than these two, like a clipboard > manager or something. But I suppose its good enough for now. Yeah... basically if you have N data source types, you want to notify the other N-1, I guess it could be made as a list of notify functions in MetaWaylandDataDevice. > > Also shouldn't we also update xwayland selection on source destruction? Right (In reply to Jonas Ådahl from comment #17) > Review of attachment 300438 [details] [review] [review]: > > I haven't looked through all of it yet, but have some comments on the parts > I have looked through. > > ::: src/wayland/meta-wayland-data-device.c > @@ +129,3 @@ > + offer->source_destroy_listener.notify = destroy_offer_data_source; > + wl_resource_add_destroy_listener (source->resource, > &offer->source_destroy_listener); > + } > > Do we really need the destroy listener any more when the offer owns a > reference to the source? > > Hmm, also, now that the offer keeps the source from destroying itself, when > a Wayland client destroys its source, it'll only unref the > MetaWaylandDataSource, but the offer will still have a reference so it won't > be destroyed. The problem is that the offer will only unref its > offer->source if it receives the destroy signal from the source resource, > which it won't since it has a reference to it. I'm slightly confused. AFAICT the 2 references on the MetaWaylandDataSource are held by the source wl_resource, one through its wl_resource_set_implementation() destructor, and another throught the destroy notify above. I wanted to make it clearer through the refcount that the source were held in several places, on second thought I can just NULLify stuff around, so the data device stays as the owner of the data source. > > Another thought about this. If we want to use some different object > lifetime, there might be some other things than needs adoption, for example > destroy_selection_data_source() will remove any previous selection, even if > there was a new selection set after the selection source resource was > destroyed by the client (but held alive by some reference holder). AFAICS that notify function is only set on the current data source. > > Maybe its easier to add a separate destroy signal to MetaWaylandDataSource. > This way we can also get notified when Xwayland destroys its source. That follows other paths I fear... that's done in meta_xwayland_selection_handle_xfixes_selection_notify(), I guess it comes down to having a common "notify all other sources" call point as pondered above.
(In reply to Carlos Garnacho from comment #18) > (In reply to Jonas Ådahl from comment #16) > > Review of attachment 300456 [details] [review] [review] [review]: > > > > ::: src/core/main.c > > @@ +81,3 @@ > > #ifdef HAVE_WAYLAND > > #include "wayland/meta-wayland.h" > > +#include "wayland/meta-xwayland-private.h" > > > > This is no longer needed right? > > > > ::: src/wayland/meta-wayland-data-device.c > > @@ +509,3 @@ > > { > > + /* Update X11 selection on any change from the wayland side */ > > + meta_xwayland_selection_update (meta_wayland_compositor_get_default ()); > > > > This should rather be a signal outside of the Wayland client data source if > > we want to have any other source type than these two, like a clipboard > > manager or something. But I suppose its good enough for now. > > Yeah... basically if you have N data source types, you want to notify the > other N-1, I guess it could be made as a list of notify functions in > MetaWaylandDataDevice. > > > > > Also shouldn't we also update xwayland selection on source destruction? > > Right > > (In reply to Jonas Ådahl from comment #17) > > Review of attachment 300438 [details] [review] [review] [review]: > > > > I haven't looked through all of it yet, but have some comments on the parts > > I have looked through. > > > > ::: src/wayland/meta-wayland-data-device.c > > @@ +129,3 @@ > > + offer->source_destroy_listener.notify = destroy_offer_data_source; > > + wl_resource_add_destroy_listener (source->resource, > > &offer->source_destroy_listener); > > + } > > > > Do we really need the destroy listener any more when the offer owns a > > reference to the source? > > > > Hmm, also, now that the offer keeps the source from destroying itself, when > > a Wayland client destroys its source, it'll only unref the > > MetaWaylandDataSource, but the offer will still have a reference so it won't > > be destroyed. The problem is that the offer will only unref its > > offer->source if it receives the destroy signal from the source resource, > > which it won't since it has a reference to it. > > I'm slightly confused. AFAICT the 2 references on the MetaWaylandDataSource > are held by the source wl_resource, one through its > wl_resource_set_implementation() destructor, and another throught the > destroy notify above. > > I wanted to make it clearer through the refcount that the source were held > in several places, on second thought I can just NULLify stuff around, so the > data device stays as the owner of the data source. Aah, I understand. The actual source is decoupled from the MetaWaylandDataSource. Reference counting here makes it a bit tricky to reason about things; I think it'd be better to have a clear life time, avoiding the ref counting. We'd have defunct source objects when their real sources are gone anyway. Could we make the "real" source (the wl_data_source object for Wayland sources, and some xwayland selection singleton in X11 case) the owner of the MetaWaylandDataSource, and make the offer only have a weak reference i.e. as you say NULLiy when the source is destroyed? > > > > > Another thought about this. If we want to use some different object > > lifetime, there might be some other things than needs adoption, for example > > destroy_selection_data_source() will remove any previous selection, even if > > there was a new selection set after the selection source resource was > > destroyed by the client (but held alive by some reference holder). > > AFAICS that notify function is only set on the current data source. Yea, it was me not incorrectly decoupling MetaWaylandDataSource object lifetime from the real source object lifetime, so what I said wouldn't happen. > > > > > Maybe its easier to add a separate destroy signal to MetaWaylandDataSource. > > This way we can also get notified when Xwayland destroys its source. > > That follows other paths I fear... that's done in > meta_xwayland_selection_handle_xfixes_selection_notify(), I guess it comes > down to having a common "notify all other sources" call point as pondered > above. Yes, or rather two. One per source destroyd signal so that offers can NULL:ify their pointers), and one per seat "current selection changed, please tell your clients" signal.
Review of attachment 300438 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +747,3 @@ + struct wl_resource *wl_resource, + gpointer user_data, + GDestroyNotify destroy_notify) The destroy_notify function is never set by any user, so I think it can be removed. The same with user_data, since it seems only to be used when calling destroy_notify.
Created attachment 301473 [details] [review] wayland: refactor MetaWaylandDataSource Expose it partly (in internal headers anyway), and pass a vtable for the data source functions, the wayland vfuncs just delegate operations on the wl_data_source resource. The resource has been also made optional, although it'll be present on all data sources from wayland clients. This allows the creation of custom/proxy data sources, which will turn out useful for X11 selection interoperation.
Created attachment 301474 [details] [review] wayland: Add X11/wayland selection interoperation This piece of code hooks in both wl_data_device and the relevant X selection events, an X11 Window is set up so it can act as the clipboard owner when any wayland client owns the selection, reacting to SelectionRequest events, and returning the data from the wayland client FD to any X11 requestor through X properties. In the opposite direction, SelectionNotify messages are received, which results in the property contents being converted then written into the wayland requestor's FD. This code also takes care of the handling incremental transfers through the INCR property type, reading/writing data chunk by chunk.
(In reply to Jonas Ådahl from comment #19) > > I wanted to make it clearer through the refcount that the source were held > > in several places, on second thought I can just NULLify stuff around, so the > > data device stays as the owner of the data source. > > Aah, I understand. The actual source is decoupled from the > MetaWaylandDataSource. Reference counting here makes it a bit tricky to > reason about things; I think it'd be better to have a clear life time, > avoiding the ref counting. We'd have defunct source objects when their real > sources are gone anyway. > > Could we make the "real" source (the wl_data_source object for Wayland > sources, and some xwayland selection singleton in X11 case) the owner of the > MetaWaylandDataSource, and make the offer only have a weak reference i.e. as > you say NULLiy when the source is destroyed? I've gone for making set_selection() and start_drag() take ownership on the data source, the resource will just leave the source struct "inert", and would be destroyed when replaced, or the drag finishes. > > That follows other paths I fear... that's done in > > meta_xwayland_selection_handle_xfixes_selection_notify(), I guess it comes > > down to having a common "notify all other sources" call point as pondered > > above. > > Yes, or rather two. One per source destroyd signal so that offers can > NULL:ify their pointers), and one per seat "current selection changed, > please tell your clients" signal. In my last patches, this is handled by global notify functions (global=per source type), and set_selection() triggering the right things on the previous data source. One minor note though, I rather separated the per-source user_data/destroy_notify into a separate function, instead of getting rid of it, it looks like it will be useful when I get to implementing DnD interoperation, even if it doesn't seems too useful now.
Review of attachment 301473 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +127,3 @@ + { + offer->source_destroy_listener.notify = destroy_offer_data_source; + wl_resource_add_destroy_listener (source->resource, &offer->source_destroy_listener); Shouldn't we hide this per-source-type behind some abstraction as well, as with the other cases? IMO there should be no source->resource access outside of the abstraction. @@ +561,3 @@ + { + data_device->selection_data_source_listener.notify = destroy_selection_data_source; + wl_resource_add_destroy_listener (source->resource, &data_device->selection_data_source_listener); Same as previous comment about source->resource. @@ +564,3 @@ + } + + meta_wayland_data_device_notify (data_device, source->type); I don't think we can include a "type" in the notification here. This way we'd only notify when a selection is set, not when unset. It'd probably be easier to just ignore the types and just do a list of callbacks that meta-xwayland-selection adds and removes itself to/from, or in the same way with a vfunc table you do for sources, which I suppose only would have a "selection updated" function. @@ +798,3 @@ +meta_wayland_data_source_set_user_data (MetaWaylandDataSource *source, + gpointer user_data, + GDestroyNotify destroy_notify) I know you mentioned this might be useful in the future but I think it'd be much better to just drop it for now, and introduce it when it is actually needed. That way its possible to review how suitable it is for how it is used. The clipboard/dnd interoperability API is not public so we can change it however we want whenever we want. ::: src/wayland/meta-wayland-data-device.h @@ +28,2 @@ #include "meta-wayland-types.h" +#include "meta-wayland-types.h" Hmm? @@ +70,3 @@ + MetaDataSourceType type; + gboolean has_target; + gint ref_count; Not counting references any more.
Review of attachment 301474 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +193,3 @@ +static void +reply_selection_request (XSelectionRequestEvent *request_event, + gboolean ack) Why is this called "ack"? For what I can see in the documentation the property parameter which "ack" ultimately affects is about setting the property the result is stored on. I'd prefer that detail is not hidden behind a arbitrary boolean (like reply_selection_request_success vs reply_selection_request_fail ?). @@ +376,3 @@ + (GDestroyNotify) x11_clipboard_data_free); + + /* Takes ownership on fd */ nit: s/on/of/ ? @@ +493,3 @@ + x11_clipboard_data_write (selection_data, prop_ret, nitems_ret); + + x11_clipboard_data_free (x11_clipboard); Won't this just immediately cancel the async write above? @@ +552,3 @@ + requestor, property, + XA_ATOM, 32, PropModeReplace, + (guchar *) targets, i); Aren't we leaking targets here? nit: it'd be a bit nicer if we didn't rely on the implicit interation index. I.e. something like int num_targets = data_source->mime_types.size + 2; g_new (Atom, num_targets); and then use that variable here as well. @@ +622,3 @@ + if (event->selection == gdk_x11_get_xatom_by_name ("CLIPBOARD_MANAGER")) + { + /* The weston clipboard should already have grabbed s/weston/mutter/ ? @@ +813,3 @@ + meta_xwayland_selection_update); + meta_wayland_data_device_notify (&compositor->seat->data_device, + META_DATA_SOURCE_TYPE_X11); We'd notify noone here (since notify ignores the xwayland receiver), but who we really should update here is the xwayland selection isn't it? But I think we should drop the type from the notifications and just use a list, so in that case this won't be a problem.
Created attachment 301762 [details] [review] wayland: refactor MetaWaylandDataSource Expose it partly (in internal headers anyway), and pass a vtable for the data source functions, the wayland vfuncs just delegate operations on the wl_data_source resource. The resource has been also made optional, although it'll be present on all data sources from wayland clients. This allows the creation of custom/proxy data sources, which will turn out useful for X11 selection interoperation.
Created attachment 301763 [details] [review] wayland: Add X11/wayland selection interoperation This piece of code hooks in both wl_data_device and the relevant X selection events, an X11 Window is set up so it can act as the clipboard owner when any wayland client owns the selection, reacting to SelectionRequest events, and returning the data from the wayland client FD to any X11 requestor through X properties. In the opposite direction, SelectionNotify messages are received, which results in the property contents being converted then written into the wayland requestor's FD. This code also takes care of the handling incremental transfers through the INCR property type, reading/writing data chunk by chunk.
Created attachment 301764 [details] [review] wayland: Add X11/wayland selection interoperation This piece of code hooks in both wl_data_device and the relevant X selection events, an X11 Window is set up so it can act as the clipboard owner when any wayland client owns the selection, reacting to SelectionRequest events, and returning the data from the wayland client FD to any X11 requestor through X properties. In the opposite direction, SelectionNotify messages are received, which results in the property contents being converted then written into the wayland requestor's FD. This code also takes care of the handling incremental transfers through the INCR property type, reading/writing data chunk by chunk.
Having progressed a bit further on XDND interoperation (still WIP though), these patches are most in line with what will be needed afterwards there. Some replies to the remaining unadressed comments: (In reply to Jonas Ådahl from comment #24) > Review of attachment 301473 [details] [review] [review]: > > ::: src/wayland/meta-wayland-data-device.c > @@ +127,3 @@ > + { > + offer->source_destroy_listener.notify = destroy_offer_data_source; > + wl_resource_add_destroy_listener (source->resource, > &offer->source_destroy_listener); > > Shouldn't we hide this per-source-type behind some abstraction as well, as > with the other cases? IMO there should be no source->resource access outside > of the abstraction. I've scratched my head a bit at this, I can only think of attach_to_data_device/offer kind of apis, I guess it's not a problem to add these, but having almost half the functions in the vtable meant for object management... > @@ +564,3 @@ > + } > + > + meta_wayland_data_device_notify (data_device, source->type); > > I don't think we can include a "type" in the notification here. This way > we'd only notify when a selection is set, not when unset. It'd probably be > easier to just ignore the types and just do a list of callbacks that > meta-xwayland-selection adds and removes itself to/from, or in the same way > with a vfunc table you do for sources, which I suppose only would have a > "selection updated" function. The purpose of this function is "notify every other data source of a new selection". Eg. the x11 handler shouldn't self-claim selection when a X11 client just did so. I don't see how to get around this unless we pass the type claiming the selection. > > @@ +798,3 @@ > +meta_wayland_data_source_set_user_data (MetaWaylandDataSource *source, > + gpointer user_data, > + GDestroyNotify > destroy_notify) > > I know you mentioned this might be useful in the future but I think it'd be > much better to just drop it for now, and introduce it when it is actually > needed. That way its possible to review how suitable it is for how it is > used. The clipboard/dnd interoperability API is not public so we can change > it however we want whenever we want. This is now dropped FWIW, and AFAICT from my ongoing XDND code it won't be needed.
(In reply to Carlos Garnacho from comment #29) > Having progressed a bit further on XDND interoperation (still WIP though), > these patches are most in line with what will be needed afterwards there. > > Some replies to the remaining unadressed comments: > (In reply to Jonas Ådahl from comment #24) > > Review of attachment 301473 [details] [review] [review] [review]: > > > > ::: src/wayland/meta-wayland-data-device.c > > @@ +127,3 @@ > > + { > > + offer->source_destroy_listener.notify = destroy_offer_data_source; > > + wl_resource_add_destroy_listener (source->resource, > > &offer->source_destroy_listener); > > > > Shouldn't we hide this per-source-type behind some abstraction as well, as > > with the other cases? IMO there should be no source->resource access outside > > of the abstraction. > > I've scratched my head a bit at this, I can only think of > attach_to_data_device/offer kind of apis, I guess it's not a problem to add > these, but having almost half the functions in the vtable meant for object > management... I wonder if it can be dealt with by the source tracking what offers its at, and dereference itself from them that way. That way the Wayland sources can deal with their difference in validity lifetime. For the selections and DND, it'd do the same xwayland-selection. Does that sound reasonable to you? > > > @@ +564,3 @@ > > + } > > + > > + meta_wayland_data_device_notify (data_device, source->type); > > > > I don't think we can include a "type" in the notification here. This way > > we'd only notify when a selection is set, not when unset. It'd probably be > > easier to just ignore the types and just do a list of callbacks that > > meta-xwayland-selection adds and removes itself to/from, or in the same way > > with a vfunc table you do for sources, which I suppose only would have a > > "selection updated" function. > > The purpose of this function is "notify every other data source of a new > selection". Eg. the x11 handler shouldn't self-claim selection when a X11 > client just did so. I don't see how to get around this unless we pass the > type claiming the selection. Wouldn't it be easier to just ignore it for updates coming from itself? For instance, where you call the notify function from xwayland_selection_init, you actually want to update, and the ignore-type based notification won't work. It'd also work for the case where you don't have a source type, for example when unsetting a selection. > > > > > @@ +798,3 @@ > > +meta_wayland_data_source_set_user_data (MetaWaylandDataSource *source, > > + gpointer user_data, > > + GDestroyNotify > > destroy_notify) > > > > I know you mentioned this might be useful in the future but I think it'd be > > much better to just drop it for now, and introduce it when it is actually > > needed. That way its possible to review how suitable it is for how it is > > used. The clipboard/dnd interoperability API is not public so we can change > > it however we want whenever we want. > > This is now dropped FWIW, and AFAICT from my ongoing XDND code it won't be > needed.
Review of attachment 301762 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +565,3 @@ + + meta_wayland_data_device_notify (data_device, source->type, + META_DATA_SELECTION_CLIPBOARD); We'd only notify when *setting* a selection. Unsetting a selection would leave xwayland-selection with an invalid state (see other reply to your previous comment).
Review of attachment 301764 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +79,3 @@ + +struct _MetaXWaylandSelection { + Selection selections[META_N_DATA_SELECTION_TYPES]; What is the point of separating the selections into types? I didn't see it in any previous patch, it seems like a rather major change. I'd rather know the reasoning behind the change before re-reviewing things. @@ +655,3 @@ + requestor, property, + XA_ATOM, 32, PropModeReplace, + (guchar *) targets, i); Still leaking, aren't we?
(In reply to Jonas Ådahl from comment #32) > Review of attachment 301764 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-selection.c > @@ +79,3 @@ > + > +struct _MetaXWaylandSelection { > + Selection selections[META_N_DATA_SELECTION_TYPES]; > > What is the point of separating the selections into types? I didn't see it > in any previous patch, it seems like a rather major change. I'd rather know > the reasoning behind the change before re-reviewing things. Yes, it's a "big" change in how data has been shuffled in structs, but the fundamentals are just the same. XDND uses selections for data transfer, and this will enable me to add an extra slot for the DnD selection adding a META_DATA_SELECTION_DND to the enum. As the life cycles of these selections are independent and can be poked in parallel, it is better to keep those separated. One notable case here is nautilus, it will poke (and possibly update) the clipboard selection after dropping is performed, so there may be requests/updates on 2 selections simultaneously. > > @@ +655,3 @@ > + requestor, property, > + XA_ATOM, 32, PropModeReplace, > + (guchar *) targets, i); > > Still leaking, aren't we? Oops yes, I apparently plugged the leak on the corresponding XGetWindowProperty
(In reply to Carlos Garnacho from comment #33) > (In reply to Jonas Ådahl from comment #32) > > Review of attachment 301764 [details] [review] [review] [review]: > > > > ::: src/wayland/meta-xwayland-selection.c > > @@ +79,3 @@ > > + > > +struct _MetaXWaylandSelection { > > + Selection selections[META_N_DATA_SELECTION_TYPES]; > > > > What is the point of separating the selections into types? I didn't see it > > in any previous patch, it seems like a rather major change. I'd rather know > > the reasoning behind the change before re-reviewing things. > > Yes, it's a "big" change in how data has been shuffled in structs, but the > fundamentals are just the same. > > XDND uses selections for data transfer, and this will enable me to add an > extra slot for the DnD selection adding a META_DATA_SELECTION_DND to the > enum. As the life cycles of these selections are independent and can be > poked in parallel, it is better to keep those separated. One notable case > here is nautilus, it will poke (and possibly update) the clipboard selection > after dropping is performed, so there may be requests/updates on 2 > selections simultaneously. I can understand why you want to deal with DND and clipboard independently, but here you mix type and origin in the same enum which I find confusing. What is the point of having two clipboard operations possible independently (by having one Wayland clipboard and one X11 clipboard in parallel).
(In reply to Jonas Ådahl from comment #34) > (In reply to Carlos Garnacho from comment #33) > > (In reply to Jonas Ådahl from comment #32) > > > Review of attachment 301764 [details] [review] [review] [review] [review]: > > > > > > ::: src/wayland/meta-xwayland-selection.c > > > @@ +79,3 @@ > > > + > > > +struct _MetaXWaylandSelection { > > > + Selection selections[META_N_DATA_SELECTION_TYPES]; > > > > > > What is the point of separating the selections into types? I didn't see it > > > in any previous patch, it seems like a rather major change. I'd rather know > > > the reasoning behind the change before re-reviewing things. > > > > Yes, it's a "big" change in how data has been shuffled in structs, but the > > fundamentals are just the same. > > > > XDND uses selections for data transfer, and this will enable me to add an > > extra slot for the DnD selection adding a META_DATA_SELECTION_DND to the > > enum. As the life cycles of these selections are independent and can be > > poked in parallel, it is better to keep those separated. One notable case > > here is nautilus, it will poke (and possibly update) the clipboard selection > > after dropping is performed, so there may be requests/updates on 2 > > selections simultaneously. > > I can understand why you want to deal with DND and clipboard independently, > but here you mix type and origin in the same enum which I find confusing. Maybe the names are too similar, but it's certainly 2 enums :): typedef enum { META_DATA_SOURCE_TYPE_WAYLAND, META_DATA_SOURCE_TYPE_X11, META_N_DATA_SOURCE_TYPES } MetaDataSourceType; typedef enum { META_DATA_SELECTION_CLIPBOARD, META_DATA_SELECTION_DND, META_N_DATA_SELECTION_TYPES } MetaDataSelectionType; > What is the point of having two clipboard operations possible independently > (by having one Wayland clipboard and one X11 clipboard in parallel). The struct Selection is meant to represent one selection (clipboard/dnd) to both sides, it maybe confuses you that it has both x11/wayland data pointers? it can certainly be only used on 1 direction at a time, as code goes. There can be META_N_DATA_SELECTION_TYPES Selection structs around, clipboard/dnd can conveivably go in opposite directions, but there can't be 2 clipboard/dnd selections around. At the risk of shoveling more in the todo without closing open questions, I'm attaching an additional patch that implements wayland->x11 DnD by adding a META_DATA_SELECTION_DND selection. As for x11->wayland DnD, I'm a bit in doubt about what's the best path ahead, if we want to forward events minding window stacking, I don't see many other options than backing the wayland surfaces with X11 Windows, I'm not sure we can abuse XdndProxy if we want to pick the window we're delivering messages to.
Created attachment 302587 [details] [review] xwayland: Implement wayland-to-X11 DnD Data device events are converted into Xdnd* messages, and selections take care of data transfers.
(In reply to Carlos Garnacho from comment #35) > (In reply to Jonas Ådahl from comment #34) > > (In reply to Carlos Garnacho from comment #33) > > > (In reply to Jonas Ådahl from comment #32) > > > > Review of attachment 301764 [details] [review] [review] [review] [review] [review]: > > > > > > > > ::: src/wayland/meta-xwayland-selection.c > > > > @@ +79,3 @@ > > > > + > > > > +struct _MetaXWaylandSelection { > > > > + Selection selections[META_N_DATA_SELECTION_TYPES]; > > > > > > > > What is the point of separating the selections into types? I didn't see it > > > > in any previous patch, it seems like a rather major change. I'd rather know > > > > the reasoning behind the change before re-reviewing things. > > > > > > Yes, it's a "big" change in how data has been shuffled in structs, but the > > > fundamentals are just the same. > > > > > > XDND uses selections for data transfer, and this will enable me to add an > > > extra slot for the DnD selection adding a META_DATA_SELECTION_DND to the > > > enum. As the life cycles of these selections are independent and can be > > > poked in parallel, it is better to keep those separated. One notable case > > > here is nautilus, it will poke (and possibly update) the clipboard selection > > > after dropping is performed, so there may be requests/updates on 2 > > > selections simultaneously. > > > > I can understand why you want to deal with DND and clipboard independently, > > but here you mix type and origin in the same enum which I find confusing. > > Maybe the names are too similar, but it's certainly 2 enums :): > > typedef enum { > META_DATA_SOURCE_TYPE_WAYLAND, > META_DATA_SOURCE_TYPE_X11, > META_N_DATA_SOURCE_TYPES > } MetaDataSourceType; > > typedef enum { > META_DATA_SELECTION_CLIPBOARD, > META_DATA_SELECTION_DND, > META_N_DATA_SELECTION_TYPES > } MetaDataSelectionType; > > > What is the point of having two clipboard operations possible independently > > (by having one Wayland clipboard and one X11 clipboard in parallel). > > The struct Selection is meant to represent one selection (clipboard/dnd) to > both sides, it maybe confuses you that it has both x11/wayland data > pointers? it can certainly be only used on 1 direction at a time, as code > goes. There can be META_N_DATA_SELECTION_TYPES Selection structs around, > clipboard/dnd can conveivably go in opposite directions, but there can't be > 2 clipboard/dnd selections around. Aah, things are clearing up. You are correct, I was confused the two. I think MetaDataSelectionType is a bit unnecessary to introduce in this patch. I'm no big fan of these "types" really. The selection source type I think can be replaced with broadcasts where the listener knows when to ignore it. That way we don't need to peek in previous selections to find out the previous type when unsetting and things like that. Regarding the selection types, I'd much rather see things being more explicit, i.e. passing around a pointer to a selection instead of a type that fetches the selection in the array. Then again, I suppose this is rather an design opinion than whether things are correct or not, so I won't "block" or anything like that. Other related comments: Is "Selection" really a good name. At a first look, I was not sure whether this was an internal data type or part of X11 (considering that it somewhat follows the naming convention of X11). Maybe name it to something making it look more mutter:y? > > At the risk of shoveling more in the todo without closing open questions, > I'm attaching an additional patch that implements wayland->x11 DnD by adding > a META_DATA_SELECTION_DND selection. > > As for x11->wayland DnD, I'm a bit in doubt about what's the best path > ahead, if we want to forward events minding window stacking, I don't see > many other options than backing the wayland surfaces with X11 Windows, I'm > not sure we can abuse XdndProxy if we want to pick the window we're > delivering messages to.
Review of attachment 301764 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +681,3 @@ + + if (!selection->wayland_selection) + return; This early exit is unnecessary since you always check this variable before entering this function.
(In reply to Jonas Ådahl from comment #37) > > Regarding the selection types, I'd much rather see things being more > explicit, i.e. passing around a pointer to a selection instead of a type > that fetches the selection in the array. > To expand a bit on this, what I had in mind, which I think would be more clear is: The Xwayland selection main struct would have one definition per type instead of an array. meta_wayland_data_source_new would take a pointer to a Selection as "void *user_data", meaning no need for data_source_to_selection_(type). atom_to_selection would just get the correct instance of Selection (instead of type). Instead of passing the type, we'd pass the Selection instance pointer. A selection instance would have its corresponding Atom (instead of selection_to_atom). The functions in meta-wayland-data-device would be explicitly per type. The benefits of this I'd say are: More clear state passing (pass state, not type which state can be derived from). No need to have a separate Atom array that relies on identical order. Possibility to extend types given what type (which is not possible in an array since they need to have the same size). Makes it easier to change to source type aware instances if we'd ever want to get rid of sometimes irrelevant fields. No need to have an invalid enum value as "invalid type" (-1). The only point I see with having them in an array is that we can iterate through the types, but that doesn't seem relevant when we'd only ever have at most 2.
Created attachment 302666 [details] [review] wayland: refactor MetaWaylandDataSource Expose it partly (in internal headers anyway), and pass a vtable for the data source functions, the wayland vfuncs just delegate operations on the wl_data_source resource. The resource has been also made optional, although it'll be present on all data sources from wayland clients. The ownership/lifetime of the DnD data source has also changed a bit, belonging now to the MetaWaylandDataSource like the selection one does, as we can't guarantee how long it will be needed after the grab is finished, it will be left inert and replaced the next time DnD is started at worst. This allows the creation of custom/proxy data sources, which will turn out useful for X11 selection interoperation.
Created attachment 302667 [details] [review] wayland: Add X11/wayland selection interoperation This piece of code hooks in both wl_data_device and the relevant X selection events, an X11 Window is set up so it can act as the clipboard owner when any wayland client owns the selection, reacting to SelectionRequest events, and returning the data from the wayland client FD to any X11 requestor through X properties. In the opposite direction, SelectionNotify messages are received, which results in the property contents being converted then written into the wayland requestor's FD. This code also takes care of the handling incremental transfers through the INCR property type, reading/writing data chunk by chunk.
Created attachment 302668 [details] [review] wayland: Refactor DnD target functions into MetaWaylandDragDestFuncs This will be useful in order to interact with drag dest surfaces in its windowing-specific ways, although everything defaults to the wayland vfuncs.
Created attachment 302669 [details] [review] xwayland: Implement wayland-to-X11 DnD X11 client windows now hook a X11-specific MetaWaylandDragDestFuncs that converts these into Xdnd* messages, and an additional selection bridge has been added to take care of XdndSelection, and the data transfers done through it.
(In reply to Jonas Ådahl from comment #37) > Regarding the selection types, I'd much rather see things being more > explicit, i.e. passing around a pointer to a selection instead of a type > that fetches the selection in the array. These last patches do something like this, I used two wl_signals in the end to notify about clipboard/dnd ownership changes, although the checks for x11/current selections are still a bit odd at places. > > Then again, I suppose this is rather an design opinion than whether things > are correct or not, so I won't "block" or anything like that. > > Other related comments: > > Is "Selection" really a good name. At a first look, I was not sure whether > this was an internal data type or part of X11 (considering that it somewhat > follows the naming convention of X11). Maybe name it to something making it > look more mutter:y? I chose Selection partly following X naming, a "selection" is underlying to both operations, and the struct is internal to meta-xwayland-selection.c. I renamed it to MetaSelectionBridge though to make it clearer. > > > > > At the risk of shoveling more in the todo without closing open questions, > > I'm attaching an additional patch that implements wayland->x11 DnD by adding > > a META_DATA_SELECTION_DND selection. > > > > As for x11->wayland DnD, I'm a bit in doubt about what's the best path > > ahead, if we want to forward events minding window stacking, I don't see > > many other options than backing the wayland surfaces with X11 Windows, I'm > > not sure we can abuse XdndProxy if we want to pick the window we're > > delivering messages to. As an update to this, I'm trying about adding a pseudo-grab that still lets the X source receive events to let XDND continue normally, and moving an internal X window over wayland surfaces as the pointer moves around, the internal window proxies then Xdnd* messages to wayland. There's still issues to fix (this is still WIP, not attached) but looks like a workable approach.
Created attachment 303274 [details] [review] wayland: Refactor DnD target functions into MetaWaylandDragDestFuncs This will be useful in order to interact with drag dest surfaces in its windowing-specific ways, although everything defaults to the wayland vfuncs.
Created attachment 303275 [details] [review] xwayland: Implement wayland-to-X11 DnD X11 client windows now hook a X11-specific MetaWaylandDragDestFuncs that converts these into Xdnd* messages, and an additional selection bridge has been added to take care of XdndSelection, and the data transfers done through it.
Created attachment 303276 [details] [review] xwayland: Implement X11-to-wayland DnD When DnD is started from an X11 client, mutter now sets up an special grab that 1) Ensures the drag source keeps receiving events, and 2) Moves an internal X Window over wayland clients as soon as the pointer enters over these. That window will act as the X-side peer for the currently focused wayland client, and will transform XdndEnter/Position/Leave/Drop messages into wayland actions. If DnD happens between X11 clients, the window will be moved away and unmapped, to let these operate as usual.
Review of attachment 302667 [details] [review]: Mostly looks good to me, only with a few nits. ::: src/wayland/meta-xwayland-selection.c @@ +49,3 @@ + gsize buffer_len; + guint incr : 1; +} WaylandSelectionData; nit: This (and X11SelectionData) are more or less the state during transfer(?). Should their naming maybe reflect that? @@ +56,3 @@ + GCancellable *cancellable; + gchar *mime_type; + guint selection : 3; Unused field. @@ +105,3 @@ + +static void +x11_selection_data_free (X11SelectionData *data) nit: Since this (and wayland_selection_data_free) does more than just free things (it cancels any ongoing transfer), maybe they should be called _destroy instead? @@ +850,3 @@ + { + XSetSelectionOwner (xdisplay, selection->selection_atom, + None, selection->timestamp); nit: incorrect indentation.
Review of attachment 303274 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +221,3 @@ return; + drag_focus_funcs_update (&drag_grab->drag_focus_funcs, surface); Not a fan of this function and manually manipulating a set of vfuncs whenever the focus type changes. My suggestion would be something more like: Introduce a DND 'focus' or 'destination' instance that is either a X11 focus or Wayland focus (similar to data source). Such an instance would have the appropriate vfuncs set when instantiating them. When we enter a window, we create a focus instance give the client type, set it as the current focus, and call enter on it. Motion events will then be sent via the object (and its vfuncs). Leaving a surface would invoke the leave vfunc and destroy the instance. I think this way is more clear, even though we maybe won't have any more state than the vfuncs yet (though maybe the MetaWaylandSurface/MetaWindow should be part of it). @@ +242,3 @@ + drag_grab->drag_focus_listener.notify = destroy_drag_focus; + wl_resource_add_destroy_listener (data_device_resource, &drag_grab->drag_focus_listener); + } Can't this be put in the Wayland focus_in function? (Same for removing the listener, but in focus_out). @@ +534,3 @@ + wl_fixed_t sx, sy; + + if (grab->drag_focus_data_device) Isn't this if redundant? How can a Wayland destination not have a data device and still receive motion events? (same for _drop). ::: src/wayland/meta-xwayland-selection.c @@ +65,3 @@ Window owner; Time timestamp; + MetaWaylandDataSource *source; /* owned by MetaWaylandDataDevice */ Hmm. Should this change maybe be in a previous commit?
Review of attachment 303275 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +82,3 @@ }; +#include "meta-xwayland-dnd.c" Including a .c? Would probably be a lot better to just create a meta-xwayland-selection-private.h or meta-xwayland-dnd.h and compile the two .c files separately like normal. @@ +463,3 @@ + { + xdnd_send_status (compositor->xwayland_manager.selection_data, + selection->owner, mime_type != NULL); Should this be part of the next patch maybe? @@ +932,3 @@ + selection->timestamp); + XFlush (xdisplay); + } This changes the semantics of this function a bit. Was the function originally assuming that we'd only get CLIPBOARD atoms, and this change is needed because we'd also get the XDND atom? I wonder because it looks a bit like this change really should just be amended into the original meta-xwayland-selection.c patch, but I might be missing something.
Review of attachment 303276 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ -370,3 @@ - - if (selection == &data->selection_data->dnd.selection) - xdnd_send_finished (data->selection_data, selection->owner, TRUE); Hmm. Should this move be part of the previous patch maybe? @@ +909,3 @@ + * makes us do the negotiation orderly on the X11 side. + */ + seat->pointer.default_grab.interface->focus (grab, surface); Not a fan of poking at internals of MetaWaylandPointer here. IMO they shouldn't even be exposed outside of meta-wayland-pointer.c. (same for other places doing this). Not even sure we should be sending anything to wl_pointer really during a drag. AFAIK no surface will have pointer focus during a drag (at least this is how it works in weston). Or is that something XDND relies on?
(In reply to Jonas Ådahl from comment #51) > Review of attachment 303276 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-selection.c > @@ -370,3 @@ > - > - if (selection == &data->selection_data->dnd.selection) > - xdnd_send_finished (data->selection_data, selection->owner, TRUE); > > Hmm. Should this move be part of the previous patch maybe? Right, there's been some shuffling, I might have missed something. > > @@ +909,3 @@ > + * makes us do the negotiation orderly on the X11 side. > + */ > + seat->pointer.default_grab.interface->focus (grab, surface); > > Not a fan of poking at internals of MetaWaylandPointer here. IMO they > shouldn't even be exposed outside of meta-wayland-pointer.c. (same for other > places doing this). Not even sure we should be sending anything to > wl_pointer really during a drag. AFAIK no surface will have pointer focus > during a drag (at least this is how it works in weston). Or is that > something XDND relies on? Absolutely, the X11 drag source still does perform DnD in its own terms, that is, grabbing the pointer, expecting all input from these, poking the window tree for the window underneath, and communicating with it through X client messages. Note that even the drag icon is X11's, moved around by the toolkit. All we do here is playing along with X11, this involves letting the app continue receiving pointer events as usual, and setting up a "drag dest" X Window where we can receive these messages on.
(In reply to Carlos Garnacho from comment #52) > (In reply to Jonas Ådahl from comment #51) > > Review of attachment 303276 [details] [review] [review] [review]: > > > > ::: src/wayland/meta-xwayland-selection.c > > @@ -370,3 @@ > > - > > - if (selection == &data->selection_data->dnd.selection) > > - xdnd_send_finished (data->selection_data, selection->owner, TRUE); > > > > Hmm. Should this move be part of the previous patch maybe? > > Right, there's been some shuffling, I might have missed something. > > > > > @@ +909,3 @@ > > + * makes us do the negotiation orderly on the X11 side. > > + */ > > + seat->pointer.default_grab.interface->focus (grab, surface); > > > > Not a fan of poking at internals of MetaWaylandPointer here. IMO they > > shouldn't even be exposed outside of meta-wayland-pointer.c. (same for other > > places doing this). Not even sure we should be sending anything to > > wl_pointer really during a drag. AFAIK no surface will have pointer focus > > during a drag (at least this is how it works in weston). Or is that > > something XDND relies on? > > Absolutely, the X11 drag source still does perform DnD in its own terms, > that is, grabbing the pointer, expecting all input from these, poking the > window tree for the window underneath, and communicating with it through X > client messages. Note that even the drag icon is X11's, moved around by the > toolkit. > > All we do here is playing along with X11, this involves letting the app > continue receiving pointer events as usual, and setting up a "drag dest" X > Window where we can receive these messages on. Ah, right. The source sends the drag position etc. I suppose we cannot start having the WM do that. Anyway, I think we should rather put up API in meta-wayland-pointer.h that does what the default grab functions do (i.e. call wl_poienter_... etc) instead of going through the internals of MetaWaylandPointer.
Attachment 302666 [details] pushed as 4b5f5ab - wayland: refactor MetaWaylandDataSource Attachment 302667 [details] pushed as 4fc1811 - wayland: Add X11/wayland selection interoperation
Created attachment 303548 [details] [review] wayland: Refactor DnD target functions into MetaWaylandDragDestFuncs This will be useful in order to interact with drag dest surfaces in its windowing-specific ways, although everything defaults to the wayland vfuncs.
Created attachment 303549 [details] [review] xwayland: Implement wayland-to-X11 DnD X11 client windows now hook a X11-specific MetaWaylandDragDestFuncs that converts these into Xdnd* messages, and an additional selection bridge has been added to take care of XdndSelection, and the data transfers done through it.
Created attachment 303550 [details] [review] xwayland: Implement X11-to-wayland DnD When DnD is started from an X11 client, mutter now sets up an special grab that 1) Ensures the drag source keeps receiving events, and 2) Moves an internal X Window over wayland clients as soon as the pointer enters over these. That window will act as the X-side peer for the currently focused wayland client, and will transform XdndEnter/Position/Leave/Drop messages into wayland actions. If DnD happens between X11 clients, the window will be moved away and unmapped, to let these operate as usual.
(In reply to Jonas Ådahl from comment #49) > Review of attachment 303274 [details] [review] [review]: > > ::: src/wayland/meta-wayland-data-device.c > @@ +221,3 @@ > return; > > + drag_focus_funcs_update (&drag_grab->drag_focus_funcs, surface); > > Not a fan of this function and manually manipulating a set of vfuncs > whenever the focus type changes. My suggestion would be something more like: > > Introduce a DND 'focus' or 'destination' instance that is either a X11 focus > or Wayland focus (similar to data source). Such an instance would have the > appropriate vfuncs set when instantiating them. > > When we enter a window, we create a focus instance give the client type, set > it as the current focus, and call enter on it. Motion events will then be > sent via the object (and its vfuncs). Leaving a surface would invoke the > leave vfunc and destroy the instance. > > I think this way is more clear, even though we maybe won't have any more > state than the vfuncs yet (though maybe the MetaWaylandSurface/MetaWindow > should be part of it). I'm not sure introducing a new object makes a lot of sense if all relevant state is somewhere else (spread around MetaWaylandDragGrab, MetaWaylandDataDevice, MetaWaylandSurface, ...). In the last iteration the vfuncs are in MetaWaylandSurface, this also seems cleaner than updating the vfuncs on the fly. > > @@ +242,3 @@ > + drag_grab->drag_focus_listener.notify = destroy_drag_focus; > + wl_resource_add_destroy_listener (data_device_resource, > &drag_grab->drag_focus_listener); > + } > > Can't this be put in the Wayland focus_in function? (Same for removing the > listener, but in focus_out). Indeed, did that. > > @@ +534,3 @@ > + wl_fixed_t sx, sy; > + > + if (grab->drag_focus_data_device) > > Isn't this if redundant? How can a Wayland destination not have a data > device and still receive motion events? (same for _drop). Right, these were kept around when moving that code, it's true that it should not happen as code is laid out now. > > ::: src/wayland/meta-xwayland-selection.c > @@ +65,3 @@ > Window owner; > Time timestamp; > + MetaWaylandDataSource *source; /* owned by MetaWaylandDataDevice */ > > Hmm. Should this change maybe be in a previous commit? I guess I missed that boat... this is actually needed by the last of the remaining commits, I moved that over there. (In reply to Jonas Ådahl from comment #50) > Review of attachment 303275 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-selection.c > @@ +82,3 @@ > }; > > +#include "meta-xwayland-dnd.c" > > Including a .c? Would probably be a lot better to just create a > meta-xwayland-selection-private.h or meta-xwayland-dnd.h and compile the two > .c files separately like normal. I'm quite undecided here... meta-xwayland-selection/dnd.c are both poking each other internals (xdnd_atoms[] on one side, MetaWaylandDragGrab on the other), and the xdnd_send_* wouldn't be as concise I guess. I'd rather have this put together in meta-xwayland-selection.c, the last patches don't do anything about it yet. > > @@ +463,3 @@ > + { > + xdnd_send_status (compositor->xwayland_manager.selection_data, > + selection->owner, mime_type != NULL); > > Should this be part of the next patch maybe? Right > > @@ +932,3 @@ > + selection->timestamp); > + XFlush (xdisplay); > + } > > This changes the semantics of this function a bit. Was the function > originally assuming that we'd only get CLIPBOARD atoms, and this change is > needed because we'd also get the XDND atom? I wonder because it looks a bit > like this change really should just be amended into the original > meta-xwayland-selection.c patch, but I might be missing something. This is indeed a refactor wrt XDND, but you're right that the original code would have been more friendly right away to this refactor.... I've put this together with the XdndSelection atom handling in the next patch.
(In reply to Carlos Garnacho from comment #58) > I'm quite undecided here... meta-xwayland-selection/dnd.c are both poking > each other internals (xdnd_atoms[] on one side, MetaWaylandDragGrab on the ^MetaSelectionBridge
Created attachment 303684 [details] [review] xwayland: Implement wayland-to-X11 DnD X11 client windows now hook a X11-specific MetaWaylandDragDestFuncs that converts these into Xdnd* messages, and an additional selection bridge has been added to take care of XdndSelection, and the data transfers done through it.
Created attachment 303685 [details] [review] xwayland: Implement X11-to-wayland DnD When DnD is started from an X11 client, mutter now sets up an special grab that 1) Ensures the drag source keeps receiving events, and 2) Moves an internal X Window over wayland clients as soon as the pointer enters over these. That window will act as the X-side peer for the currently focused wayland client, and will transform XdndEnter/Position/Leave/Drop messages into wayland actions. If DnD happens between X11 clients, the window will be moved away and unmapped, to let these operate as usual.
Review of attachment 303548 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +330,3 @@ wl_container_of (listener, drag_grab, drag_data_source_listener); + drag_grab->drag_data_source = drag_grab->seat->data_device.dnd_data_source = NULL; nit: This makes it easily missed that you clear two semi-related pointers. I'd prefer non-trivial (i.e. not only i = j = 0; etc) assignments to be on separate lines. ::: src/wayland/meta-wayland-surface.c @@ +753,3 @@ +{ + surface->drag_dest_funcs = meta_wayland_data_device_get_drag_dest_funcs (); +} To have a "sync role dependent vfuncs" function won't work I think. I'd suggest adding a "role set" signal, so that we won't end up with surfaces switching between drag functions. A "role set" handler could initialize the state that was not possible to initialize prior to that signal being sent. Or maybe just set it when setting the role directly, using switch (role) { ... }; Seems we don't yet have nor set a "Xwayland window" role. So that needs to be fixed prior to that. ::: src/wayland/meta-wayland-surface.h @@ +101,3 @@ int32_t offset_x, offset_y; GList *subsurfaces; + const MetaWaylandDragDestFuncs *drag_dest_funcs; Could we do as with subsurfaces and popups and add this into a isolated anonymous struct? like struct { const MetaWa... ...funcs; } dnd;
Review of attachment 303684 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +757,3 @@ + surface->drag_dest_funcs = meta_xwayland_selection_get_drag_dest_funcs (); + else + surface->drag_dest_funcs = meta_wayland_data_device_get_drag_dest_funcs (); nit: think this logic (or a future switch (role) { .. }) maybe rather belongs in meta-wayland-data-device.c. But maybe not, not sure. ::: src/wayland/meta-xwayland-selection.c @@ +81,3 @@ }; +#include "meta-xwayland-dnd.c" I still want to object to this practice. The only semi valid use cases I've seen for including .c files has been when they have been generated in a way making it very inconvenient compiling them separately, but then they have even been called something like .inl or .inc to make this obvious. But since making meta-xwayland-dnd.c a normal C file would probably only require introducing a .h file and maybe moving some structs etc, I see no point in going down this road. @@ +843,3 @@ + if (event->message_type == xdnd_atoms[ATOM_DND_STATUS]) + { + if (event->data.l[1] & 1) question: do you think its useful to add the meaning of this? I had to look at the protocol to understand this line. For a future code reader that don't know the XDND protocol by heart, this will probably happen to them to. @@ +851,3 @@ + if (data_source) + data_source->has_target = TRUE; + } Don't we need an else here and set data_source->has_target to FALSE? @@ +858,3 @@ + { + meta_wayland_data_device_set_dnd_source (&compositor->seat->data_device, + NULL); Couldn't his potentially unset the wrong DND source?
Review of attachment 303685 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +412,3 @@ + + if (icon_surface->resource) + { This check is not needed right? How could a MetaWaylandSurface not have a wl_surface resource? ::: src/wayland/meta-xwayland-dnd.c @@ +79,3 @@ + + xev.xclient.data.l[0] = selection->dnd_window; + xev.xclient.data.l[1] = 2; /* Dest wants XdndPosition messages */ nit: Can we make this a 1 << 1 instead? So that its obvious its bit 1, not the number 2, that is set. @@ +84,3 @@ + { + xev.xclient.data.l[1] |= 1; /* Dest accepts the drop */ + xev.xclient.data.l[4] = xdnd_atoms[ATOM_DND_ACTION_COPY]; Hmm. Just curious. When was it concluded that the default action is copy? weston-dnd seems to "move" its blobs when DND:ing. ::: src/wayland/meta-xwayland-selection.c @@ +419,3 @@ + if (selection == &selection->x11_selection->selection_data->dnd.selection) + xdnd_send_finished (selection->x11_selection->selection_data, + selection->owner, TRUE); Do we need to send xdnd_send_finished with a failer when g_clear_pointer (&selection->x11_selection, ...) in meta_x11_source_send? Maybe we should replace all g_clear_pointer (&selection->x11_selection, (GDestroyNotify) x11_selection_data_free); with either a transfer_completed or a transfer_failed which calls g_clear_pointer and xdnd_send_finished accordingly? @@ +865,3 @@ + +static void +drop_position_repick (MetaWaylandCompositor *compositor, bikeshed: maybe repick_drop_surface is a better name? @@ +892,3 @@ + { + XMoveResizeWindow (xdisplay, dnd->dnd_window, -1, -1, 1, 1); + XUnmapWindow (xdisplay, dnd->dnd_window); Do we ever unmap internal dnd window other than when moving the pointer away from a surface? Shouldn't we? @@ +979,3 @@ + if (event->message_type == xdnd_atoms[ATOM_DND_ENTER]) + { + if (!(event->data.l[1] & 1)) Here as well. A note about what the protocol say would be helpful. @@ +1015,3 @@ + clutter_event_set_coords (motion, pos.x, pos.y); + clutter_event_set_device (motion, seat->pointer.device); + clutter_event_set_source_device (motion, seat->pointer.device); This doesn't set the event time. Not sure how you would, since the clock used is backend dependent. Hmm. This makes me wonder if ClutterEvent is the right thing to pass to drag_dest_motion, since we only need a position and a time. Still the time issue; I don't know of a proper way to get he correct event time. If we happen to use the native backend, then it'd be CLOCK_MONOTONIC since its what libinput requests, but running nested, I suspect it'd not be the correct time. @@ +1088,3 @@ + selection->timestamp); + XFlush (xdisplay); + } It'd be easier if this part would be its own commit, but meh.
(In reply to Jonas Ådahl from comment #62) > Review of attachment 303548 [details] [review] [review]: > > ::: src/wayland/meta-wayland-data-device.c > @@ +330,3 @@ > wl_container_of (listener, drag_grab, drag_data_source_listener); > > + drag_grab->drag_data_source = > drag_grab->seat->data_device.dnd_data_source = NULL; > > nit: This makes it easily missed that you clear two semi-related pointers. > I'd prefer non-trivial (i.e. not only i = j = 0; etc) assignments to be on > separate lines. Sure. > > ::: src/wayland/meta-wayland-surface.c > @@ +753,3 @@ > +{ > + surface->drag_dest_funcs = meta_wayland_data_device_get_drag_dest_funcs > (); > +} > > To have a "sync role dependent vfuncs" function won't work I think. I'd > suggest adding a "role set" signal, so that we won't end up with surfaces > switching between drag functions. > > A "role set" handler could initialize the state that was not possible to > initialize prior to that signal being sent. > > Or maybe just set it when setting the role directly, using switch (role) { > ... }; > > Seems we don't yet have nor set a "Xwayland window" role. So that needs to > be fixed prior to that. The sync_drag_dest_funcs() maybe a bit of a misnomer, although coherent with the other similar functions around. It is expected to be called very early during initialization, and never change after. Can be called twice though (one on surface creation, other on meta_wayland_surface_set_window), the latter is not mandatory in eg. subsurfaces AFAICT. Also, note that this is not dependent on the role, but on the client type, as seen in the following patches. > > ::: src/wayland/meta-wayland-surface.h > @@ +101,3 @@ > int32_t offset_x, offset_y; > GList *subsurfaces; > + const MetaWaylandDragDestFuncs *drag_dest_funcs; > > Could we do as with subsurfaces and popups and add this into a isolated > anonymous struct? like struct { const MetaWa... ...funcs; } dnd; Sure. (In reply to Jonas Ådahl from comment #63) > Review of attachment 303684 [details] [review] [review]: > > ::: src/wayland/meta-wayland-surface.c > @@ +757,3 @@ > + surface->drag_dest_funcs = meta_xwayland_selection_get_drag_dest_funcs > (); > + else > + surface->drag_dest_funcs = meta_wayland_data_device_get_drag_dest_funcs > (); > > nit: think this logic (or a future switch (role) { .. }) maybe rather > belongs in meta-wayland-data-device.c. But maybe not, not sure. I was undecided there too, you first suggested a "DND 'focus' or 'destination' instance" containing surface/funcs, but if the mapping between these is always 1:1, the data pointer rather feels like belonging to the surface, as for the initialization... > > ::: src/wayland/meta-xwayland-selection.c > @@ +81,3 @@ > }; > > +#include "meta-xwayland-dnd.c" > > I still want to object to this practice. The only semi valid use cases I've > seen for including .c files has been when they have been generated in a way > making it very inconvenient compiling them separately, but then they have > even been called something like .inl or .inc to make this obvious. > > But since making meta-xwayland-dnd.c a normal C file would probably only > require introducing a .h file and maybe moving some structs etc, I see no > point in going down this road. I've finally folded meta-xwayland-dnd.c into meta-xwayland-selection.c. All in all, it was just 290 LOC, meta-xwayland-selection.c is going to be the only caller ever, and it requires shuffling quite some structs around that can be perfectly kept private in meta-xwayland-selection.c. > > @@ +843,3 @@ > + if (event->message_type == xdnd_atoms[ATOM_DND_STATUS]) > + { > + if (event->data.l[1] & 1) > > question: do you think its useful to add the meaning of this? I had to look > at the protocol to understand this line. For a future code reader that don't > know the XDND protocol by heart, this will probably happen to them to. I've added comments close to all the obscure bit manipulation I've spotted. > > @@ +851,3 @@ > + if (data_source) > + data_source->has_target = TRUE; > + } > > Don't we need an else here and set data_source->has_target to FALSE? Right, we should be doing that if that bit is 0, I've changed that. > > @@ +858,3 @@ > + { > + meta_wayland_data_device_set_dnd_source > (&compositor->seat->data_device, > + NULL); > > Couldn't his potentially unset the wrong DND source? Hmm, it would be very evil of apps to send out-of-band XdndFinished messages. I'm unsure what check can we do here, the drop site/data requestor windows don't survive as long (this is after the transfer is finished), and we'd still be relying on the message's data.l[0] to be accurate. As a rough shot, I'm currently checking whether this isn't happening mid-grab, that's a fine enough paranoia check. Generally, this function might be doing a better work at tracking whether windows match with expectancies, and message ordering makes sense. I would suggest to handle the pesimistic cases in a separate bug though. (In reply to Jonas Ådahl from comment #64) > Review of attachment 303685 [details] [review] [review]: > > ::: src/wayland/meta-wayland-data-device.c > @@ +412,3 @@ > + > + if (icon_surface->resource) > + { > > This check is not needed right? How could a MetaWaylandSurface not have a > wl_surface resource? Right, removed. > > ::: src/wayland/meta-xwayland-dnd.c > @@ +79,3 @@ > + > + xev.xclient.data.l[0] = selection->dnd_window; > + xev.xclient.data.l[1] = 2; /* Dest wants XdndPosition messages */ > > nit: Can we make this a 1 << 1 instead? So that its obvious its bit 1, not > the number 2, that is set. Sure, done. > > @@ +84,3 @@ > + { > + xev.xclient.data.l[1] |= 1; /* Dest accepts the drop */ > + xev.xclient.data.l[4] = xdnd_atoms[ATOM_DND_ACTION_COPY]; > > Hmm. Just curious. When was it concluded that the default action is copy? > weston-dnd seems to "move" its blobs when DND:ing. I didn't think weston-dnd is much else than a demo app :). My reasonings were: - It is the default in GTK+ for eg. text across widgets/clients - gdk/wayland also defaults internally to it currently. - It is more universal than move, drag sources usually offer copy/move or copy, but barely ever move alone. - Also proved safer in my testing :) (hint: nautilus was involved) FYI, I've got further patches locally adding DnD actions v2 on top of all of this, so actions are also translated both ways. I would really like to consider "copy as default" a temporary measure :). > > ::: src/wayland/meta-xwayland-selection.c > @@ +419,3 @@ > + if (selection == > &selection->x11_selection->selection_data->dnd.selection) > + xdnd_send_finished (selection->x11_selection->selection_data, > + selection->owner, TRUE); > > Do we need to send xdnd_send_finished with a failer when g_clear_pointer > (&selection->x11_selection, ...) in meta_x11_source_send? > > Maybe we should replace all g_clear_pointer (&selection->x11_selection, > (GDestroyNotify) x11_selection_data_free); with either a transfer_completed > or a transfer_failed which calls g_clear_pointer and xdnd_send_finished > accordingly? I've refactored that now into a x11_selection_data_finish(). > > @@ +865,3 @@ > + > +static void > +drop_position_repick (MetaWaylandCompositor *compositor, > > bikeshed: maybe repick_drop_surface is a better name? Sure :), renamed. > > @@ +892,3 @@ > + { > + XMoveResizeWindow (xdisplay, dnd->dnd_window, -1, -1, 1, 1); > + XUnmapWindow (xdisplay, dnd->dnd_window); > > Do we ever unmap internal dnd window other than when moving the pointer away > from a surface? Shouldn't we? Right, good catch. I'm now unmapping it as soon as DnD finishes. > > @@ +1015,3 @@ > + clutter_event_set_coords (motion, pos.x, pos.y); > + clutter_event_set_device (motion, seat->pointer.device); > + clutter_event_set_source_device (motion, seat->pointer.device); > > This doesn't set the event time. Not sure how you would, since the clock > used is backend dependent. Hmm. > > This makes me wonder if ClutterEvent is the right thing to pass to > drag_dest_motion, since we only need a position and a time. Still the time > issue; I don't know of a proper way to get he correct event time. If we > happen to use the native backend, then it'd be CLOCK_MONOTONIC since its > what libinput requests, but running nested, I suspect it'd not be the > correct time. Right, we could be relying on the timestamp contained in the XdndPosition message, but that has the same translation issues on non-nested. I'm going for storing the last motion event timestamp and using it there, this XdndPosition message and its timestamp will be a result of it, so looks like a safe bet. I've done nothing about passing anything else than ClutterEvent though. > > @@ +1088,3 @@ > + selection->timestamp); > + XFlush (xdisplay); > + } > > It'd be easier if this part would be its own commit, but meh. I'm putting this in a separate commit. Patches coming :).
Created attachment 304190 [details] [review] wayland: Refactor DnD target functions into MetaWaylandDragDestFuncs This will be useful in order to interact with drag dest surfaces in its windowing-specific ways, although everything defaults to the wayland vfuncs.
Created attachment 304191 [details] [review] xwayland: Refactor XFixesSelectionNotifyEvent handler Prepare it for more selection atoms (i.e. XdndSelection) to come.
Created attachment 304192 [details] [review] xwayland: Implement wayland-to-X11 DnD X11 client windows now hook a X11-specific MetaWaylandDragDestFuncs that converts these into Xdnd* messages, and an additional selection bridge has been added to take care of XdndSelection, and the data transfers done through it.
Created attachment 304193 [details] [review] xwayland: Implement X11-to-wayland DnD When DnD is started from an X11 client, mutter now sets up an special grab that 1) Ensures the drag source keeps receiving events, and 2) Moves an internal X Window over wayland clients as soon as the pointer enters over these. That window will act as the X-side peer for the currently focused wayland client, and will transform XdndEnter/Position/Leave/Drop messages into wayland actions. If DnD happens between X11 clients, the window will be moved away and unmapped, to let these operate as usual.
(In reply to Carlos Garnacho from comment #65) > (In reply to Jonas Ådahl from comment #62) > > > > To have a "sync role dependent vfuncs" function won't work I think. I'd > > suggest adding a "role set" signal, so that we won't end up with surfaces > > switching between drag functions. > > > > A "role set" handler could initialize the state that was not possible to > > initialize prior to that signal being sent. > > > > Or maybe just set it when setting the role directly, using switch (role) { > > ... }; > > > > Seems we don't yet have nor set a "Xwayland window" role. So that needs to > > be fixed prior to that. > > The sync_drag_dest_funcs() maybe a bit of a misnomer, although coherent with > the other similar functions around. It is expected to be called very early > during initialization, and never change after. Can be called twice though > (one on surface creation, other on meta_wayland_surface_set_window), the > latter is not mandatory in eg. subsurfaces AFAICT. > > Also, note that this is not dependent on the role, but on the client type, > as seen in the following patches. A role is also dependent on the client really. Just that we don't set a role for xwayland surfaces. I'm fine with leaving for the future since we'd have to do unrelated fixing to be able to depend on roles. > > > > Hmm. Just curious. When was it concluded that the default action is copy? > > weston-dnd seems to "move" its blobs when DND:ing. > > I didn't think weston-dnd is much else than a demo app :). My reasonings > were: > > - It is the default in GTK+ for eg. text across widgets/clients > - gdk/wayland also defaults internally to it currently. > - It is more universal than move, drag sources usually offer copy/move or > copy, but barely ever move alone. > - Also proved safer in my testing :) (hint: nautilus was involved) > > FYI, I've got further patches locally adding DnD actions v2 on top of all of > this, so actions are also translated both ways. I would really like to > consider "copy as default" a temporary measure :). > Fair enough. > > > > @@ +1015,3 @@ > > + clutter_event_set_coords (motion, pos.x, pos.y); > > + clutter_event_set_device (motion, seat->pointer.device); > > + clutter_event_set_source_device (motion, seat->pointer.device); > > > > This doesn't set the event time. Not sure how you would, since the clock > > used is backend dependent. Hmm. > > > > This makes me wonder if ClutterEvent is the right thing to pass to > > drag_dest_motion, since we only need a position and a time. Still the time > > issue; I don't know of a proper way to get he correct event time. If we > > happen to use the native backend, then it'd be CLOCK_MONOTONIC since its > > what libinput requests, but running nested, I suspect it'd not be the > > correct time. > > Right, we could be relying on the timestamp contained in the XdndPosition > message, but that has the same translation issues on non-nested. I'm going > for storing the last motion event timestamp and using it there, this > XdndPosition message and its timestamp will be a result of it, so looks like > a safe bet. > > I've done nothing about passing anything else than ClutterEvent though. Alright. I suppose it is not that bad, since we only use the parts from the event we know we set.
Review of attachment 304190 [details] [review]: Looks good to me now.
Review of attachment 304191 [details] [review]: Seems alright.
Review of attachment 304192 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +712,3 @@ + if (source->mime_types.size != 0) + return TRUE; + The only time I see meta_xwayland_data_source_fetch_mimetype_list() being called is when the source is newly created. I assume this means that this check is redundant and should be removed. After taking a sneak peek at the next patch, it seems this chunk maybe leaked over from there?
Review of attachment 304193 [details] [review]: ::: src/wayland/meta-xwayland-selection.c @@ +1255,3 @@ + mimetype = gdk_x11_get_xatom_name (event->data.l[i]); + meta_wayland_data_source_add_mime_type (dnd->selection.source, + mimetype); In the fetch in the else block below you early-out if the mime types are already fetched. Do you need to do something like that here as well?
(In reply to Jonas Ådahl from comment #70) > (In reply to Carlos Garnacho from comment #65) > > (In reply to Jonas Ådahl from comment #62) > > > > > > To have a "sync role dependent vfuncs" function won't work I think. I'd > > > suggest adding a "role set" signal, so that we won't end up with surfaces > > > switching between drag functions. > > > > > > A "role set" handler could initialize the state that was not possible to > > > initialize prior to that signal being sent. > > > > > > Or maybe just set it when setting the role directly, using switch (role) { > > > ... }; > > > > > > Seems we don't yet have nor set a "Xwayland window" role. So that needs to > > > be fixed prior to that. > > > > The sync_drag_dest_funcs() maybe a bit of a misnomer, although coherent with > > the other similar functions around. It is expected to be called very early > > during initialization, and never change after. Can be called twice though > > (one on surface creation, other on meta_wayland_surface_set_window), the > > latter is not mandatory in eg. subsurfaces AFAICT. > > > > Also, note that this is not dependent on the role, but on the client type, > > as seen in the following patches. > > A role is also dependent on the client really. Just that we don't set a role > for xwayland surfaces. I'm fine with leaving for the future since we'd have > to do unrelated fixing to be able to depend on roles. Right. I expected the xwayland surfaces role to be dependent on xwayland as a client tbh. I agree this needs tidying up in the future. (In reply to Jonas Ådahl from comment #73) > Review of attachment 304192 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-selection.c > @@ +712,3 @@ > + if (source->mime_types.size != 0) > + return TRUE; > + > > The only time I see meta_xwayland_data_source_fetch_mimetype_list() being > called is when the source is newly created. I assume this means that this > check is redundant and should be removed. > > After taking a sneak peek at the next patch, it seems this chunk maybe > leaked over from there? Right :). On the next patch we create a MetaWaylandDataSource which persists for the whole X11 drag. XDND announces the mimetype list on each XdndEnter, but as we are playing on behalf of all wayland clients, we must set these up just the first time. (In reply to Jonas Ådahl from comment #74) > Review of attachment 304193 [details] [review] [review]: > > ::: src/wayland/meta-xwayland-selection.c > @@ +1255,3 @@ > + mimetype = gdk_x11_get_xatom_name (event->data.l[i]); > + meta_wayland_data_source_add_mime_type > (dnd->selection.source, > + mimetype); > > In the fetch in the else block below you early-out if the mime types are > already fetched. Do you need to do something like that here as well? Oops, right. I added a similar check around. As per our IRC talk, I'm fixing this up, pushing to master and (finally!) closing this bug :).
Attachment 304190 [details] pushed as f53eea2 - wayland: Refactor DnD target functions into MetaWaylandDragDestFuncs Attachment 304191 [details] pushed as b449ba9 - xwayland: Refactor XFixesSelectionNotifyEvent handler Attachment 304192 [details] pushed as ccb7833 - xwayland: Implement wayland-to-X11 DnD Attachment 304193 [details] pushed as 4a968c3 - xwayland: Implement X11-to-wayland DnD
Thanks for the fix! Will it get backported to 3.16?