GNOME Bugzilla – Bug 762561
Implement primary selection protocol
Last modified: 2016-02-26 19:04:59 UTC
Attaching some patches to allow gtk+ to implement the primary selection protocol, this is the counterside to bug #762560, and uses the same internal protocol. This allows middle click paste between gtk+ apps.
Created attachment 322173 [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 322174 [details] [review] wayland: Make the function to get the last serial a seat one This will be useful for primary selection.
Created attachment 322175 [details] [review] wayland: Implement the (so far internal) primary selection protocol Implement it using the internal copy of the protocol. Otherwise, we just deal with it the same than clipboard selection, just mapping it to the PRIMARY atom instead of the CLIPBOARD one.
Review of attachment 322173 [details] [review]: Looks good except missing .gitignore entries.
Review of attachment 322174 [details] [review]: ::: gdk/wayland/gdkdevice-wayland.c @@ +3031,3 @@ + wayland_seat = GDK_WAYLAND_SEAT (seat); + g_hash_table_iter_init (&iter, wayland_seat->touches); This is not related to this patch, but I fail to see when touches are added to seat->touches. I do see when they are added to some GdkWaylandDeviceData though. If seat->touches is always empty we'd regress here anyhow. ::: gdk/wayland/gdkwindow-wayland.c @@ +1337,3 @@ parent_impl->display_server.wl_surface, seat, + _gdk_wayland_seat_get_last_implicit_grab_serial (gdk_seat, NULL), Completely only an opinion, but I think it'd be easier to read to fetch this value before, and pass it as a variable "grab_serial" or something.
Review of attachment 322175 [details] [review]: ::: gdk/wayland/gdkdevice-wayland.c @@ +931,3 @@ +primary_selection_data_offer (void *data, + struct gtk_primary_selection_device *wp_primary_selection_device, + struct gtk_primary_selection_offer *wp_primary_offer) wp? :P ::: gdk/wayland/gdkselection-wayland.c @@ +272,3 @@ { selection_buffer_ref (buffer); + g_input_stream_read_bytes_async (buffer->stream, READ_BUFFER_SIZE, G_PRIORITY_DEFAULT, The changes to this function and selection_buffer_read_cb doesn't look related to the patch. @@ +285,2 @@ info = g_slice_new0 (DataOfferData); info->offer = offer; Should info->offer now be called "offer_data" or something now instead maybe? @@ +432,3 @@ +static void +primary_offer_offer (void *data, + struct gtk_primary_selection_offer *wp_offer, wp_ here again and in a few other places. @@ +879,3 @@ + { + window = wayland_selection->primary_owner; + selection = atoms[ATOM_PRIMARY]; Why do you check if the source is a primary selection source here? I wouldn't expect that wl_data_source.send would ever be called for the primary selection. @@ +914,3 @@ atom = atoms[ATOM_DND]; + else if (source == (gpointer) wayland_selection->primary_source) + atom = atoms[ATOM_PRIMARY]; Same here. This is a wl_data_source related function. Why would you check if its a primary selection source?
(In reply to Jonas Ådahl from comment #4) > Review of attachment 322173 [details] [review] [review]: > > Looks good except missing .gitignore entries. gtk+ autogenerates those :). (In reply to Jonas Ådahl from comment #5) > Review of attachment 322174 [details] [review] [review]: > > ::: gdk/wayland/gdkdevice-wayland.c > @@ +3031,3 @@ > > + wayland_seat = GDK_WAYLAND_SEAT (seat); > + g_hash_table_iter_init (&iter, wayland_seat->touches); > > This is not related to this patch, but I fail to see when touches are added > to seat->touches. I do see when they are added to some GdkWaylandDeviceData > though. If seat->touches is always empty we'd regress here anyhow. GdkWaylandDeviceData is currently a typedef to GdkWaylandSeat, there's a big rename fest awaiting to be done. I've been waiting for this, mainly in prevision for some of the branches had waiting to be merged (wayland-tablet is going to be fun...). I wanted to get to that during freeze times though. In the time being, GdkWaylandDeviceData is GdkWaylandSeat. > > ::: gdk/wayland/gdkwindow-wayland.c > @@ +1337,3 @@ > > parent_impl->display_server.wl_surface, > seat, > + > _gdk_wayland_seat_get_last_implicit_grab_serial (gdk_seat, NULL), > > Completely only an opinion, but I think it'd be easier to read to fetch this > value before, and pass it as a variable "grab_serial" or something. Yeah... the 80col marker here also makes me think similarly. (In reply to Jonas Ådahl from comment #6) > Review of attachment 322175 [details] [review] [review]: > > ::: gdk/wayland/gdkdevice-wayland.c > @@ +931,3 @@ > +primary_selection_data_offer (void *data, > + struct gtk_primary_selection_device > *wp_primary_selection_device, > + struct gtk_primary_selection_offer > *wp_primary_offer) > > wp? :P yeah :), my replace spree wasn't too thorough. > > ::: gdk/wayland/gdkselection-wayland.c > @@ +272,3 @@ > { > selection_buffer_ref (buffer); > + g_input_stream_read_bytes_async (buffer->stream, READ_BUFFER_SIZE, > G_PRIORITY_DEFAULT, > > The changes to this function and selection_buffer_read_cb doesn't look > related to the patch. Right, part of other fixes I found during development, moved outside of these patches. > > @@ +285,2 @@ > info = g_slice_new0 (DataOfferData); > info->offer = offer; > > Should info->offer now be called "offer_data" or something now instead maybe? Yeah, sounds better. > @@ +879,3 @@ > + { > + window = wayland_selection->primary_owner; > + selection = atoms[ATOM_PRIMARY]; > > Why do you check if the source is a primary selection source here? I > wouldn't expect that wl_data_source.send would ever be called for the > primary selection. Oops, yeah, leftover from a previous hackish attempt. > > @@ +914,3 @@ > atom = atoms[ATOM_DND]; > + else if (source == (gpointer) wayland_selection->primary_source) > + atom = atoms[ATOM_PRIMARY]; > > Same here. This is a wl_data_source related function. Why would you check if > its a primary selection source? Same thing.
Created attachment 322419 [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 322420 [details] [review] wayland: Make the function to get the last serial a seat one This will be useful for primary selection.
Created attachment 322421 [details] [review] wayland: Implement the (so far internal) primary selection protocol Implement it using the internal copy of the protocol. Otherwise, we just deal with it the same than clipboard selection, just mapping it to the PRIMARY atom instead of the CLIPBOARD one.
(In reply to Carlos Garnacho from comment #7) > (In reply to Jonas Ådahl from comment #4) > > Review of attachment 322173 [details] [review] [review] [review]: > > > > Looks good except missing .gitignore entries. > > gtk+ autogenerates those :). Neat! > > (In reply to Jonas Ådahl from comment #5) > > Review of attachment 322174 [details] [review] [review] [review]: > > > > ::: gdk/wayland/gdkdevice-wayland.c > > @@ +3031,3 @@ > > > > + wayland_seat = GDK_WAYLAND_SEAT (seat); > > + g_hash_table_iter_init (&iter, wayland_seat->touches); > > > > This is not related to this patch, but I fail to see when touches are added > > to seat->touches. I do see when they are added to some GdkWaylandDeviceData > > though. If seat->touches is always empty we'd regress here anyhow. > > GdkWaylandDeviceData is currently a typedef to GdkWaylandSeat, there's a big > rename fest awaiting to be done. I've been waiting for this, mainly in > prevision for some of the branches had waiting to be merged (wayland-tablet > is going to be fun...). I wanted to get to that during freeze times though. > > In the time being, GdkWaylandDeviceData is GdkWaylandSeat. Aahaaa. Thats not confusing at all :P
Review of attachment 322419 [details] [review]: Marking as 'reviewed' since it'll be updated with the changes we discussed on IRC.
Review of attachment 322420 [details] [review]: Looks good.
Review of attachment 322421 [details] [review]: Looks good. Marking as reviewed just because it'll be updated given the protocol change.
Pushed with the aforementioned protocol changes, a serial is no longer needed by gtk_primary_selection_offer.receive. Attachment 322419 [details] pushed as 787e1d7 - wayland: Add gtk-primary-selection protocol Attachment 322420 [details] pushed as f9f5586 - wayland: Make the function to get the last serial a seat one Attachment 322421 [details] pushed as ed3c87d - wayland: Implement the (so far internal) primary selection protocol