[Wayland-bugs] [Bug 697855] Implement DnD in wayland

[gtk+ (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=697855
  gtk+ | Backend: Wayland | 3.8.x

--- Comment #31 from Carlos Garnacho <carlosg at gnome.org> 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.