It's pretty basic (and I'm sure incomplete), but there's some initial mockups here:

https://raw.github.com/gnome-design-team/gnome-mockups/master/games/iagno/iagno.png

Nice work!

Created attachment 257325 [details] [review]
Have a play around implementing Allan's design

Main issues with the patch are:
- Grid requests to be square but is assigned more width than height; this causes the offset to the left.
- Alignment of widgets to the right of the grid

Created attachment 257326 [details]
Screenshot of new design implementation

Review of attachment 257325 [details] [review]:

I take it a certain operating system was recently frozen? :)

That already looks pretty good. But can we please track this in Bug #710125 instead, and leave this bug for the new game screen only? Thanks.

Reassessing whether we really want new game screens for all of our games....

We definitely want a new game screen (or new game sidebar) for Iagno, since we do not want to use the app menu for choosing between human and computer opponents.

What is requirement of this bug fix and which design needs to be implemented?

(In reply to comment #7)
> What is requirement of this bug fix and which design needs to be implemented?

For Iagno, we have a design in comment #1 that uses a new game sidebar instead of a new game screen.

I thought I already commented this bug, but must have been on IRC. I see some problems with Allan’s design of a “new-game sidebar”.

Firstly, the empty board at the left doesn’t look cool; and it could be filled with the end of the previous game only if one have been played. And if it’s already filled with the beginning position, some people will try to play before having a look to the start button… What happens when you hit “Start Over” during an existing play isn’t defined, and is a related problem (can you continue to play..?).

Secondly, looks like you have some “difficulty options” displayed even if you choose a two-players game; unsensitive, probably (meaning we cannot implement a “hint” in two players mode?), but definitively displayed, and taking many place on the column.

Thirdly, a really important thing: it doesn’t let you choose when you’re playing solo if you want to play dark or light, or randomly/alternatively (this third choice –if not imposed, needs discussions– is lacking for now but is necessary I think).

Here are some reasons why I’m not really fan of this new-game sidebar. Of course, the problem that appears is the emptiness of the new-game screen… but for that, I have a proposition. I think many games should implement a “variant game”, something that offers other playing fun but with quite the same rules. I have in mind to implement a “cylindric” Four-in-a-row for example (if you see how this is done). For Iagno, the variant is evident, it’s the “reverse reversi”, the misère variant of the game (you have to finish with less tiles than your opponent).

So a new-game screen would be a quite better idea in my mind (including all my personal opinions on games ^^), a design[1] like the one of the toy-game 15-puzzle called Taquin[2] I coded last week.

[1] https://github.com/Obsidien/Taquin/blob/master/help/C/figures/start-screen.png
[2] https://github.com/Obsidien/Taquin

(In reply to comment #9)
> Firstly, the empty board at the left doesn’t look cool; and it could be filled
> with the end of the previous game only if one have been played. And if it’s
> already filled with the beginning position, some people will try to play before
> having a look to the start button… 

Good point.

> What happens when you hit “Start Over”
> during an existing play isn’t defined, and is a related problem (can you
> continue to play..?).

The game starts over?

> Secondly, looks like you have some “difficulty options” displayed even if you
> choose a two-players game; unsensitive, probably (meaning we cannot implement a
> “hint” in two players mode?), but definitively displayed, and taking many place
> on the column.

Well they're going to have to exist, either insensitive or hidden, in either a sidebar or a new game screen. I don't think this relates at all to hints: if we implement hints, they should always use the hard AI.

> Thirdly, a really important thing: it doesn’t let you choose when you’re
> playing solo if you want to play dark or light, or randomly/alternatively (this
> third choice –if not imposed, needs discussions– is lacking for now but is
> necessary I think).

Well, imagine this option exists in the design. It's not something we need to remove. Whether we use a new game screen or a new game sidebar, we'll keep the existing options.

> Here are some reasons why I’m not really fan of this new-game sidebar. Of
> course, the problem that appears is the emptiness of the new-game screen… but
> for that, I have a proposition. I think many games should implement a “variant
> game”, something that offers other playing fun but with quite the same rules. I
> have in mind to implement a “cylindric” Four-in-a-row for example (if you see
> how this is done). For Iagno, the variant is evident, it’s the “reverse
> reversi”, the misère variant of the game (you have to finish with less tiles
> than your opponent).

For this, a new game screen would be better, yes.

I'm actually leaning towards a new game screen now, like we do in the other games.  It's a good pattern that's worked well where we've used it so far.

(In reply to comment #10)
> > What happens when you hit “Start Over”
> > during an existing play isn’t defined, and is a related problem (can you
> > continue to play..?).
> 
> The game starts over?

A bad click and your game is lost… We really need a way to undo that. Going to a start screen offer to have the usual “back” button (the one of Sudoku) a really easy way.

> > Secondly, looks like you have some “difficulty options” displayed even if you
> > choose a two-players game; unsensitive, probably (meaning we cannot implement a
> > “hint” in two players mode?), but definitively displayed, and taking many place
> > on the column.
> 
> Well they're going to have to exist, either insensitive or hidden, in either a
> sidebar or a new game screen. I don't think this relates at all to hints: if we
> implement hints, they should always use the hard AI.

Implemented as in my Taquin, this options would be a small menubutton (with popover) that is less important in the UI (as that’s not a thing you change at each play… you find the good AI, and you always play with after).

> […]
> I'm actually leaning towards a new game screen now, like we do in the other
> games.  It's a good pattern that's worked well where we've used it so far.

We could introduce cool transitions this cycle (see Taquin, notably adapted as it’s a sliding puzzle). =)

(In reply to comment #11) 
> A bad click and your game is lost… We really need a way to undo that. Going to
> a start screen offer to have the usual “back” button (the one of Sudoku) a
> really easy way.

Well that's one possibility. See also bug #736654

Created attachment 286702 [details] [review]
Selecting game mode and level moved from preferences to new game screen.

Review of attachment 286702 [details] [review]:

Thanks Amisha!  This looks like a good start.

I only found a couple of bugs:

* The Start Over button on the new game screen should read "Start Game."
* The subtitle should be cleared when entering the new game screen. Test by selecting an invalid position before pressing New Game.
* Edge case: If you make a move, then press New Game before the computer moves, it will move while you are on the new game screen, and the player won't get to see what happened. Instead, the computer's move should be canceled and then restarted when returning to the game from the new game screen. (Or the animation could just be delayed.)  This might be a little difficult to handle and it's not a big deal, so don't spend too much time trying to fix this if you run into trouble.

The patch works well, but I don't really like the combo boxes.  Instead, I'd use two rows of three buttons each, like this:

Game Mode

Play Dark           Play Light          Two Players
(Moves First)       (Moves Second)

Computer Player

Easy                Medium              Hard

Created attachment 286841 [details] [review]
Selecting game mode and level moved from preferences to new game screen.

Review of attachment 286841 [details] [review]:

I think that with this change, the New Game button ought to be sensitive at all times, so that you can start over with new settings before the first move.

Anyway, this patch works almost perfectly! My comments are all minor. We also need to play around some more with the spacing of the widgets to make it look better, but you've got it close.

::: data/iagno.ui
@@ +77,3 @@
             </child>
             <child>
+              <object class="GtkBox" id="mode-combo">

It shouldn't be named "mode-combo" any more... how about "game-mode-box?"

@@ +86,3 @@
                     <property name="halign">center</property>
+                    <property name="valign">center</property>
+                    <property name="tooltip-text" translatable="yes">Moves First</property>

Eh, this tooltip isn't really discoverable. I'd remove it; we can think of a better way to display this information later, by packing a custom GtkLabel into the button. (You don't have to worry about that for now.)

@@ +101,3 @@
                     <property name="visible">True</property>
                     <property name="use_underline">True</property>
+                    <property name="label" translatable="yes">_Play Light</property>

Hm, this has the same mnemonic as the Play Dark button (P).  How about using "Play _Dark" and "Play _Light" instead?

@@ +104,3 @@
                     <property name="halign">center</property>
                     <property name="valign">center</property>
+                    <property name="tooltip-text" translatable="yes">Moves Second</property>

Remove this tooltip too.

@@ +122,3 @@
+                    <property name="halign">center</property>
+                    <property name="valign">center</property>
+                    <property name="tooltip-text" translatable="yes"></property>

Empty is the default, so you can remove this too.

@@ +144,3 @@
+            </child>
+            <child>
+              <object class="GtkBox" id="level-combo">

It shouldn't be named "level-combo" anymore... maybe "level-box?"

@@ +208,3 @@
+                <property name="action-name">app.start-game</property>
+                <property name="tooltip-text" translatable="yes">Start a game</property>
+                <property name="width-request">120</property>

Let's make this button wider to make it easier to see relative to the others.  How about 222 pixels, which is what Arnaud used for Taquin.

@@ +212,1 @@
               </object>

Then before closing the object tag, you can give it a style class like this:

  <property name="height-request">60</property>
  <style>
    <class name="suggested-action"/>
  </style>
</object>

::: src/iagno.vala
@@ +46,3 @@
+    private Gtk.ToggleButton Easy_button;
+    private Gtk.ToggleButton Medium_button;
+    private Gtk.ToggleButton Hard_button;

By convention, we use only lowercase letters for variable names. I'd name these play_dark_button, play_light_button, two_players_button, easy_button, medium_button, and hard_button.

@@ +199,3 @@
+
+        Dark_button.set_active(true);
+        Easy_button.set_active(true);

Missing spaces before the opening parenthesis here.

@@ +201,3 @@
+        Easy_button.set_active(true);
+
+        var level_combo = builder.get_object ("level-combo") as Gtk.Box;

This will need changed too.

@@ +211,3 @@
+    }
+
+    private void toggle_cb_light (Gtk.ToggleButton button)

By convention, _cb should be at the end of the function name. I'd probably name this play_light_button_toggled_cb().

@@ +230,3 @@
+    }
+
+    private void toggle_cb_dark (Gtk.ToggleButton button)

play_dark_button_toggled_cb()

@@ +249,3 @@
+    }
+
+    private void toggle_cb_two_players (Gtk.ToggleButton button)

two_players_button_toggled_cb()

@@ +268,3 @@
+    }
+
+    private void toggle_cb_easy (Gtk.ToggleButton button)

easy_button_toggled_cb()

@@ +287,3 @@
+    }
+
+    private void toggle_cb_medium (Gtk.ToggleButton button)

medium_button_toggled_cb()

@@ +306,3 @@
+    }
+
+    private void toggle_cb_hard (Gtk.ToggleButton button)

hard_button_toggled_cb()

@@ +371,3 @@
     }
 
+    private void back_cb ()

Disable the back action here, since it doesn't make sense for that to be active unless you're on the new game screen.

@@ +378,3 @@
+    }
+
+    private void show_new_game_screen ()

And here you can enable the back action.

@@ +403,3 @@
+            game_box.remove (view);
+
+        back_cb ();

It's a bit confusing to invoke callbacks directly, since the name of the function doesn't give you any clue what it will do. The way around this is to rename back_cb() to, say, show_game_board(), call that here instead, and then provide a new back_cb() that just calls show_game_board().

Review of attachment 286841 [details] [review]:

One more thing: I didn't anticipate the need to set the buttons insensitive. That doesn't look quite right.  We'll probably need some custom CSS here, but this is outside my expertise; can you ask Arnaud to help you with it?

Created attachment 286937 [details] [review]
Selecting game mode and level moved from preferences to new game screen.

Review of attachment 286937 [details] [review]:

OK, great. I was worried this would be lot harder. Thanks for helping Arnaud!

I see one compiler warning that should be fixed:

iagno.vala:365.13-365.71: warning: local variable `new_game_action' declared but never used
        var new_game_action = (SimpleAction) lookup_action ("new-game");

I already said I'd try to help with this:

(iagno:28598): Gtk-CRITICAL **: gtk_radio_button_set_group: assertion '!g_slist_find (group, radio_button)' failed

I'm still trying to figure this out. You seem to have handled the groups exactly the same as Arnaud did in Taquin. I'm not sure why his is working and yours isn't.

::: data/iagno.ui
@@ +86,3 @@
                     <property name="halign">center</property>
+                    <property name="valign">center</property>
+                     <property name="width-request">120</property>

Extra space here

I guess let's ignore the warning for now, since it doesn't seem to be hurting anything....

Created attachment 286971 [details] [review]
Use actions for new-game screen buttons.

I’ve made some test but don’t understand the warning. As it’s done, here is a patch using actions (or showing how to use them).

Arnaud are you still getting those warnings?

Yeps. I can make them disappear removing the
  <property name="group">...</property>
lines (but the radiobuttons are no more in groups), or adding *two warnings* (by attribute) by adding the group to the radiobutton that have the id= attribute. And I don’t understand how it could work in Taquin and not here.

Great, thanks both of you!

Let's leave this bug open as a reminder that we have a runtime warning and an odd deprecation warning (which looks wrong?) to figure out.

Attachment 286971 [details] pushed as 3e3a186 - Use actions for new-game screen buttons.

Oh, I wish I'd noticed this before I pushed... we also need to load the settings from gsettings when starting the game, to figure out the initial state of the buttons. Right now it always starts at Play Dark and Easy.  Oops!

TODO:
* load and sync settings and buttons;
* command-line (and with it jumplist) should skip new-game screen;
* the back button should be hidden when launched;
* boxes containing buttons might be linked? ( https://wiki.gnome.org/HowDoI/Buttons#Linked_buttons )
* animations between screens;
* correct these #@!*±? warnings.

Created attachment 287015 [details] [review]
Add transitions.

The same as for Taquin, they work well even if it’s not a sliding puzzle. ^^

Created attachment 287018 [details] [review]
Back button disabled on game launch.

Created attachment 287019 [details] [review]
Add transitions.

Faster for beginning the game, a little slower for the others.

Created attachment 287020 [details] [review]
Make options skip the new-game screen.

Are only concerned --first, --second & --two-players, of course.

Review of attachment 287018 [details] [review]:

I think you removed too much code; now the back button can appear to the right of the undo button in the game view.

Review of attachment 287019 [details] [review]:

I'm not sure if I like this or not; for the game view it's fine, but I'm not sure about buttons and controls flying off in various directions.  Maybe it's fine, but let's get designer input here. Can you try to find Allan on IRC (username: aday) and ask him to try it out? We can do it with Mines and Sudoku too if he likes it.

Review of attachment 287020 [details] [review]:

Yup.

Actually I think we need a mockup anyway to help us with the positioning of the buttons and the headings (Game mode, Computer Player). I'll try to ask Allan if you don't get to him first, Arnaud.

(In reply to comment #26)
> TODO:

Yeah, I was too hasty here. :( Oh well; seems easiest at this point to roll with it.

"If you make a move, then press New Game before the computer moves,
it will move while you are on the new game screen, and the player won't get to
see what happened." <-- Forgot about this, still a problem. Makes a weird piece flip sound from nowhere. Should be easy to fix now that I added ComputerPlayer.cancel_move().

Comment on attachment 287020 [details] [review]
Make options skip the new-game screen.

Attachment 287020 [details] pushed as da69e0a - Make options skip the new-game screen.

Created attachment 287034 [details] [review]
Don't allow AI to move while human is on the new game screen

Created attachment 287066 [details] [review]
Remove app-menu’s new-game entry.

The “New game” entry of the app-menu isn’t needed anymore. Let’s remove it.

Created attachment 287067 [details] [review]
Load and sync settings.

A patch that does many things at once, but splitting it is hard. Cleans the loading of the settings, sync them with what’s needed even at launch, put the sound on/off in the app-menu.

Attachment 287034 [details] pushed as f5696b9 - Don't allow AI to move while human is on the new game screen

Review of attachment 287067 [details] [review]:

Just needs a more descriptive commit message, since moving the sound option to the app menu really needs to be mentioned. (Are you sure that's hard to split into its own patch; it seems like it could easily be a follow-up?)

Created attachment 287089 [details] [review]
Load settings in startup() and sync them with new-game screen buttons.

Looks like you’re right, the sound-in-app-menu part could in fact be splitted without problem (well, I hope…).

Created attachment 287091 [details] [review]
Move the sound switch to the app-menu.

Attachment 287089 [details] pushed as 8e6a8cb - Load settings in startup() and sync them with new-game screen buttons.
Attachment 287091 [details] pushed as 1b3ca44 - Move the sound switch to the app-menu.

Created attachment 287155 [details] [review]
Back button disabled on game launch.

Arnaud: "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`. ^^ )"

Bug proposed to correction elsewhere.

(In reply to comment #26)
> TODO:
> * load and sync settings and buttons;
> * command-line (and with it jumplist) should skip new-game screen;
> * the back button should be hidden when launched;

These are fixed.

> * boxes containing buttons might be linked? (
> https://wiki.gnome.org/HowDoI/Buttons#Linked_buttons )

Maybe. I want to see how it looks.

> * animations between screens;

Did you manage to get Allan's attention about this?

> * correct these #@!*±? warnings.

Most likely fixed by https://git.gnome.org/browse/gtk+/commit/?h=gtk-3-14&id=4fc47c800d1abe467a82db157491efba41cc58f4

We also need to adjust the spacing of the buttons and in particular the headings; it doesn't look good yet.

(In reply to comment #47)
> These are fixed.

Not really for “load and sync settings and buttons”: the sync part is buggy, but it’s probably a problem of dconf. Try changing the gsetting why the application is running, and you have a loop of values that bugs the app, quite bad.

> > * boxes containing buttons might be linked? (
> > https://wiki.gnome.org/HowDoI/Buttons#Linked_buttons )
> 
> Maybe. I want to see how it looks.
> […]
> We also need to adjust the spacing of the buttons and in particular the
> headings; it doesn't look good yet.

Tweaking this new-game screen is not in my todo-list, as I want to add an alternative game and so rework it more importantly. But it shouldn’t be difficult to do.

> > * animations between screens;
> 
> Did you manage to get Allan's attention about this?

I’d prefer discussing on that when releasing Taquin + Four-in-a-Row vala’s rewrite, as I try to sync the three layouts & behaviors. It might be moved in another bug.

> Most likely fixed by
> https://git.gnome.org/browse/gtk+/commit/?h=gtk-3-14&id=4fc47c800d1abe467a82db157491efba41cc58f4

That would be great news!

(In reply to comment #48)
> Not really for “load and sync settings and buttons”: the sync part is buggy,
> but it’s probably a problem of dconf. Try changing the gsetting why the
> application is running, and you have a loop of values that bugs the app, quite
> bad.

I didn't review your patch closely enough. Try putting the settings object into delayed-apply mode.

> Tweaking this new-game screen is not in my todo-list, as I want to add an
> alternative game and so rework it more importantly. But it shouldn’t be
> difficult to do.

I'll play around with it then. I'd like it to look decent even if you change it in the future.

Have you gotten your GNOME account yet, by the way? It feels like it's been a while. Feel free to add yourself as a maintainer of Iagno.

Created attachment 287718 [details] [review]
Improve spacing and alignment on new game screen

Comment on attachment 287718 [details] [review]
Improve spacing and alignment on new game screen

Attachment 287718 [details] pushed as fb6b49e - Improve spacing and alignment on new game screen

(In reply to comment #49)
> > Not really for “load and sync settings and buttons”: the sync part is buggy,
> > but it’s probably a problem of dconf. Try changing the gsetting why the
> > application is running, and you have a loop of values that bugs the app, quite
> > bad.
> 
> I didn't review your patch closely enough. Try putting the settings object into
> delayed-apply mode.

Doesn’t work. Or at least, I didn’t found how. But I’d be surprised it could: all is done automagically (“add_action (settings.create_action ("play-as"));”). Could you confirm please?

I failed to parse your comment. Say again please?

Well, maybe I misunderstood how delay-apply works, but I don’t think it could be used here. The code makes use of some functions that *should* do all the work completely and correctly, but fail to do it; so for me the bug is in dconf, not in Iagno.

(In reply to comment #49)
> Have you gotten your GNOME account yet, by the way? It feels like it's been a
> while. Feel free to add yourself as a maintainer of Iagno.

Oh yeah I have it, just didn’t had time to “play with it”. ^^

Created attachment 287940 [details] [review]
Don't remove the view when restarting.

I didn’t understood that.

Created attachment 287953 [details] [review]
Remove quit_cb() and reorder.

Attachment 287940 [details] pushed as 7ee6cbb - Don't remove the view when restarting.
Attachment 287953 [details] pushed as 3762775 - Remove quit_cb() and reorder.

(In reply to comment #49)
> (In reply to comment #48)
> > Not really for “load and sync settings and buttons”: the sync part is buggy,
> > but it’s probably a problem of dconf. Try changing the gsetting why the
> > application is running, and you have a loop of values that bugs the app, quite
> > bad.

So this is the only reason we're keeping this bug open, right? Can we close this enhancement-priority bug and create a new this-is-actually-a-bug-priority bug?

Created attachment 295479 [details]
New mockups

Created attachment 296400 [details] [review]
Implement new design for new game screen

Arnaud, can you review this please? The gsettings hack gave me some trouble and I think I have this working without regressions, but it was... non-intuitive :)

Created attachment 296405 [details] [review]
Add transitions

Here are your transitions again, but with a slightly shorter duration.

Created attachment 296434 [details] [review]
Bump Gtk+ required version to 3.15.0.

I reopen.

Firstly, here are some patches to correct the previous ones:
* the first and the second is just a gtk+ & vala bump and a cleaning in the Makefile;
* the third uses CSS to do the markup (that’s a little bit overkill, I could have done the same using just the GtkBuilder file, but I like having a CSS file =) ) and uses the “homogeneous” property of GtkBox to make button have a consistent size (and without GtkSizeGroup);
* the fourth corrects a graphical “bug” I discovered in Taquin with animations, which have to begin with the border of the window, not after its 25-px margin.

I have by the way two problems with the use of the color instead of a “begins”/“play second”:
* the first one is that it’s not an evidence that “black” means “plays first”, notably because chess uses exactly the opposite rule;
* the second is that we (want to) support themes, and so “black” doesn’t always means “black”; it could be blue with stars or pink.
So I really think this has to be changed back.

Created attachment 296435 [details] [review]
Do not install files used via GResource.

Created attachment 296436 [details] [review]
Use CSS (a little).

Created attachment 296437 [details] [review]
Stack should use all space because of animations.

By the way, I think “num-players” (under another name) should be a boolean, it’d be easier to bind it.

Review of attachment 296434 [details] [review]:

OK

Review of attachment 296435 [details] [review]:

OK

Review of attachment 296436 [details] [review]:

Overkill, but sure

::: src/iagno.vala
@@ +371,2 @@
+        mark_icon_dark.visible = game.current_color == Player.DARK;
+        mark_icon_light.visible = game.current_color == Player.LIGHT;

I like to use unnecessary parentheses here, though.

x = (y == z);

Review of attachment 296437 [details] [review]:

Good catch!

(In reply to Arnaud Bonatti from comment #62)
> I have by the way two problems with the use of the color instead of a
> “begins”/“play second”:
> * the first one is that it’s not an evidence that “black” means “plays
> first”, notably because chess uses exactly the opposite rule;

This is true. I still prefer to let the player pick color, and discover for himself which color moves first, rather than let him pick whether he goes first and discover which color he gets.

> * the second is that we (want to) support themes, and so “black” doesn’t
> always means “black”; it could be blue with stars or pink.
> So I really think this has to be changed back.

Hm, well we use the terms "dark" and "light" to allow for variations in color (as in chess, e.g. chess pieces are often red and cream). I suppose themes will not necessarily honor that, though.

Anyway, if we BOTH want to bring back themes (I thought you were opposed to themes), maybe we should revert the patches that ripped out theme support, which went in after 3.14? We haven't done a stable release without themes yet....

(In reply to Arnaud Bonatti from comment #66)
> By the way, I think “num-players” (under another name) should be a boolean,
> it’d be easier to bind it.

Really? How would it be significantly easier to use? (Fine by me, though.)

(In reply to Michael Catanzaro from comment #71)
> Anyway, if we BOTH want to bring back themes (I thought you were opposed to
> themes), maybe we should revert the patches that ripped out theme support,
> which went in after 3.14? We haven't done a stable release without themes
> yet....

Actually we haven't done any release without themes yet, this next one will be the first without themes. I guess we need to move themes into the app menu, or make a new theme dialog like is used in Mines. Or go back to the old preferences dialog with just options for theme and sound. I probably won't have time to work on this before feature freeze on Monday, so we could release 3.16 off the 3.14 branch if need be to avoid removing theme support if we're just planning to restore it later.

Created attachment 296538 [details]
screenshot with inspector

Allan points out that the button labels are not vertically centered. The problem is more obvious if you use the inspector. But I don't see where the extra padding along the bottom is coming from....

I’ll push some of the (corrected) patches.

> Anyway, if we BOTH want to bring back themes (I thought you were opposed to
> themes), maybe we should revert the patches that ripped out theme support,
> which went in after 3.14? We haven't done a stable release without themes
> yet....

I’m not opposed to themes, just to what they are until now: a superficial change of the pixmaps of the pieces, presented as a little combobox’ preference somewhere lost in a general Preferences window. Either we need a quite complex engine that let themes’ designers make really different things, or it’s better in my opinion to drop completely the support. Themes should be a cool feature, or not be.

I haven’t a strong opinion on if we need themes or not, so was going in long-term with “having them”, including a rewrite of the drawing code. I was planning to push back the support with a new window before the code freeze, as I prefer that than having themes removed and readded the next cycle.

>> By the way, I think “num-players” (under another name) should be a boolean,
>> it’d be easier to bind it.
> 
> Really? How would it be significantly easier to use? (Fine by me, though.)

Well, in fact, looks like some things are not working as simply as I hoped, so that’s not needed. But I conceptually don’t understand why a two-options thing like player_1–player_2 or one_player–two_players in a classic game should be coded on something else than a boolean or an equivalent struct; integers are always source of confusion.

> Allan points out that the button labels are not vertically centered. The
> problem is more obvious if you use the inspector. But I don't see where the
> extra padding along the bottom is coming from....

Didn’t Allan see that in the inspector too? The result is what I want for such a thing as a button: the text is centered on something half between the x-height of the letters. That’s hard to implement when dealing with boxes, so implementations are usually tricky. Here the alignment thing lies in the “text-button” CSS class of the buttons; and well, related on the implementation of GtkLabel, all looks quite logical here to me.

Created attachment 296654 [details] [review]
License under GPLv3.

I’d prefer to have the next (big) patch released under GPLv3(+), so here’s a patch to change the licensing.

Maybe we could just after decide to rename the game to “Reversi”, as I planed to do it when all would be finished, as it’s probably easier to do all the changes of this kind in the same release.

Created attachment 296655 [details] [review]
Themes’ dialog.

Here’s a patch for readding themes. I probably forgot some things here or there in it, but it should at least give an idea of what I have in mind on this subject.

Created attachment 296665 [details] [review]
Themes’ dialog.

Here is an update of the previous patch that should (!) correctly handle the translations of the name of the themes. But I don’t understand why I need all that in the configure.ac file (it’s copied from Evince’s code).

Review of attachment 296654 [details] [review]:

OK

(In reply to Arnaud Bonatti from comment #75)
> Maybe we could just after decide to rename the game to “Reversi”, as I
> planed to do it when all would be finished, as it’s probably easier to do
> all the changes of this kind in the same release.

Go for it.

Review of attachment 296665 [details] [review]:

I don't like that the UI shows two themes, Adwaita and Default, that are the same. Adwaita is probably not a good name for the theme, so let's just go with Default.

I also don't want to use the label Appearance in the app menu for Mines, but Theme in the app menu for Iagno. Can you please chat with Robert (evfool) and pick one to use for both games?

Can you take care of the TODO items in game-view.vala? They look straightforward. (But let's get this in before freeze and leave TODOs for later if need be.)

::: configure.ac
@@ +50,3 @@
 
+m4_pattern_allow([AM_V_GEN])dnl Make autoconf not complain about the rule below
+EV_INTLTOOL_IAGNO_THEME_RULE='%.theme:   %.theme.in   $(INTLTOOL_MERGE) $(wildcard $(top_srcdir)/po/*.po) ; $(AM_V_GEN) LC_ALL=C $(INTLTOOL_MERGE) -d -u -c $(top_builddir)/po/.intltool-merge-cache $(top_srcdir)/po $< [$]@'

Instead of adding the EV_INTLTOOL_IAGNO_THEME_RULE alias here you can just use it directly in the makefile since you only need it in one place. I guess this translates lines in the theme file that start with _

::: data/iagno-themes.ui
@@ +7,3 @@
+    <property name="height-request">450</property>
+    <property name="resizable">False</property>
+    <!-- <property name="modal">True</property> -->

Remove the comment?

(I don't think it needs to be modal.)

@@ +8,3 @@
+    <property name="resizable">False</property>
+    <!-- <property name="modal">True</property> -->
+    <property name="title" translatable="yes">Select theme</property>

Capital T

@@ +77,3 @@
+                              <object class="GtkLabel">
+                                <property name="visible">True</property>
+                                <property name="label">+</property>      translatable="yes"

wat. Oh it's commented out, why?

::: src/iagno.vala
@@ +331,3 @@
+                               "version", VERSION,
+                               "copyright",
+                               "Copyright © 1998–2008 Ian Peters\nCopyright © 2013–2015 Michael Catanzaro",

You've got to add your own name. :)

::: src/themes.vala
@@ +29,3 @@
+
+    [GtkChild]
+    private ListBox listbox;

GtkTemplates are pretty nice :)

@@ +54,3 @@
+                    key.load_from_file (Path.build_filename (DATA_DIRECTORY, "themes", "key", filename), GLib.KeyFileFlags.NONE);
+                }
+                catch (GLib.KeyFileError e) { warning ("oops: %s", e.message); }

I don't really like putting this all on one line

Created attachment 296727 [details] [review]
Themes’ dialog.

Here is a little update, that includes sound management by the themes.

> I don't like that the UI shows two themes, Adwaita and Default, that
> are the same. Adwaita is probably not a good name for the theme,
> so let's just go with Default.

I renamed “Default” in “Let me Pick One”, and I think it’s good. We need these two themes, because “Adwaita” is fixed when the other depends on Gtk settings (notably for high contrast…).

> I also don't want to use the label Appearance in the app menu for Mines,
> but Theme in the app menu for Iagno.

I’ll see with him.

> Can you take care of the TODO items in game-view.vala? They look
> straightforward. (But let's get this in before freeze and
> leave TODOs for later if need be.)

Cleaned some, others should be done before the freeze, but it’s not sooo important.

>> +    <property name="title" translatable="yes">Select theme</property>
> 
> Capital T

Changed to “Theme”.

>> +               <property name="label">+</property>      translatable="yes"
> 
> wat. Oh it's commented out, why?

Because the code for managing that doesn’t exist for now. ^(^’ It’s removed.

>> "Copyright © 1998–2008 Ian Peters\nCopyright © 2013–2015 Michael Catanzaro",
> 
> You've got to add your own name. :)

Done there. =)

> GtkTemplates are pretty nice :)

Yeps, I’ll play with that now.

Review of attachment 296727 [details] [review]:

OK, commit after these tweaks:

::: data/iagno-themes.ui
@@ +52,3 @@
+                              <object class="GtkLabel">
+                                <property name="visible">True</property>
+                                <property name="label" translatable="yes">Let me Pick One</property>

Ah, no, switch it back to Default please. That did not translate as well as you wanted it to. :p

::: src/iagno.vala
@@ +570,3 @@
+            case Sound.FLIP     : name = view.sound_flip     ; break;
+            case Sound.GAMEOVER : name = view.sound_gameover ; break;
+            default: return;

Well please match the style of the rest of the project:

switch (sound)
{
case Sound.FLIP:
    name = view.sound_flip;
    break;
// ...
default:
    return;
}

End-of-line opening braces are only used for lambdas

Also let's finally close this bug when you've committed this patch, since we're no longer working on the new game screen at all, and open a new bug if you have more. :)

Not sure I won’t touch the new-game screen until Monday’s freeze, but let’s close here. I’ve pushed the two remaining patches (GPL v3 & themes’ dialog).