https://bugzilla.gnome.org/show_bug.cgi?id=697855 gtk+ | Backend: Wayland | 3.8.x
--- Comment #31 from Carlos Garnacho <[email protected]> 2014-08-26 12:35:18 UTC --- (In reply to comment #20) > Review of attachment 284203 [details]: > > This is a little unclear to me - are there really two separate sets of > modifiers, and not just one set of 'current modifiers' ? > I could maybe do this the other way around, and ensure deliver_key_event() and keyboard_handle_modifiers() leave GDK_BUTTON?_MASK untouched, right now any key event will erase buttons from the modifiers mask. (In reply to comment #23) > Review of attachment 284206 [details]: > > I would say 'backend-dependent' or 'dependent on the protocol', but ok (In reply to comment #24) > Review of attachment 284207 [details]: > > The commit message is slightly misleading - it was already X11-specific. It > was > just missing the runtime check in addition to the build time check. I'll reword these (In reply to comment #25) > Review of attachment 284208 [details]: > > Shouldn't gdk_window_get_position return 0,0 for wayland windows anyway ? Right, this commit was mostly done to avoid having this bug depend on https://bugzilla.gnome.org/show_bug.cgi?id=729215#c5 , now that this is in master, this patch can be obsoleted. (In reply to comment #26) > Review of attachment 284209 [details]: > > Its a start. We should perhaps replace that FIXME by an actual utf8 validation Yeah, I agree. (In reply to comment #27) > Review of attachment 284210 [details]: > > ::: gdk/wayland/gdkselection-wayland.c > @@ +16,3 @@ > */ > > +#define _GNU_SOURCE > > No longer needed - instead just move the config.h include up first. We use > AC_USE_SYSTEM_EXTENSIONS in configure.ac now. > > @@ +115,3 @@ > + > + buffer = g_new0 (SelectionBuffer, 1); > + buffer->stream = stream; > > You don't take a ref on stream here > > @@ +147,3 @@ > + > + if (buffer_data->stream) > + g_object_unref (buffer_data->stream); > > ...but you unref it here. Yeah, the SelectionBuffer struct takes ownership on the stream, but the stream may be destroyed right after reading is finished, even though the SelectionBuffer persists. I can maybe make this more evident by adding a ref there, and unreff'ing the stream after selection_buffer_new(). > > @@ +177,3 @@ > +{ > + if (!g_list_find (buffer->requestors, requestor)) > + buffer->requestors = g_list_prepend (buffer->requestors, requestor); > > Do you need to take a ref here ? Yeah, might be a good idea if some window vanishes in between. > > @@ +254,3 @@ > + } > + > + return selection; > > Might be nicer to tie this to the display singleton, and actually clean it up > when the display is closed. Yeah, I think you're right. > > @@ +265,3 @@ > + > + selection->targets = g_list_prepend (selection->targets, > + gdk_atom_intern (type, FALSE)); > > Can this list have duplicates ? Would it be a problem ? Hmm, good point, it might have duplicates (anything that is offered through wl_data_source_offer(), on the source side), but all we supposedly care here is whether an atom is or not on the list, I'll add some sanity check anyway. > > @@ +457,3 @@ > + else > + { > + pipe2 (pipe_fd, O_CLOEXEC); > > Could use g_unix_open_pipe here Hmm, according to the docs that function does not take O_CLOEXEC :(. (In reply to comment #28) > Review of attachment 284211 [details]: > > ::: gdk/wayland/gdkselection-wayland.c > @@ +389,3 @@ > + if (mode == GDK_PROP_MODE_APPEND) > + g_array_append_vals (array, selection->stored_selection.data, > + selection->stored_selection.data_len - 1); > > Isn't this the wrong way around ? if the mode is APPEND, you should append the > new data to the stored_selection.data. It looks like you are instead appending > the stored_selection.data to the new data. Oops, thinko :), thanks for spotting. > > @@ +617,3 @@ > + > + gdk_wayland_device_set_selection (device, NULL); > + g_clear_pointer (&wayland_selection->clipboard_source, > wl_data_source_destroy); > > Jasper pointed out in another review that he prefers protocol calls to not be > hidden as clear_pointer callbacks. Yeah, will do that here too. (In reply to comment #30) > Review of attachment 284216 [details]: > > ok. gdk_wayland_window_map is getting a bit messy - might be nice to put a > table in the docs somewhere, summarizing the various heuristics we use for > whether to use an xdg_popup or xdg_surface or neither. Sounds like a good idea, in this case, setting a dnd window with an xdg_surface interface crashes weston. I think I saw some ongoing talking in the wayland ML on how to deal with unsupported mixes of interfaces. -- Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Wayland-bugs mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-bugs
