Created attachment 269311 [details] [review]
Use a GtkHeaderBar

Review of attachment 269311 [details] [review]:

Er, surprise last-minute patch! This is done pretty well. Mario, think we can sneak this in before 3.11.90?

(Allan doesn't really want us packing buttons into the header bars for the games, but I think it's a good transition strategy -- and maybe even a final design -- for the games that have lots of buttons, like Mahjongg. I already adopted it for Chess, for example.  Looks much better than the toolbar.)

We can't forget to bump the GTK+ dep in configure.ac to 3.10.

I'd like to add tooltips to all the buttons using the tooltip_text property of Gtk.Widget. To be consistent with the other games:

Undo: "Undo your last move"
Redo: "Redo your last move" (or something like that -- I don't think any other game has Redo)
Pause: "Pause the game"
Unpause: "Unpause the game"
Hint: "Receive a hint for your next move"

::: src/gnome-mahjongg.vala
@@ +101,3 @@
+        undo_image.icon_size = Gtk.IconSize.BUTTON;
+        undo_image.icon_name = "edit-undo-symbolic";
+        var undo_button = new Gtk.Button ();

We can do all this one line:

var undo_button = new Gtk.Button.from_icon_name ("edit-undo-symbolic", Gtk.IconSize.BUTTON);

Same for the other buttons

But see below

@@ +110,3 @@
+        var redo_image = new Gtk.Image ();
+        redo_image.icon_size = Gtk.IconSize.BUTTON;
+        redo_image.icon_name = "edit-redo-symbolic";

With undo and redo we need to check the text direction so that we use a different icon in RTL languages (edit-undo-rtl-symbolic and edit-redo-rtl-symbolic). The test would look like:

if (undo_button.get_direction() == Gtk.TextDirection.RTL)

@@ +120,3 @@
+        var hint_image = new Gtk.Image ();
+        hint_image.icon_size = Gtk.IconSize.BUTTON;
+        hint_image.icon_name = "dialog-information-symbolic";

Can you please change this to dialog-question-symbolic; we recently started using that one for hints instead.

(I almost thought dialog-information-symbolic was a great idea, since if applied consistently it would resolve the ambiguity between Hint in most games and Resolve in Tetravex, and let us drop the text labels from Tetravex, which are inconsistent with the other games.  But I think the question mark is just a really good icon for hints, and we can probably find something else for Tetravex eventually.)

@@ +643,3 @@
         var display_name = dpgettext2 (null, "mahjongg map name", game_view.game.map.name);
         /* Translators: This is the window title for Mahjongg which contains the map name, e.g. 'Mahjongg - Red Dragon' */
+        title.set_label (_("Mahjongg - %s").printf (display_name));

Let's use the map name for the window title; we don't need to include the program name anymore, that just adds clutter. This change would require updating the translator comment, of course.

Because Super Time Crunch, I'm just going to implement all the review comments myself instead of waiting!

Created attachment 269337 [details] [review]
Use a GtkHeaderBar

Review of attachment 269337 [details] [review]:

Very minor comments follow:

::: src/gnome-mahjongg.vala
@@ +100,1 @@
+        var undo_image = "";

How about undo_image_name (and redo_image_name) instead?

@@ +104,3 @@
+                undo_image = "edit-undo-symbolic";
+
+        var undo_button = new Gtk.Button.from_icon_name (undo_image,  Gtk.IconSize.BUTTON);

Every time you use the Gtk.Button.from_icon_name constructor, there's an extra space between the two parameters.

@@ +574,2 @@
         }
+        pause_button.image = pause_image;

This does work, but it'd be cleaner to update the old image instead of creating a new one each time. You can get the original image like this:

var pause_image = (Gtk.Image) pause_button.image;

Then you don't need to create the new image, or set its icon size, or reassign it to the button.

@@ +641,3 @@
         var display_name = dpgettext2 (null, "mahjongg map name", game_view.game.map.name);
         /* Translators: This is the window title for Mahjongg which contains the map name, e.g. 'Mahjongg - Red Dragon' */
+        title.set_label (_("%s").printf (display_name));

We don't need the printf anymore, just:

title.set_label (_(display_name));

@@ +649,3 @@
+        pause_image.icon_size = Gtk.IconSize.BUTTON;
+        pause_image.icon_name = "media-playback-pause-symbolic";
+        pause_button.image = pause_image;

See above

(In reply to comment #5)
> This does work, but it'd be cleaner to update the old image instead of creating
> a new one each time. You can get the original image like this:

Er actually, I'm not sure if this works after all. ^^  You can ignore these comments if you can't get it to work.

Created attachment 269340 [details] [review]
Use a GtkHeaderBar

Created attachment 269341 [details] [review]
Use a GtkHeaderBar

Review of attachment 269341 [details] [review]:

LGTM.

Review of attachment 269341 [details] [review]:

That was unexpected and just in the nick of time. Thank you Alex, it looks nice and works well. Because it is so pressing, I fixed the remaining intendation and whitespace-issues myself.

Since I pushed the patch only moments ago, I am pretty confident, it will make it into the release tonight ;)

Thanks again and thank you Michael for reviewing.