Created attachment 279802 [details] [review]
Don't show HeaderBar / app menu on shells that don't support it

This is the patch we're shipping in Ubuntu so we can have new Mines and it fit 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.

Review of attachment 279802 [details] [review]:

Guess it's time to figure out how to handle this at last. Goals we probably(?) both agree on:

a) Mines and all our other GNOME games must continue to look good in future versions of Ubuntu.
b) We should handle this consistently between all GNOME games at the least, and preferably between all GNOME apps (but that's not likely to happen).

More probable(?) points of agreement:

c) Downstream patches are OK, but not good.
d) Having two different title bar and menu structures for each game is OK, but not good.
e) 100 extra lines of code is not a big deal.

I don't want to do this, but it's probably the only way to meet goal (a), short of convincing you that header bars are cool (which I guess is not very likely).

Some initial thoughts -- not changing the patch status since the whole team needs to discuss this.

::: src/gnome-mines.vala
@@ +202,3 @@
+        if (!has_app_menu ())
+        {
+            var menubar = new Gtk.MenuBar ();

Can this please go in a UI file instead?

@@ +307,3 @@
+        gtk_settings.get ("gtk-shell-shows-app-menu", &show_app_menu, "gtk-shell-shows-menubar", &show_menubar, null);
+
+        return show_app_menu && !show_menubar;

I definitely do not want to show a menu bar anywhere besides Unity. Nowadays we show the app menu with a menu button in the header bar, which I strongly prefer to a menu bar. So please just directly store Gtk.Settings.get_default ().get ("gtk_shell_shows_menu_bar") into a bool member variable has_global_menu (instead of has_app_menu) and use that to determine which menu structure to use.

I said the same in Bug #712831 -- I definitely should have CCed you there, sorry.

Opinions?

Oh, and what about preferences dialogs -- not an issue for Mines, but for other games. I guess you want those header bars to be conditional as well?  Do you want the Close buttons in the action area that we've gotten rid of, or will the window manager's close button suffice?

Created attachment 279805 [details]
screenshot of header bars in unity

jhbuild-built Chess and Calculator in 14.04.  I honestly think this is very close to OK, except for the incorrect minimize/maximize/close buttons (which is configurable).

Other issues: corners of the header bar aren't rounded, the title is in a different font (and centered rather than left-aligned, but I think we're stuck with that), and the minimize/maximize/close buttons need to be hidden when the window is maximized (probably not currently possible, haven't investigated).

Created attachment 279807 [details]
screenshot of Mines, before patch

Mostly I'm grateful that the board looks right. Wasn't really sure what to expect.

See bug #732658 for the same thing in Mahjongg.

Created attachment 279809 [details]
screenshot of menu, before patch

I do think header bars are cool, but only when they're used consistently across all applications (i.e. in GNOME). In particular they don't work well in Ubuntu because maximising a window blends the window decorations into Unity top bar and this doesn't happen with HeaderBars. 

Yes, agree on all a) b) c).

I suspect the preferences dialogs are a non-issue - aren't dialogs moving to be rendered inside the window so the WM will no longer be involved at all?

I'll update the patch to use the .ui file. Was also holding myself back from making a GtkTemplate patch - I just converted Simple Scan and Vala+GtkBuilder+GtkTemplate is super nice :)

Created attachment 279810 [details] [review]
Don't show HeaderBar / app menu on shells that don't support it

Updated to only check gtk-shell-shows-menubar

Created attachment 279812 [details] [review]
Don't show HeaderBar / app menu on shells that don't support it

Put menubar in .ui file

OK, regarding the menu, after reading bug #722092 it looks like Ryan and Lars are favoring traditional menu bars when the shell does not show the menu bar (Xfce, Cinnamon). One thing I am positive about: we do not ever want to show a menu bar in the window with a header bar. My preference is still to use header bar and fallback app menu in these environments -- I think it looks a lot better since our menus are small -- but I'm open to the alternative.

Screenshot upcoming for anyone unfamiliar with 3.12 fallback app menus.  (The popover is configurable on GTK+ level and I think it defaults to false.) These only work if the app uses a header bar in the main window: if not, the 3.10-style menu bar fallback is used.

(In reply to comment #9)
> I do think header bars are cool, but only when they're used consistently across
> all applications (i.e. in GNOME). In particular they don't work well in Ubuntu
> because maximising a window blends the window decorations into Unity top bar
> and this doesn't happen with HeaderBars. 

This just seems like a very fixable thing.  And the rest is just theming and settings. I mean, if header bars looked horribly out of place, that'd be one thing, but you're already pretty darn close.  Just seems like it'd be easiest for both GNOME and Ubuntu to roll with them, even if that wasn't your original plan.

As I mentioned on IRC, GNOME is pushing really hard to use header bars all over the place, and maintaining a patch set will not be fun.  But maybe you have to: I doubt you're going to be keen on the action dialog pattern [1] that popped up in 3.12 and is used *pervasively* in 3.13.  (The games do not currently use any of these.)

> I suspect the preferences dialogs are a non-issue - aren't dialogs moving to be
> rendered inside the window so the WM will no longer be involved at all?

Er, I'm not sure. We're not changing anything about how dialogs are created, I don't think, we just make a GtkDialog and set it transient for the main window. And that there is the extent of my expertise on this topic, but I think the WM must be involved, because if the WM does not draw a title bar and there is no header bar, how would the user be able to close the preferences dialog (now that the dialog's action area is deprecated -- we no longer have close, apply, OK, cancel, etc. buttons on the bottom of dialogs)?

> I'll update the patch to use the .ui file. Was also holding myself back from
> making a GtkTemplate patch - I just converted Simple Scan and
> Vala+GtkBuilder+GtkTemplate is super nice :)

Yeah I'm looking at an example [2] and my only wonder is why we didn't start using this a year ago.

[1] https://wiki.gnome.org/Design/HIG/Dialogs
[2] http://blogs.gnome.org/tvb/2013/05/29/composite-templates-lands-in-vala/

Created attachment 279813 [details]
fallback app menu screenshot (not used in Unity)

Review of attachment 279812 [details] [review]:

Can you take a look at how the menu is created in the patch in Bug #712831; that's a much cleaner approach.

::: src/gnome-mines.vala
@@ +209,3 @@
+
+            var item = ui_builder.get_object ("new_game_menuitem") as Gtk.MenuItem;
+            item.activate.connect (() => { new_game_cb (); });

Hm, I think you don't need any of this code because of the action-name property of GtkMenuItem, inherited from GtkActionable.  (But don't do that, since that's still not as good as the approach taken in Bug #712831.)

Created attachment 279815 [details] [review]
Don't show HeaderBar / app menu on shells that don't support it

Use Gtk.Application.set_menubar

I'm personally fine with having a fallback for DEs not wanting headerbars, as long as we keep the diff minimal, and Robert's patch does that. I still have to test it on Ubuntu (will do that later today), right now I'm on Fedora, but right now I'd give it a commit_now review based solely on the code review.

Review of attachment 279815 [details] [review]:

Currently this crashes on gnome-shell as the window object you are trying to set the headerbar for (gnome-mines.vala:line 145) is initialized only later (gnome-mines.vala:line 187). Could you please fix that?

Created attachment 279877 [details] [review]
Don't show HeaderBar / app menu on shells that don't support it

Whoops, fixed

Review of attachment 279877 [details] [review]:

Robert Roth, I think this is good to push?

(In reply to comment #19)
> Review of attachment 279877 [details] [review]:
> 
> Robert Roth, I think this is good to push?

I'm not sure, for me the menu is not shown with XFCE (which has gtk-shell-shows-menubar=1 in .config/gtk+3.0/settings.ini ). I will test this later today with Unity, and will commit if it works as I expect it to work.

The menu should only be shown in the window if gtk-shell-shows-menubar=0, so our code is correct. With that misconfiguration it's a wonder you see menu bars in any of your apps, but I guess the apps that display menu bars simply aren't using GMenu to do so.

Warning: if you try to test this patch in Unity with jhbuild, you'll need to install unity-gtk-module from Launchpad for it to work as expected. I couldn't find instructions so I haven't done so.

(In reply to comment #19) 
> Robert Roth, I think this is good to push?

Actually, let's wait until bug #732658 is resolved, since that one is more complicated (preferences dialog plus toolbar).

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.