Created attachment 301023 [details] [review]
add alternate option

Why is alternate useful?

I'm sorry but I don't see a reason why its needed. The player can select the color he wants to be using "Play As".


I would request you to file a bug and get it confirmed,
before starting to work on it for something new that you feel needs to be added.

Hi, he originally planned to do a Random option and checked it with me first, but Arnaud suggested he change to Alternate. I think it's OK since it's just adding a new option to the combo box, and it's only a small amount of code. I warned him that it might need to go away when we switch to a new game screen. Seems like a reasonable option to me; I would probably use it, and Arnaud is considering adding it to Taquin.

Review of attachment 301023 [details] [review]:

::: src/gnome-chess.vala
@@ +2390,3 @@
+            }
+
+            if (play_as == "white")

This should be an else if, for clarity.

(In reply to Michael Catanzaro from comment #4)
> Review of attachment 301023 [details] [review] [review]:
> 
> ::: src/gnome-chess.vala
> @@ +2390,3 @@
> +            }
> +
> +            if (play_as == "white")
> 
> This should be an else if, for clarity.

Changing this to an else-if wouldn't work as play_as is set to the good value in the first if (when alternate is set), and the next ifs would not be evaluated. I don't really see how to use an else-if without code duplication.

(In reply to Michael Catanzaro from comment #3)
> Hi, he originally planned to do a Random option and checked it with me
> first, but Arnaud suggested he change to Alternate. I think it's OK since
> it's just adding a new option to the combo box, and it's only a small amount
> of code. I warned him that it might need to go away when we switch to a new
> game screen. Seems like a reasonable option to me; I would probably use it,
> and Arnaud is considering adding it to Taquin.

Okay, I looked online for what other chess apps do and 
I'm happy to change my opinion on this.


Ref: http://www.chess.com/blog/groteskbold/quotoh-no-i-have-to-play-as-black-nowquot

Created attachment 301146 [details] [review]
add alternate patch version 2

(In reply to Michael Catanzaro from comment #4)
> Review of attachment 301023 [details] [review] [review] [review]:
> 
> ::: src/gnome-chess.vala
> @@ +2390,3 @@
> +            }
> +
> +            if (play_as == "white")
> 
> This should be an else if, for clarity.

I followed your advice and made the code clearer, albeit a bit more verbose, but I understand that readability is very important.

TBH I think the original way is a bit easier to read; I just skimmed too fast. :) Whichever you prefer is fine.

Sahil, do you want to handle reviewing/pushing this patch, or shall I?

Review of attachment 301146 [details] [review]:

Please get rid of "For that, it changes the 'play-as-white' (bool) setting to 'play-as' (int),
and if set to 'alternate' a new game will be played as the opposite color
of the new setting 'last-side'." in the description.

Its best to keep the description brief and to the point without including the implementation details.

::: data/org.gnome.chess.gschema.xml
@@ +8,3 @@
   </enum>
 
+  <enum id="org.gnome.chess.SideColor">

Why not rename it as org.gnome.chess.PlayAs?

@@ +104,3 @@
+      <description>The board side to play as</description>
+    </key>
+    <key name="last-side" enum="org.gnome.chess.SideColor">

key-name="last-played-as"

::: src/gnome-chess.vala
@@ +2381,3 @@
         if (engine_name != null && engine_name != "human")
         {
+            var play_as = settings.get_string ("play-as");

Please change this to an enum with three valid states and an invalid one(See lib/chess-clock.vala for an example)
or
add an else block at the end and assert with invalid play-as.

(In reply to Sahil Sareen from comment #9)
> add an else block at the end and assert with invalid play-as.

Should I use an "assert_not_reached ()" to do that?

(In reply to Johan Manuel from comment #10)
> (In reply to Sahil Sareen from comment #9)
> > add an else block at the end and assert with invalid play-as.
> 
> Should I use an "assert_not_reached ()" to do that?

Yes.

Created attachment 301231 [details] [review]
add alternate patch 3

I hope I got things right this time! I used the first patch as a base, as in the end I prefer this version.
I also moved the line setting last-played-as outside the first if to prevent from playing twice as the same color when switching from black to alternate.

Review of attachment 301231 [details] [review]:

The patch looks good now! 
Thanks for incorporating my suggestions.


I'll test it and merge in a day or two.

(In reply to Sahil Sareen from comment #13)
> Review of attachment 301231 [details] [review] [review]:
> 
> The patch looks good now! 
> Thanks for incorporating my suggestions.
> 
> 
> I'll test it and merge in a day or two.

Thank you!

Pushed to master. Thanks Johan! :D

To ssh://ssareen@git.gnome.org/git/gnome-chess
   dd80c84..ba128dd