Re: window leaks from gtk_drag_set_icon_widget() (fwd)

[Tim Janik <timj gtk org>]

On 10 Feb 1999, Owen Taylor wrote:

> 
> 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.

it imposes the same window widget leak (and therefore memory leak).

> > 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.

the idea behind here is that windows need to be handled completely
different wrt reference counting and destruction. normal widgets
and objects may get destroyed through simply _ref/_unref pairs
(or _new/_unref for that matter), while windows have to be explicitely
destroyed due to their "magic" toplevel ref count.
the API just reflects that in that it explicitely requires window widgets.

> 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). 

s/destroy/auto_destroy/ in the above prototype and we are set. ;)

> 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. 

i tend to disagree, both of the examples you mentioned did actually
expect the window to be destroyed and in the end leaked window widgets.

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

with people starting to extend use of the dnd API, this would just
result in an unneccessary amount of code duplication, where gtk could
very simply provide the additional convenience of automated destruction.

> 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);

i presume the second line meant to say
   gtk_widget_unref (window);
which wouldn't work at all since gtk_window_destroy() will call
gtk_container_unregister_toplevel() which would result in stealing a
reference count from the current signal emission (::destroy) and we
would end up with an _unref of 0 warning somewhere from the signal
or event handling code.

> 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.

within the dnd code itself you probably wouldn't want to hook up
an extra signal handler to a custom widget, just to destroy an internal
window widget. if people for some reason invoke gtk_drag_set_icon_widget()
you'd even get multiple unused windows floating around just to wait
for their destruction.
instead, a simply dataset destroy notification handler on info->context
seems more appropriate.

> > 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.

but still confusing. we use lots of warnings inside of gtk that could
be skipped since their effect is obvious, just try gtk_widget_show (NULL).
eventhough that would obviously fail on you, we still return_if_fail
widget != NULL there.
i'd say that figuring out what type of widget needs to be passed into
gtk_drag_set_icon_widget() is less obvious than whether gtk_widget_show's
widget should be != NULL, so the return_if_fail statement is very well
deserved in that place.

> 
> Regards,
>                                         Owen
> 

---
ciaoTJ