GNOME Bugzilla – Bug 639419
Deprecate gtk_widget_set_window in favor of gtk_widget_attach_window() which manages the ref count.
Last modified: 2016-01-24 06:24:09 UTC
Created attachment 178221 [details] [review] Patch that adds gtk_widget_attach_window As the title says, basically we've been getting confusion around this api as one would expect gtk_widget_set_window() to manage the refcount for us, which it doesnt. This patch deprecates the old gtk_widget_set_window() and adds a gtk_widget_attach_window() which manages the refcount and also does the gdk_window_set_user_data() bit. The patch updates all of GTK+ to use the new api instead, except for GtkPlug, which does this weird push/pop error thingy which I was not sure exactly how to handle (although I'm sure it can be properly handled with the new api too).
Maybe would be a good idea deprecate this in 2.24 so we can remove the former API in GTK+3?
Mathias Clasen asked me to explain why this is an issue, so I'll try: gtk_widget_set_window() does not ref the GdkWindow that it's given, but it does unref it later when the GtkWidget is unrealized. (gtk_widget_real_unrealize() calls gdk_window_destroy(), which unrefs the GdkWindow.). So this looks like a simple reference-counting error, leading to crashes or critical warnings, and that error is documented already: http://git.gnome.org/browse/gtk+/commit/?id=a6a036ce22c2fa21b47a405e625360c02a140c91 (though it should also be documented as not unrefing the existing window when you pass it NULL.) It's "just" a simple reference counting error, but those are the hard ones to deal with, particularly when you don't expect the error to be in GTK+ itself. This is more of a problem in language bindings, such as gtkmm, which generally hide the reference-counting, dealing with the various inconsistencies. In this case, we can probably fix it in gtkmm by doing the ref/unrefing ourselves, but GTK+ 3 is an opportunity to get this right for everyone.u
(In reply to comment #2) > Mathias Clasen asked me to explain why this is an issue, so I'll try: > > gtk_widget_set_window() does not ref the GdkWindow that it's given, but it does > unref it later when the GtkWidget is unrealized. (gtk_widget_real_unrealize() > calls gdk_window_destroy(), which unrefs the GdkWindow.). I don't quite follow your 'it does unref it later' argument. gtk_widget_set_window is not involved at all in unrealize. If it were, your case would be much stronger. > So this looks like a simple reference-counting error, leading to crashes or > critical warnings, and that error is documented already: > http://git.gnome.org/browse/gtk+/commit/?id=a6a036ce22c2fa21b47a405e625360c02a140c91 > (though it should also be documented as not unrefing the existing window when > you pass it NULL.) You are not allowed to pass it NULL. If you were, the docs would say so. > It's "just" a simple reference counting error, but those are the hard ones to > deal with, particularly when you don't expect the error to be in GTK+ itself. > > This is more of a problem in language bindings, such as gtkmm, which generally > hide the reference-counting, dealing with the various inconsistencies. In this > case, we can probably fix it in gtkmm by doing the ref/unrefing ourselves, but > GTK+ 3 is an opportunity to get this right for everyone.u My problem with adding a function that does a little more is that there is also the need to call gdk_window_set_user_data (window, widget). Now, we could make your new function do that as well, (like the patch does) but then things start to look really odd for widgets that create a second window (for whatever reason), because the will have to do the set_user_data call manually for the other windows. And gtk_widget_attach_window really sounds a lot like it could just be called for every window that belongs to the widget, which is not the case. Finally, I notice that we have just traded the need to call g_object_ref on inherited windows for the need to call g_object_unref on non-inherited windows. In summary, I'm not convinced that we really win enough here to justify duplicating a really special-purpose function, with a subtly different really special purpose function, this close to release.
I feel compelled to reply here. (In reply to comment #3) > (In reply to comment #2) > > Mathias Clasen asked me to explain why this is an issue, so I'll try: > > > > gtk_widget_set_window() does not ref the GdkWindow that it's given, but it does > > unref it later when the GtkWidget is unrealized. (gtk_widget_real_unrealize() > > calls gdk_window_destroy(), which unrefs the GdkWindow.). > > I don't quite follow your 'it does unref it later' argument. > gtk_widget_set_window is not involved at all in unrealize. > If it were, your case would be much stronger. > > > So this looks like a simple reference-counting error, leading to crashes or > > critical warnings, and that error is documented already: > > http://git.gnome.org/browse/gtk+/commit/?id=a6a036ce22c2fa21b47a405e625360c02a140c91 > > (though it should also be documented as not unrefing the existing window when > > you pass it NULL.) > > You are not allowed to pass it NULL. If you were, the docs would say so. Yes you are allowed to pass it NULL. That's how any unrealize implementation would generally unset the window pointer after destroying it. (yes, all internal GTK+ widgets that create a window chain up in ->unrealize() instead, if that's something that was manditory then it would be documented as such, right ?). The code of gtk_widget_set_window explicitly does: g_return_if_fail (window == NULL || GDK_IS_WINDOW (window)); (i.e. if the gtk-doc annotation is missing the @allow-none attribute, thats a bug in the docs). > > It's "just" a simple reference counting error, but those are the hard ones to > > deal with, particularly when you don't expect the error to be in GTK+ itself. > > > > This is more of a problem in language bindings, such as gtkmm, which generally > > hide the reference-counting, dealing with the various inconsistencies. In this > > case, we can probably fix it in gtkmm by doing the ref/unrefing ourselves, but > > GTK+ 3 is an opportunity to get this right for everyone.u > > My problem with adding a function that does a little more is that there is also > the need to call gdk_window_set_user_data (window, widget). Now, we could make > your new function do that as well, (like the patch does) but then things start > to look really odd for widgets that create a second window (for whatever > reason), because the will have to do the set_user_data call manually for the > other windows. And gtk_widget_attach_window really sounds a lot like it could > just be called for every window that belongs to the widget, which is not the > case. Well, just trying to save a couple lines of code there, I dont think they are all that odd anyway (someone who needs an event window has to know how the event window works). My reasoning for putting that line inside gtk_widget_attach_window() was from looking at "all the stuff" that gtk_widget_set_parent() does. I thought it could be convenient to put that cal inside the function, but it could be left out of the function for purist reasons, The function is not meant to reduce the lines of code that a GTK+ programmer has to write. It's meant to make the code make sense. > Finally, I notice that we have just traded the need to call g_object_ref on > inherited windows for the need to call g_object_unref on non-inherited windows. Not exactly. We've traded unsafe unrefcounted code for safe refcounted code. If you want to know that gtk_widget_set_window() doesnt take a reference to the GdkWindow, which everyone would expect it to, then you have to go to the docs (and that's just not true for every other gtk_foo_set_bar() function in GTK+ that already works as the user would expect). You can agrue that we haven't lost any significant lines of code but that does not mean the code is not simplified. Now it makes sense. Doing: win = gdk_window_new (...); gtk_widget_set_window (widget, win); And then mosying off carelessly without releasing the initial reference count of the GdkWindow would lead me to believe at first glance of the code, that the GdkWindow must be GInitiallyUnowned, but it's not... so I have to ask myself as an innocent bystander, where is this reference count going ? I just created the window in my stack frame, the widget itself doest take a reference to it in gtk_widget_set_window()... so where is this ref going ? So no we dont lose lines of code, but we do something by the book in a sensitive area where something is currently done in a weird way (a way that is inconsistent with the rest of our api). > In summary, I'm not convinced that we really win enough here to justify > duplicating a really special-purpose function, with a subtly different really > special purpose function, this close to release.
(In reply to comment #3) > (In reply to comment #2) > > Mathias Clasen asked me to explain why this is an issue, so I'll try: > > > > gtk_widget_set_window() does not ref the GdkWindow that it's given, but it does > > unref it later when the GtkWidget is unrealized. (gtk_widget_real_unrealize() > > calls gdk_window_destroy(), which unrefs the GdkWindow.). > > I don't quite follow your 'it does unref it later' argument. > gtk_widget_set_window is not involved at all in unrealize. > If it were, your case would be much stronger. As Kjell found in bug #606903#c23, gtk_widget_real_unrealize() calls gdk_window_destroy(priv->window), which is documented as unrefing the GdkWindow. And that's the same priv->window that was stored by gtk_widget_set_window(). So, it _does_ seem to be involved.
(In reply to comment #4) > > That's how any unrealize implementation would generally unset the > window pointer after destroying it. (yes, all internal GTK+ widgets > that create a window chain up in ->unrealize() instead, if that's something > that was manditory then it would be documented as such, right ?). Requirements for widget subclassing are generally undocumented. Yes, that is a problem.
(In reply to comment #5) > As Kjell found in bug #606903#c23, gtk_widget_real_unrealize() calls > gdk_window_destroy(priv->window), which is documented as unrefing the > GdkWindow. And that's the same priv->window that was stored by > gtk_widget_set_window(). So, it _does_ seem to be involved. So, what do you think?
I think that deriving widgets is 'know-what-you-do' land for now, and we are not going to add api at the last minute to make one aspect of it a little better, when it is generally undocumented.
OK. I'm disappointed that you don't see how impossible it is to use this function properly at the moment, particularly from language bindings, but thanks for making a decision. We'll just try to avoid wrapping this in gtkmm. Hopefully people won't need to call it when creating custom widgets. Can we keep this open for some future GTK+ 4?
we do have gtk_widget_register_window now; I don't think we want to add more window setting apis in the mix