Re: window leaks from gtk_drag_set_icon_widget()

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj@gtk.org> writes:

> hi owen,
> 
> you might have noticed (assuming you have export GTK_DEBUG=objects in
> your .profile ;) that most DnD operations leak winodws with ref counts
> of 1.
> this is due to code similar to
> 
> static void
> gtk_color_selection_drag_begin (GtkWidget      *widget,
>                                 GdkDragContext *context,
>                                 gpointer        data)
> {
>   [...]
>   window = gtk_window_new (GTK_WINDOW_POPUP);
>   [...]
>   gtk_drag_set_icon_widget (context, window, -2, -2);
>   /* i.e. set and forget about the window */
> }
> 
> for the colorselection, i just hooked up a drag_end handler that will
> care about the window's destruction, so at least the colorselection
> doesn't leak windows anymore.
> a bunch of other code that does similar things in their drag_begin
> handler probably needs to be adapted to care about the destruction
> as well.

Yes, this is a leak, but it isn't a bunch of code. 
gtk_drag_set_icon_widget() is used in exactly two places.

 gtkcolorsel.c
 gtkdnd.c

Nowhere else in all of GNOME CVS. Most placs 
gtk_drag_set_icon_pixmap() which handles memory fine
if you unref your pixmap/mask after calling the function.

> but given that the DnD internals do the same thing in
> gtk_drag_set_icon_pixmap(), i'd rather see us taking a different path,
> i.e.
> introduce
> 
> /* Set the image window being dragged around, the window will
>  * be auto-destructed at the end of the drag.
>  */
> void gtk_drag_set_icon_window  (GdkDragContext    *context,
>                                 GtkWidget         *widget,
>                                 gint               hot_x,
>                                 gint               hot_y);
> 
> which will auto-destroy the window in gtk_drag_remove_icon().

Actually, this is really bad from a couple of perspectives:

 gtk_drag_set_icon_window()

destroys the window at the end of the drag.
But:

 gtk_drag_set_icon_widget()

doesn't. That is about as non-intuitive as it gets. And,
it's API bloat. If we want to go this route, we should
do:

 void gtk_drag_set_icon_window  (GdkDragContext    *context,
                                 GtkWidget         *widget,
                                 gint               hot_x,
                                 gint               hot_y,
                                 gboolean           destroy);

And mark set_icon_widget as deprecated. (I actually
suspect that gtkcolorsel.c and gtkdnd.c are the only callers 
of gtk_drag_set_icon_widget() anywhere). 

But do we need to go this route?

gtk_drag_set_icon_window() is an extremely uncommon operation.
80% of people will use the default drag icon, 19% will use
gtk_drag_set_icon_pixmap(). I tend to suspect most of the
people who are calling gtk_drag_set_icon_window() _won't_
want their windows destroyed. 

So, I really think think that adding two-line "drag_end" 
handlers is probably the right way to handle things.

The other alternative, is that we could almost get away
with just using reference counting here. For the
existing cases, it would work fine to do:

  gtk_drag_set_icon_widget (context, window, -2, -2);
  gtk_widget_unref (context);

The only reason why this isn't, in general, good, is the
problem of people creating refcount cycles intepreted
languages, like:

  $window->signal_connect ("expose", {
    $window->window->draw_box (...);
  }

So, on balance, I vote for simply addding a "drag_end"
handler into gtkdnd.c.

> if people want to keep around the window for further drag operations,
> they can still use gtk_drag_set_icon_widget(). for a related matter,
> does it make sense at all to pass anything else than a window widget
> into gtk_drag_set_icon_widget? i presume not, and so we should probably
> add an appropriate g_return_if_fail statement.

Sure, we can add a g_return_if_fail statement(). The widget
really needs to be, not only a !NO_WINDOW widget, but
also a toplevel, but I don't see that enforcing that is
useful - the effects will be obvious.

Regards,
                                        Owen