Created attachment 274362 [details] [review]
Improvements to the buttons area

Improvements:
* clock label separated from pause/resume button
* pause/resume button made text-only

Review of attachment 274362 [details] [review]:

OK, that's not the full design, but I think it's sufficient for the purposes of this bug. But can you please rename the label from Start to New Game -- this makes it less confusing that it takes you to the new game screen.

Also, I think these buttons should also have accelerators (access keys - for when holding down Alt).

(In reply to comment #2)
Thanks for the quick review.
> Review of attachment 274362 [details] [review]:
> 
> OK, that's not the full design, but I think it's sufficient for the purposes of
> this bug. 
Yes, there are plenty of bugs discussing the new design, I will try to tackle each separately, but the patches will probably depend on eachother.
> But can you please rename the label from Start to New Game -- this
> makes it less confusing that it takes you to the new game screen.
Now that you have mentioned, I have tried that button, and that takes me to the difficulty selection screen, so I guess that should be the Change difficulty button (that's the closest one I could think of), as New Game in Allan's mockups doesn't exist, it's rather Play again or Start again for playing with the same difficulty or Change difficuly for selecting another difficulty.
> 
> Also, I think these buttons should also have accelerators (access keys - for
> when holding down Alt).
You've got a point there, adding the accelerators, preparing an updated patch, and I'll submit it for a review as soon as it's ready.

(In reply to comment #3)
> (In reply to comment #2)
> Thanks for the quick review.
> > Review of attachment 274362 [details] [review] [details]:
> > 
> > OK, that's not the full design, but I think it's sufficient for the purposes of
> > this bug. 
> Yes, there are plenty of bugs discussing the new design, I will try to tackle
> each separately, but the patches will probably depend on eachother.

Yeah, it's a bit confusing... but it also helps to have it broken down into little chunks.  I'm actually surprised how little there is left to do, with Allan providing the new icons.

> > But can you please rename the label from Start to New Game -- this
> > makes it less confusing that it takes you to the new game screen.
> Now that you have mentioned, I have tried that button, and that takes me to the
> difficulty selection screen, so I guess that should be the Change difficulty
> button (that's the closest one I could think of), as New Game in Allan's
> mockups doesn't exist, it's rather Play again or Start again for playing with
> the same difficulty or Change difficuly for selecting another difficulty.

Right, this button would eventually become Change Difficulty, but since there is no Play Again button yet it makes sense to just call it New Game for now.  (Or you could add the Play Again button :)

Created attachment 274400 [details] [review]
Additional patch

This patch applied over the previous one updates the buttons to match the latest mockups by hiding/showing them based on the game status.
The suggestions from the previous review have also been implemented, so the buttons now have mnemonics set for easy access.

Review of attachment 274400 [details] [review]:

Looking good.  One problem is that the button layout gets quite confused if the game gets paused before it has started, but that's actually an issue in 3.12 as well, so let's punt that to another bug #728305.

Another problem is that the buttons would look better if they were wider, as shown in the design.  You can fix that now as part of this bug, or we can do so later, I don't mind either way.

::: src/gnome-mines.vala
@@ +200,3 @@
         buttons_box.pack_start (hint_button, false, false, 0);
         size.add_widget (hint_button);
+        hint_button.show_all();

There are a few places in this patch where you miss the space before the opening parenthesis; can you fix those all please?

Hey Allan, Robert implemented you design where the Play Again and Change Difficulty buttons appear when the game is paused.  I think it's a bit confusing that the presence of these buttons depends on whether or not the game is paused -- it'd make more sense to me if they were either always present, or only present after the game is over.

Created attachment 274412 [details] [review]
Complete patch

The previous two patches merged and fixed based on reviews, implementing the button bar as seen in the mockups.

Review of attachment 274412 [details] [review]:

Meh, now 8x8 mode looks pretty bad -- any chance you can figure out what to do about this?  If not, we'll just have to go back to the skinnier buttons for now.

::: src/gnome-mines.vala
@@ +181,3 @@
         buttons_box.margin_right = 15;
         buttons_box.margin_left = 15;
+        buttons_box.set_size_request (140, -1);

How about 100 instead of 140?  This would closer match Allan's design, and I think the buttons are a bit too wide otherwise.

@@ +219,3 @@
+        play_pause_button.hide ();
+
+        high_scores_button = new Gtk.Button.with_mnemonic (_("_Best\nTimes"));

I appreciate the attention to detail, but we're not really supposed to use newlines in translatable strings. The labels would be fine on a single line, but maybe they'll wrap naturally if the buttons were not so wide.

Created attachment 274416 [details]
screenshot, 8x8 mode

Created attachment 274421 [details] [review]
Updated complete patch

Updated based on review:
* removed new lines from translatable strings
* removed minimum size request for buttons-box
* added sane minimum size request (420x420) for the window based on the smallest size the window looks bearable, but still close to the mockup (the best screen to test with is the game lost screen containing three long-texted buttons, and the top button should not overlap the clock)

Created attachment 274536 [details] [review]
Updated buttons to match the mockups

Hm, that looks even worse. :D

I'm not sure what else you changed in that last patch (I guess the icon sizes), but by reverting the last two points, I now have it looking good. Can you please review this new version of the patch to make sure you're OK with it (since it has your name on it)? Thanks!

Review of attachment 274536 [details] [review]:

Looks fine for me, with one minor issue:
* Start 8x8 game
* Resize to minimum size
* pause the game
Expected: the game pauses
What happens: the game pauses and resizes the window vertically
If you're fine with this, it's ok for me, I will come up with a fix for that separately later :)

Eh, turns out the size request wasn't hurting anything, and we can fix that by just setting a larger value.

Con: might not work on themes with really large buttons.

Thanks for iterating on this, Robert!

The following fixes have been pushed:
d7681d8 Set a reasonable size request for the main window
dd11c7d Updated buttons to match the mockups

Created attachment 274556 [details] [review]
Set a reasonable size request for the main window

Otherwise, the window can be resized to excessively small sizes. Besides
being unuseful in its own right, this causes the window to expand when
pausing the game, since pausing the game adds more buttons.

Created attachment 274557 [details] [review]
Updated buttons to match the mockups

* Updated the buttons to match the mockups
* Set a sane minimum size for the window
* Reduce spacing between the new game screen buttons to 18px

Created attachment 274599 [details]
modified screenshot

This looks great! Big improvement. I see two issues though:

1. The padding on the buttons could be better. Right now they are too tall, and there isn't enough horizontal space around the labels. I would reduce the height and change "Change Difficulty" to "Difficulty" (see the modified screenshot).

2. When I start a game I see a "Change Difficulty" button on its own. Once I make my first move, it changes to "Pause". This feels a bit odd to take that option away from the user. Also, I've just selected the difficulty - why would I want to then change it again? A better solution (at least to me) would be to start the timer as soon as the game view opens - so you always see "Pause".

1. Yes, all the padding is better in your mockup and in your modified screenshot.

2. I'd prefer for the timer to start as soon as the player makes the first move and not before, like in our other timed games.

What confused me slightly is that the New Game and Change Difficulty options appear while the game is paused; is there any reason for that? I think it'd be less confusing for them to either be available at all times, or else only after the game is over. (Why should the player need to know that pausing the game allows him to start over?)

Created attachment 274639 [details] [review]
Fix remaining sidebar button issues

Fixed various sidebar button issues
    
* Reworded Change Difficulty button to Difficulty
* Added button padding to match the mockups (12 px top and bottom, 24 px left and right)
* Do not show the Difficulty button between game start and timer start

Looks slick, thanks