GNOME Bugzilla – Bug 673177
Shows separators in app menu that it should hide
Last modified: 2012-06-01 12:26:45 UTC
gnome-shell-3.4.0-1.fc17.x86_64 <menu id="appmenu"> <section> <item> <attribute name="label" translatable="yes">_Open</attribute> <attribute name="action">app.open</attribute> <attribute name="accel"><Ctrl>o</attribute> </item> <item> <attribute name="label" translatable="yes">Open _Location</attribute> <attribute name="action">app.open-location</attribute> <attribute name="accel"><Ctrl>l</attribute> </item> <section id="save-placeholder"/> </section> </menu> $ gdbus call --session --dest org.gnome.Totem --object-path /org/gnome/Totem/menus/appmenu --method org.gtk.Menus.Start "[0]" ([(uint32 0, uint32 0, [{':section': <(uint32 0, uint32 1)>}]), (0, 1, [{'action': <'app.open'>, 'label': <'_Open'>, 'accel': <'<Ctrl>o'>}, {'action': <'app.open-location'>, 'label': <'Open _Location'>, 'accel': <'<Ctrl>l'>}, {':section': <(uint32 0, uint32 2)>}])],) With this menu I get: Open Open Location [separator] [separator] After telling GTK+ to show the menu: $ gsettings set org.gnome.settings-daemon.plugins.xsettings overrides "{'Gtk/ShellShowsAppMenu':<0>}" The menu shows no separators in Totem itself.
Created attachment 210957 [details] [review] popupMenu: Don't add separators for empty sections Make sure that a section has more than one item before adding a separator for it.
Review of attachment 210957 [details] [review]: The fix makes sense, but then why doesn't src/test-gapplication.js (which has an explicit test for both empty submenus and empty sections) fail?
(In reply to comment #2) > Review of attachment 210957 [details] [review]: > > The fix makes sense, but then why doesn't src/test-gapplication.js (which has > an explicit test for both empty submenus and empty sections) fail? I can't make the test app run at all[1], but notice how the 2nd section is a level deeper than the top-level in the definition (that's because only top-level sections should have separators, I've been told). The patch works for me. [1]: Spits out, in a tight loop: GLib-GIO-CRITICAL **: g_dbus_connection_register_object: assertion `G_IS_DBUS_CONNECTION (connection)' failed
Created attachment 210964 [details] [review] test-gapplication: Untabify, add modeline
Created attachment 210965 [details] [review] test-gapplication: Fix the test set_app_menu() requires that we have a bus, which means that we need to do this in startup.
(In reply to comment #2) > Review of attachment 210957 [details] [review]: > > The fix makes sense, but then why doesn't src/test-gapplication.js (which has > an explicit test for both empty submenus and empty sections) fail? Because of _updateSeparatorVisibility. I have a feeling for what would cause this bug to happen, but I'm going to need a reproducible testcase.
Created attachment 210980 [details] [review] test-gapplication: Add new empty-section test
Created attachment 210983 [details] [review] popupMenu: Rework _updateSeparatorVisibility Instead of calculating separator visiblity for each separator on a signal, do it once when we open the menu.
Created attachment 210984 [details] [review] popupMenu: Expand sections before testing for consecutive separators If we have a model like: Separator Section Separator Separator Then we won't detect two consecutive separators, and thus we won't hide any of them. Fix this by expanding sections before testing separator consecutivity.
Created attachment 210985 [details] [review] gactionmuxer: Fix list_actions The code there before was just completely wrong
Seems that the new code is just a little wrong ;) gactionmuxer.c: In function 'g_action_muxer_list_actions': gactionmuxer.c:106:3: warning: passing argument 2 of 'g_hash_table_iter_next' from incompatible pointer type [enabled by default] In file included from /home/hadess/Projects/gnome-install/include/glib-2.0/glib.h:52:0, from /home/hadess/Projects/gnome-install/include/glib-2.0/gobject/gbinding.h:30, from /home/hadess/Projects/gnome-install/include/glib-2.0/glib-object.h:25, from /home/hadess/Projects/gnome-install/include/glib-2.0/gio/gioenums.h:30, from /home/hadess/Projects/gnome-install/include/glib-2.0/gio/giotypes.h:30, from /home/hadess/Projects/gnome-install/include/glib-2.0/gio/gio.h:28, from gactionmuxer.h:25, from gactionmuxer.c:24: /home/hadess/Projects/gnome-install/include/glib-2.0/glib/ghash.h:105:13: note: expected 'void **' but argument is of type 'gchar **'
(In reply to comment #11) > Seems that the new code is just a little wrong ;) > > gactionmuxer.c: In function 'g_action_muxer_list_actions': > gactionmuxer.c:106:3: warning: passing argument 2 of 'g_hash_table_iter_next' > from incompatible pointer type [enabled by default] > In file included from > /home/hadess/Projects/gnome-install/include/glib-2.0/glib.h:52:0, > from > /home/hadess/Projects/gnome-install/include/glib-2.0/gobject/gbinding.h:30, > from > /home/hadess/Projects/gnome-install/include/glib-2.0/glib-object.h:25, > from > /home/hadess/Projects/gnome-install/include/glib-2.0/gio/gioenums.h:30, > from > /home/hadess/Projects/gnome-install/include/glib-2.0/gio/giotypes.h:30, > from > /home/hadess/Projects/gnome-install/include/glib-2.0/gio/gio.h:28, > from gactionmuxer.h:25, > from gactionmuxer.c:24: > /home/hadess/Projects/gnome-install/include/glib-2.0/glib/ghash.h:105:13: note: > expected 'void **' but argument is of type 'gchar **' Yeah, I fixed that locally. Just change the (gchar **) cast to (void **).
Created attachment 211230 [details] [review] Make it possible for plugins to add themselves to the app menu
Comment on attachment 211230 [details] [review] Make it possible for plugins to add themselves to the app menu Wrong bug, sorry.
Review of attachment 210957 [details] [review]: No.
Review of attachment 210983 [details] [review]: No.
Review of attachment 210984 [details] [review]: No.
I think we decided that the model in this bug is completely broken and has undefined behaviour, so we're not going to land the patches. The gactionmuxer and test-gapplication.js fixed should still land, though.
Review of attachment 210985 [details] [review]: This is the same fix that was applied to the gtk copy of gactionmuxer.c. Makes sense to keep the copies in sync.
Comment on attachment 210985 [details] [review] gactionmuxer: Fix list_actions Attachment 210985 [details] pushed as a36de92 - gactionmuxer: Fix list_actions
Review of attachment 210964 [details] [review]: Definitely
Review of attachment 210965 [details] [review]: And this one too. (to be honest, I had it locally, and just didn't push because I thought nobody used test-gapplication...)