GNOME Bugzilla – Bug 723338
Game can be stuck after Hint and Undo if computer plays first
Last modified: 2014-03-07 20:23:59 UTC
This can only happen in games where the computer plays first. After the computer makes its first move, select Hint, then Undo. Now the game is stuck!
Created attachment 270633 [details] [review] Solved the bug by disabling the undo button. Disabling the undo button made sense because it served two purposes: Solved the bug Remove any confusion in minds of potential users of what undo would do in such a situation when it clearly couldn't do any meaningful action.
Review of attachment 270633 [details] [review]: This looks like it will solve the bug: good job! ::: src/main.c @@ +25,3 @@ #include <stdlib.h> #include <locale.h> +#include <stdio.h> This new include isn't needed for any of the code you added, so it shouldn't go in this patch. If this was accidental, you might want to try 'git add -p' instead of plain 'git add' -- this takes you to an interactive staging mode where you can add individual portions of a file, so you have to approve each individual change. @@ +646,3 @@ + if(moves > 0) hint_enable=1; + if(moves==1 && is_player_human()) hint_enable=0; + g_simple_action_set_enabled (G_SIMPLE_ACTION (undo_action), hint_enable); There are a few issues with this code. Here are a few things to keep in mind: * C89 does not permit mixed declarations and code, and I think Four-in-a-row tries to adhere to this standard, so the declaration of hint_enable needs to go at the top of the function, before the first line of executable code. * The variable should be named undo_enable (or enable_undo), not hint_enable. * Instead of assigning 0 and 1 to the gboolean, you can use the TRUE and FALSE macros. (These are provided by GLib.) * Try to pay attention to the style of the surrounding code. Four-in-a-row always uses spaces before opening parentheses (both when opening if statements and when making function calls), always uses spaces around operators (including the copy assignment operator), and always uses blank lines after conditionals. With all that in mind, I think it'd be better to not introduce a new variable. You could write the fix like this, for example: if (something) g_simple_action_set_enabled (G_SIMPLE_ACTION (undo_action), TRUE)); else g_simple_action_set_enabled (G_SIMPLE_ACTION (undo_action), FALSE));
Created attachment 270692 [details] [review] Updated patch Thanks for the git add -p tip. Should be really handy. :) Attached is an updated patch based on your recommendations. Does this look fine?
Review of attachment 270692 [details] [review]: Yes, this is good. I do have one style nit below, but the only significant issue with this patch is that it wasn't produced with 'git format-patch', so it's missing your name/email/date and the commit message. Your original patch had all of this, so if you make the patch the same way you did the first one, it should be good. Your first patch did not follow our rules for git commit messages [1], but I haven't shown them to you yet, so I wouldn't have expected it to. Take a look over those (in particular, the rules for line breaks), but don't worry about the "linking to bugs" section: when I push your patch, I'll add a link to the bug at the bottom of the commit message. [1] https://wiki.gnome.org/Git/CommitMessages ::: src/main.c @@ +643,3 @@ g_free (s); + if (moves <= 0 || (moves == 1 && is_player_human ()) ) We use a space before the opening parenthesis, but not before the closing parenthesis. So that should be: is_player_human ()))
Created attachment 271264 [details] [review] The final patch after all the style adjustments. The latest patch. This should've hopefully taken care of all the issues. :)
Created attachment 271265 [details] [review] Proper git commit message Git commit message corrected.
Great, thanks!