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 601409 - action area presence modifies notebook behaviour
action area presence modifies notebook behaviour
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkNotebook
2.19.x
Other Linux
: Normal major
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 116650 138020
 
 
Reported: 2009-11-10 14:38 UTC by Christian Persch
Modified: 2009-11-28 17:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (995 bytes, patch)
2009-11-10 14:41 UTC, Christian Persch
accepted-commit_now Details | Review

Description Christian Persch 2009-11-10 14:38:11 UTC
Steps to reproduce:
0) Start gnome-terminal
1) Observe no critical warnings on console; quit g-t
2) Modify gnome-terminal/src/terminal-window.c:terminal_window_init() by adding the following line directly after the "priv->notebook = gtk_notebook_new ();" line:

  gtk_notebook_set_action_widget (GTK_NOTEBOOK (priv->notebook), gtk_hbox_new (FALSE, 6), GTK_PACK_START);

3) Compile, start g-t
4) Observe critical warning on console, once for each new tab created:

(gnome-terminal:10359): GLib-GObject-CRITICAL **: g_object_get_data: assertion `G_IS_OBJECT (object)' failed
(gdb) where
  • #0 g_log
    at gmessages.c line 568
  • #1 g_return_if_fail_warning
  • #2 g_object_get_data
    at gobject.c line 2627
  • #3 terminal_tabs_menu_update
    at ../../src/terminal-tabs-menu.c line 465
  • #4 notebook_page_added_cb
    at ../../src/terminal-tabs-menu.c line 225
  • #5 _gtk_marshal_VOID__OBJECT_UINT
    at gtkmarshalers.c line 2387
  • #6 g_closure_invoke
    at gclosure.c line 767
  • #7 signal_emit_unlocked_R
    at gsignal.c line 3247
  • #8 g_signal_emit_valist
    at gsignal.c line 2980
  • #9 g_signal_emit
    at gsignal.c line 3037
  • #10 gtk_notebook_real_insert_page
    at gtknotebook.c line 4387
  • #11 gtk_notebook_insert_page_menu
    at gtknotebook.c line 6589
  • #12 gtk_notebook_insert_page
    at gtknotebook.c line 6519
  • #13 terminal_window_add_screen
    at ../../src/terminal-window.c line 2308
  • #14 terminal_app_new_terminal
    at ../../src/terminal-app.c line 1863
  • #15 terminal_app_handle_options
    at ../../src/terminal-app.c line 1800
  • #16 main
    at ../../src/terminal.c line 485

Analysis:
gtk_container_get_chilren() docs say "Returns the container's non-internal children. [...]" However, with the patch from bug 116650, the action widgets are included in this list when getting the notebook children. IMHO these widgets are internal children like the tab label widgets, and should therefore not be included. This certainly breaks g-t, and probably any other gnome MDI app (epiphany, gedit, ...).
Comment 1 Christian Persch 2009-11-10 14:41:34 UTC
Created attachment 147380 [details] [review]
patch
Comment 2 Matthias Clasen 2009-11-10 18:56:07 UTC
I would say the boundary between 'internal' and 'regular' children is at least somewhat fuzzy. Really, if the application provides a tab label widget, that is not an 'internal' widget. But if the app just provides a string, then the label widget is arguably internal. 

Also, it is somewhat hard to argue that MDI apps would be broken by this when they have to explicitly use the new api to get into the problematic situation in the first place.

Anyway, we should consider treating action widgets as internal.
Comment 3 Christian Persch 2009-11-10 19:15:47 UTC
(In reply to comment #2)
> I would say the boundary between 'internal' and 'regular' children is at least
> somewhat fuzzy. Really, if the application provides a tab label widget, that is
> not an 'internal' widget. But if the app just provides a string, then the label
> widget is arguably internal.

The current notebook makes no distinction there. gtk_container_get_children() gets exactly the list of tab content widgets. Which IMO is the right behaviour.

> Also, it is somewhat hard to argue that MDI apps would be broken by this when
> they have to explicitly use the new api to get into the problematic situation
> in the first place.

That's not exactly the case. E.g. when the app itself doesn't use that API, but an extension/plugin adds some action widget into the notebook (e.g. an epiphany extension adding a NewTab button). Then the app breaks.
Comment 4 Matthias Clasen 2009-11-10 19:21:22 UTC
I'm sure extensions have more than enough rope to break epiphany easily in other ways...
Comment 5 Matthias Clasen 2009-11-28 14:50:01 UTC
Comment on attachment 147380 [details] [review]
patch

Ok, lets go with this for now. Might be worth adding a line to the docs somewhere about it.
Comment 6 Christian Persch 2009-11-28 17:08:16 UTC
Thanks! Committed to master.

I added this line to the docs for gtk_notebook_set_action_widget:

 * Note that action widgets are "internal" children of the notebook and thus
 * not included in the list returned from gtk_container_foreach().