Created attachment 281346 [details] [review]
Use an AspectFrame to ensure all looks good.

Review of attachment 281345 [details] [review]:

k

Review of attachment 281346 [details] [review]:

So the difference is the placement of the window buttons.  I think I do like your version better.

Parin, do you agree? Please handle Arnaud's response and commit if you like it. You're more than ready to start reviewing others' patches.

::: data/gnome-sudoku.ui
@@ +4,3 @@
     <object class="GtkApplicationWindow" id="sudoku_app">
         <property name="title" translatable="yes">Sudoku</property>
+        <property name="border-width">25</property>

Maybe a little bit less would look better with smaller windows? (20?)

::: src/gnome-sudoku.vala
@@ +119,3 @@
+        {
+            /* Pixel-perfect compatibility with games that have a Button without ButtonBox. */
+            var data = """GtkButtonBox { -GtkButtonBox-child-internal-pad-x:0; }""";

* Do you really need the triple quotes? Those shouldn't be necessary since the string fits on one line, right?

Before committing, please wait until we all agree on the Iagno’s tweaks’ bug [1], I’ll post an update here, so the two games will have exactly the same behaviour (except the minimum size of the window).

By the way, the lines between the cells are too small, so/and they disappear on my screen when I resize the window. Do you have the same problem (on lower resolutions) ? And of course this patch, with its complex layout, doesn’t help, so I’m looking for a solution.

I’ll try with the border-width. And I used the triple quotes for the case where the CSS will contain a quote character, but as there isn’t, it’s technically unuseful.

[1] https://bugzilla.gnome.org/show_bug.cgi?id=733521

(In reply to comment #4)
> By the way, the lines between the cells are too small, so/and they disappear on
> my screen when I resize the window. Do you have the same problem (on lower
> resolutions) ? And of course this patch, with its complex layout, doesn’t help,
> so I’m looking for a solution.

No, I've never seen them disappear. :/ Allan requested thin lines but this sounds like a bad problem (for a different bug report).

(In reply to comment #5)
> No, I've never seen them disappear. :/ Allan requested thin lines but this
> sounds like a bad problem (for a different bug report).

Unless you mean that the black border (of the same width) between 3x3 regions disappears when maximized -- I am seeing this.

Also, bug #710626 is related

> Unless you mean that the black border (of the same width) between 3x3 regions
> disappears when maximized -- I am seeing this.
Yes it’s that, but it’s not a maximizing-only problem here: it is caused by the approximations of the computer (calculations and have-to-fit-to-a-pixel) when drawing the board’s tiles, so it is common on big resolution with a big board, and having a complex layout (with more calculations…) probably makes the problem more important (that’s why I mentionned it here).

Created attachment 281402 [details] [review]
Use an AspectFrame to ensure all looks good.

Here’s the update, to go with my second proposition (id=281401) on Iagno’s bug [1].

[1] https://bugzilla.gnome.org/show_bug.cgi?id=733521#c4

(In reply to comment #8)
> Created an attachment (id=281402) [details] [review]
> Use an AspectFrame to ensure all looks good.

Hi Arnaud, can you please rebase your patch with the current master ?
Patch fails to apply in gnome-sudoku.vala -
error: patch failed: src/gnome-sudoku.vala:173
error: src/gnome-sudoku.vala: patch does not apply

Comment on attachment 281345 [details] [review]
Correct indentation.

Pushed as 1c79ccd

Created attachment 281682 [details] [review]
Use an AspectFrame to ensure all looks good.

Created attachment 281693 [details]
Screenshot showing reduced board size and spacing inconsistency

(In reply to comment #11)
> Created an attachment (id=281682) [details] [review]
> Use an AspectFrame to ensure all looks good.

I like the changes this patch has made. Having Iagno and Sudoku follow the same behaviour on resize is good, and much desirable.

But the new UI is different than Allan's mockup (https://raw.github.com/gnome-design-team/gnome-mockups/master/games/sudoku/sudoku.png).
- Default size has inconsistent spacing. 'New Puzzle' and 'Clear Board' buttons are not centered in the right sidebar, and board has more margin on top/bottom than left/right.
Iagno does not have this problem, since the default size is such that all the spacings are consistent. 'Start Over' button is centered in the right sidebar, and the board has equal margins for all sides. Only when it is resized to sudoku, the inconsistency arises.
Allan's mockup had consistent spacing all over, and I think we should not remove that.

- Default board size has been reduced considerably.
Iagno board cells do not have to show any text, and so the small size of the board is okay.
But since we have to show value and earmarks, I think the size of the board should be larger.

Created attachment 281722 [details] [review]
Use an AspectFrame to ensure all looks good.

> But the new UI is different than Allan's mockup
> - Default size has inconsistent spacing. 'New Puzzle' and 'Clear Board' buttons
> are not centered in the right sidebar, and board has more margin on top/bottom
> than left/right.
>
> Iagno does not have this problem, since the default size is such that all the
> spacings are consistent. 'Start Over' button is centered in the right sidebar,
> and the board has equal margins for all sides. Only when it is resized to
> sudoku, the inconsistency arises.
> Allan's mockup had consistent spacing all over, and I think we should not
> remove that.
Allan’s mockup(s) is (are usually) quite cool for planning the visible features of the main window, but that doesn’t mean they show all the things. Playing maximized on a 16:9 (touch)screen is really fun, but you don’t want your board to be on the left, and you don’t want your “new puzzle” button to be the quarter of the width of your screen with a ridiculous height; at the opposite, playing on a small screen / with a little window constrain a compactable layout. That’s why this AspectFrame is really important, and that’s why I prefer working first on a layout that works correctly for more than one game, than to directly implement a mockup.

This update of the patch should correct the bugs of the previous, related to the spacings and margins on a small window.

> Iagno board cells do not have to show any text, and so the small size of the
> board is okay.
> But since we have to show value and earmarks, I think the size of the board
> should be larger.
The board should be able to reduce, to ensure we could play on every screen; it’s completely readable on the actual smaller size. Not sure it’s necessary to default on a larger window, but I don’t have a strong opinion on that. More important is that the app remembers the size of the window, and that’s the goal of the next patch.

Created attachment 281723 [details] [review]
Remember window size and state.

Created attachment 281724 [details] [review]
Notations.

A notations patch I already submitted elsewhere, but the faster it is applied, the better I could work on top of it. ^^

Review of attachment 281722 [details] [review]:

This is much better, very close to what I had in mind.
Please make the changes I've described below, and I'll commit this patch.

- The default size is very less, and it makes the earmarks almost unreadable. Please increase the size request so that the board looks bigger, and the game window occupies around 665 x 570 px - http://i.imgur.com/HPtMD3z.png , or some near round figure.

::: src/sudoku-view.vala
@@ -183,3 @@
 
-    public override void get_preferred_width (out int minimal_width, out int natural_width)
-    {

This patch is only for replacing the game box with AspectFrame, right ?
So, this function should not be removed here. Include the removal in your Notations patch.

@@ -191,3 @@
-
-    public override void get_preferred_height (out int minimal_height, out int natural_height)
-        side = width > height ? width : height;

Same with this one.

Review of attachment 281724 [details] [review]:

Thanks for working on this, Arnaud !

This patch is good, and is almost fit to be merged.
Just two things -
- Please include the get_preferred_width/height functions removal in this patch.
- "Notations" doesn't sound much informative. Can you please change the commit message to reflect the corrections you've done to code style, and cleanup ?

That's all :-)

Review of attachment 281723 [details] [review]:

Please change the default width/height, and this patch will be ready to be committed.

Comment on attachment 281722 [details] [review]
Use an AspectFrame to ensure all looks good.

Increased size request from 350 to 480 and pushed as 72266fe

Comment on attachment 281723 [details] [review]
Remember window size and state.

Changed default size from 540x500 to 730x630 and pushed as ccb67c0

Comment on attachment 281724 [details] [review]
Notations.

Changed commit message to "Correct code style and clean-up unused code" and pushed as 73d58d1

Created attachment 283591 [details] [review]
Adjust default and minimum sizes.

Sorry for being late, I had a problem of stolen computer so I spent the end of my holidays in real life instead of on Internet. ^^

I strongly suggest changing at least the default size of the window, to fit naturally in 800~960 × 600 screens (with a panel), like some netbooks’ one… and really don’t see why setting a (big) minimum size, as it’s definitively readable when the board requests 350 × 350, and as long as it remains a user’s choice. See the patch.

Thanks for the pushs and the other things. =)

Why 707 and not 700?

> Why 707 and not 700?
To avoid extra top and bottom margins around the board. I choosed the 580 height as explained, and selected the minimal width for the window that permits that the board has no extra margin.