Created attachment 347400 [details] [review]
printers: Move options dialog spinner out of the action area

For an infinitesimal amount of time after opening the options dialog, a
spinner is displayed at the start of the action area while the dialog
loads asynchronously. Display the spinner in the center of the dialog
instead, using a GtkStack to switch between spinner mode and normal
mode. Test by removing the calls to printer_get_ppd_async(),
get_named_dest_async(), and get_ipp_attributes_async() from the bottom
of populate_options().

Why? (1) It looks better this way. (2) Need to stop using the action
area in order to switch to a header bar.

https://bugzilla.gnome.org/show_bug.cgi?id=755713

Created attachment 347401 [details] [review]
printers: Drop PpOptionsDialog action_area

There's no need for the dialog action_area since we are moving
towards a header bar dialog.

Created attachment 347402 [details] [review]
printers: Use headerbar in PpOptionsDialog

Created attachment 347403 [details] [review]
printers: Add print "Test Page" button to Options Dialog

Hi, I've tested the patches today but I've got crash here. It is enough to open the options dialog, press "Test Page" and close it and do it once again.
It seems that once the print job is sent to CUPS, the list of PpPrinterEntries is actualized and a callback (printer_get_ppd_cb()) gets pointer to already freed PpPrinterEntry.

These seem to be solved after Bug 779846.

Review of attachment 347400 [details] [review]:

Thank you for the patch. There is just one minor request from me before commit.

::: panels/printers/pp-options-dialog.c
@@ +795,1 @@
   gtk_widget_show (widget);

This is not needed now since the spinner is visible.

Review of attachment 347400 [details] [review]:

Thank you for the patch. There is just one minor request from me before commit.

::: panels/printers/pp-options-dialog.c
@@ +795,1 @@
   gtk_widget_show (widget);

This is not needed now since the spinner is visible.

Review of attachment 347401 [details] [review]:

This looks good to me.

Review of attachment 347402 [details] [review]:

This one too.

Thanks for addressing these patches!

Attachment 347400 [details] pushed as 50160be - printers: Move options dialog spinner out of the action area
Attachment 347401 [details] pushed as ca2d97f - printers: Drop PpOptionsDialog action_area
Attachment 347402 [details] pushed as 00a45b3 - printers: Use headerbar in PpOptionsDialog

I'll continue on the last one tomorrow.

Review of attachment 347403 [details] [review]:

Thank you for the patch but I think that since we are modifying this part we should rewrite the printing in async way.

::: panels/printers/options-dialog.ui
@@ +20,3 @@
+          <object class="GtkButton" id="print-test-page">
+            <property name="visible">True</property>
+            <property name="label" translatable="yes">Test Page</property>

Add a comment for translators please.

::: panels/printers/pp-options-dialog.c
@@ +66,3 @@
   gboolean sensitive;
+
+  GCancellable *print_test_page_cancellable;

Lets not use cancellable. User just wants to hit the button and see the printout.

@@ +920,3 @@
+            }
+
+          http = httpConnectEncrypt (cupsServer (), ippPort (), cupsEncryption ());

Lets make this async. Something like pp_printer_print_file_async() and pp_printer_print_file_finish(). You can hide the whole request in a threaded GTask together with the getting of "printer-type" option.

Created attachment 352943 [details] [review]
printers: Introduce pp_printer_print_file_async

An asynchronous wrapper to print files.

Created attachment 352944 [details] [review]
printers: Add print "Test Page" button to Options Dialog

Review of attachment 352943 [details] [review]:

Looks good, just one minor request.

::: panels/printers/pp-printer.c
@@ +551,3 @@
+      ippAddString (request, IPP_TAG_OPERATION, IPP_TAG_NAME,
+                    "job-name", NULL, print_file_data->job_name);
+      response = cupsDoFileRequest (http, request, resource, print_file_data->filename);

Use CUPS_HTTP_DEFAULT instead of the "http". You don't need to connect "manually" then.

Review of attachment 352944 [details] [review]:

Thank you for the modifications. I have just 2 free requests.
Maybe one other thing we should consider is whether to make the button insensitive if the printer is rejecting jobs (in another patch).

::: panels/printers/pp-options-dialog.c
@@ +852,3 @@
+{
+  pp_printer_print_file_finish (PP_PRINTER (source_object),
+                                result, NULL);

Unref the source object please.

@@ +881,3 @@
+        }
+
+      if (filename)

Free the filename in this branch please (and check it against NULL here :) ).

Created attachment 353319 [details] [review]
printers: Introduce pp_printer_print_file_async

An asynchronous wrapper to print files.

Created attachment 353320 [details] [review]
printers: Add print "Test Page" button to Options Dialog

Review of attachment 353319 [details] [review]:

Thank you for the modification.

Review of attachment 353320 [details] [review]:

This also looks good to me. Thank you for the patches.

Thanks for your reviews!

Attachment 353319 [details] pushed as 995d642 - printers: Introduce pp_printer_print_file_async
Attachment 353320 [details] pushed as ee42831 - printers: Add print "Test Page" button to Options Dialog