After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 759168 - wayland: Implement DND icon hotspot API
wayland: Implement DND icon hotspot API
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Wayland
unspecified
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-12-08 11:23 UTC by Jonas Ådahl
Modified: 2015-12-11 14:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: Implement DND icon hotspot API (5.32 KB, patch)
2015-12-08 11:23 UTC, Jonas Ådahl
none Details | Review
wayland: Implement DND icon hotspot API (4.24 KB, patch)
2015-12-09 03:00 UTC, Jonas Ådahl
committed Details | Review

Description Jonas Ådahl 2015-12-08 11:23:50 UTC
This implements the missing parts. Though, I have not tested this with any other hotspot than (0, 0). Is there a sample client that uses something else than (0, 0) as hotspot (i.e. not tests/testdnd)?
Comment 1 Jonas Ådahl 2015-12-08 11:23:55 UTC
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.
Comment 2 Matthias Clasen 2015-12-08 16:15:08 UTC
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.
Comment 3 Matthias Clasen 2015-12-08 16:21:14 UTC
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 ?
Comment 4 Matthias Clasen 2015-12-08 16:51:03 UTC
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 ?
Comment 5 Matthias Clasen 2015-12-08 16:52:47 UTC
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 ?
Comment 6 Matthias Clasen 2015-12-08 16:53:07 UTC
Review of attachment 316924 [details] [review]:

.
Comment 7 Jonas Ådahl 2015-12-09 01:00:35 UTC
(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.
Comment 8 Jonas Ådahl 2015-12-09 01:03:33 UTC
(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.
Comment 9 Jonas Ådahl 2015-12-09 01:30:13 UTC
(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.
Comment 10 Jonas Ådahl 2015-12-09 03:00:15 UTC
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.
Comment 11 Jonas Ådahl 2015-12-09 03:00:47 UTC
mutter part filed as bug 759222.
Comment 12 Matthias Clasen 2015-12-09 03:36:47 UTC
Review of attachment 316996 [details] [review]:

looks ok now
Comment 13 Matthias Clasen 2015-12-11 14:17:41 UTC
Attachment 316996 [details] pushed as 5618333 - wayland: Implement DND icon hotspot API