After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762561 - Implement primary selection protocol
Implement primary selection protocol
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Wayland
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2016-02-23 19:51 UTC by Carlos Garnacho
Modified: 2016-02-26 19:04 UTC
See Also:
GNOME target: 3.20
GNOME version: ---


Attachments
wayland: Add gtk-primary-selection protocol (12.05 KB, patch)
2016-02-23 19:52 UTC, Carlos Garnacho
none Details | Review
wayland: Make the function to get the last serial a seat one (5.53 KB, patch)
2016-02-23 19:52 UTC, Carlos Garnacho
none Details | Review
wayland: Implement the (so far internal) primary selection protocol (29.33 KB, patch)
2016-02-23 19:52 UTC, Carlos Garnacho
none Details | Review
wayland: Add gtk-primary-selection protocol (12.15 KB, patch)
2016-02-25 23:46 UTC, Carlos Garnacho
committed Details | Review
wayland: Make the function to get the last serial a seat one (5.61 KB, patch)
2016-02-25 23:46 UTC, Carlos Garnacho
committed Details | Review
wayland: Implement the (so far internal) primary selection protocol (27.69 KB, patch)
2016-02-25 23:46 UTC, Carlos Garnacho
committed Details | Review

Description Carlos Garnacho 2016-02-23 19:51:02 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.
Comment 1 Carlos Garnacho 2016-02-23 19:52:34 UTC
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.
Comment 2 Carlos Garnacho 2016-02-23 19:52:41 UTC
Created attachment 322174 [details] [review]
wayland: Make the function to get the last serial a seat one

This will be useful for primary selection.
Comment 3 Carlos Garnacho 2016-02-23 19:52:47 UTC
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.
Comment 4 Jonas Ådahl 2016-02-24 04:36:34 UTC
Review of attachment 322173 [details] [review]:

Looks good except missing .gitignore entries.
Comment 5 Jonas Ådahl 2016-02-24 04:46:01 UTC
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.
Comment 6 Jonas Ådahl 2016-02-24 06:56:12 UTC
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?
Comment 7 Carlos Garnacho 2016-02-25 23:45:51 UTC
(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.
Comment 8 Carlos Garnacho 2016-02-25 23:46:35 UTC
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.
Comment 9 Carlos Garnacho 2016-02-25 23:46:40 UTC
Created attachment 322420 [details] [review]
wayland: Make the function to get the last serial a seat one

This will be useful for primary selection.
Comment 10 Carlos Garnacho 2016-02-25 23:46:46 UTC
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.
Comment 11 Jonas Ådahl 2016-02-26 03:17:34 UTC
(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
Comment 12 Jonas Ådahl 2016-02-26 03:23:22 UTC
Review of attachment 322419 [details] [review]:

Marking as 'reviewed' since it'll be updated with the changes we discussed on IRC.
Comment 13 Jonas Ådahl 2016-02-26 03:25:40 UTC
Review of attachment 322420 [details] [review]:

Looks good.
Comment 14 Jonas Ådahl 2016-02-26 03:36:20 UTC
Review of attachment 322421 [details] [review]:

Looks good. Marking as reviewed just because it'll be updated given the protocol change.
Comment 15 Carlos Garnacho 2016-02-26 19:04:46 UTC
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