GNOME Bugzilla – Bug 732658
Don't use app menu / HeaderBar on shells that don't support it well
Last modified: 2014-07-07 00:16:25 UTC
Currently Mahjongg uses an application menu / GtkHeaderBar which looks great in GNOME but not necessarily great in other shells (e.g. Unity). In those shells a traditional menubar / decorations fit in better.
Created attachment 279808 [details] [review] Don't show HeaderBar / app menu on shells that don't support it his is the patch we're shipping in Ubuntu so we can have new Mahjongg and it fits into the Unity shell. For those running GNOME it looks the same as current master. I appreciate this might not fit in with the goals of the Mines project so I'll leave it to the maintainers to decide if this is an appropriate upstream addition. Obviously it adds some extra code / testing but I think it will probably not grow in complexity over time.
See bug #732655 which is the same thing for Mines. Probably best to keep the discussion in that bug as it's much the same thing.
Created attachment 279811 [details] [review] Don't show HeaderBar / app menu on shells that don't support it Updated to only check gtk-shell-shows-menubar
Review of attachment 279811 [details] [review]: I think it looks better in Unity WITH the header bar. At least, the double title bar does not seem super natural to me. But I'm fine with this if you're sure it's what you want. Note that Mahjongg's preferences dialog still has a header bar (in master, not 3.12); I guess you probably don't want that? ::: src/gnome-mahjongg.vala @@ +89,3 @@ + bool shell_shows_menubar; + Gtk.Settings.get_default ().get ("gtk-shell-shows-app-menu", out shell_shows_menubar); Oops, you mean gtk-shell-shows-menubar @@ +142,3 @@ + { + var menubar = new Gtk.MenuBar (); + In your Mines patch, you nest GLib.Menus and then call set_menu_bar(), can you do that here too? It's significantly more compact than creating a Gtk.MenuBar with a bunch of Gtk.MenuItems.
Created attachment 280013 [details] [review] Don't show HeaderBar / app menu on shells that don't support it Fix GTK+ setting check, use Glib.Menu instead of Gtk.Menu, don't use HeaderBar for preferences dialog
Review of attachment 280013 [details] [review]: OK to push after fixing this nit: ::: src/gnome-mahjongg.vala @@ +119,3 @@ var title_box = new Gtk.Box (Gtk.Orientation.VERTICAL, 2); title_box.pack_start (title, false, false, 0); + var alignment = new Gtk.Alignment (0.5f, 0.5f, 0.0f, 0.0f); Let's not use GtkAlignment since it was deprecated about a month ago... and I'm pretty sure it was evil anyway. halign is a better was to center widgets: status_box.halign = Gtk.Align.CENTER;
Thanks for the reviewing Michael!