I agree, but I haven't seen it crash; could you post a stacktrace if you can reproduce that?

Created attachment 269580 [details] [review]
Don't allow computer vs computer games

Review of attachment 269580 [details] [review]:

Cool, thanks!

Can you please move Play As above Opposing Player, and ensure Play As is insensitive whenever Opposing Player is set to Human?  Also, it might be good to rename Opposing Player to simply Opponent, to remove all the whitespace currently underneath the word player.  (If you don't want to rename it, then it should read Opposing player with a lowercase "player.")

I'm also noticing some weirdness regarding the computer player's moves: you might be calling computer_player.move () too often.  There should be at least a two-second delay between my move and the computer's next move.

::: data/org.gnome.iagno.gschema.xml
@@ +8,1 @@
       <default>1</default>

Let's default to 2 (for dark) so that the human still plays first by default.

::: src/iagno.vala
@@ +31,3 @@
     private Gtk.Label light_score_label;
 
+    private ComputerPlayer? player = null;

This field should be removed (see below)

@@ +247,1 @@
             undo_action.set_enabled (false);

See the comment above for why this branch now needs to be removed (it prevents you from using Undo).

@@ +367,3 @@
         cancel_pending_computer_moves ();
+        if (game.current_color == settings.get_int("play-as"))
+            player.move ();

But player is always null, so I think this will cause criticals.  You don't need this branch (or the player field) at all: calling move() just starts the AI thinking.

@@ +444,1 @@
     }

Looks like there's an extra blank line here

I'm also worried about:

* What will happen if the player changes the difficulty while the AI is currently pondering its next move: does the game hang forever? (Scratch that, I see you addressed it below.)
* If I play the game on Easy, then change the difficulty to Hard before my final move, will the result be recorded in my high scores for Hard?

So please don't change the difficulty of the AI in the middle of the match -- it's best if this setting isn't applied until the next game.

@@ +461,3 @@
     private void propbox_response_cb (Gtk.Widget widget, int response_id)
     {
+        game_move_cb (game);

This causes the piece flip sound to play.  I take it this is to start the AI thinking if the player changed the difficulty level in the middle of the match? Then computer_move_cb() should suffice. (But I think it's best we remove this entirely, as I wrote above.)

Created attachment 269743 [details] [review]
Don't allow computer vs computer games

Thanks for your comments.
the level and the color setting are not applied until the next game, the reason is same that you wrote: change the color before the final move.

Review of attachment 269743 [details] [review]:

::: src/iagno.vala
@@ +36,2 @@
+    /* Human player */
+    private int player_color = 1;

I think this is nicer than using settings.get_int() everywhere was, but please use the existing Player enum instead of an int.  Also, the default value is not meaningful and can be omitted.

It's a bit hard to think of a meaningful name... player_color doesn't really make sense as there could be two players.  Maybe player_one?  (Well, that doesn't really seem great either.)

@@ +229,3 @@
+            computer = null;
+
+        player_color = settings.get_int("play-as");

Don't forget the space before the opening parentheses.

@@ +237,3 @@
          * thinking - but only a short delay for the first move)
          */
+        if (Player.DARK != player_color)

This is a Yoda conditional; how about if (player_color != Player.DARK)

@@ +272,2 @@
         /* Undo once if the human player just moved, otherwise undo both moves */
+        if (computer != null)

This breaks undo after you have moved, but before the computer player has moved.  We really do need to compare game.current_color against the identity of the computer, which is the opposite color of player_one/human_player if and only if computer != null.

Try something like:

if (computer != null &&
(human_player == Player.LIGHT && game.current_color == Player.DARK) ||
(human_player == Player.DARK && game.current_color == Player.LIGHT))

@@ +353,3 @@
          * but not so long as to become boring.
          */
+        if (computer != null)

Again, I think this is causing the AI to start on the human's turn.

It occurs to me that the check you need here is exactly the same as the one you need above, so how about splitting it into a new boolean function is_computer_player_to_move()

@@ +497,3 @@
         box.add (grid);
 
+

Extra blank line

@@ +525,3 @@
         grid.attach (label, 0, 1, 1, 1);
+        var combo = new Gtk.ComboBox ();
+        combo.changed.connect (() => computer_level_changed_cb(combo, color_combo));

Missing space before opening parenthesis

@@ +551,3 @@
             combo.set_active_iter (iter);
         grid.attach (combo, 1, 1, 1, 1);
+        

Still got the extra blank line here

Created attachment 269948 [details] [review]
Don't allow computer vs computer games

Review of attachment 269948 [details] [review]:

Much better: all the weirdness with the AI is fixed.

I messed up one of my previous review comments where I asked you to move Play As above Opponent in the preferences dialog.  Since changing the state of Opponent affects the sensitivity of Play As, Opponent needs to be on top.  It was right the first time; sorry about that.

::: src/iagno.vala
@@ +36,2 @@
+    /* Human player */
+    private int player_one;

Again, this should be declared as a Player, not an int.

@@ +237,3 @@
          * thinking - but only a short delay for the first move)
          */
+        if (player_one != Player.DARK)

Also, make sure computer != null

Created attachment 270301 [details] [review]
Don't allow computer vs computer games

Review of attachment 270301 [details] [review]:

Seems good. Thanks for the work and for iterating on it!

It's too late for this to go into 3.12 since we're already into UI freeze and this is a relatively minor issue, but I'll commit it after branching.

(In reply to comment #10)
> It's too late for this to go into 3.12 since we're already into UI freeze and
> this is a relatively minor issue, but I'll commit it after branching.

ping

Oops, thanks for the ping!

Attachment 270301 [details] pushed as 778f5b8 - Don't allow computer vs computer games