GNOME Bugzilla – Bug 709781
Implement of the new design
Last modified: 2013-10-12 01:31:46 UTC
The new design of gnome-mines: https://raw.github.com/gnome-design-team/gnome-mockups/master/games/mines/mines.png I missing the flag symbol and the clock symbol, so in the meantime I add a strings instead.
Created attachment 256864 [details] [review] Implement the new deisng
Review of attachment 256864 [details] [review]: Wow, this was extremely helpful (and unexpected). Thank you! The one big problem I see is fullscreen; once you've entered fullscreen view, there's no longer any way out unless you know the F11 keyboard shortcut. If we're going to keep fullscreen, we need some way to get out of it with the mouse. My opinion is that it would be best to simply remove fullscreen entirely. I know Allan would prefer that the games focus on fullscreen play, but the graphics right now are definitely optimized for smaller windows, and the benefit of fullscreen over maximizing the window seems unclear even if they were. Minesweeper isn't an immersive 3D adventure, it's a five minute game. Not only do I think the game looks better maximized than it does in fullscreen, but fullscreen makes it difficult to access the application menu, the top bar of the shell, other windows, and the close button, since you have to leave fullscreen to do anything. This isn't a Mines-specific issue by any means: it applies to all the games. A secondary issue: it'd be nice if the "Play Again" button would change to "Start Over" or "New Game" (with an appropriate symbolic icon) when a game is in progress, and only display "Play Again" after triggering a mine. ::: src/gnome-mines.vala @@ +148,3 @@ window.maximize (); + var title = new Gtk.Label (_("Mines")); Is this variable, and the call to gtk_header_bar_set_custom_title below, redundant? My understanding is that set_custom_title is used to temporarily override the title set by gtk_header_bar_set_title, but here they are both set to "Mines." @@ +554,2 @@ new_game_action.set_enabled (false); + new_game_button.hide(); Between here and line 573 you occasionally forget to leave a space between the function name and the empty argument list. @@ +573,3 @@ + pause_button.show_all(); + flag_label.show (); + clock_label.show (); Since all these buttons/labels are in a GtkBox, please call show_all and (up above) hide on that box instead of on each contained element. (This is easy if you make buttons_box a private member.)
Created attachment 256874 [details] [review] Require Gtk+ 3.10 Used for GtkHeaderBar
(In reply to comment #2) > Review of attachment 256864 [details] [review]: > > Wow, this was extremely helpful (and unexpected). Thank you! > > The one big problem I see is fullscreen; once you've entered fullscreen view, > there's no longer any way out unless you know the F11 keyboard shortcut. If > we're going to keep fullscreen, we need some way to get out of it with the > mouse. > > My opinion is that it would be best to simply remove fullscreen entirely. I > know Allan would prefer that the games focus on fullscreen play, but the > graphics right now are definitely optimized for smaller windows, and the > benefit of fullscreen over maximizing the window seems unclear even if they > were. Minesweeper isn't an immersive 3D adventure, it's a five minute game. > Not only do I think the game looks better maximized than it does in fullscreen, > but fullscreen makes it difficult to access the application menu, the top bar > of the shell, other windows, and the close button, since you have to leave > fullscreen to do anything. This isn't a Mines-specific issue by any means: it > applies to all the games. I'm inclined to agree - full screen isn't especially useful in the current incarnation of Mines. > A secondary issue: it'd be nice if the "Play Again" button would change to > "Start Over" or "New Game" (with an appropriate symbolic icon) when a game is > in progress, and only display "Play Again" after triggering a mine. Sounds good. I'd probably go for "Start Over" rather than "New Game".
Created attachment 256890 [details] [review] Implement the new deisng (In reply to comment #2) > Review of attachment 256864 [details] [review]: > > Wow, this was extremely helpful (and unexpected). Thank you! No problem. > > The one big problem I see is fullscreen; once you've entered fullscreen view, > there's no longer any way out unless you know the F11 keyboard shortcut. If > we're going to keep fullscreen, we need some way to get out of it with the > mouse. > > My opinion is that it would be best to simply remove fullscreen entirely. I > know Allan would prefer that the games focus on fullscreen play, but the > graphics right now are definitely optimized for smaller windows, and the > benefit of fullscreen over maximizing the window seems unclear even if they > were. Minesweeper isn't an immersive 3D adventure, it's a five minute game. > Not only do I think the game looks better maximized than it does in fullscreen, > but fullscreen makes it difficult to access the application menu, the top bar > of the shell, other windows, and the close button, since you have to leave > fullscreen to do anything. This isn't a Mines-specific issue by any means: it > applies to all the games. OK, I removed the fullscreen mode, also from gsetting. > > A secondary issue: it'd be nice if the "Play Again" button would change to > "Start Over" or "New Game" (with an appropriate symbolic icon) when a game is > in progress, and only display "Play Again" after triggering a mine. > > ::: src/gnome-mines.vala > @@ +148,3 @@ > window.maximize (); > > + var title = new Gtk.Label (_("Mines")); > > Is this variable, and the call to gtk_header_bar_set_custom_title below, > redundant? My understanding is that set_custom_title is used to temporarily > override the title set by gtk_header_bar_set_title, but here they are both set > to "Mines." Oh, it was just for good screenshot (see bug #707999), I just remember to removed this before patches... > > @@ +554,2 @@ > new_game_action.set_enabled (false); > + new_game_button.hide(); > > Between here and line 573 you occasionally forget to leave a space between the > function name and the empty argument list. > > @@ +573,3 @@ > + pause_button.show_all(); > + flag_label.show (); > + clock_label.show (); > > Since all these buttons/labels are in a GtkBox, please call show_all and (up > above) hide on that box instead of on each contained element. (This is easy if > you make buttons_box a private member.) OK. (In reply to comment #4) > (In reply to comment #2) > > A secondary issue: it'd be nice if the "Play Again" button would change to > > "Start Over" or "New Game" (with an appropriate symbolic icon) when a game is > > in progress, and only display "Play Again" after triggering a mine. > > Sounds good. I'd probably go for "Start Over" rather than "New Game". This mean just the „Start Over” is shown, because in end of game, dialog for moved to new game is opened.
Created attachment 256920 [details] screenshot taken at end of game (In reply to comment #5) > > This mean just the „Start Over” is shown, because in end of game, > dialog for moved to new game is opened. Here's what I see at the end of the game; I'm not getting any such dialog? It'd be good to switch between the "Start Over" and "Play Again" labels. (I suppose that icon works for both "Start Over" and "Play Again".)
Review of attachment 256890 [details] [review]: ::: src/gnome-mines.vala @@ +639,3 @@ { hint_action.set_enabled (true); + pause_image.icon_name = "media-playback-pause"; How about media-playback-pause-symbolic, please and thank you!
(In reply to comment #6) > Created an attachment (id=256920) [details] > screenshot taken at end of game > > (In reply to comment #5) > > > > This mean just the „Start Over” is shown, because in end of game, > > dialog for moved to new game is opened. > > Here's what I see at the end of the game; I'm not getting any such dialog? > > It'd be good to switch between the "Start Over" and "Play Again" labels. (I > suppose that icon works for both "Start Over" and "Play Again".) When the game is ends I see the score dialog. I don't know why, but this waht I see.
(In reply to comment #8) > When the game is ends I see the score dialog. > I don't know why, but this waht I see. Ah, you are winning! I was losing, and there's no dialog for a loss.
(In reply to comment #9) > (In reply to comment #8) > > When the game is ends I see the score dialog. > > I don't know why, but this waht I see. > > Ah, you are winning! I was losing, and there's no dialog for a loss. Oh, I see. So if I understand right, when the game is in progress - show the „ Start Over” label, and in end of a game (for a loss) to show the „Play Again”? And always the action is app.new-game?
Yup, and also address comment #7. I don't see any other issues remaining.
Created attachment 256973 [details] [review] Implement the new deisng
Review of attachment 256973 [details] [review]: ::: src/gnome-mines.vala @@ +29,3 @@ + private Gtk.Button new_game_button; + private Gtk.Label new_game_label; + private Gtk.ToggleButton pause_button; There's one more bug with the pause button that I missed in the last review - it stays visually depressed after you press it. It ought to be a normal GtkButton, not a GtkToggleButton. I'm just going to change it myself and then push it. Thanks a bunch for your work!
Created attachment 257071 [details] [review] Implement the new deisng * add a header bar * remove the fullscreen mode * remove the toolbar * remove the face images * uses symbolic icons This implements the following design: https://raw.github.com/gnome-design-team/gnome-mockups/master/games/mines/mines.png