<html> <head> <base href="https://bugzilla.gnome.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - Implement primary selection protocol" href="https://bugzilla.gnome.org/show_bug.cgi?id=762561#c7">Comment # 7</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - Implement primary selection protocol" href="https://bugzilla.gnome.org/show_bug.cgi?id=762561">bug 762561</a> from <a href="page.cgi?id=describeuser.html&login=carlosg%40gnome.org" title="Carlos Garnacho <carlosg@gnome.org>"> Carlos Garnacho</a> <pre> (In reply to Jonas Ådahl from <a href="show_bug.cgi?id=762561#c4">comment #4</a>) > Review of <a href="attachment.cgi?id=322173&action=diff" name="attach_322173" title="wayland: Add gtk-primary-selection protocol">attachment 322173</a> <a href="attachment.cgi?id=322173&action=edit" title="wayland: Add gtk-primary-selection protocol">[details]</a> <a href='review?bug=762561&attachment=322173'>[review]</a> [review]: > > Looks good except missing .gitignore entries. gtk+ autogenerates those :). (In reply to Jonas Ådahl from <a href="show_bug.cgi?id=762561#c5">comment #5</a>) > Review of <a href="attachment.cgi?id=322174&action=diff" name="attach_322174" title="wayland: Make the function to get the last serial a seat one">attachment 322174</a> <a href="attachment.cgi?id=322174&action=edit" title="wayland: Make the function to get the last serial a seat one">[details]</a> <a href='review?bug=762561&attachment=322174'>[review]</a> [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 <a href="show_bug.cgi?id=762561#c6">comment #6</a>) > Review of <a href="attachment.cgi?id=322175&action=diff" name="attach_322175" title="wayland: Implement the (so far internal) primary selection protocol">attachment 322175</a> <a href="attachment.cgi?id=322175&action=edit" title="wayland: Implement the (so far internal) primary selection protocol">[details]</a> <a href='review?bug=762561&attachment=322175'>[review]</a> [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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>