After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 779708 - Add print "Test Page" button in the printer options dialog
Add print "Test Page" button in the printer options dialog
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Printers
git master
Other Linux
: Normal normal
: ---
Assigned To: Marek Kašík
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-03-07 15:29 UTC by Felipe Borges
Modified: 2017-06-14 08:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
printers: Move options dialog spinner out of the action area (12.10 KB, patch)
2017-03-07 15:30 UTC, Felipe Borges
committed Details | Review
printers: Drop PpOptionsDialog action_area (1.69 KB, patch)
2017-03-07 15:30 UTC, Felipe Borges
committed Details | Review
printers: Use headerbar in PpOptionsDialog (1012 bytes, patch)
2017-03-07 15:30 UTC, Felipe Borges
committed Details | Review
printers: Add print "Test Page" button to Options Dialog (8.65 KB, patch)
2017-03-07 15:30 UTC, Felipe Borges
none Details | Review
printers: Introduce pp_printer_print_file_async (5.83 KB, patch)
2017-05-31 11:58 UTC, Felipe Borges
none Details | Review
printers: Add print "Test Page" button to Options Dialog (6.27 KB, patch)
2017-05-31 11:58 UTC, Felipe Borges
none Details | Review
printers: Introduce pp_printer_print_file_async (5.64 KB, patch)
2017-06-07 12:25 UTC, Felipe Borges
committed Details | Review
printers: Add print "Test Page" button to Options Dialog (6.34 KB, patch)
2017-06-07 12:25 UTC, Felipe Borges
committed Details | Review

Description Felipe Borges 2017-03-07 15:29:32 UTC
See https://bugzilla.gnome.org/show_bug.cgi?id=779506#c5
Comment 1 Felipe Borges 2017-03-07 15:30:10 UTC
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
Comment 2 Felipe Borges 2017-03-07 15:30:16 UTC
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.
Comment 3 Felipe Borges 2017-03-07 15:30:21 UTC
Created attachment 347402 [details] [review]
printers: Use headerbar in PpOptionsDialog
Comment 4 Felipe Borges 2017-03-07 15:30:26 UTC
Created attachment 347403 [details] [review]
printers: Add print "Test Page" button to Options Dialog
Comment 5 Marek Kašík 2017-03-09 18:03:56 UTC
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.
Comment 6 Felipe Borges 2017-03-14 10:49:17 UTC
These seem to be solved after Bug 779846.
Comment 7 Marek Kašík 2017-05-29 16:30:30 UTC
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.
Comment 8 Marek Kašík 2017-05-29 16:32:26 UTC
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.
Comment 9 Marek Kašík 2017-05-29 16:40:34 UTC
Review of attachment 347401 [details] [review]:

This looks good to me.
Comment 10 Marek Kašík 2017-05-29 16:40:49 UTC
Review of attachment 347402 [details] [review]:

This one too.
Comment 11 Felipe Borges 2017-05-29 17:35:29 UTC
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
Comment 12 Marek Kašík 2017-05-29 17:43:03 UTC
I'll continue on the last one tomorrow.
Comment 13 Marek Kašík 2017-05-30 11:48:22 UTC
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.
Comment 14 Felipe Borges 2017-05-31 11:58:24 UTC
Created attachment 352943 [details] [review]
printers: Introduce pp_printer_print_file_async

An asynchronous wrapper to print files.
Comment 15 Felipe Borges 2017-05-31 11:58:30 UTC
Created attachment 352944 [details] [review]
printers: Add print "Test Page" button to Options Dialog
Comment 16 Marek Kašík 2017-06-06 16:39:08 UTC
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.
Comment 17 Marek Kašík 2017-06-06 16:53:28 UTC
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 :) ).
Comment 18 Felipe Borges 2017-06-07 12:25:26 UTC
Created attachment 353319 [details] [review]
printers: Introduce pp_printer_print_file_async

An asynchronous wrapper to print files.
Comment 19 Felipe Borges 2017-06-07 12:25:32 UTC
Created attachment 353320 [details] [review]
printers: Add print "Test Page" button to Options Dialog
Comment 20 Marek Kašík 2017-06-12 11:12:45 UTC
Review of attachment 353319 [details] [review]:

Thank you for the modification.
Comment 21 Marek Kašík 2017-06-12 11:13:21 UTC
Review of attachment 353320 [details] [review]:

This also looks good to me. Thank you for the patches.
Comment 22 Felipe Borges 2017-06-14 08:48:05 UTC
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