When implementing this design, we decided to reverse the order of the difficulty levels such that Easy appears on top and Very Hard on the bottom.

Created attachment 278085 [details] [review]
Simplify Print Multiple Puzzles dialog

Some clarifications -
- I've set SUDOKUS_PER_PAGE to 2.

- Since we're now using radiobuttons, I've removed the 4 gsettings keys - 'print_easy', 'print_medium', ... which store whether the corresponding checkbox is active or not.
Instead a single key 'print-multiple-sudoku-difficulty' will store the difficulty selected, as string - 'easy', 'medium', ... .

- The two options removed in the new design - 'Include old games' and 'Mark as played' have been set to TRUE in the code. (i.e. - previously played puzzles will be included in printing (we can change this if needed when the generator is fixed), and the puzzles which have been printed will be mark as finished.)

and sorry for the massive patch.

Implementing the new mockup seemed logical to do in one patch, and due to the .ui file cleanup, the size increased a lot.

Review of attachment 278085 [details] [review]:

Looks quite good, as usual. Thanks Parin!

Somewhat related: Can we center the puzzles horizontally on the page? I think there should also be slightly more vertical space between the two puzzles. We might have to shrink them slightly to achieve this. (If you don't want to fix those as part of this bug, please file a new one.)

The only issue I see with the dialog is that the content is not quite centered. If you look at Allan's mockup, his is a bit wider than yours, with more unused space off to the right, which allows you to center the radio buttons.

(In reply to comment #2)
> Created an attachment (id=278085) [details] [review]
> - The two options removed in the new design - 'Include old games' and 'Mark as
> played' have been set to TRUE in the code. (i.e. - previously played puzzles
> will be included in printing (we can change this if needed when the generator
> is fixed)

Indeed, that ought to be false rather than true.

::: data/print-games.ui
@@ +114,3 @@
+                    <property name="valign">start</property>
+                    <property name="label" translatable="yes">_Difficulty </property>
+                    <property name="use_underline">True</property>

This label actually should not have a mnemonic, since it doesn't map to anything.

Review of attachment 278085 [details] [review]:

I see that we generally need to rename DifficultyCatagory to DifficultyCategory (and in some instances wrong variable names) to remove this typo. You might want to change that at some point.

::: data/org.gnome.sudoku.gschema.xml
@@ +50,3 @@
       <description>Height of application window in pixels</description>
     </key>
+    <key name="print-multiple-sudoku-difficulty" type="s">

I would like to have this as an int and refer to the Difficulty enum already in sudokuSolver (you might have to move that but it would be nice if we had it centralized and as a proper type).

::: src/sudoku-printer.vala
@@ +49,3 @@
     {
+        int remainder = (n_sudokus % SUDOKUS_PER_PAGE) == 0 ? 0 : 1;
+        operation.set_n_pages ((n_sudokus / SUDOKUS_PER_PAGE) + remainder);

this would be more readable with
int pages = (n_sudokus / SUDOKUS_PER_PAGE);
while(pages*SUDOKUS_PER_PAGE < n_sudokus) {
 // add overflow page
 pages += 1;
}
We don't need to do any brainwork with the remainder of the division here and the while-loops makes the intention clear.

@@ +63,3 @@
 
+        var start = page_nr * SUDOKUS_PER_PAGE;
+        var end = (start + SUDOKUS_PER_PAGE) > boards.length ? boards.length : (start + SUDOKUS_PER_PAGE);

this is actually min(start+SUDOKUS_PER_PAGE,boards.length);
That should be clearer. We want start+SPP or boards.length, whatever is smaller.

@@ +322,1 @@
+        if (easy_button.get_active ())

This should be nicer with the enum in place.

@@ +350,3 @@
         {
             dialog.hide ();
+            foreach (SudokuBoard i in boards)

The naming ;)

::: src/sudoku-store.vala
@@ +110,3 @@
     }
 
+    public SudokuBoard[] get_sorted_boards(int n, DifficultyCatagory level, bool exclude_finished = false)

I think we should rename that to get_boards_sorted since we get a number of boards in a sorted fashion instead of getting sorted boards. Also "n" is a bad name. "Amount" or "board_count" or "number_of_boards" would be better.
And please add a comment before the method that states by which property and in which order (ascending or descending - and what that means, e.g. easy to hard or other way?) the boards are sorted.

@@ +117,1 @@
+        for (var i = 0; i < n; i++)

I am not sure about this. I think we expect, if we say we want n boards, to get n boards. So I think while len(boards) < n is more appropriate. This also gives us a nice invariant.

I don't know how this fits with puzzle generation because this leaves us open for an infinite loop if no puzzles are available/generated but we want to remove those issues anyways.

@@ +131,3 @@
         });
+
+        foreach (SudokuBoard i in boards)

If it isn't an integer, we do not name it i. People get really confused when the longest side of a triangle isn't named c. I think "board" is appropriate.

And what Michael says ;)

> ::: data/org.gnome.sudoku.gschema.xml
> @@ +50,3 @@
>        <description>Height of application window in pixels</description>
>      </key>
> +    <key name="print-multiple-sudoku-difficulty" type="s">
> 
> I would like to have this as an int and refer to the Difficulty enum already in
> sudokuSolver (you might have to move that but it would be nice if we had it
> centralized and as a proper type).

I'd initially set it to int, but having a string makes it easier to set the saved setting active.
The saved strings are - 'easy', ... 'very_hard' and since the button id prefixes are 'easyRadioButton', ... 'very_hardRadioButton', the code required to set it active gets reduced to one line -
settings.get_string (DIFFICULTY_KEY_NAME) + "RadioButton"

Having enum would need its own control flow to achieve the same.

However if you feel having an enum would be better, let me know, I'll update the patch :-)
I've corrected rest of the mistakes you pointed out.

As we talked in IRC:

Purely from the design perspective, we want to have proper types at all time and only convert them to string when needed.
Also, if done properly, we do not need to know which specific saved value corresponds to which difficulty. We don't really want to know that.

Created attachment 278118 [details] [review]
Simplify Print Multiple Puzzles dialog

Patch updated.

I'll file a bug to center puzzles horizontally and adjust their sizes.
Michael's suggestions will be implemented as a separate patch.

Review of attachment 278118 [details] [review]:

This looks very good. The enum seems to work fine, good work. There are only very few outstanding issues and we're good here.

I marked the patch as accepted but please look at the two or three remaining issues and only then push it to master.

Can you, inbetween the actual tasks, fix the naming of DifficultyCatagory? I think we can manage that one without a review.

::: src/sudoku-printer.vala
@@ +50,3 @@
+        int pages = n_sudokus / SUDOKUS_PER_PAGE;
+        if ((n_sudokus % SUDOKUS_PER_PAGE) != 0)
+            pages += 1;

Again with the modulo-operation. But alright, if you really like it that way ;)

@@ +67,3 @@
+        var start = page_nr * SUDOKUS_PER_PAGE;
+        // minimum of (start + SUDOKUS_PER_PAGE) and boards.length
+        var end = (start + SUDOKUS_PER_PAGE) > boards.length ? boards.length : (start + SUDOKUS_PER_PAGE);

I meant, that this could be written as
var end = int.min(start + SUDOKUS_PER_PAGE, boards.length);
which is much clearer. There is almost never a need for the ternary if and we would only use it, if there is no cleaner expression.

Please change that :)

::: src/sudoku-store.vala
@@ +110,3 @@
     }
 
+    // Get boards sorted ascending based on difficulty rating

If you can add here, what exactly that means, we're good. Do they get increasingly harder or increasingly easier?

Here's a link to the min-method:
http://valadoc.org/#!api=glib-2.0/int.min

And I am sorry that I don't like modulo but we really don't need it here and we're actually checking

if pages*SUDOKUS_PER_PAGE is less than n_sudokus or not. I do know that having the remainder of the division nonzero is enough to be certain that there is a page missing, but if we don't want to include the proof in the comment, we can be more precise on what we are actually checking.

> Can you, inbetween the actual tasks, fix the naming of DifficultyCatagory? I
> think we can manage that one without a review.

sure
 
> ::: src/sudoku-printer.vala
> @@ +50,3 @@
> +        int pages = n_sudokus / SUDOKUS_PER_PAGE;
> +        if ((n_sudokus % SUDOKUS_PER_PAGE) != 0)
> +            pages += 1;
> 
> Again with the modulo-operation. But alright, if you really like it that way ;)

corrected it.

> @@ +67,3 @@
> +        var start = page_nr * SUDOKUS_PER_PAGE;
> +        // minimum of (start + SUDOKUS_PER_PAGE) and boards.length
> +        var end = (start + SUDOKUS_PER_PAGE) > boards.length ? boards.length :
> (start + SUDOKUS_PER_PAGE);
> 
> I meant, that this could be written as
> var end = int.min(start + SUDOKUS_PER_PAGE, boards.length);
> which is much clearer. There is almost never a need for the ternary if and we
> would only use it, if there is no cleaner expression.
> 
> Please change that :)

done.

> ::: src/sudoku-store.vala
> @@ +110,3 @@
>      }
> 
> +    // Get boards sorted ascending based on difficulty rating
> 
> If you can add here, what exactly that means, we're good. Do they get
> increasingly harder or increasingly easier?

added the comment -
// Get boards sorted ascending based on difficulty rating
// i.e. - the first board returned will be the easiest, and boards will become increasingly harder

Pushed the patch as 9c17bef