Created attachment 297917 [details] [review]
Fix spinner to start three seconds after Print button is pressed

Review of attachment 297917 [details] [review]:

OK, great!

::: src/print-dialog.vala
@@ +21,3 @@
 
+using GLib;
+using Gtk;

using GLib is implicit. using Gtk just makes it harder to read code that uses GTK+ widgets, IMO. So I'd rather not add these.

@@ +27,3 @@
 {
     private SudokuSaver saver;
+    private GLib.Settings settings;

Should not need to change this line if you remove 'using Gtk'

@@ +40,3 @@
     private Gtk.RadioButton very_hard_radio_button;
 
+    private Gtk.Revealer revealer;

Why did you add the 'using Gtk' if you're going to fully-qualify it everywhere? :)

@@ +60,3 @@
         spinner = new Gtk.Spinner ();
+        revealer = new Gtk.Revealer ();
+        revealer.add (spinner);

Ah, that was a good idea. I think I tried this in the past and had a problem with the contents of the header bar shifting when the spinner was set visible. I guess Revealer avoids that, right?

@@ +83,2 @@
     {
         // The spinner still has a floating reference if it wasn't added to the header bar.

Update the comment to mention the revealer too!

@@ +103,3 @@
+        revealer.set_reveal_child (true);
+
+        return false;

You should return Source.REMOVE rather than false.

As a style nit, I don't think I'd add a blank line here when the function is otherwise all one block, but up to you.

Created attachment 298011 [details] [review]
Fix spinner to start three seconds after Print button is pressed

Created attachment 298012 [details] [review]
Fix spinner to start three seconds after Print button is pressed

Removes blank line

Created attachment 298014 [details]
Screencast

> @@ +60,3 @@
>          spinner = new Gtk.Spinner ();
> +        revealer = new Gtk.Revealer ();
> +        revealer.add (spinner);
> 
> Ah, that was a good idea. I think I tried this in the past and had a problem
> with the contents of the header bar shifting when the spinner was set
> visible. I guess Revealer avoids that, right?
> 

The title still shifts to left but it looks good if it shifts at the same time as the animation. I can probably make it stay fixed if you want (you know better as you have more experience with other modules and the behavior of their spinners etc). It won't probably ever show unless you print a really high number of puzzles.

Review of attachment 298012 [details] [review]:

Ah, I didn't have the animation. Thanks!

Attachment 298012 [details] pushed as 0fb636a - Fix spinner to start three seconds after Print button is pressed

I only fixed one part of the bug, it should remain open. 

Things left to fix:
* The dialog should not be insensitive after clicking Print. Instead, only the Print button should be insensitive. Clicking Cancel should cancel the operation.

I told myself to remember to leave this open... but did I? Of course not.

Created attachment 299251 [details] [review]
Fix dialog being insensitive after clicking Print

I'm not sure how to cancel the generate_boards operation and print dialog opening. I tried to end the async method but I'm not sure what parameters should I give to end () for that to happen.

Also, clicking Cancel while generating should close the dialog or just stop operation?

(In reply to Iulian Radu from comment #10)> 
> I'm not sure how to cancel the generate_boards operation and print dialog
> opening. I tried to end the async method but I'm not sure what parameters
> should I give to end () for that to happen.

Async functions are a bit confusing if you haven't worked with them before. (Actually, I have to look at the documentation each time I use them: see https://wiki.gnome.org/Projects/Vala/Tutorial#Asynchronous_Methods) You should only attempt to call .end() from the callback function that you pass as the last parameter to the async function, which the async function will call when it completes. In this case, it's the big lambda at the bottom of print-dialog.vala. The only parameter you can pass is the AsyncResult res that gets passed by the async function to the lambda. Using .end() converts the AsyncResult to a return value.

To cancel the async function early, before it completes, you will want to modify generate_boards_async to receive a Cancellable as a parameter. Traditionally it should be a nullable Cancellable, and if passed null the function just cannot be canceled. Then with the Cancellable you connect to the cancel signal (using the connect() function rather than connecting directly, to avoid a race since you have multiple threads) and stop doing work if that gets fired.

See:
http://valadoc.org/#!api=gio-2.0/GLib.Cancellable
http://valadoc.org/#!api=gio-2.0/GLib.AsyncResult

In particular: "The callback for an asynchronous operation is called only once, and is always called, even in the case of a cancelled operation. On cancellation the result is a g_io_error_cancelled error."

There is a bit more to it... I will let you investigate. :)

> Also, clicking Cancel while generating should close the dialog or just stop
> operation?

It should close the dialog (and also stop the operation)

Created attachment 299446 [details] [review]
Fix dialog being insensitive after clicking Print

Your comment helped a lot. I managed to get it work really fast but I couldn't connect on the cancel button's 'clicked' for some reason. Nothing was happening when clicking cancel, except for the Dialog being destroyed. Instead, I canceled/stopped the method when the Dialog was destroyed.

Review of attachment 299446 [details] [review]:

Well, this doesn't work, because nowhere do you stop the sudoku generation, but like I said it's confusing, so nice try. :p

The first thing I'll say is that there's no point in passing the Cancellable to the async function if you're not going to use it within the function. Within generate_boards_async (not PrintDialog) is where you want to use cancellable.connect(). That's where you stop the thread pool from performing further work. But actually, it looks like there is no way to stop the thread pool right now. I see this in sudoku-generator.c:

#define _g_thread_pool_free0(var) ((var == NULL) ? NULL : (var = (g_thread_pool_free (var, FALSE, TRUE), NULL)))

But we need to do g_thread_pool_free (var, TRUE, FALSE). So I guess we should file a bug against vala requesting a way to stop the thread pool, and in the meantime just pretend that we have canceled the Sudoku generation. Remember that "On cancellation the result [of the async function] is a g_io_error_cancelled error" so you'll want to call set_error_if_cancelled after the yield before returning; that will (presumably) throw the I/O error. (That's a really terrible name for the function; I would have named it throw_error_if_cancelled in the vapi....)

Then mark generate_boards_async with "throws IOError", surround the call to generate_boards_async.end with a try/catch to handle the IOError, check if it's IOError.Cancelled, and handle that appropriately.

P.S. I haven't tried any of this, so I hope I got that right. :)

::: lib/sudoku-generator.vala
@@ +81,3 @@
     }
 
+    public async static Gee.List<SudokuBoard> generate_boards_async (int nboards, DifficultyCategory category, Cancellable cancellable) throws ThreadError

This should take a Cancellable? so that it's not mandatory. Then you'll want to... use it. :p

::: src/gnome-sudoku.vala
@@ +406,3 @@
         back_button.sensitive = false;
+        var cancellable = new Cancellable ();
+        SudokuGenerator.generate_boards_async.begin (1, selected_difficulty, cancellable, (obj, res) => {

Here you should pass null instead of the cancellable, since you don't ever intend to cancel the operation.

::: src/print-dialog.vala
@@ +58,3 @@
 
+        this.destroy.connect (() => {
+            cancellable.cancel ();

This is actually pretty close to what I would recommend. Instead of using destroy, though, connect to response instead, and make sure the response type is CANCEL or DELETE_EVENT before canceling cancellable. That would be better than trying to connect to the clicked signal of the button, so that you don't miss delete events (the Escape key).

@@ +147,3 @@
+                cancellable.connect (() => {
+                    try {
+                        boards = SudokuGenerator.generate_boards_async.end (res);

By this point, generate_boards_async has already completed and you've already got the result in boards. Remember that the function passed to generate_boards_async gets called when it is completed, and generate_boards_async.end just retrieves the result (and does not stop it).

Created attachment 299467 [details] [review]
Fix dialog being insensitive after clicking Print

A step forward, but... still not sure:
1) If should still connect to the cancelled signal (not sure what to do if I can't stop the ThreadPool)
2) What should be handled when catching the exception (in print-dialog.vala if I do nothing 'finished()' signal will be emitted and the end of the function will be reached)

Review of attachment 299467 [details] [review]:

(In reply to Iulian Radu from comment #14)
> A step forward, but... still not sure:

A big step forward; this looks much better!

> 1) If should still connect to the cancelled signal (not sure what to do if I
> can't stop the ThreadPool)

Actually you're right, you don't need to do this until we have a way to stop the thread pool. I filed Bug #746262.

The current result is non-ideal in that the Cancel button will not be responsive until all the threads have completed, but there's no reasonable way to handle that without a way to stop the thread pool. So I think the current code is fine.

> 2) What should be handled when catching the exception (in print-dialog.vala
> if I do nothing 'finished()' signal will be emitted and the end of the
> function will be reached)

If it's canceled, just don't open the GtkPrintDialog. :)

::: src/gnome-sudoku.vala
@@ +416,1 @@
             }

I don't like empty catch blocks. Even though you shouldn't reach this, since it's an empty Cancellable, the function says "throws IOError" so we should expect any generic IOError. Maybe catch (Error e) and remove "Thread error:" from the error message.

::: src/print-dialog.vala
@@ +165,3 @@
+            {
+                if (e is IOError.CANCELLED)
+                {

How about: if e is NOT IOError.CANCELLED, print a warning with the error message. (If it is IOError.CANCELLED, then nothing is wrong, so do nothing.)

(In reply to Iulian Radu from comment #14)
> 2) What should be handled when catching the exception (in print-dialog.vala
> if I do nothing 'finished()' signal will be emitted and the end of the
> function will be reached)

That's right -- you always want to emit finished. gnome-sudoku.vala checks that to know when to destroy the dialog. But maybe we could get rid of this signal somehow by having the dialog destroy itself, then gnome-sudoku.vala can use the destroy signal instead....

Created attachment 299519 [details] [review]
Fix dialog being insensitive after clicking Print

All seems to be good this time.

Review of attachment 299519 [details] [review]:

OK, this looks nearly good to me.

* Although I don't want the entire dialog to be insensitive after clicking Print, I do want the Number of puzzles and Difficulty to both be insensitive. That is, I want only the Cancel button to be insensitive. (Since it would be confusing to allow changing those after you've clicked print. What would you expect that to do?)
* It would really be better as two separate patches: one that does not remove the finished signal, and then a second that removes finished and starts using destroy instead. But I'll leave it up to you to decide whether you want to do the work to split it or not.

P.S. For some reason, QQwing is much slower to generate Hard puzzles than any of the other difficulties. Testing 100 Hard puzzles might help if your computer is too fast. I just noticed/remembered that now, or I'd have mentioned it earlier.

(In reply to Michael Catanzaro from comment #18)
> That is, I want only the Cancel button to be insensitive.

...to be sensitive (not insensitive)

Created attachment 299575 [details] [review]
Keep the Cancel button sensitive after clicking Print

Created attachment 299576 [details] [review]
Remove finished signal

Review of attachment 299575 [details] [review]:

::: src/print-dialog.vala
@@ +144,3 @@
+        medium_radio_button.sensitive = false;
+        hard_radio_button.sensitive = false;
+        very_hard_radio_button.sensitive = false;

You can set insensitive just print_button and the box they are all in, instead of each individually. It's named print_box in the UI file.

Review of attachment 299576 [details] [review]:

Looks good

Created attachment 299578 [details] [review]
Keep the Cancel button sensitive after clicking Print

print_box only contains the spin button and it's label. vbox1 contains the print_box and the difficulty_box. Weird naming.

I could rename it this way if you want:
dialog-vbox1 -> dialog_print_box
vbox1 -> print_box
print_box -> n_sudokus_box

Um, probably best to just remove the names from everything except the box you need... once upon a time each element in a UI file had to have a name, so we gave them stupid serialized names like vbox1 vbox2, but nowadays they're optional.

Created attachment 299584 [details] [review]
Rename UI elements creating confusion

Created attachment 299585 [details] [review]
Keep the Cancel button sensitive after clicking Print

Gracias

Attachment 299584 [details] pushed as bfa1956 - Rename UI elements creating confusion
Attachment 299585 [details] pushed as 5c73b73 - Keep the Cancel button sensitive after clicking Print

Created attachment 299727 [details] [review]
Stop generating sudokus when cancelled

Bug #746262 adds API we can use to actually stop the thread pool.

Attachment 299727 [details] pushed as d446a89 - Stop generating sudokus when cancelled