Created attachment 296390 [details] [review]
Add support for Fischer and Bronstein clocks

I haven't been able to handle the restart(close and reopen) cases very well.
Fischer: Extra_time is added unnecessarily.
Bronstein: White/Black prev_move_time is set to time when game restarted.

Review of attachment 296390 [details] [review]:

OK, this looks like a good first patch. Some issues I notice:

* The grid in the Appearance is broken (the move format label is placed in a different row than its combo box).
* I don't like that the preferences dialog expands horizontally; that's pretty jarring. Instead, move the time increment setting below Clock type, and just make it insensitive if the clock type is Simple. (Or, make it visible even when the clock is Simple, and use it for the delay before the clock starts counting down as described on Wikipedia, where 0 would be the current behavior.) I think we can also assume the increment is specified in seconds, and just allow the count in seconds to be set a bit higher (say, limit it to 500 seconds rather than 59); that will simplify things more.
* There should be some label to explain what the time next to Clock type does. "Time increment" would work, for example.
* The clock type setting should be directly below the time limit setting. Note that if Opposing player is Human, then Play as and Difficulty become insensitive, but Clock type does not.
* ...and if Time limit is No limit, then Clock type should become insensitive. So hierarchically, it should be beneath Time limit.

What problem did you run into when trying to prevent the Fischer time increment from being improperly added when loading a game? That ought to be fixed.

For the Bronstein clock, you could add another field to the PGN file to store how much time was counted down before the game was saved, but this would probably be overkill. I think the current behavior is fine, since the player can't use it to improperly increment his clock, and the lost time will usually be quite small.

Lastly, we should try to fix the jerky behavior from the clock when switching from one player to the next. That's a preexisting problem but it's exacerbated by more confusing time controls. I think switching from Timeout.add_seconds() to Timeout.add() will help (at the cost of increased power consumption, but I think we need the better accuracy here).

::: data/org.gnome.chess.gschema.xml
@@ +77,3 @@
       <description>The duration of a game in seconds (0 for no limit)</description>
     </key>
+    <key name="clock-type" type="i">

This should be an enum, not an int. Look at the top of the gschema file for examples on how to use enums.

@@ +89,3 @@
+    <key name="clock-type-duration-multiplier" type="i">
+      <default>1</default>
+      <summary>The multiplier for duration set corresponding to clock type (1 for seconds, 60 for minutes)</summary>

I don't think we need to store this. Just store clock-type-duration in seconds, and if it's divisible by 60 then you can display it in minutes when the preferences dialog is opened.

::: lib/chess-clock.vala
@@ +57,3 @@
+                break;
+            case ClockType.FISCHER:
+            if (active_color == Color.WHITE)

This style of indentation is difficult to read. The case line should be indented less than its body, and the break should be indented with the body. Like this:

switch (clock_type)
{
case ClockType.FISCHER:
    if (active_color == Color.WHITE)
    ...
    break;
}

See one of the switch statements in gnome-chess.vala for an example (except the one in show_promotion_type_selector which is indented differently, which I will fix now).

@@ +70,3 @@
+                    white_extra_seconds += extra_seconds;
+                else
+                    white_extra_seconds += white_move_used;

How about white_extra_seconds += int.min (extra_seconds, white_move_used). (And same for black below.)

Created attachment 296518 [details] [review]
Add support for Fischer and Bronstein clocks

(In reply to Michael Catanzaro from comment #2)
> Review of attachment 296390 [details] [review] [review]:
> 
> OK, this looks like a good first patch. Some issues I notice:
> 

Thanks for the quick review and comments mike!

> * The grid in the Appearance is broken (the move format label is placed in a
> different row than its combo box).

Sorry, I forgot to check that. 

> * I don't like that the preferences dialog expands horizontally; that's
> pretty jarring. Instead, move the time increment setting below Clock type,
> and just make it insensitive if the clock type is Simple. (Or, make it
> visible even when the clock is Simple, and use it for the delay before the
> clock starts counting down as described on Wikipedia, where 0 would be the
> current behavior.) I think we can also assume the increment is specified in
> seconds, and just allow the count in seconds to be set a bit higher (say,
> limit it to 500 seconds rather than 59); that will simplify things more.

Improved it. 
Changed to a two column dialog.

> * There should be some label to explain what the time next to Clock type
> does. "Time increment" would work, for example.

Added a label 'Time increment'.

> * The clock type setting should be directly below the time limit setting.
> Note that if Opposing player is Human, then Play as and Difficulty become
> insensitive, but Clock type does not.
> * ...and if Time limit is No limit, then Clock type should become
> insensitive. So hierarchically, it should be beneath Time limit.

Done!
> 
> What problem did you run into when trying to prevent the Fischer time
> increment from being improperly added when loading a game? That ought to be
> fixed.

Fixed this!
The problem was a switch from WHITE(default in lib/chess-clock) to BLACK when the game comes up with BLACK as the current_player.

> 
> For the Bronstein clock, you could add another field to the PGN file to
> store how much time was counted down before the game was saved, but this
> would probably be overkill. I think the current behavior is fine, since the
> player can't use it to improperly increment his clock, and the lost time
> will usually be quite small.

Okay. Agree it would be an overkill.
> 
> Lastly, we should try to fix the jerky behavior from the clock when
> switching from one player to the next. That's a preexisting problem but it's
> exacerbated by more confusing time controls. I think switching from
> Timeout.add_seconds() to Timeout.add() will help (at the cost of increased
> power consumption, but I think we need the better accuracy here).
> 

Improved the accuracy by replacing Timeout.add_seconds() with Timeout.add().

> ::: data/org.gnome.chess.gschema.xml
> @@ +77,3 @@
>        <description>The duration of a game in seconds (0 for no
> limit)</description>
>      </key>
> +    <key name="clock-type" type="i">
> 
> This should be an enum, not an int. Look at the top of the gschema file for
> examples on how to use enums.

Changed to enum.
> 
> @@ +89,3 @@
> +    <key name="clock-type-duration-multiplier" type="i">
> +      <default>1</default>
> +      <summary>The multiplier for duration set corresponding to clock type
> (1 for seconds, 60 for minutes)</summary>
> 
> I don't think we need to store this. Just store clock-type-duration in
> seconds, and if it's divisible by 60 then you can display it in minutes when
> the preferences dialog is opened.

Right. Removed this.
> 
> ::: lib/chess-clock.vala
> @@ +57,3 @@
> +                break;
> +            case ClockType.FISCHER:
> +            if (active_color == Color.WHITE)
> 
> This style of indentation is difficult to read. The case line should be
> indented less than its body, and the break should be indented with the body.
> Like this:
> 
> switch (clock_type)
> {
> case ClockType.FISCHER:
>     if (active_color == Color.WHITE)
>     ...
>     break;
> }
> 

Sorry for the bad indentation. 
I've fixed the other bad switch blocks in gnome-chess files too.

> See one of the switch statements in gnome-chess.vala for an example (except
> the one in show_promotion_type_selector which is indented differently, which
> I will fix now).
> 
> @@ +70,3 @@
> +                    white_extra_seconds += extra_seconds;
> +                else
> +                    white_extra_seconds += white_move_used;
> 
> How about white_extra_seconds += int.min (extra_seconds, white_move_used).
> (And same for black below.)

Okay. Replaced with this version.

Review of attachment 296518 [details] [review]:

OK, this looks good much better. From the UI the only problem I see is that there's more space between the time limit and clock type combos than there is between the other combos. From a quick look in the UI file I don't immediately see why; you might need to investigate with the inspector (Ctrl+Shift+D with GTK+ 3.14 or higher).

Actually I see one more problem: the clock type is not saved in the PGN, so if you save a game using the Fischer clock, change your setting to Bronstein, and start playing again, a Bronstein clock will be used. I would expect the clock type to be saved in the PGN rather than determined based on the value of the setting when the game is loaded. (This will only be confusing until we replace the first page of the preferences dialog with a new game screen.) You can just make up a name for the new tag, but make sure we fall back to Simple for games where the new tag isn't present.

::: data/org.gnome.chess.gschema.xml
@@ +86,3 @@
+      <default>'simple'</default>
+      <summary>The type of clock</summary>
+      <description>The type of clock(simple/fischer/bronstein)</description>

Add a spare here before the opening parenthesis...

@@ +96,3 @@
+      <default>0</default>
+      <summary>Seconds white has used until last move.(0 for first move)</summary>
+      <description>Seconds white has used until last move.(0 for first move)</description>

...and here...

@@ +101,3 @@
+      <default>0</default>
+      <summary>Seconds black has used until last move.(0 for first move)</summary>
+      <description>Seconds black has used until last move.(0 for first move)</description>

...and here.

::: lib/chess-clock.vala
@@ +212,3 @@
     {
         /* Wake up each second */
+        tick_timeout_id = Timeout.add (1000, tick_cb);

Can you please move this to a separate patch (which you can push without further review). It doesn't seem to have made as big a difference as I'd hoped, but can't hurt. (Actually it can hurt, it will use more power, but oh well.)

::: lib/chess-game.vala
@@ -274,3 @@
             _clock.active_color = current_player.color;
         }
-

Spurious whitespace change

::: src/gnome-chess.vala
@@ +578,3 @@
+        int clock_type_adj_value = settings.get_int ("clock-type-duration");
+
+        ClockType clock_type = ClockType.string_to_enum(settings.get_string ("clock-type"));

Missing space before opening parenthesis here. But more importantly, you can use settings.get_enum() which returns an int to simplify this more.

@@ +1731,3 @@
+
+        clock_type_units_combo = (Gtk.ComboBox) preferences_builder.get_object ("clock_type_units_combo");
+        set_clock_type ((int)ClockType.string_to_enum(settings.get_string ("clock-type")));

Missing space before opening parenthesis here. Also, we normally use a space after the closing parenthesis in a cast.

@@ +1822,3 @@
+    {
+        var model = clock_type_combo.model;
+        Gtk.TreeIter iter, reqd_iter = {};

What does reqd stand for? Can you think of a better name for this?

@@ +2009,3 @@
 
+    [CCode (cname = "G_MODULE_EXPORT clock_type_units_changed_cb", instance_pos = -1)]
+    public void clock_type_units_changed_cb (Gtk.Widget widget)

Can you share some of this code with duration_changed_cb(), or would that be difficult?

@@ +2023,3 @@
+                {
+                case 1:
+                    model.set (iter, 0, ngettext (/* Preferences Dialog: Combo box entry for a custom game timer set in seconds */

Can you update the translator comment here?

@@ +2027,3 @@
+                    break;
+                case 60:
+                    model.set (iter, 0, ngettext (/* Preferences Dialog: Combo box entry for a custom game timer set in minutes */

Same

@@ +2080,3 @@
+
+        if (duration == 0)
+            set_clock_type( ClockType.SIMPLE );

Weird spacing

@@ +2102,3 @@
+        clock_type_box.visible = clock_type > 0;
+        timer_increment_label.visible = clock_type > 0;
+        settings.set_string ("clock-type", clock_type.to_string());

Another missing space

(In reply to Michael Catanzaro from comment #4)
> Review of attachment 296518 [details] [review] [review]:
> 
> OK, this looks good much better. From the UI the only problem I see is that
> there's more space between the time limit and clock type combos than there
> is between the other combos. From a quick look in the UI file I don't
> immediately see why; you might need to investigate with the inspector
> (Ctrl+Shift+D with GTK+ 3.14 or higher).

I think it's related to the hidden combo box between them, when a custom time limit is not in use. If you set the grid's row-spacing to 0 then the issue goes away, but then there is not enough space between the rows. This might be tricky to fix. I think you'll need to actually remove the custom time widget from the grid entirely when it's not to be shown. If you can't figure out how to fix it, you can file a new bug as a reminder for us to fix it later.

(In reply to Michael Catanzaro from comment #4)
> Review of attachment 296518 [details] [review] [review]:
> 
> OK, this looks good much better. From the UI the only problem I see is that
> there's more space between the time limit and clock type combos than there
> is between the other combos. From a quick look in the UI file I don't
> immediately see why; you might need to investigate with the inspector
> (Ctrl+Shift+D with GTK+ 3.14 or higher).
> 
> Actually I see one more problem: the clock type is not saved in the PGN, so
> if you save a game using the Fischer clock, change your setting to
> Bronstein, and start playing again, a Bronstein clock will be used. I would
> expect the clock type to be saved in the PGN rather than determined based on
> the value of the setting when the game is loaded. (This will only be
> confusing until we replace the first page of the preferences dialog with a
> new game screen.) You can just make up a name for the new tag, but make sure
> we fall back to Simple for games where the new tag isn't present.

In that case, I should also be moving the clock-type-duration to pgn to be consistent.
I thought making more disk access wasn't a good idea but I'm ok having both these(clock-type and clock-type-duration) in pgn too.

I will be doing the new game screen next.

Pl confirm what needs tbd here?

(In reply to Sahil Sareen from comment #6)> 
> In that case, I should also be moving the clock-type-duration to pgn to be
> consistent.

Yes absolutely, I missed that.

> I thought making more disk access wasn't a good idea but I'm ok having both
> these(clock-type and clock-type-duration) in pgn too.

No, reading two more fields out of a file is not a problem. :p

Created attachment 296838 [details] [review]
Add support for Fischer and Bronstein clocks

Fixed mike's comments.
I'll file a bug for the space issue in the preferences dialog.

Review of attachment 296838 [details] [review]:

OK, I'm almost satisfied now, just a few more comments.

By the way, no need to rush to finish this if you don't mind missing 3.16, since I don't really care whether this goes into 3.16 or 3.18. If you care, you should rush. :p

::: data/preferences.ui
@@ +273,3 @@
+                    <property name="can_focus">False</property>
+                    <property name="halign">start</property>
+                    <property name="label" translatable="yes" comments="Preferences Dialog: Label before timer increment combo box">_Timer increment:</property>

Don't use t for the mnemonic (press Alt in the dialog to see how these work), since that's already in use by _Time limit.  How about i instead? (Timer _increment)

::: lib/chess-clock.vala
@@ +99,3 @@
+            {
+                white_extra_seconds += int.min(extra_seconds, white_move_used);
+            } else {

Style nit: brace should be on the next line. But actually, don't use the braces at all for one-line statements.

::: lib/chess-pgn.vala
@@ +26,3 @@
     else if (name == "Result")
         return 6;
+    else if (name == "Time")

I think you don't want to change this. These tags are the Seven Tag Roster and they are supposed to appear before all other tags in the PGN. I've pushed a copy of the PGN spec to the top-level directory of our repo; take a look at section 8.1.1.  (Mildly-related, check out section 9.5.1.)

Anyway, this change looks like it would break compare_tag() (and thus the order). But gnome-chess doesn't care when it loads the PGN, and we have no tests to make sure the export format is correct, so it's no wonder you didn't notice.

@@ +131,3 @@
+    {
+        get { return tags.lookup ("ClockType"); }
+        set { tags.insert ("ClockType", value); }

OK. How about X-GNOME-ClockType to make it clear that it's an extension?

I wish I had done that with WhiteTimeRemaining and BlackTimeRemaining as well. As a separate issue, I think I will change it to use X-GNOME-WhiteSecondsRemaining and X-GNOME-BlackSecondsRemaining, but still recognize WhiteTimeRemaining and BlackTimeRemaining.

@@ +136,3 @@
+    {
+        get { return tags.lookup ("ClockTypeDuration"); }
+        set { tags.insert ("ClockTypeDuration", value); }

For this one, I would almost prefer to use the standard TimeControl tag (section 9.6.1), and just ignore everything besides the increment portion of the field. But I say "almost" because I don't see much value in supporting only a small portion of that field, so a separate tag like this is probably the way to go. I'm not sure I like ClockTypeDuration for the name of the tag, though. Maybe X-GNOME-TimerIncrement?

::: src/gnome-chess.vala
@@ +579,3 @@
+        if (pgn_game.clock_type_duration != null) {
+            clock_type_adj_value = int.parse (pgn_game.clock_type_duration);
+            settings.set_int ("clock-type-duration", clock_type_adj_value);

Hm, no, I don't think loading a game should change the user's settings. It should override the setting only for the current game.

@@ +588,3 @@
+        if (pgn_game.clock_type != null) {
+            clock_type = ClockType.string_to_enum (pgn_game.clock_type);
+            settings.set_string ("clock-type", clock_type.to_string ());

Same

Created attachment 296855 [details] [review]
Add support for Fischer and Bronstein clocks

This is hopefully good to push up!

Review of attachment 296855 [details] [review]:

Well I keep finding problems. :p  You can push after you fix these:

::: data/preferences.ui
@@ +257,3 @@
+                    <property name="can_focus">False</property>
+                    <property name="halign">start</property>
+                    <property name="label" translatable="yes" comments="Preferences Dialog: Label before clock type(Fischer/Bronstein) combo box">_Clock type:</property>

Missing space before the opening parenthesis

@@ +259,3 @@
+                    <property name="label" translatable="yes" comments="Preferences Dialog: Label before clock type(Fischer/Bronstein) combo box">_Clock type:</property>
+                    <property name="use_underline">True</property>
+                    <property name="mnemonic_widget">side_combo</property>

This is the widget that gets focused when you trigger the mnemonic (Alt+C in this case) so I guess it should be clock_type_combo. Try it with your current patch and notice that Play As ((which "side" to play on) gets focused instead.

@@ +275,3 @@
+                    <property name="label" translatable="yes" comments="Preferences Dialog: Label before timer increment combo box">Timer _increment:</property>
+                    <property name="use_underline">True</property>
+                    <property name="mnemonic_widget">side_combo</property>

And this one should probably be timer_increment_spin.

::: src/gnome-chess.vala
@@ +594,3 @@
+        }
+
+        if (game.clock != null) {

Brace should be on the next line

Created attachment 296903 [details] [review]
Add support for Fischer and Bronstein clocks

Fixed final review comments, pushing to master shortly.

Pushed to master.
Marking the bug as resolved! :D

To ssh://ssareen@git.gnome.org/git/gnome-chess
   ed10440..d00db10  master -> master