GNOME Bugzilla – Bug 762560
Implement primary selection protocol
Last modified: 2016-02-28 19:56:40 UTC
I'm adding some patches to implement a (so far private) gtk-primary-selection protocol, this is a mirror of the protocol being proposed for wayland-protocols, just with prefix/suffix changes so we can jump on it later more or less seamlessly. This adds the ability to middle-click paste between/across gtk+-wayland and x11 apps.
Created attachment 322170 [details] [review] wayland: Store pointer button press serial click_serial is currently unused, store the button press serial in it.
Created attachment 322171 [details] [review] wayland: Add gtk-primary-selection protocol This protocol is an internal mirror of the primary selection drafts being proposed for wayland-protocols. No changes besides prefix/suffix changes.
Created attachment 322172 [details] [review] wayland: Implement the (so far internal) primary selection protocol Add an additional MetaWaylandDataSource implementation for primary selection sources, and methods to set primary selection offers. Primary selection sets altogether a different channel than the clipboard selection, those don't cross in any way. Also, the bridge for the X11 PRIMARY selection atom has been added, which adds all the necessary handling to translate primary selection both ways with wayland and X11 applications.
Review of attachment 322170 [details] [review]: Why only on press? Why shouldn't it be just as fine to do it on button-release?
Review of attachment 322171 [details] [review]: Looks good.
Review of attachment 322171 [details] [review]: Ah, one changed needed: add .gitignore entries for the generated files.
Review of attachment 322172 [details] [review]: Looks mostly good. Comments below. ::: src/wayland/meta-wayland-data-device-private.h @@ +36,3 @@ + meta_wayland_data_source_primary, + META, WAYLAND_DATA_SOURCE_PRIMARY, + MetaWaylandDataSourceWayland); Should we maybe also make the "regular" data source its own type. Then no class would override some others functions and we'd have a clearer split. The base class would do everything that would work the same way. ::: src/wayland/meta-wayland-data-device.c @@ +466,3 @@ +static const struct gtk_primary_selection_offer_interface primary_offer_interface = { + primary_offer_receive, + data_offer_destroy, I know this is for convenience, but think could be potentially dangerous if for some reason we add something to that function. By looking at the name, I wouldn't expect it to be used anywhere else. @@ +632,3 @@ + data_source_offer, + data_source_destroy, +}; The same here. The data_source_destroy is easy enough to just add a special version. The offer is slightly more complex, so I think it'd be more safe to abstract the definition of that function to something like static void meta_wayland_data_source_wayland (MetaWaylandDataSourceWayland *source_wayland) { if (!meta_wayland_data_source_add_mime_type (META_WAYLAND_DATA_SOURCE (source_wayland), type)) wl_resource_post_no_memory (source_wayland->resource); } and call that from the respective data_source_offer and primary_source_offer functions. @@ +1489,3 @@ + MetaWaylandSeat *seat = wl_container_of (data_device, seat, data_device); + struct wl_resource *data_device_resource, *offer; + struct wl_client *focus_client; I think we should consider adding a serial to the primary_data_davecie.set_selection request, that works in the same way as the one of wl_data_device.set_selection. I also think both wl_data_device.set_selection and gtk/wp_primary_selection_data_device.set_selection handlers should also check that for the given serial, the client still has an active focus on that device type. Otherwise a client can fake the serial and successfully update the current data source. @@ +1497,3 @@ + primary_source_destroyed, + data_device); + data_device->primary_data_source = NULL; nit: Don't need this line since you do it unconditionally three lines below. @@ +1546,3 @@ +static const struct gtk_primary_selection_device_interface primary_device_interface = { + primary_device_set_selection, + data_device_release, Same note about interface event handlers. An alternative to duplicating all the only-do-wl_resource_destroy()-handlers would be to have a generic one with a generic name and set that instead everywhere. Like "default_destructor" or something. @@ +1620,3 @@ +primary_device_manager_destroy (struct wl_client *client, + struct wl_resource *manager_resource) +{ Why doesn't this destroy the resource? @@ +1704,3 @@ + } + else + wl_data_device_send_selection (data_device_resource, NULL); nit: Coding style inconsistent with what is below. IIRC the one below is the correct one. ::: src/wayland/meta-xwayland-selection.c @@ +1066,3 @@ + else if (selection->selection_atom == gdk_x11_get_xatom_by_name ("PRIMARY")) + { + meta_wayland_data_device_set_primary (&compositor->seat->data_device, data_source); Same here about adding a serial to "set_selection".
(In reply to Jonas Ådahl from comment #4) > Review of attachment 322170 [details] [review] [review]: > > Why only on press? Why shouldn't it be just as fine to do it on > button-release? Yeah it could. One problem I found though is the press-middle-button-to-scroll handling libinput does for trackpoint buttons. The button press and release are sent at the same time just in case the middle button press would produce scrolling, both are handled fast enough in mutter, so at the time the client replies from the press wl_pointer.button event, mutter already has the button release serial stored. I could perhaps store these two separately, and check both when handling the paste. Leaving at the moment as is, can amend before pushing. (In reply to Jonas Ådahl from comment #7) > Review of attachment 322172 [details] [review] [review]: > > Looks mostly good. Comments below. > > ::: src/wayland/meta-wayland-data-device-private.h > @@ +36,3 @@ > + meta_wayland_data_source_primary, > + META, WAYLAND_DATA_SOURCE_PRIMARY, > + MetaWaylandDataSourceWayland); > > Should we maybe also make the "regular" data source its own type. > > Then no class would override some others functions and we'd have a clearer > split. The base class would do everything that would work the same way. Done. > > ::: src/wayland/meta-wayland-data-device.c > @@ +466,3 @@ > +static const struct gtk_primary_selection_offer_interface > primary_offer_interface = { > + primary_offer_receive, > + data_offer_destroy, > > I know this is for convenience, but think could be potentially dangerous if > for some reason we add something to that function. By looking at the name, I > wouldn't expect it to be used anywhere else. I've gone for the generic destroy handler that you recommended below. > > @@ +632,3 @@ > + data_source_offer, > + data_source_destroy, > +}; > > The same here. The data_source_destroy is easy enough to just add a special > version. The offer is slightly more complex, so I think it'd be more safe to > abstract the definition of that function to something like > > static void > meta_wayland_data_source_wayland (MetaWaylandDataSourceWayland > *source_wayland) > { > if (!meta_wayland_data_source_add_mime_type (META_WAYLAND_DATA_SOURCE > (source_wayland), > type)) > wl_resource_post_no_memory (source_wayland->resource); > } nah, IMO doesn't make sense for a two-liner. Made a separate primary_source_offer(). > @@ +1489,3 @@ > + MetaWaylandSeat *seat = wl_container_of (data_device, seat, data_device); > + struct wl_resource *data_device_resource, *offer; > + struct wl_client *focus_client; > > I think we should consider adding a serial to the > primary_data_davecie.set_selection request, that works in the same way as > the one of wl_data_device.set_selection. > > I also think both wl_data_device.set_selection and > gtk/wp_primary_selection_data_device.set_selection handlers should also > check that for the given serial, the client still has an active focus on > that device type. Otherwise a client can fake the serial and successfully > update the current data source. Agreed, replicated the same serial checking there is for core protocol selection. I'm not too certain on the serial checking there is there though... > > @@ +1497,3 @@ > + primary_source_destroyed, > + data_device); > + data_device->primary_data_source = NULL; > > nit: Don't need this line since you do it unconditionally three lines below. Right > @@ +1620,3 @@ > +primary_device_manager_destroy (struct wl_client *client, > + struct wl_resource *manager_resource) > +{ > > Why doesn't this destroy the resource? Forgotten :). Now using the same generic destroy handler. > > @@ +1704,3 @@ > + } > + else > + wl_data_device_send_selection (data_device_resource, NULL); > > nit: Coding style inconsistent with what is below. IIRC the one below is the > correct one. Right. > > ::: src/wayland/meta-xwayland-selection.c > @@ +1066,3 @@ > + else if (selection->selection_atom == gdk_x11_get_xatom_by_name > ("PRIMARY")) > + { > + meta_wayland_data_device_set_primary > (&compositor->seat->data_device, data_source); > > Same here about adding a serial to "set_selection". Replicated again the same we do for the CLIPBOARD selection.
Created attachment 322417 [details] [review] wayland: Add gtk-primary-selection protocol This protocol is an internal mirror of the primary selection drafts being proposed for wayland-protocols. No changes besides prefix/suffix changes.
Created attachment 322418 [details] [review] wayland: Implement the (so far internal) primary selection protocol Add an additional MetaWaylandDataSource implementation for primary selection sources, and methods to set primary selection offers. Primary selection sets altogether a different channel than the clipboard selection, those don't cross in any way. Also, the bridge for the X11 PRIMARY selection atom has been added, which adds all the necessary handling to translate primary selection both ways with wayland and X11 applications.
(In reply to Carlos Garnacho from comment #8) > (In reply to Jonas Ådahl from comment #4) > > Review of attachment 322170 [details] [review] [review] [review]: > > > > Why only on press? Why shouldn't it be just as fine to do it on > > button-release? > > Yeah it could. One problem I found though is the > press-middle-button-to-scroll handling libinput does for trackpoint buttons. > The button press and release are sent at the same time just in case the > middle button press would produce scrolling, both are handled fast enough in > mutter, so at the time the client replies from the press wl_pointer.button > event, mutter already has the button release serial stored. > > I could perhaps store these two separately, and check both when handling the > paste. Leaving at the moment as is, can amend before pushing. Hmm, I see. That I suppose could be problematic if one clicks three times, expecting three pastes, but due to some lag, one only receives one or two. If we keep a "range" of serials from click/press/touch per device, we could match against those, but then we wouldn't know if a certain serial should drop the request since those ranges might overlap. That'd mean we need to store each serial that was sent per device during the time it had focus, which doesn't seem reasonable. But at the same time it doesn't seem to be reasonable to limit to button-down. I just tested with xterm and firefox, and paste seems to happen on button-up for example. What situations are we trying to avoid with the serials anyway?
Review of attachment 322417 [details] [review]: Looks good, but marking as reviewed as this'll be replaced by a new version as discussed on IRC.
(In reply to Carlos Garnacho from comment #8) > > > > ::: src/wayland/meta-wayland-data-device-private.h > > @@ +36,3 @@ > > + meta_wayland_data_source_primary, > > + META, WAYLAND_DATA_SOURCE_PRIMARY, > > + MetaWaylandDataSourceWayland); > > > > Should we maybe also make the "regular" data source its own type. > > > > Then no class would override some others functions and we'd have a clearer > > split. The base class would do everything that would work the same way. > > Done. Hmm, I can't see this in the new patch. I still only see one new type. What i meant was having a MetaWaylandDataSourceWayland that doesn't implement the vfuncs send and cancel. A MetaWaylandDataSourceDefault (or Normal, or DataDevice or some other name) that has the send and cancel implementations of the current MetaWaylandDataSourceWayland. A MetaWaylandDataSourcePrimary that does what the one you introduced does.
Review of attachment 322418 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +1495,3 @@ + if (data_device->primary_data_source && + data_device->primary_serial - serial < UINT32_MAX / 2) + return; As discussed, should we also do some focus checking? So that a client with a non-focused window cannot arbitrarily override the current selection? @@ +1772,3 @@ + source_primary->resource = resource; + wl_resource_set_implementation (resource, &primary_source_interface, + source_primary, destroy_data_source); Since you never set the "source_wayland->resource" to anything, the destroy_data_source function won't work. I think this is another reason why we should put anything wl_data_source related thing in another type (that inherits MetaWaylandDataDeviceWayland) and anything gtk_primary_selection_source in the MetaWaylandDataSourcePrimary. ::: src/wayland/protocol/gtk-primary-selection.xml @@ +107,3 @@ </description> <arg name="source" type="object" interface="gtk_primary_selection_source" allow-null="true"/> + <arg name="serial" type="uint" summary="serial of the event that triggered this request"/> Shouldn't this be part of the add-protocol patch?
(In reply to Jonas Ådahl from comment #13) > (In reply to Carlos Garnacho from comment #8) > > > > > > ::: src/wayland/meta-wayland-data-device-private.h > > > @@ +36,3 @@ > > > + meta_wayland_data_source_primary, > > > + META, WAYLAND_DATA_SOURCE_PRIMARY, > > > + MetaWaylandDataSourceWayland); > > > > > > Should we maybe also make the "regular" data source its own type. > > > > > > Then no class would override some others functions and we'd have a clearer > > > split. The base class would do everything that would work the same way. > > > > Done. > > Hmm, I can't see this in the new patch. I still only see one new type. What > i meant was having a > > MetaWaylandDataSourceWayland that doesn't implement the vfuncs send and > cancel. > > A MetaWaylandDataSourceDefault (or Normal, or DataDevice or some other name) > that has the send and cancel implementations of the current > MetaWaylandDataSourceWayland. > > A MetaWaylandDataSourcePrimary that does what the one you introduced does. Hmm, If I interpret you correctly you want a hierarchy: -MetaWaylandDataSource |-MetaWaylandDataSourceXWayland \-MetaWaylandDataSourceWayland |-MetaWaylandDataSourcePrimary \-MetaWaylandDataSourceDefault Whereas the current one is: -MetaWaylandDataSource |-MetaWaylandDataSourceXWayland |-MetaWaylandDataSourcePrimary \-MetaWaylandDataSourceWayland That's fair enough, however the changes (esp. the MetaWaylandDataSourceWayland->MetaWaylandDataSourceDefault rename) spill over too many places in wl_data_source. Since this is more of a cosmetic change (the paths where the resource is used are non-colliding, so it's not possible to receive crossed wayland sources), I'd prefer to keep this out of the current patch. I'll be managing this in a separate bug right after. (In reply to Jonas Ådahl from comment #14) > Review of attachment 322418 [details] [review] [review]: > > ::: src/wayland/meta-wayland-data-device.c > @@ +1495,3 @@ > + if (data_device->primary_data_source && > + data_device->primary_serial - serial < UINT32_MAX / 2) > + return; > > As discussed, should we also do some focus checking? So that a client with a > non-focused window cannot arbitrarily override the current selection? Done in the patches I'm attaching now. > > @@ +1772,3 @@ > + source_primary->resource = resource; > + wl_resource_set_implementation (resource, &primary_source_interface, > + source_primary, destroy_data_source); > > Since you never set the "source_wayland->resource" to anything, the > destroy_data_source function won't work. > > I think this is another reason why we should put anything wl_data_source > related thing in another type (that inherits MetaWaylandDataDeviceWayland) > and anything gtk_primary_selection_source in the > MetaWaylandDataSourcePrimary. I've separated this so far to another destroy_primary_source() function. To be folded together when we have a wayland base class. > > ::: src/wayland/protocol/gtk-primary-selection.xml > @@ +107,3 @@ > </description> > <arg name="source" type="object" > interface="gtk_primary_selection_source" allow-null="true"/> > + <arg name="serial" type="uint" summary="serial of the event that > triggered this request"/> > > Shouldn't this be part of the add-protocol patch? Right, rebase mishap. Fixed too.
Created attachment 322484 [details] [review] wayland: Add gtk-primary-selection protocol This protocol is an internal mirror of the primary selection drafts being proposed for wayland-protocols. No changes besides prefix/suffix changes.
Created attachment 322485 [details] [review] wayland: Implement the (so far internal) primary selection protocol Add an additional MetaWaylandDataSource implementation for primary selection sources, and methods to set primary selection offers. Primary selection sets altogether a different channel than the clipboard selection, those don't cross in any way. Also, the bridge for the X11 PRIMARY selection atom has been added, which adds all the necessary handling to translate primary selection both ways with wayland and X11 applications.
The only remaining issue will be taken in a different bug. I'm pushing the current patches then. Attachment 322484 [details] pushed as c6aad6e - wayland: Add gtk-primary-selection protocol Attachment 322485 [details] pushed as 7c11436 - wayland: Implement the (so far internal) primary selection protocol