I'm attaching a bunch of patches that implement selections/clipboard/dnd on gdk/wayland, this is made to work in a way that little changes are required so far in the gtk side, I've also pushed the work to the wayland-selections branch. A few caveats in the current state are:

- There's no DnD icon yet, I've been looking into this, but I got no worthwhile solution yet.
- only GDK_ACTION_COPY is implied and works, I haven't many ideas how the action negotiation is meant to work best on wayland

Also, the patches here need the patches for bug #734374, the serial sequence is needed to know whether the drag was triggered by mouse or touch, and cancel the proper state.

Created attachment 284202 [details] [review]
wayland: ensure gdk_device_manager_get_client_pointer() returns a master pointer

The internal ordering of devices isn't quite guaranteed, so ensure we're returning
the right one.

Created attachment 284203 [details] [review]
wayland: Store button and key modifiers separately

That way the key and modifiers() and key handlers can't unset the button
state, both modifier masks are put together when creating the events.

Created attachment 284204 [details] [review]
wayland: Fix x/y coordinate arguments on wl_data_device events

Those are wl_fixed_t, not int.

Created attachment 284205 [details] [review]
gdk: Add GDK_DRAG_PROTO_WAYLAND

To be used on the wayland windowing backend as the DnD protocol.

Created attachment 284206 [details] [review]
gdk: Remove check for source window

This is windowing dependent, on wayland there is no such source window,
not even on local DnD situations.

Created attachment 284207 [details] [review]
gtkselection: Make the message length limit X11 specific

So it doesn't apply to Wayland.

Created attachment 284208 [details] [review]
gtkdnd: set root coordinates to 0,0 for widget lookup

Each window is in its local "root" coordinate system, so make sure
we use 0, 0 for DnD widget lookup.

Created attachment 284209 [details] [review]
wayland: implement text_property_to_utf8_list()

Of some sort, this is enough to get text transfers on wayland, anything but
utf8 as plain/text should be pretty rare.

Created attachment 284210 [details] [review]
wayland: Implement drop/destination side of selections

This implementation makes the destination side of selections work
similarly to X11's, gdk_selection_convert() triggers data fetching,
which is notified through GDK_SELECTION_NOTIFY events on arrival,
the buffered data is then available through gdk_selection_property_get().

Created attachment 284211 [details] [review]
wayland: Implement drag/source side of selections

This has been made to work similarly to X11, requests for the data device
contents are notified through GDK_SELECTION_REQUEST events, the data stored
in the GDK_SELECTION property as a reaction to that event is then stored
into the wayland selection implementation, and written to the fd when
requested/available.

Created attachment 284212 [details] [review]
wayland: Replace clipboard implementation

The wayland specific clipboard functions have been replaced by something
more similar to the hooking the win32 backend does, which allows for just
using the default GtkClipboard code in GTK+. As a consequence, the
wayland-specific GtkClipboard implementation is now gone.

Created attachment 284213 [details] [review]
wayland: Implement the dropping part of DnD

All destination-side events are emitted, and updates to the drop context
are notified through the currently handled wl_data_offer.

Created attachment 284214 [details] [review]
wayland: Implement drag sources

The wl_data_source is retrieved from the selection object for the DnD
selection, and used to initiate a drag. When the drag is finished, a
button release or touch end event is synthesized to finish the DnD
operation after the compositor grab is gone.

Created attachment 284216 [details] [review]
wayland: Don't set an xdg surface to DnD windows

One caveat I forgot to mention, local DnD is somewhat botched, there seems to be no way to short circuit petitions, nor to know whether the other side is the same client. And there's also this bug I'm chasing where it sometimes won't detect the local window as a drop destination until the pointer leaves and enters the window.

(In reply to comment #16)
> One caveat I forgot to mention, local DnD is somewhat botched, there seems to
> be no way to short circuit petitions, nor to know whether the other side is the
> same client. And there's also this bug I'm chasing where it sometimes won't
> detect the local window as a drop destination until the pointer leaves and
> enters the window.

Are you trying to enforce only local (which I assume means only windows from the source client?) or to do something special in the local case?

Local only is supported by providing a NULL data source.  In that case, the compositor will only send dnd enter/leave to surfaces from the same client, and the client is responsible for communicating the available data types and the data transfer internally.

As for doing something special in case of a local dnd, it should be possible to detect by noticing that data_devices wl_seat is one that the client currently has an active dnd grab for.

(In reply to comment #17)
> (In reply to comment #16)
> > One caveat I forgot to mention, local DnD is somewhat botched, there seems to
> > be no way to short circuit petitions, nor to know whether the other side is the
> > same client. And there's also this bug I'm chasing where it sometimes won't
> > detect the local window as a drop destination until the pointer leaves and
> > enters the window.
> 
> Are you trying to enforce only local (which I assume means only windows from
> the source client?) or to do something special in the local case?

No, thing is... GtkSelection has means to detect when a selection is owned locally, this is used in several ways in DnD (eg. textview sending buffer pointers, text widgets defaulting to move instead of paste, etc), I was unable to detect these cases, so this is all currently behaving as alway-remote.

> 
> As for doing something special in case of a local dnd, it should be possible to
> detect by noticing that data_devices wl_seat is one that the client currently
> has an active dnd grab for.

Oh, I somehow didn't think of checking how did wl_data_sources match. Thanks for the hint :), I'll be playing with that.

Review of attachment 284202 [details] [review]:

ok

Review of attachment 284203 [details] [review]:

This is a little unclear to me - are there really two separate sets of modifiers, and not just one set of 'current modifiers' ?

::: gdk/wayland/gdkdevice-wayland.c
@@ +73,3 @@
   GHashTable *touches;
 
   GdkModifierType modifiers;

Shouldn't this field be renamed to key_modifiers then, for clarity ?

Review of attachment 284204 [details] [review]:

ok

Review of attachment 284205 [details] [review]:

ok

Review of attachment 284206 [details] [review]:

I would say 'backend-dependent' or 'dependent on the protocol', but ok

Review of attachment 284207 [details] [review]:

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.

Review of attachment 284208 [details] [review]:

Shouldn't gdk_window_get_position return 0,0 for wayland windows anyway ?

Review of attachment 284209 [details] [review]:

Its a start. We should perhaps replace that FIXME by an actual utf8 validation

Review of attachment 284210 [details] [review]:

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

@@ +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 ?

@@ +254,3 @@
+    }
+
+  return selection;

Might be nicer to tie this to the display singleton, and actually clean it up when the display is closed.

@@ +265,3 @@
+
+  selection->targets = g_list_prepend (selection->targets,
+                                       gdk_atom_intern (type, FALSE));

Can this list have duplicates ? Would it be a problem ?

@@ +457,3 @@
+      else
+        {
+          pipe2 (pipe_fd, O_CLOEXEC);

Could use g_unix_open_pipe here

Review of attachment 284211 [details] [review]:

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

@@ +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.

Review of attachment 284212 [details] [review]:

nice to ditch one clipboard implementation in the frontend

Review of attachment 284216 [details] [review]:

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.

(In reply to comment #20)
> Review of attachment 284203 [details] [review]:
> 
> 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] [review]:
> 
> I would say 'backend-dependent' or 'dependent on the protocol', but ok
(In reply to comment #24)
> Review of attachment 284207 [details] [review]:
> 
> 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] [review]:
> 
> 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] [review]:
> 
> 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] [review]:
> 
> ::: 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] [review]:
> 
> ::: 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] [review]:
> 
> 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.

Attachment 284202 [details] pushed as 001327d - wayland: ensure gdk_device_manager_get_client_pointer() returns a master pointer
Attachment 284204 [details] pushed as 2e7d5b2 - wayland: Fix x/y coordinate arguments on wl_data_device events

Created attachment 284713 [details] [review]
wayland: Store pressed buttons and key modifiers separately

That way the key and modifiers() and key handlers can't unset the button
state, both modifier masks are put together when creating the events.

Created attachment 284714 [details] [review]
wayland: Implement drop/destination side of selections

This implementation makes the destination side of selections work
similarly to X11's, gdk_selection_convert() triggers data fetching,
which is notified through GDK_SELECTION_NOTIFY events on arrival,
the buffered data is then available through gdk_selection_property_get().

Created attachment 284715 [details] [review]
wayland: Implement drag/source side of selections

This has been made to work similarly to X11, requests for the data device
contents are notified through GDK_SELECTION_REQUEST events, the data stored
in the GDK_SELECTION property as a reaction to that event is then stored
into the wayland selection implementation, and written to the fd when
requested/available.

I've just updated the patches that got most changes. Commit log rewordings, and other minor things like utf8 checks have been kept in the branch.

It is also worth noting I got the DnD icon working in the branch (last 3 patches there), although it involves some further hacks in GtkWindow to have it use the GdkWindow/surface specified on wl_data_device_start_drag(). Would be nice to have a better way to do this, although I guess that'll have to wait for 3.14. I feel this problem shares a lot with popup-window-on-subsurfaces though.

I've also been investigating the local drag issues, I figured local dragging is broken until after the pointer leaves/enters the window because wl_data_source_offer() should be called before wl_data_device_start_drag(), something which is done inversely in the upper layers. As a result no targets are offered, until the leave/enter provides a new wl_data_offer, so a bit fruitless here so far.

Review of attachment 284713 [details] [review]:

Still slightly confused about why this is necessary - don't all wayland events carry the full 'current input state', including both buttons and modifiers ?

Review of attachment 284714 [details] [review]:

::: gdk/wayland/gdkselection-wayland.c
@@ +259,3 @@
+
+  return display->selection;
+}

maybe turn this into a gdk_wayland_selection_new() that gets calls from the display code, plus
a gdk_wayland_display_get_selection() to obtain the singleton ?

Looks slightly odd to have this poked directly into the display here, but have the free call on the display side.

Review of attachment 284715 [details] [review]:

What I find slightly confusing here is that we seem to be dealing with CLIPBOARD, GDK_SELECTION and GdkWaylandSelection as atoms here - one too many, or is GDK_SELECTION different from the other two, since it is used as a property name, not as a selection ?

(In reply to comment #37)
> Review of attachment 284713 [details] [review]:
> 
> Still slightly confused about why this is necessary - don't all wayland events
> carry the full 'current input state', including both buttons and modifiers ?

Hmm, trying to remember the reasons why this was necessary, and I'm thinking this is some miscellaneous thing I found, having text selection and other gestures stuck after pressing a key because gdk_window_get_device_position/gdk_device_get_state would return masks with no buttons. But as far as I can look now, shouldn't be hit too often on DnD, so maybe best to treat this separately.

In any case, wayland input listeners are fairly light in information each, it is up to gdk to maintain positions/modifiers/axes/scroll diffs the GdkWaylandDeviceData so it can compose GdkEvents, and there's also gdk_device_get_state() and friends to cater for...

(In reply to comment #38)
> Review of attachment 284714 [details] [review]:
> 
> ::: gdk/wayland/gdkselection-wayland.c
> @@ +259,3 @@
> +
> +  return display->selection;
> +}
> 
> maybe turn this into a gdk_wayland_selection_new() that gets calls from the
> display code, plus
> a gdk_wayland_display_get_selection() to obtain the singleton ?
> 
> Looks slightly odd to have this poked directly into the display here, but have
> the free call on the display side.

Yeah I agree, this turned out a bit odd.

(In reply to comment #39)
> Review of attachment 284715 [details] [review]:
> 
> What I find slightly confusing here is that we seem to be dealing with
> CLIPBOARD, GDK_SELECTION and GdkWaylandSelection as atoms here - one too many,
> or is GDK_SELECTION different from the other two, since it is used as a
> property name, not as a selection ?

Exactly, GDK_SELECTION is just the "property" used to store the data through gdk_property_change(). There's some atom internship madness going on in the patch though... I'll clean up so we can compare with eg. selections[CLIPBOARD]

Created attachment 284823 [details] [review]
wayland: Implement drop/destination side of selections

This implementation makes the destination side of selections work
similarly to X11's, gdk_selection_convert() triggers data fetching,
which is notified through GDK_SELECTION_NOTIFY events on arrival,
the buffered data is then available through gdk_selection_property_get().

Created attachment 284824 [details] [review]
wayland: Implement drag/source side of selections

This has been made to work similarly to X11, requests for the data device
contents are notified through GDK_SELECTION_REQUEST events, the data stored
in the GDK_SELECTION property as a reaction to that event is then stored
into the wayland selection implementation, and written to the fd when
requested/available.

Created attachment 284825 [details] [review]
wayland: Implement the dropping part of DnD

All destination-side events are emitted, and updates to the drop context
are notified through the currently handled wl_data_offer.

Created attachment 284826 [details] [review]
wayland: Implement drag sources

The wl_data_source is retrieved from the selection object for the DnD
selection, and used to initiate a drag. When the drag is finished, a
button release or touch end event is synthesized to finish the DnD
operation after the compositor grab is gone.

I've updated the patches with the most meaningful changes since the last time, although there are other minor changes in other patches in the wayland-selections branch. 

In the current state, local clipboards/dnd works, and I only observe bugs when dropping to a blank area of the compositor (eg. background), after this the pointer grab stays on and the app still thinks it owns the DnD selection/wl_data_source. I'm trying to figure out what's the expected behavior of wl_data_source.cancelled from compositors, that's never being emitted for DnD data_sources, nor the drag source client gets notified in any way when dropping on non-responsive areas happens, that looks like a bug to me.

Anway, this is behaving a lot better than on previous iterations.

Review of attachment 284823 [details] [review]:

Loosk good to me now

Review of attachment 284824 [details] [review]:

looks ok to me

Review of attachment 284825 [details] [review]:

looks ok to me

Review of attachment 284826 [details] [review]:

Looks ok

I've now played a bit with the branch under weston (my mutter doesn't get any input, so not very useful for testing dnd...).

I've used the clipboard example in gtk3-demo.

Results:

1) copy/paste works for both text and images

2) the snapback animation after a rejected drop doesn't show

3) the image drag doesn't get the dnd window. You probably run into the case where we try to do it all with a cursor. We must not do that under wayland, I guess

4) the cursor does not update to indicate whether a drop is possible

5) the right click menu on the images in this example is mispositioned

Also tested copy/paste between weston-terminal and gtk-demo. Mostly works, but I notice that copying text in weston-terminal does not make the paste menuitem in the gtk-demo source views context menu sensitive

the paste menuitem does not go sensitive for a local copy to the clipboard either

(In reply to comment #50)
> I've now played a bit with the branch under weston (my mutter doesn't get any
> input, so not very useful for testing dnd...).

Thanks for testing :)

> 
> I've used the clipboard example in gtk3-demo.
> 
> Results:
> 
> 1) copy/paste works for both text and images
> 
> 2) the snapback animation after a rejected drop doesn't show

Yeah... Such feedback would be basically a responsibility of the compositor. Weston doesn't do much here as you can see, and mutter/g-s doesn't even display DnD icons yet... I would want to get to the latter and add the snapback animation there.

> 
> 3) the image drag doesn't get the dnd window. You probably run into the case
> where we try to do it all with a cursor. We must not do that under wayland, I
> guess

Hmm, I'll investigate that... but yeah, there's basically a bunch of code in gtkdnd.c we eventually want to not run on wayland, all the code to update the DnD window position while the button is pressed is also useless there, although harmless. There some useful bits there that could need rethinking, like pointer cursor changes.

> 
> 4) the cursor does not update to indicate whether a drop is possible

Right... see above.

> 
> 5) the right click menu on the images in this example is mispositioned

Hmm, indeed, I also see some gdk warnings when trying to fetch the pointer position, not sure how related to these patches though. I'll have a look anyway.

(In reply to comment #51)
> Also tested copy/paste between weston-terminal and gtk-demo. Mostly works, but
> I notice that copying text in weston-terminal does not make the paste menuitem
> in the gtk-demo source views context menu sensitive
(In reply to comment #52)
> the paste menuitem does not go sensitive for a local copy to the clipboard
> either

Hmm, that's weird, I do get menu sensitivity updating properly, although I've just observed that only the first paste works, as opposed to Ctrl-V.

Attachment 284205 [details] pushed as 9c16d8b - gdk: Add GDK_DRAG_PROTO_WAYLAND
Attachment 284206 [details] pushed as 5fcf2de - gdk: Remove check for source window
Attachment 284209 [details] pushed as 32bf03c - wayland: implement text_property_to_utf8_list()
Attachment 284212 [details] pushed as f48b3cc - wayland: Replace clipboard implementation
Attachment 284216 [details] pushed as 867302e - wayland: Don't set an xdg surface to DnD windows
Attachment 284823 [details] pushed as 3b95304 - wayland: Implement drop/destination side of selections
Attachment 284824 [details] pushed as 7744799 - wayland: Implement drag/source side of selections
Attachment 284825 [details] pushed as 9b0b88d - wayland: Implement the dropping part of DnD
Attachment 284826 [details] pushed as 7b85a34 - wayland: Implement drag sources