GNOME Bugzilla – Bug 414712
gtk_container_set_focus_child leaks widget
Last modified: 2010-09-01 14:08:26 UTC
if I do gtk_container_set_focus_child(bin,widget); and later don't do gtk_container_set_focus_child(bin,NULL); I end up with a refcount leak of widget. Besides this function should have docs. The question is should thsi be documented and its up to the developer to unset the focus child, or is there something missing in containers dispose.
Without reading the code to remind myself of the details - this a very, very specialized function, and I wouldn't expect it to be called outside of GTK+ internals and the implementation of a few very specialized widgets. If you are just looking to focus a widget, the way to do that is gtk_widget_grab_focus(); gtk_container_set_focus_child() is used for maintaining internal state used for tab-navigation.
I have a notbook widgets and I want to tell gtk which widget should get the focus when switching to this tab. Would you say that I should better use gtk_container_get_focus_chain() move_my_widget_to_head() gtk_container_set_focus_chain() ? I would like to add docs for this method as a outcome of this discussion.
Owen, what would you suggest to be used instead for my use case? Is there an alternative?
I have had a look at the gstcontainer.c code and this is from gtk_container_real_set_focus_child (GtkContainer *container, GtkWidget *child) { ... if (child != priv->focus_child) { if (priv->focus_child) g_object_unref (priv->focus_child); priv->focus_child = child; if (priv->focus_child) g_object_ref (priv->focus_child); } Thus unrelated wheter I do thing right or wrong, it is probably a good idea to add if (priv->focus_child) { g_object_unref (priv->focus_child); priv->focus_child = NULL; } to gtk_container_destroy(). How does that sounds like?
Use gtk_widget_grab_focus in a notify::page handler, maybe ? I haven't tested it, but I think it should work.
Matthias, that could work, will try. Still shall we do some changes on GtkContainer, like - fix the leak - change the docs to make it explicit that its not for apps
Yes, those both sound like good ideas indeed.
Created attachment 168383 [details] [review] don't leak the focus_child ref count
Created attachment 168384 [details] [review] clarify the docs I'd appreciate double checking the docs change to make sure I understood it right. If so I will also add a doc-blob for the signal.
(In reply to comment #5) > Use gtk_widget_grab_focus in a notify::page handler, maybe ? > I haven't tested it, but I think it should work. Its a bit problematic :/ It works most of the time, but sometimes the widget that should be focused on the new page is not realized yet, or not realized and mapped even. I have deferred the gtk_widget_grab_focus() even to an idle handler from the notify::page handler. No improvement :/ As a side note, the docs for gtk_widget_grab_focus() should tell that the widget must be realized and mapped.
Solved: I was using "switch-page", when using "notify::page" it works. Seems "swicth-page" is emitted too early.
Sorry for the noise, but after I switched all "switch-page" handler to use "notify::page" I have the problem back. So please ignore comment #11.
> As a side note, the docs for gtk_widget_grab_focus() should tell that the > widget must be realized and mapped. I agree
Review of attachment 168383 [details] [review]: Looks good
Review of attachment 168384 [details] [review]: Looks good.
pushed both to gtk-2-22 and master. I also pushed the gtk_widget_grab_focus() addition.