GNOME Bugzilla – Bug 734866
Undo is broken with two human players
Last modified: 2014-08-16 21:56:12 UTC
Undo is broken with two human players -- it undos two moves instead of one, and does nothing before the second ply.
Created attachment 283588 [details] [review] Correct undo when in two-players mode. Here is a patch. The function can_undo() changes: it’s goal is to test if we already played enough to undo a move (or to restart the game), depending on the game mode; the undo() function shouldn’t be callable if undoing is impossible.
Created attachment 283589 [details] [review] Correct undo when in two-players mode. Oops, no need to have a public gamemode (for now).
Review of attachment 283589 [details] [review]: I'm a bit confused by this patch. There was previously nothing wrong with can_undo(): you know that because the undo button was always sensitive or insensitive at the correct times. ::: src/iagno.vala @@ +336,3 @@ + if (gamemode == "two-players") + { This here is what actually fixes the bug. There's a logic error in the if statement below that causes us to take the game.undo (2) case instead of the game.undo (1) case when the game has two players. I think that by returning early here, you skip the logic that handles undoing an infinite number of times as long as the player was forced to pass, which should cause undo to fail after a pass. But none of this matters since we can just fix the original logic error now that we've found it.
Created attachment 283592 [details] [review] Fix undo in two-player games I think this patch also fixes it without breaking undo after passing; do you see any problems with it?
I find all the undo-related code confusing, that’s why I tried a better definition of some parts, but you’re right, there’s a bug in my patch. Yours isn’t perfect on the exact same case, but at least it undoes. ^^ On a two-players game: 1) light player plays; 2) dark player plays; 3) light can’t play, dark player plays; 4) light can’t play, dark player plays; 5) light can’t play, dark player plays; 6) “undo” pressed, back to… 2). I think it’s not exactly the expected result. ^^
Created attachment 283621 [details] [review] Fix undo in two-player games Adapted from work by Arnaud Bonatti
Created attachment 283623 [details] [review] Fix undo in two-players game. I think that works.
Review of attachment 283623 [details] [review]: Makes sense to me. I think it'd be a bit cleaner to modify the first if statement instead of adding a new one: if (game.current_color != player_one || (computer == null && game.can_move (game.current_color))
Created attachment 283626 [details] [review] Fix undo in two-players mode. I think it’s clearer like that (and looks like it works and doesn’t break anything else).
Review of attachment 283626 [details] [review]: Yes, this is best.
Attachment 283626 [details] pushed as df86d60 - Fix undo in two-players mode.