After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 414712 - gtk_container_set_focus_child leaks widget
gtk_container_set_focus_child leaks widget
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: Other
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2007-03-04 21:59 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2010-09-01 14:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
don't leak the focus_child ref count (963 bytes, patch)
2010-08-20 10:58 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
clarify the docs (1.72 KB, patch)
2010-08-20 11:06 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2007-03-04 21:59:38 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.
Comment 1 Owen Taylor 2007-03-05 04:15:40 UTC
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.
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2007-03-05 07:40:39 UTC
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.
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-19 06:41:08 UTC
Owen, what would you suggest to be used instead for my use case? Is there an alternative?
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-19 10:09:27 UTC
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?
Comment 5 Matthias Clasen 2010-08-19 13:31:10 UTC
Use gtk_widget_grab_focus in a notify::page handler, maybe ? 
I haven't tested it, but I think it should work.
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-19 13:35:39 UTC
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
Comment 7 Matthias Clasen 2010-08-19 14:14:59 UTC
Yes, those both sound like good ideas indeed.
Comment 8 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-20 10:58:17 UTC
Created attachment 168383 [details] [review]
don't leak the focus_child ref count
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-20 11:06:01 UTC
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.
Comment 10 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-20 21:00:03 UTC
(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.
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-20 21:06:42 UTC
Solved: I was using "switch-page", when using "notify::page" it works. Seems "swicth-page" is emitted too early.
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2010-08-20 21:17:54 UTC
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.
Comment 13 Matthias Clasen 2010-08-23 15:04:21 UTC
> As a side note, the docs for gtk_widget_grab_focus() should tell that the
> widget must be realized and mapped.

I agree
Comment 14 Matthias Clasen 2010-08-25 02:31:31 UTC
Review of attachment 168383 [details] [review]:

Looks good
Comment 15 Matthias Clasen 2010-08-25 02:33:32 UTC
Review of attachment 168384 [details] [review]:

Looks good.
Comment 16 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-01 14:07:39 UTC
pushed both to gtk-2-22 and master. I also pushed the gtk_widget_grab_focus() addition.