Created attachment 293435 [details] [review]
Saving a completed game due to timeup saves the timer as infinity

1. I couldn't find a way to display the final game stats in the header-subtitle as we don't save it in the  file. It would be best to simply continue showing the file_name in the header-subtitle.
2. Fixes showing the infinite time issue.

Review of attachment 293435 [details] [review]:

Works well, thanks.

You have some tab characters that are messing up the indentation; please replace each one with eight spaces.

::: lib/chess-game.vala
@@ +31,3 @@
     DEATH,
+    BUG,
+    NOP_GAME_OVER

I would call this UNKNOWN, and I'd like it to be the first rule in the list -- that way, it gets used by default if we ever forget to initialize a ChessRule explicitly (like in the case you had to guard against here, where the rule defaulted to CHECKMATE).

::: src/gnome-chess.vala
@@ +1530,1 @@
     private string make_clock_text (ChessClock? clock, Color color)

I'd rather the ChessClock be set properly before entering this method, so that you don't have to modify make_clock_text() at all. In start_game(), look in the conditional under the comment "We only support simple timeouts." There is a check to make sure no player is out of time before creating the ChessClock -- remove that check so that the ChessClock gets created even if one player is out of time, and then make_clock_text() should work properly.

Now, the only question is: why did that check exist? I think it was just a bug; I can't think of anything that would break if we remove it.

@@ +1530,3 @@
     private string make_clock_text (ChessClock? clock, Color color)
     {
+	if (pgn_game.result != PGNGame.RESULT_IN_PROGRESS && pgn_game.white_time_left != null)

Even though you'll delete this code anyway, I want to note that it's not safe to trust that black_time_left != null just because white_time_left != null -- the PGN file is untrusted input.

Created attachment 293477 [details] [review]
Saving a completed game due to timeup saves the timer as infinity

Thanks for the suggestions Michael!
---
Tested with "no time limit" and with time limits.

Review of attachment 293477 [details] [review]:

::: src/gnome-chess.vala
@@ -466,3 @@
         if (pgn_game.time_control != null && int.parse (pgn_game.time_control) != 0)
         {
-            if (pgn_game.white_time_left != null && pgn_game.black_time_left != null)

But you need this conditional, because again, you can't assume that the PGN file is well-formed. The file might have the time_control flag but not the white_time_left or black_time_left flag, so either of those variables could be null here. Hence this check is needed.

@@ +471,1 @@
+            if (white_seconds >= 0 && black_seconds >= 0)

Did you test this? :)  This condition states that if either player is out of time (which is necessarily the case when this bug occurs), the ChessClock should not be created. This is the one that I think needs to be removed.

(In reply to comment #4)
> @@ +471,1 @@
> +            if (white_seconds >= 0 && black_seconds >= 0)
> 
> Did you test this? :)  This condition states that if either player is out of
> time (which is necessarily the case when this bug occurs), the ChessClock
> should not be created. This is the one that I think needs to be removed.

You pointed out on IRC that you changed the signs from > to >=, so this is actually OK (and better than removing the check, since this way we don't accidentally show negative times remaining).

Created attachment 293484 [details] [review]
Saving a completed game due to timeup saves the timer as infinity

Adding back the pgn_game.white_time_left != null && pgn_game.black_time_left != null to handle broken pgn's.

Attachment 293484 [details] pushed as 3c99d3d - Saving a completed game due to timeup saves the timer as infinity