Review of attachment 286542 [details] [review]:

I like this; let's do it. Does it really feel slower to you? It seems instantaneous to me.

Did you consider using a Gee.List or Gee.Stack instead of two different member variables? That might (or might not) be easier to read.

::: src/iagno-game.vala
@@ +16,3 @@
 
+    public int width { get { return tiles.length[0]; }}
+    public int height { get { return tiles.length[1]; }}

This doesn't help readability.

@@ +35,3 @@
     /* The number of tiles on the board */
+    private int _n_tiles = 4;
+    public int n_tiles { get { return _n_tiles; }}

By convention, you should also provide a private set, so that you can assign directly to n_tiles instead of having to use _n_tiles.  Think of the underscore as a big warning sign that says "don't use me!"

@@ +97,2 @@
     public bool is_complete ()
+        ensures (result || n_tiles <= width * height)

Hm, this looks wrong. If is_complete returns false, the board had better not be completely full.

Created attachment 286583 [details] [review]
Better history.

(In reply to comment #1)
> I like this; let's do it. Does it really feel slower to you? It seems
> instantaneous to me.
> 
> Did you consider using a Gee.List or Gee.Stack instead of two different member
> variables? That might (or might not) be easier to read.

I think the code is clear. My only concern is, would it be faster? and I’m not sure. Let’s have this in mind for another patch?

> @@ +97,2 @@
>      public bool is_complete ()
> +        ensures (result || n_tiles <= width * height)
> 
> Hm, this looks wrong. If is_complete returns false, the board had better not be
> completely full.

Right, that was a temporary hack to test a “bug” (warning messages) appearing when undoing after the end of the game. It may be corrected, I can’t reproduce it right now.

Created attachment 286605 [details] [review]
Reorder iagno-game.vala.

A reorder patch. Notably removes count_tiles(), one of these loop functions I hate.

Oops, the last patch bugs, don’t apply it, I’ll correct it.

Created attachment 286613 [details] [review]
Reorder iagno-game.vala.

Here is it. (I hope.)

(In reply to comment #2)
> I think the code is clear. My only concern is, would it be faster? and I’m not
> sure. Let’s have this in mind for another patch?

Speed is a really insignificant concern here.  It's surely lightning fast compared to an AI search....

Anyway, I'm fine with both of your patches. I'm just marking them as reviewed since they're based on the renamed filenames, which we can argue about later. :p

Created attachment 286641 [details] [review]
Better history.

(In reply to comment #6)
> Speed is a really insignificant concern here.  It's surely lightning fast
> compared to an AI search....

The history is a big part of the AI search…

Created attachment 286642 [details] [review]
Reorder game.vala.

Created attachment 286643 [details] [review]
Add --size option, for testing.

Here is an option that could be used for finding some issues, on drawing notably. Or for playing, but I don’t plan to add it to the UI for now (and it would require some more work on the AIs). It integrates all the changes of the last patch of the Add Player.INVALID bug[1] apart the controversial optimization that will come back later. =) 

[1] https://bugzilla.gnome.org/show_bug.cgi?id=735615

Can you move these last two patches to another bug? You can set the bug to depend on this one if need be.

Review of attachment 286641 [details] [review]:

Good point. But considering how much trouble we had fixing an undo bug a couple weeks ago, I think it's appropriate to prioritize readability here.

Just one question:

::: src/game-view.vala
@@ -209,3 @@
         }
         /* An undo occurred after the game was complete */
-        else if (flip_final_result_now && !game.is_complete ())

What does this fix?

Created attachment 286671 [details] [review]
Better history.

(In reply to comment #11)
> Just one question:
> 
> ::: src/game-view.vala
> @@ -209,3 @@
>          }
>          /* An undo occurred after the game was complete */
> -        else if (flip_final_result_now && !game.is_complete ())
> 
> What does this fix?

An unnecessary call to a hated function.
    if (flip_final_result_now && game.is_complete ())
    […]
    else if (flip_final_result_now && !game.is_complete ())
Had been transferred in the reorder bug[1].

[1] https://bugzilla.gnome.org/show_bug.cgi?id=737003

Attachment 286671 [details] pushed as 3c5e223 - Better history.

Reopening because unfortunately you were right about this being too slow.

Created attachment 286728 [details] [review]
Faster history.

Should be faster.

Don’t push it, there’s a bug, the AI plays stupidly.

Created attachment 286734 [details] [review]
Faster history.

Moved a line for readability without noticing it was useful there. ^^’

Don’t push it for now, I have a better solution.

Created attachment 286740 [details] [review]
Faster history.

This should be stronger.

Review of attachment 286740 [details] [review]:

Looks good. It's noticeably faster.

My general remark is "please add more comments;" even if it's just a nicer version of what was already there, it's not quite self-documenting.

::: src/game.vala
@@ +23,3 @@
 
+    /* Undoing. Stack should be big enough. */
+    private int? undo_stack[1000];

The comment should contain a proof, and the code needs an assertion when incrementing history_index.

Alternatively, use a Gee.ArrayList, reserve 1000 slots when constructed, and don't worry about it.

@@ +295,3 @@
+            return history_index >= 2;
+        else
+            return history_index >= 5;

Here in particular would be a good place for a comment, since the correctness is not obvious; can you add one please?

Created attachment 286748 [details] [review]
Faster history.

Review of attachment 286748 [details] [review]:

::: src/game.vala
@@ +350,3 @@
+        undo_stack[history_index] = x + y * size;
+    }
+    private void add_end_of_turn_to_undo_stack ()

Just missing a blank line here.

Created attachment 286770 [details] [review]
Faster history.

Created attachment 286771 [details] [review]
Little things in computer-player.vala.

Unrelated patch.

Created attachment 286772 [details] [review]
Little things in computer-player.vala.

…

Created attachment 286774 [details] [review]
Little things in computer-player.vala.

…

Created attachment 286783 [details] [review]
Correct a broken test.

I’ve broken a test with the strict can_undo() checks. I can either add a history_index parameter to Game.from_strings(), or do that. What’s the best?

Review of attachment 286770 [details] [review]:

OK

Review of attachment 286774 [details] [review]:

::: src/computer-player.vala
@@ +309,1 @@
     private static int is_empty (Game g, int x, int y)

This should return a bool, and the result shouldn't be used for subtraction. ^^

Review of attachment 286783 [details] [review]:

I guess the problem is that can_undo (2) might return true after the first move if more than one tile is flipped? :/

I can't think of any good solution to this.  Modifying the test is terrible. I don't see how adding a history_index parameter to Game.from_strings() would help, and that would also be terrible since then weird stuff would happen if you undo too much.

I guess modifying the test is the least-bad approach, but instead of avoiding the unexpected behavior, explicitly test that can_undo returns an unexpected result, with a comment explaining the weirdness.

(In reply to comment #29)
> This should return a bool, and the result shouldn't be used for subtraction. ^^

I won’t write a patch that overcomplicates the code for nothing. Yes, is_empty() gives an int, and I prefer it gives a 1-for-true-and-0-for-false than to have a -1 result, or eight conditional operators in around()…

(In reply to comment #30)
> I guess the problem is that can_undo (2) might return true after the first move
> if more than one tile is flipped? :/

No, it’s the opposite. The function can_undo() is strict, it only returns true if the history has (at least) six entries for a can_undo(2), and that’s what’s happens for a reversi beginning; but having a move and a pass (like in the test) only makes four entries. This history is not as you said “a nicer version of what was already there”, it works a quite different way.

> I don't see how adding a history_index parameter to Game.from_strings() would
> help, and that would also be terrible since then weird stuff would happen if
> you undo too much.

Game.from_strings() could create a fake, empty or not, history, for tests like this one that *need* an history (it only needs an history for one more move, history that I create in this patch… in doing one more move in the test). These functions *are* weird, they always created a board without its history; now that can_undo() is “more strict”, dedicated to a reversi beginning, it’s visible.

> I guess modifying the test is the least-bad approach, but instead of avoiding
> the unexpected behavior, explicitly test that can_undo returns an unexpected
> result, with a comment explaining the weirdness.

Another way is to modify can_undo() to check the number of moves including a possible pass. That basically means writing -1 in the six first entries of history, and counting the number of null entries in can_undo(). But I’d hate having game’s code acting weird only for pleasing a test…

(In reply to comment #30)
> I guess the problem is that can_undo (2) might return true after the first move
> if more than one tile is flipped? :/

Oh, no, I understand what you mean. Yes, it’s the case. And the initial way to do that was bugged the same. ^^

For reference:

Initial:

    public bool can_undo (int count = 1)
        requires (count == 1 || count == 2)
    {
        /* Can undo one move if a move has been played */
        if (count == 1)
            return undo_index > 0;
        else /* Can undo two moves only after the second ply; after the first ply, undo_index == 3 */
            return undo_index > 3;
    }

Using a chained list (yeah, works! but because there’s already code for counting the number of moves…):

    public bool can_undo (int count = 1)
        requires (count == 1 || count == 2)
    {
        return number_of_moves >= count;
    }

Actual:

    public bool can_undo (int count = 1)
        requires (count == 1 || count == 2)
    {
        /* The two first moves both have three entries in the undo stack:
         * for the flipped tile, the placed tile, and for the end of turn */
        if (count == 1)
            return history_index >= 2;
        else
            return history_index >= 5;
    }

(In reply to comment #33)
>         if (count == 1)
>             return history_index >= 2;
>         else
>             return history_index >= 5;

If we change the 5 to a 3, the code will surely work in-game. If I understand your code correctly, a pass after the first move would push the history index up to 3, placating the tests, correct?

Created attachment 286903 [details] [review]
Faster history.

Another proposition…

Created attachment 286904 [details] [review]
Little things in computer-player.vala.

Rebased.

(In reply to comment #34)
> If we change the 5 to a 3, the code will surely work in-game. If I understand
> your code correctly, a pass after the first move would push the history index
> up to 3, placating the tests, correct?

It did, yeah, but I wasn’t happy with all that, so I tried maintaining a count of the number of moves.

Created attachment 286925 [details] [review]
Faster history.

A less strict way than the previous (no sync between number_of_moves and current_color…), but more readable.

Created attachment 286926 [details] [review]
Faster history.

-_-

Created attachment 286930 [details] [review]
set_tile() always sets to current_color.

There’s a TODO in “Faster history”, I thought it needed a separate patch. Here is it.

Created attachment 286934 [details] [review]
Remove can_undo().

With a move counter, the can_undo() function becomes meaningless. Let’s remove it.

Review of attachment 286904 [details] [review]:

OK

Review of attachment 286926 [details] [review]:

Looks OK (haven't tested it though).

Review of attachment 286930 [details] [review]:

Looks OK

Review of attachment 286934 [details] [review]:

::: src/test-iagno.vala
@@ -34,3 @@
         assert (game.place_tile (7, 2) > 0);
         assert (!game.can_move (Player.LIGHT));
-        assert (game.can_undo ());

You should test the number of moves instead!

Created attachment 286944 [details] [review]
Remove can_undo().

(In reply to comment #45)
> You should test the number of moves instead!

Yeah, why not.

Attachment 286904 [details] pushed as 84eb5d3 - Little things in computer-player.vala.
Attachment 286926 [details] pushed as 85c758b - Faster history.
Attachment 286930 [details] pushed as 053a03d - set_tile() always sets to current_color.
Attachment 286944 [details] pushed as 16da7a8 - Remove can_undo().