Created attachment 287145 [details] [review]
AI: Don't manage a pass as a move.

Another way to write the AI loop.

Created attachment 287153 [details] [review]
Don't make unnecessary moves.

We don’t need to apply the move when we’re just trying to know how many tiles it will flip.

Created attachment 287157 [details] [review]
Create get_possible_moves_sorted().

What’s the Good Way™ to do this for having zero copy of the array?

Created attachment 287205 [details] [review]
Do the first iteration in a different function.

Is that the Good Way™ to do this for having zero copy of the array? (This is a real patch, not as the previous one.)

Review of attachment 287134 [details] [review]:

Eh, I feel like iagno.vala was simpler originally....

The computer-player changes LOOK safe, but I'm wary of any changes in this code. Just be sure to test carefully.

::: src/game.vala
@@ +32,3 @@
 
+    /* Indicate who's the next player who can move */
+    public bool current_can_move { get; private set; default = true; }

current_player_can_move

@@ +37,2 @@
     /* Indicate that a player should move */
+    public signal void emit_end_of_turn ();

turn_ended ();

(Certainly don't name signals with "emit"...)

@@ +242,3 @@
+        var enemy = Player.flip_color (current_color);
+        var opponent_can_move = false;
+        for (var x = 0; x < size; x++)

On loops, I use braces if the body is more than one line long, even if it is all one statement.

@@ +337,3 @@
         if (count == 2)
             undo (1);
+        else

I only omit braces around conditionals if the body of every conditional in the chain is one line long.

::: src/iagno.vala
@@ +386,3 @@
         }
 
+        play_sound ("flip-piece");

Why can't you call this at the top of game_move_cb/turn_ended_cb? Having it in three places instead of one is hardly nice.

@@ +430,3 @@
     }
 
+    private void game_emit_end_of_turn ()

turn_ended_cb ()

@@ +441,3 @@
+    }
+
+    private void should_move ()

I don't like this name. The function should be named for what it does, not when it's called, and this name doesn't even clearly convey that much.

@@ +459,3 @@
     }
 
+    private void cannot_move ()

This function also needs a better name.

@@ +462,2 @@
     {
+        play_sound ("flip-piece");

Actually, why do you play this sound if it's a pass?

@@ -471,2 @@
         if (game.n_light_tiles > game.n_dark_tiles)
-        {

I use braces if there's multiple lines in the body, even if one is just a comment.

Review of attachment 287145 [details] [review]:

I bet this breaks something. :(

Review of attachment 287153 [details] [review]:

::: src/game.vala
@@ +196,3 @@
     \*/
 
+    public int place_tile (int x, int y, bool apply)

You could use a default argument here, since it's public; then you don't have to change the other calls, since the extra boolean parameter is hard to read.  My rule of thumb is to err on the side of default arguments to public functions, but not to private functions.

@@ +271,3 @@
             return 0;
 
+        if (apply)

Braces here.

Review of attachment 287205 [details] [review]:

This patch is terrifying too. Make sure to test a lot more than usual.  Integrating Dr. Paul's tests would be great....

::: src/computer-player.vala
@@ +178,3 @@
          * strategy. */
+        var g = new Game.copy (game);
+        var depth = difficulty_level * 2;

Don't change the depth in this patch.

@@ +262,3 @@
+    private void get_possible_moves_sorted (Game g, ref List<PossibleMove?> moves)
+    {
+        for (var x = 0; x < g.size; x++)

Braces here

Created attachment 287226 [details] [review]
Analyse the game at the end of turn.

(In reply to comment #5)
> Eh, I feel like iagno.vala was simpler originally....

I “just” splitted the code of game_move_cb() in two function, as it was called in both cases if the player can move and if he cannot (!), and managed which iagno.vala’s function is called in iagno.vala instead of in game.vala (via signals).

> +        play_sound ("flip-piece");
> 
> Why can't you call this at the top of game_move_cb/turn_ended_cb? Having it in
> three places instead of one is hardly nice.

The sound “flip-piece” isn’t played when you or your opponent flip pieces and finish the game (in game_complete()). That is quite strange, but I didn’t changed it. A patch may be required.

> +    private void should_move ()
> 
> I don't like this name. The function should be named for what it does, not when
> it's called, and this name doesn't even clearly convey that much.

The problem is that this function doesn’t do much, notably on a two-players game. It could be called end_of_turn_and_after_move(), I just used game_wants_move(), is that better? feel free to change.

> +        play_sound ("flip-piece");
> 
> Actually, why do you play this sound if it's a pass?

For the same reason we had this line at the beginning of game_move_cb().

Created attachment 287227 [details] [review]
AI: Don't manage a pass as a move.

Just replaced a “g.current_can_move”.

Created attachment 287228 [details] [review]
Don't make unnecessary moves.

Created attachment 287229 [details] [review]
Do the first iteration in a different function.

(In reply to comment #8)
> Integrating Dr. Paul's tests would be great....

Not sure it would help for this.

> ::: src/computer-player.vala
> @@ +178,3 @@
>           * strategy. */
> +        var g = new Game.copy (game);
> +        var depth = difficulty_level * 2;
> 
> Don't change the depth in this patch.

The depth changes because we do the initial iteration without decreasing it.

Created attachment 287230 [details] [review]
Don't use six lines for a corner-case.

A clear way to correct that.

Created attachment 287231 [details] [review]
Don't use six lines for a corner-case.

…with a clearer comment. ^^

There is a bug that exists in master: when you finish a game (in your turn), go to the new-game screen, go back to the (finished) game, the computer tries to play (and the game crashes).

(Don’t forget to use `jhbuild run iagno --size 4 --level 1 --second`. ^^ )

Review of attachment 287226 [details] [review]:

::: src/game.vala
@@ +238,3 @@
+    }
+
+    private void test_moves ()

Can you think about how to make the meaning of this function more clear? I think it'd be fine with just a better name, but I can't think of what to name it.  As it stands I had to squint a bit too long to see what it does (update current_player_can_move and is_complete).

This is admittedly the sort of nitpicking I would often let slide, but for a patch that just shuffles code around....

::: src/iagno.vala
@@ +439,3 @@
+    }
+
+    private void game_wants_move ()

Yeah, these names didn't work well either. It might be best to just have a longer turn_ended_cb()....

Review of attachment 287228 [details] [review]:

OK.

Review of attachment 287229 [details] [review]:

Oh, duh.  I was even looking for that when I saw it.  And it was 5 PM when I made that review, so I can't have been tired....

Review of attachment 287231 [details] [review]:

Should work.

Created attachment 287482 [details] [review]
Analyse the game at the end of turn.

(In reply to comment #16)
> This is admittedly the sort of nitpicking I would often let slide, but for a
> patch that just shuffles code around....

Let’s say that… ^^’

> Yeah, these names didn't work well either. It might be best to just have a
> longer turn_ended_cb()....

Not a good idea, the function won’t be easily understandable. Here’s another proposition.

Created attachment 287484 [details] [review]
Remove view.redraw().

Created attachment 287485 [details] [review]
Correct some end-of-game bugs.

Here is a correction of the crasher bug, and some others of type “bad headerbar subtitle” (when game finished, depending of who finished; and going back from the new-game screen to a finished game).

Review of attachment 287485 [details] [review]:

::: src/iagno.vala
@@ +278,3 @@
             computer.move_async.begin (SLOW_MOVE_DELAY);
+        else if (game.is_complete)
+            game_complete (false);

Why don't you want to play the sound?

Attachment 287227 [details] pushed as fc29247 - AI: Don't manage a pass as a move.
Attachment 287228 [details] pushed as f07ee51 - Don't make unnecessary moves.
Attachment 287229 [details] pushed as f9cb9bf - Do the first iteration in a different function.
Attachment 287482 [details] pushed as b066c3d - Analyse the game at the end of turn.

(In reply to comment #23)
> Why don't you want to play the sound?

Why should it play a sound when coming back from new-game screen? Is something happening in the game?

Attachment 287485 [details] pushed as 3f74d09 - Correct some end-of-game bugs.