GNOME Bugzilla – Bug 732779
Add a timer and pause button
Last modified: 2014-09-15 21:40:33 UTC
The convention for GNOME games with timers is to have a play/pause button to allow the user to stop the clock. This will draw an overlay over the board so that the user can't see the board while the clock is stopped. Some games also stop the clock when the window becomes unfocused, but I think I'd like to shy away from this since it can be pretty jarring if you're opening the app menu, for example, or if you actually do want to look at the game while the window is unfocused (which is more likely to make sense for complicated games, like Chess and Sudoku, more than others). P.S. This is nice-to-have for 3.14, but not a priority.
One more note: it only makes sense to have a timer if we also use it to display high scores.
Created attachment 283624 [details] [review] Timer and play/pause button added.
Review of attachment 283624 [details] [review]: Thanks Amisha! First off, I see a lot of whitespace errors when applying the patch. Space characters should never appear before tabs, and never at the end of a line, so you'll want to fix these: Applying: Added play/pause button and a timer /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:46: space before tab in indent. </child> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:48: space before tab in indent. <packing> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:49: space before tab in indent. <property name="expand">True</property> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:50: space before tab in indent. <property name="fill">True</property> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:51: space before tab in indent. <property name="position">0</property> warning: squelched 39 whitespace errors warning: 44 lines add whitespace errors. Using index info to reconstruct a base tree... M lib/sudoku-board.vala M lib/sudoku-game.vala M src/gnome-sudoku.vala <stdin>:46: space before tab in indent. </child> <stdin>:48: space before tab in indent. <packing> <stdin>:49: space before tab in indent. <property name="expand">True</property> <stdin>:50: space before tab in indent. <property name="fill">True</property> <stdin>:51: space before tab in indent. <property name="position">0</property> warning: squelched 39 whitespace errors warning: 44 lines applied after fixing whitespace errors. Falling back to patching base and 3-way merge... Auto-merging src/gnome-sudoku.vala Auto-merging lib/sudoku-game.vala Auto-merging lib/sudoku-board.vala ::: data/gnome-sudoku.ui @@ +2,3 @@ <interface> <!-- interface-requires gtk+ 3.10 --> + <object class="GtkSizeGroup" id="buttons_size_group"> A size group is used to force all the widgets in the size group to have the same size. Since you only have one widget in this size group, you don't need it. @@ +179,3 @@ <property name="spacing">25</property> + <child> + <object class="GtkAspectFrame" id="minefield_aspect"> Hm, the spacing seems really messed up here and throughout the rest of this file. After opening a new XML tag, be sure to indent four more spaces, following the style of the rest of the file. @@ +192,3 @@ + <property name="can_focus">False</property> + <child> + <placeholder/> A placeholder child is only useful if you're going to add something to it manually, in the code. You can probably remove this. ::: lib/sudoku-board.vala @@ +72,3 @@ public Map<Coord?, Gee.List<Coord?>> coords_for_block; + /* Game timer */ I think it'd be better to keep all of the timer-related code in gnome-sudoku.vala. Then you won't need these signals. But maybe you don't need any of this code at all: see my comment in sudoku-game.vala. @@ +347,2 @@ if (complete) + { Looks like the indentation is wrong here. ::: lib/sudoku-game.vala @@ +67,3 @@ { timer.reset(); + board.start_clock(); Hm, this reminds me what I forgot to mention to you: Sudoku already has a timer that's tracking time. The result is only displayed at the end of the game, though. You'll want to either use that timer instead of your new one, or remove the old one, because we don't want two different timers with two different counts. ::: src/gnome-sudoku.vala @@ +54,3 @@ {"reset", reset_cb }, {"back", back_cb }, + { "pause", toggle_pause_cb }, You have an extra space here @@ +210,3 @@ } + private bool window_focus_out_event_cb (Gdk.EventFocus event) We actually discussed that we did not want the board to ever pause automatically when the window loses focus, unlike the game you copied this from. I see that's not working anyway, but the code should be removed. You then do not need the pause_requested member variable. That simplifies things quite a bit! @@ +451,2 @@ dialog.show (); + } Looks like the indentation is wrong here.
Created attachment 283925 [details] [review] Integrated play/pause button using the original timer
Review of attachment 283925 [details] [review]: Hey Amisha, we recently made a change that causes your patch to no longer compile. Could you run 'git pull --rebase' to update your local copy of gnome-sudoku, and then update your patch to reflect the newest version? gnome-sudoku.vala:294.17-294.25: error: Access to private member `SudokuView.grid' denied view.grid.hide(); ^^^^^^^^^ gnome-sudoku.vala:302.17-302.25: error: Access to private member `SudokuView.grid' denied view.grid.show(); ^^^^^^^^^ Compilation failed: 2 error(s), 0 warning(s) For now, you can submit a patch that does not hide the grid, since we're not in any hurry here (your patch can't go in until after feature freeze) and can review the rest of the patch in the meantime. I think we probably don't want to hide the entire grid, anyway, just the numbers within it.
Created attachment 283949 [details] [review] Integrated play/pause button using the original timer
Created attachment 283950 [details] [review] Integrated play/pause button using the original timer
Created attachment 283951 [details] [review] i
Created attachment 283952 [details] [review] Integrated play/pause button using the original timer
Review of attachment 283952 [details] [review]: OK, this is looking a lot better! I have one request for a UI change: could you please try to move the clock icon and timer label out of the sidebar and into the header bar instead? The pause button should stay right where it is, but I'd like you to pack the clock and timer label into the end of the header bar. You'll probably need to make the clock icon a bit smaller, and position the label to the right of the clock. Also, I see a critical warning when I start a new game while the clock is running: (gnome-sudoku:30542): GLib-CRITICAL **: g_timer_continue: assertion 'timer->active == FALSE' failed ::: data/gnome-sudoku.ui @@ +201,3 @@ + <property name="can_focus">False</property> + <property name="margin_bottom">12</property> + <property name="label" translatable="yes">0:00</property> It'd be better to not set a default label here. If you did, you'd have to use the unicode ratio character and the LTR symbols that you used when setting the label from Vala, and also add a translator comment to explain. Much easier to just deal with it in Vala. ::: src/gnome-sudoku.vala @@ +97,3 @@ + public signal void clock_started (); + public signal void paused_changed (); + public signal void tick (); Since we're doing everything within one class, there should not be any need for signals. Can you remove all these signals and replace them with direct function calls? @@ +293,3 @@ + { + if (paused) + { Watch your indentation here! @@ +316,3 @@ + private void toggle_pause_cb () + { + paused=!paused; Leave a space before the = here. @@ +326,3 @@ + var seconds = elapsed_time - hours * 3600 - minutes * 60; + + // var time_str = SudokuGame.seconds_to_hms_string (time); You should remove this old comment.
Created attachment 284014 [details] [review] Integrated play/pause button using the original timer
Review of attachment 284014 [details] [review]: OK, looking good. Can you do me a favor and run 'git pull --rebase' to update your local repository: your patches have a merge conflict since we changed some nearby code recently. ::: data/gnome-sudoku.ui @@ +17,3 @@ <style> + <class name="raised"/> + <class name="linked"/> Thanks for fixing the spacing here, but we like to avoid unrelated changes in our patches. I'd be great if you could submit a separate patch for this, instead of sneaking the change into this patch. @@ +84,3 @@ </child> + <child> + <object class="GtkImage" id="clock_image"> OK, this is looks quite close to what I want, but two more changes here please: * I'd actually like the timer to be at the end of the header bar, not the start. You can accomplish this by adding a packing tag and setting the pack_type to end, just like you did for the play_pause_button earlier in this file. * Let's make sure to vertically center both the image and the label by setting their halign property to center. ::: src/gnome-sudoku.vala @@ +74,3 @@ + private uint clock_timeout; + private bool _paused = false; + public bool paused I've been thinking about how we're going to implement the overlay that will hide the numbers on the board while the game is paused, and also how we're going to prevent the user from changing numbers while paused. We're going to need to know whether the clock is paused from inside the SudokuView class. I think the easiest way to do this would be to move all of these new functions from here to SudokuGame (in lib/SudokuGame.vala); you'll need to make some (but not all) of them public. Then you can make the actual Timer object in SudokuGame private, since you would no longer need to access it directly from outside that class. With those functions in SudokuGame, we'd be able to check whether the game is paused from SudokuView, and use that do decide whether to draw numbers on the board or allow the user to open a popover or change a number using the keyboard. That would be the next step, if you're interested in working on that too. @@ +295,3 @@ + redo_action.set_enabled (false); + } + else if (game.get_total_time_played() > 0) Nitpick here: we leave one space before the opening parentheses in the function call. You got this right almost everywhere else! @@ +364,3 @@ game = new SudokuGame (board); + start_clock(); Another missing space here. @@ +386,3 @@ var time = game.get_total_time_played (); var time_str = SudokuGame.seconds_to_hms_string (time); + stop_clock(); Another missing space here.
Created attachment 284284 [details] [review] Integrated play/pause button using the original timer I added a function named as function in sudokuview.That is just for temporary purpose,to reach to different cells after pressing pause button.
Review of attachment 284284 [details] [review]: Can you please fix these whitespace errors and merge conflicts? (You can update your repository using 'git pull --rebase'.) Applying: Added play/pause button using original timer /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:46: space before tab in indent. <property name="can_focus">True</property> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:47: space before tab in indent. <property name="receives_default">True</property> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:48: space before tab in indent. <property name="action_name">app.pause</property> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:49: space before tab in indent. <property name="use_underline">True</property> /home/mcatanzaro/jhbuild/checkout/gnome-sudoku/.git/rebase-apply/patch:52: space before tab in indent. <child> warning: squelched 15 whitespace errors warning: 20 lines add whitespace errors. Using index info to reconstruct a base tree... M data/gnome-sudoku.ui M lib/sudoku-game.vala M src/gnome-sudoku.vala M src/sudoku-view.vala <stdin>:46: space before tab in indent. <property name="can_focus">True</property> <stdin>:47: space before tab in indent. <property name="receives_default">True</property> <stdin>:48: space before tab in indent. <property name="action_name">app.pause</property> <stdin>:49: space before tab in indent. <property name="use_underline">True</property> <stdin>:52: space before tab in indent. <child> warning: squelched 15 whitespace errors warning: 20 lines applied after fixing whitespace errors. Falling back to patching base and 3-way merge... Auto-merging src/sudoku-view.vala CONFLICT (content): Merge conflict in src/sudoku-view.vala Auto-merging src/gnome-sudoku.vala CONFLICT (content): Merge conflict in src/gnome-sudoku.vala Auto-merging lib/sudoku-game.vala Auto-merging data/gnome-sudoku.ui Failed to merge in the changes. Patch failed at 0001 Added play/pause button using original timer ::: lib/sudoku-game.vala @@ +7,3 @@ public SudokuBoard board; public GLib.Timer timer; + public bool _paused = false; I was actually thinking that you would move the symbols you added in gnome-sudoku.vala, such as clock_timeout, paused, start_clock, stop_clock, continue_clock, and timeout_cb, into sudoku-game.vala instead. Then you don't need this extra _paused member and you can make the timer private.
Actually i tried moving those symbols to the file sudoku-game.vala but that was very troublesome to deal with.And instead this paused member is not extra.Rather it was the one which was used in gnome-sudoku.vala .So in order to make code simpler ,i just moved one symbol to sudoku-game.vala. And yes i will rectify these white space errors soon.
Created attachment 284749 [details] [review] Integrated play/pause button using the original timer
Review of attachment 284749 [details] [review]: This looks better. I only have a few issues with it: ::: lib/sudoku-game.vala @@ +71,3 @@ public void reset () { + board.previous_played_time = 0; I haven't looked into this and it may be a dumb question but can you explain why the timer does no longer need to be reset? ::: src/gnome-sudoku.vala @@ +324,3 @@ var time = game.get_total_time_played (); var time_str = SudokuGame.seconds_to_hms_string (time); + game.stop_clock (); I think the game instance should subscribe to the completed-signal itself and stop its own clock itself @@ +388,3 @@ { game.reset (); + game.start_clock (); When the game is reset, the clock should be started with the game and shouldn't need outside action
Review of attachment 284749 [details] [review]: One more note: the interface is a bit too complicated. I don't think SudokuGame should expose start_clock(), stop_clock(), and also paused: I notice that at the start of the game you set paused to false and then call start_clock(), which is too many steps. Try either making paused public get but private set, or else try making start_clock() and stop_clock() private.
Created attachment 284764 [details] [review] Integrated pause/resume button using the original timer
Review of attachment 284764 [details] [review]: There's still a merge conflict in lib/sudoku-game.vala. So be sure to fix that (use 'git pull -r' before creating your patch), and then fix these two nitpicks and I'll mark your patch as accepted. I'll try to work on disabling the board when the game is paused, and drawing an overlay over the board, soon. ::: lib/sudoku-game.vala @@ +164,3 @@ + var next = (int) (elapsed + 1.0); + var wait = next - elapsed; + clock_timeout = Timeout.add ((int) (wait * 1000), timeout_cb); Instead of Timeout.add(), let's use Timeout.add_seconds(). That's easier on the CPU, and you also don't have to multiply by 1000. @@ +171,3 @@ + } + + public bool paused Normally we declare member variables and properties at the top of the class, and functions at the bottom. So why don't you move this up to the top of the class, and put it right below _paused with no blank line in between _paused and paused.
Created attachment 284792 [details] [review] Integrated play/pause button using the original timer
Review of attachment 284792 [details] [review]: The merge conflict still needs fixed: Applying: Added play/pause button using original timer. Using index info to reconstruct a base tree... M data/gnome-sudoku.ui M lib/sudoku-game.vala M src/gnome-sudoku.vala Falling back to patching base and 3-way merge... Auto-merging src/gnome-sudoku.vala Auto-merging lib/sudoku-game.vala CONFLICT (content): Merge conflict in lib/sudoku-game.vala Auto-merging data/gnome-sudoku.ui Failed to merge in the changes. Patch failed at 0001 Added play/pause button using original timer. Everything else looks good.
Created attachment 284903 [details] [review] Integrated play/pause button using the original timer
Review of attachment 284903 [details] [review]: OK great! This was a lot of work, so for the purposes of your OPW application I think it's fair to consider this patch accepted. Before we can merge it, I still need to handle hiding the sudoku board when the game is paused. Also, I recently noticed that the timer appears on the new game page: we should pause the timer it and hide the clock icon and label when visiting the new game page.
Created attachment 284907 [details] [review] Integrated play/pause button using the original timer
Review of attachment 284907 [details] [review]: Works great. ::: src/gnome-sudoku.vala @@ +371,3 @@ + saver = new SudokuSaver (); + var savegame = saver.get_savedgame (); + if (savegame != null) Why are you creating a SudokuSaver, loading the saved game from disk, checking if the saved game is null, and then using the normal game instead? I'm not sure why SudokuSaver is involved here at all? I think you can delete these three lines.... @@ +372,3 @@ + var savegame = saver.get_savedgame (); + if (savegame != null) + game.stop_clock (); Indentation is wrong here. @@ +428,3 @@ + clock_image.show (); + if (game != null) + game.continue_clock (); Indentation is wrong here.
Actually, it'd be nice if the game did not unpause itself if you pause the game, visit the new game screen, and then return to the game via the back button.
Created attachment 284935 [details] [review] Disable the board when the game is paused
Created attachment 284936 [details] [review] Draw overlay over the board when game is paused
Created attachment 284937 [details] [review] Draw overlay over the board when game is paused
Created attachment 284955 [details] [review] Integrated play/pause button using the original timer
Created attachment 284962 [details] [review] Disable the board when the game is paused
Created attachment 284963 [details] [review] Draw overlay over the board when game is paused
Parin, after reviewing these patches, mark them accepted-commit-after-freeze if you're satisfied, since we can't commit them until after we create the gnome-3-14 branch in the gnome-sudoku repository. Thanks!
Created attachment 286148 [details] [review] Added play/pause button using original timer. Rebased and removed timer label from translations
Created attachment 286149 [details] [review] Disable the board when the game is paused
Created attachment 286150 [details] [review] Draw overlay over the board when game is paused
I've branched gnome-3-14, so I'm committing these to master now. Thanks Amisha!
Attachment 286148 [details] pushed as e12896d - Added play/pause button using original timer. Attachment 286149 [details] pushed as 930d94a - Disable the board when the game is paused Attachment 286150 [details] pushed as 77f23dd - Draw overlay over the board when game is paused