After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723338 - Game can be stuck after Hint and Undo if computer plays first
Game can be stuck after Hint and Undo if computer plays first
Status: RESOLVED FIXED
Product: four-in-a-row
Classification: Applications
Component: general
3.10.x
Other Linux
: Normal major
: ---
Assigned To: four-in-a-row-maint
four-in-a-row-maint
Depends on:
Blocks:
 
 
Reported: 2014-01-31 03:22 UTC by Michael Catanzaro
Modified: 2014-03-07 20:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Solved the bug by disabling the undo button. (1.17 KB, patch)
2014-03-01 17:55 UTC, Nikhar
needs-work Details | Review
Updated patch (534 bytes, patch)
2014-03-02 12:06 UTC, Nikhar
reviewed Details | Review
The final patch after all the style adjustments. (1021 bytes, patch)
2014-03-07 19:51 UTC, Nikhar
none Details | Review
Proper git commit message (1.02 KB, patch)
2014-03-07 20:12 UTC, Nikhar
none Details | Review

Description Michael Catanzaro 2014-01-31 03:22:15 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!
Comment 1 Nikhar 2014-03-01 17:55:20 UTC
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.
Comment 2 Michael Catanzaro 2014-03-01 18:37:29 UTC
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));
Comment 3 Nikhar 2014-03-02 12:06:06 UTC
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?
Comment 4 Michael Catanzaro 2014-03-02 16:11:52 UTC
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 ()))
Comment 5 Nikhar 2014-03-07 19:51:20 UTC
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. :)
Comment 6 Nikhar 2014-03-07 20:12:25 UTC
Created attachment 271265 [details] [review]
Proper git commit message

Git commit message corrected.
Comment 7 Michael Catanzaro 2014-03-07 20:23:59 UTC
Great, thanks!