Created attachment 316924 [details] [review]
wayland: Implement DND icon hotspot API

In Wayland, the hotspot of a DND icon is set using the buffer offset in
wl_buffer.attach. To implement this, add a private API to cause the
next wl_surface.attach to offset the new buffer with a given offset.
Setting a DND icon hotspot sets this offset while also queuing a redraw
of the window to trigger the wl_surface.attach.

You can use testentryicons. Its been on my list to write a more versatile dnd testing app that iterates through the various possibilities for setting icons and hotspots. To make it more obvious, you can crank the offset up to 24,24.

Review of attachment 316924 [details] [review]:

::: gdk/wayland/gdkdnd-wayland.c
@@ +51,3 @@
   gdouble y;
+  gint prev_hot_x;
+  gint prev_hot_y;

I don't see the need to store these in the context. They can just be locals in set_hot_spot

@@ +329,3 @@
+                                .width = 1,
+                                .height = 1,
+                              },

This kind of on-the-fly struct looks a little out of style in the GTK+ codebase. Do we do this elsewhere in the Wayland backend ?

Review of attachment 316924 [details] [review]:

Some confusion here as to whether this is public api (and documented) or private and _-prefixed. What is the intention ?

Review of attachment 316924 [details] [review]:

Testing this with 

diff --git a/tests/testentryicons.c b/tests/testentryicons.c
index df1d02b..e876199 100644
--- a/tests/testentryicons.c
+++ b/tests/testentryicons.c
@@ -17,7 +17,7 @@ drag_begin_cb (GtkWidget      *widget,
 
   pos = gtk_entry_get_current_icon_drag_source (GTK_ENTRY (widget));
   if (pos != -1)
-    gtk_drag_set_icon_name (context, "dialog-information", 2, 2);
+    gtk_drag_set_icon_name (context, "dialog-information", 24, 24);
 }
 
 static void


shows that it kinda works, but the offset is much bigger than 24 - maybe it gets applied repeatedly, or there is some scale confusion ?

Review of attachment 316924 [details] [review]:

.

(In reply to Matthias Clasen from comment #4)
> Review of attachment 316924 [details] [review] [review]:
> 
> Some confusion here as to whether this is public api (and documented) or
> private and _-prefixed. What is the intention ?

Sorry, just me rushing things. I first added it to gdkwaylandwindow.h thinking it was the private header, and copied what other API did, then remembered gdkwayland-private.h and put it there - but didn't move it completely. The intention is that is a private and I'll fix it up in the next version.

(In reply to Matthias Clasen from comment #3)
> Review of attachment 316924 [details] [review] [review]:
> 
> ::: gdk/wayland/gdkdnd-wayland.c
> @@ +51,3 @@
>    gdouble y;
> +  gint prev_hot_x;
> +  gint prev_hot_y;
> 
> I don't see the need to store these in the context. They can just be locals
> in set_hot_spot

True, this was part of an idea of making the offset being retrieved in another way, but now that things are done this way its not needed. Will remove.

> 
> @@ +329,3 @@
> +                                .width = 1,
> +                                .height = 1,
> +                              },
> 
> This kind of on-the-fly struct looks a little out of style in the GTK+
> codebase. Do we do this elsewhere in the Wayland backend ?

Hmm, no. We use it in mutter here and there, but if you prefer I can leave it out of GTK+.

In fact, we might not need the invalidation call at all if we can safely assume that the hotspot is always set while setting an icon that will be drawn anyway. The reason for adding the invalidation is to ensure we get a wl_surface.attach.

(In reply to Matthias Clasen from comment #5)
> Review of attachment 316924 [details] [review] [review]:
> 
> Testing this with 
> 
> diff --git a/tests/testentryicons.c b/tests/testentryicons.c
> index df1d02b..e876199 100644
> --- a/tests/testentryicons.c
> +++ b/tests/testentryicons.c
> @@ -17,7 +17,7 @@ drag_begin_cb (GtkWidget      *widget,
>  
>    pos = gtk_entry_get_current_icon_drag_source (GTK_ENTRY (widget));
>    if (pos != -1)
> -    gtk_drag_set_icon_name (context, "dialog-information", 2, 2);
> +    gtk_drag_set_icon_name (context, "dialog-information", 24, 24);
>  }
>  
>  static void
> 
> 
> shows that it kinda works, but the offset is much bigger than 24 - maybe it
> gets applied repeatedly, or there is some scale confusion ?

Indeed, it seems to kind of work. Though I suspect the error lies within mutter and not this patch, since the it works just fine under weston. Will investigate.

Created attachment 316996 [details] [review]
wayland: Implement DND icon hotspot API

In Wayland, the hotspot of a DND icon is set using the buffer offset in
wl_buffer.attach. To implement this, add a private API to cause the
next wl_surface.attach to offset the new buffer with a given offset.
Setting a DND icon hotspot sets this offset while also queuing a redraw
of the window to trigger the wl_surface.attach.

mutter part filed as bug 759222.

Review of attachment 316996 [details] [review]:

looks ok now

Attachment 316996 [details] pushed as 5618333 - wayland: Implement DND icon hotspot API