GNOME Bugzilla – Bug 748763
warnings when starting drag from GtkEntries
Last modified: 2015-12-02 04:49:22 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.
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.
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.
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 ?
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.
(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...
(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 ?
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.
(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 ?
(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"
(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 ?
(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.
Review of attachment 303768 [details] [review]: .
Attachment 303768 [details] pushed as 9ff5d2e - entry: Set up text drag icon within drag_begin()
The warnings are gone. I think this can be closed ?
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.
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).
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.
yet another approach, reparenting into a dedicated toplevel.
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).
Review of attachment 316594 [details] [review]: we going with another approach