[Wayland-bugs] [Bug 762561] Implement primary selection protocol

[gtk+ (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=762561

--- Comment #7 from Carlos Garnacho <carlosg at gnome.org> ---

(In reply to Jonas Ådahl from comment #4)
> Review of attachment 322173 [details] [review]:
> 
> Looks good except missing .gitignore entries.

gtk+ autogenerates those :).

(In reply to Jonas Ådahl from comment #5)
> 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.

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]:
> 
> ::: 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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-bugs/attachments/20160225/1b04ab7b/attachment-0001.html>