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 748763 - warnings when starting drag from GtkEntries
warnings when starting drag from GtkEntries
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Wayland
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2015-05-01 17:38 UTC by Carlos Garnacho
Modified: 2015-12-02 04:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
window: Avoid destroying the hardcoded window (1.29 KB, patch)
2015-05-01 17:39 UTC, Carlos Garnacho
reviewed Details | Review
wayland: Keep DnD icon surfaces' wl_surface static (1.63 KB, patch)
2015-05-01 17:39 UTC, Carlos Garnacho
none Details | Review
entry: Set up text drag icon within drag_begin() (3.83 KB, patch)
2015-05-21 16:04 UTC, Carlos Garnacho
committed Details | Review
dnd: Fix some issues with dnd icons under Wayland (6.98 KB, patch)
2015-12-01 13:16 UTC, Matthias Clasen
rejected Details | Review
dnd: Fix issues with Wayland dnd surfaces (5.77 KB, patch)
2015-12-01 15:37 UTC, Matthias Clasen
none Details | Review

Description Carlos Garnacho 2015-05-01 17:38:36 UTC
On wayland, dragging text from any GtkEntry causes warnings around GdkWindow calls, and the drag icon remains invisible

From what I investigated this comes from gtkdnd machinery which first sets a default "text" icon, which is then replaced by the surface containing the text itself. This causes the old DnD icon widget to be destroyed, along with its resources.

But on wayland we hardcode the icon widget icon to be gdk_wayland_drag_context_get_dnd_window() (which basically holds the wl_surface we give on start_drag), this window should survive throughout all of DnD, although it's currently being destroyed with the first DnD icon.

I'm attaching a couple of patches/hacks that ensure the wl_surface live as long as DnD does, I guess it would be cleaner to make gtkdnd.c have an internal GtkWindow that's never destroyed, but something like the second patch would be needed anyway, because such window would be hidden anyway.
Comment 1 Carlos Garnacho 2015-05-01 17:39:21 UTC
Created attachment 302730 [details] [review]
window: Avoid destroying the hardcoded window

We don't control the lifetime of this hacked in GdkWindow, so we
shouldn't attempt to destroy it, so ensure it's cleanly detached
from the widget.
Comment 2 Carlos Garnacho 2015-05-01 17:39:26 UTC
Created attachment 302731 [details] [review]
wayland: Keep DnD icon surfaces' wl_surface static

If the DnD icon is replaced along gtkdnd.c, the window will be
subsequently unmapped, destroying the wl_surface with it.

As the wl_surface is supposed to remain during all the operation,
and just be left an empty buffer if no DnD icon is not meant to be
shown at some point, make it sure the DnD icon wl_surface survives
hide/show cycles.
Comment 3 Matthias Clasen 2015-05-01 19:52:50 UTC
Review of attachment 302730 [details] [review]:

::: gtk/gtkwindow.c
@@ +7333,3 @@
+      gtk_container_forall (GTK_CONTAINER (widget),
+                            (GtkCallback) gtk_widget_unrealize,
+                            NULL);

Hmm, this seems to manually do what the parent_class unrealize would do anyway ?

It would be nicer if we could still unconditionally chain up - just clear out the hardcoded_window first ?
Comment 4 Matthias Clasen 2015-05-01 20:12:53 UTC
Grr, this is all so ugly.

Would it be possible to get GtkWindow out of the loop here, at least for the cases where we are not force by a gtk_drag_set_icon_widget call ? We're mainly using gtk_drag_set_icon_surface/pixbuf/default in GTK+, the one exception being GtkNotebook, and we should really rather fix that.
Comment 5 Carlos Garnacho 2015-05-01 22:58:48 UTC
(In reply to Matthias Clasen from comment #3)
> Review of attachment 302730 [details] [review] [review]:
> 
> ::: gtk/gtkwindow.c
> @@ +7333,3 @@
> +      gtk_container_forall (GTK_CONTAINER (widget),
> +                            (GtkCallback) gtk_widget_unrealize,
> +                            NULL);
> 
> Hmm, this seems to manually do what the parent_class unrealize would do
> anyway ?

Mostly, except destroying the window...

> 
> It would be nicer if we could still unconditionally chain up - just clear
> out the hardcoded_window first ?

I tried to do that, ideally the window should be removed (and removed from being used as gtk_widget_set_window()) before the widget is hidden/unrealized, it wasn't any easy at that time without rising a bunch other warnings :(

(In reply to Matthias Clasen from comment #4)
> Grr, this is all so ugly.
> 
> Would it be possible to get GtkWindow out of the loop here, at least for the
> cases where we are not force by a gtk_drag_set_icon_widget call ? We're
> mainly using gtk_drag_set_icon_surface/pixbuf/default in GTK+, the one
> exception being GtkNotebook, and we should really rather fix that.

We're clearly hitting the limits of the hardcoded_window hack. I guess It'll be cleaner overall to expose the drag icon wl_surface on wayland-only api, and have some gdk_wayland_window_foreign_new (wl_surface*) for these cases where we can avoid window widgets. I'm concerned that we can't behave properly for the gtk_drag_set_icon_widget cases though...
Comment 6 Matthias Clasen 2015-05-04 11:53:25 UTC
(In reply to Carlos Garnacho from comment #5)
> (In reply to Matthias Clasen from comment #3)
> > Review of attachment 302730 [details] [review] [review] [review]:
> > 
> > ::: gtk/gtkwindow.c
> > @@ +7333,3 @@
> > +      gtk_container_forall (GTK_CONTAINER (widget),
> > +                            (GtkCallback) gtk_widget_unrealize,
> > +                            NULL);
> > 
> > Hmm, this seems to manually do what the parent_class unrealize would do
> > anyway ?
> 
> Mostly, except destroying the window...
> 
> > 
> > It would be nicer if we could still unconditionally chain up - just clear
> > out the hardcoded_window first ?
> 
> I tried to do that, ideally the window should be removed (and removed from
> being used as gtk_widget_set_window()) before the widget is
> hidden/unrealized, it wasn't any easy at that time without rising a bunch
> other warnings :(
> 
> (In reply to Matthias Clasen from comment #4)
> > Grr, this is all so ugly.
> > 
> > Would it be possible to get GtkWindow out of the loop here, at least for the
> > cases where we are not force by a gtk_drag_set_icon_widget call ? We're
> > mainly using gtk_drag_set_icon_surface/pixbuf/default in GTK+, the one
> > exception being GtkNotebook, and we should really rather fix that.
> 
> We're clearly hitting the limits of the hardcoded_window hack. I guess It'll
> be cleaner overall to expose the drag icon wl_surface on wayland-only api,
> and have some gdk_wayland_window_foreign_new (wl_surface*) for these cases
> where we can avoid window widgets. I'm concerned that we can't behave
> properly for the gtk_drag_set_icon_widget cases though...

May be possible to hack those using gtk_widget_draw, like we do the magnifier in GtkInspector ?
Comment 7 Carlos Garnacho 2015-05-21 16:04:07 UTC
Created attachment 303768 [details] [review]
entry: Set up text drag icon within drag_begin()

the drag_begin() vmethod is meant for this, and the internal DnD code
will set up a drag icon if ::drag_begin didn't do so, which means
we are first getting a "default" icon, and then replacing it with the
text surface.

This is completely harmless in X11, but causes issues on wayland as
the DnD icon window is expected to remain unchanged during DnD there.
Comment 8 Matthias Clasen 2015-05-21 18:35:52 UTC
(In reply to Carlos Garnacho from comment #7)

> This is completely harmless in X11, but causes issues on wayland as
> the DnD icon window is expected to remain unchanged during DnD there.

Really ? I thought we can draw on that surface just like any other surface ?
Comment 9 Carlos Garnacho 2015-05-21 20:00:22 UTC
(In reply to Matthias Clasen from comment #8)
> (In reply to Carlos Garnacho from comment #7)
> 
> > This is completely harmless in X11, but causes issues on wayland as
> > the DnD icon window is expected to remain unchanged during DnD there.
> 
> Really ? I thought we can draw on that surface just like any other surface ?

Sure, redraws/resizes are fine, let me remark "icon window" :). What I'm trying to avoid here is multiple calls to gtk_drag_set_icon_window(), which destroys the former DnD icon GdkWindow/surface. This function is currently at the core of every other gtk_drag_set_icon* call...

About the general case, I wonder if this is the way to go, and we:
- ensure surface/pixbuf/stock APIs don't end up replacing the entire icon window every time they're called
- Document gtk_drag_set_icon_widget() as "one time call on ::drag-begin, no way back to surface/pixbuf/stock icons, you are free to replace contents, but don't destroy"
Comment 10 Matthias Clasen 2015-05-21 21:04:55 UTC
(In reply to Carlos Garnacho from comment #9)

> Sure, redraws/resizes are fine, let me remark "icon window" :). What I'm
> trying to avoid here is multiple calls to gtk_drag_set_icon_window(), which
> destroys the former DnD icon GdkWindow/surface. This function is currently
> at the core of every other gtk_drag_set_icon* call...

Ah, I misunderstood.

> About the general case, I wonder if this is the way to go, and we:
> - ensure surface/pixbuf/stock APIs don't end up replacing the entire icon
> window every time they're called

That sounds like a good idea.

> - Document gtk_drag_set_icon_widget() as "one time call on ::drag-begin, no
> way back to surface/pixbuf/stock icons, you are free to replace contents,
> but don't destroy"

I assume in practice that is how it is used anyway ?
Comment 11 Carlos Garnacho 2015-05-21 21:40:19 UTC
(In reply to Matthias Clasen from comment #10)
> (In reply to Carlos Garnacho from comment #9)
> 
> > Sure, redraws/resizes are fine, let me remark "icon window" :). What I'm
> > trying to avoid here is multiple calls to gtk_drag_set_icon_window(), which
> > destroys the former DnD icon GdkWindow/surface. This function is currently
> > at the core of every other gtk_drag_set_icon* call...
> 
> Ah, I misunderstood.
> 
> > About the general case, I wonder if this is the way to go, and we:
> > - ensure surface/pixbuf/stock APIs don't end up replacing the entire icon
> > window every time they're called
> 
> That sounds like a good idea.

Great :), I'll make patches along the lines.

> 
> > - Document gtk_drag_set_icon_widget() as "one time call on ::drag-begin, no
> > way back to surface/pixbuf/stock icons, you are free to replace contents,
> > but don't destroy"
> 
> I assume in practice that is how it is used anyway ?

Yeah, pretty much I'd say. TBH I've yet to see any place where the drag icon is visibly replaced mid-DnD.
Comment 12 Matthias Clasen 2015-05-26 10:42:15 UTC
Review of attachment 303768 [details] [review]:

.
Comment 13 Carlos Garnacho 2015-05-26 16:06:12 UTC
Attachment 303768 [details] pushed as 9ff5d2e - entry: Set up text drag icon within drag_begin()
Comment 14 Matthias Clasen 2015-11-24 19:20:22 UTC
The warnings are gone. I think this can be closed ?
Comment 15 Carlos Garnacho 2015-11-24 20:28:38 UTC
I forgot about this one... the items in comment #9 should still be addressed IMO. We fixed the GtkEntry case, but some other widget might fall into the same issue.
Comment 16 Matthias Clasen 2015-12-01 13:16:51 UTC
Created attachment 316594 [details] [review]
dnd: Fix some issues with dnd icons under Wayland

The hardcoded window hack that we use under Wayland
can't withstand the drag widget getting destroyed
and recreated. So, avoid doing that when possible.

Even with this fix, there are still situations where
we will destroy the drag surface prematurely (e.g.
when setting first a drag icon, and then replacing
it with a widget).
Comment 17 Matthias Clasen 2015-12-01 15:37:09 UTC
Created attachment 316612 [details] [review]
dnd: Fix issues with Wayland dnd surfaces

The Wayland dnd surface must remain in place until the drag
is over. Setting it directly as the hardcoded window of the
widget we construct carries the danger that it might get
destroyed prematurely, e.g. when the application calls
gtk_drag_set_icon_name more than once and we recreate the
widget.

Instead, create a dedicated toplevel, and reparent the widget
into it.
Comment 18 Matthias Clasen 2015-12-01 15:37:34 UTC
yet another approach, reparenting into a dedicated toplevel.
Comment 19 Matthias Clasen 2015-12-01 17:46:49 UTC
from irc discussion, the plan going forward is to make gtk_drag_context_get_dnd_window a general api, and always create that toplevel with this window. Then we can move updating the window position to gdk for x11 (in the wayland case, the compositor does it for us).
Comment 20 Matthias Clasen 2015-12-01 18:26:30 UTC
Review of attachment 316594 [details] [review]:

we going with another approach