GNOME Bugzilla – Bug 762014
A blank screen sometimes appears when ending a game
Last modified: 2016-02-14 18:55:23 UTC
Created attachment 321090 [details] [review] Always show 'Play Again' button after ending a game There are a few cases when ending a game shows a blank screen: - player finishes game with score == 0 - a new high score is reached - number of human players is > 1 After talking to Iulian we've concluded that the Game Over screen should always contain a 'Play Again' button with the addition of the score earned if there was only one human player and the number of points needed to hit the leaderboard if there was no high score. This patch also fixes bug 761316 in another manner by having the 'Pause' button disabled inside 'game_over' method.
Review of attachment 321090 [details] [review]: ::: src/gnome-nibbles.vala @@ -715,3 @@ (object, result) => { - pause_action.set_enabled (false); FWIW, this should have been added outside the async callback anyway; I didn't review your previous patch closely enough. :) The difference is that putting it outside the callback causes the action to be disabled immediately, whereas putting it inside the callback causes it to only be disabled once the disk I/O completes... if your have your home directory on NFS or something, that could be a significant window of time in which you could click the pause button and get your score added twice. @@ +824,3 @@ pause_action.set_enabled (false); + var is_high_score = score > last_score ? true : false; Do you really need the ternary here? I think you should be able to write: var is_high_score = (score > last_score); I bet it will work without the parenthesis, too (but it's easier to read that way).
(In reply to Michael Catanzaro from comment #1) > if your have your home directory on > NFS or something, that could be a significant window of time in which you > could click the pause button and get your score added twice. Ah, by chance I went through my GNOME to-do list today for the first time in a while, and guess what I found: "nibbles clicking new game during a network hang would create duplicate score." At first I was confused and chided myself for not having enough detail on my to-do list, but it's clear that the call to game_over() from within the async callback is unsafe, as the next game could have already started if the user were to click new game before the scores are saved! I think the easiest solution is to disable the new game action and the pause action immediately before the call to add_score(), so we don't have to worry about that happening. We should add a comment to explain why, else we'll remove it in a year or two having forgotten about this issue.
I guess since I broke this, I ought to fix it, but I'll give you a chance first, since your patch is already touching this code.
Created attachment 321107 [details] [review] Always show 'Play Again' button after ending a game (In reply to Michael Catanzaro from comment #1) > + var is_high_score = score > last_score ? true : false; > > Do you really need the ternary here? Indeed, no need for that :D (In reply to Michael Catanzaro from comment #2) > I think the easiest solution is to disable the new game action and the pause > action immediately before the call to add_score(), so we don't have to worry > about that happening. We should add a comment to explain why, else we'll > remove it in a year or two having forgotten about this issue. Okay I guess it should be fine now.
Review of attachment 321107 [details] [review]: ::: src/gnome-nibbles.vala @@ +694,3 @@ private void log_score_cb (int score) { + /* Disable these here to prevent the user clicking the pause button and getting his score added twice */ "Disable these here to prevent the user clicking the buttons before the score is saved" @@ +699,3 @@ + + var scores = scores_context.get_high_scores (get_scores_category (game.speed, game.fakes)); + var last_score = scores.size == 10 ? scores.last ().score : -1; I would use extra parentheses for clarity: var last_score = (scores.size == 10 ? scores.last ().score : -1); @@ +726,3 @@ } // Not a high score... Remove this comment, it's no longer true as this code will now be executed unconditionally after the score is added. @@ +824,1 @@ private void game_over (int score, long last_score) In a follow-up commit, you could rename "last_score" to "lowest_high_score" as I keep getting confused, thinking it's score from the previous game. :) @@ +857,3 @@ game_over_label.destroy (); + + if (game.numhumans == 1) These if (game.numhumans == 1) changes are really a separate issue; they look good to me, but you should commit them in a separate patch.
Created attachment 321127 [details] [review] Prevent user clicking buttons before the score is saved
Created attachment 321128 [details] [review] Always show 'Play Again' button after ending a game