GNOME Bugzilla – Bug 755626
New Printer jobs dialog
Last modified: 2016-01-21 14:10:48 UTC
Created attachment 312145 [details] Screencast of the new Printer Jobs dialog I have updated the Printer jobs dialog in order to match the new designs at https://wiki.gnome.org/Design/SystemSettings/Printers Allan Day has been closely following the development of this feature, giving me suggestions and insights for it. I have attached a screencast of it.
Created attachment 312146 [details] [review] printers: redesign the Printer Jobs Dialog Update the Printer Jobs Dialog to match the current designs at https://wiki.gnome.org/Design/SystemSettings/Printers
Created attachment 312147 [details] [review] printers: add an empty state for the printer jobs dialog
Created attachment 312148 [details] Screenshot of the Printer jobs dialog empty state
Review of attachment 312147 [details] [review]: ::: panels/printers/pp-jobs-dialog.c @@ +189,3 @@ + stack = (GtkStack*) gtk_builder_get_object (GTK_BUILDER (dialog->builder), "stack"); + gtk_stack_set_visible_child_name (GTK_STACK (stack), num_of_jobs > 0 ? "list-jobs-page" : "no-jobs-page"); You need to sync the state of the "Clear All" button as well. No jobs -> nothing to clear.
Review of attachment 312146 [details] [review]: ::: panels/printers/pp-job.c @@ +23,3 @@ +enum +{ + PROP_ID = 1, We usually add a PROP_0, instead. @@ +40,3 @@ +static GParamSpec *properties[LAST_PROPERTY] = { NULL, }; + +G_DEFINE_TYPE (PpJob, pp_job, G_TYPE_OBJECT) G_DEFINE_TYPE_WITH_PRIVATE? ::: panels/printers/pp-job.h @@ +27,3 @@ + +typedef struct _PpJob PpJob; +typedef struct _PpJobClass PpJobClass; G_DECLARE_FINAL_TYPE ? ::: panels/printers/pp-jobs-dialog.c @@ +66,1 @@ { _cb_cb()? Do we want to do something here? @@ +87,3 @@ + + g_object_get (job, "id", &job_id, NULL); + g_object_get (job, "state", &job_state, NULL); That should be a single call. @@ +181,3 @@ gpointer user_data) { + PpJobsDialog *dialog = (PpJobsDialog *) user_data; No need to cast.
Created attachment 312353 [details] [review] printers: redesign the Printer Jobs Dialog Update the Printer Jobs Dialog to match the current designs at https://wiki.gnome.org/Design/SystemSettings/Printers
Review of attachment 312353 [details] [review]: ::: panels/printers/pp-job.c @@ +23,3 @@ +typedef struct _PpJobPrivate PpJobPrivate; + +struct _PpJobPrivate typedef struct { } PpJobPrivate; directly. @@ +34,3 @@ +enum +{ + PROP_0 = 0, "= 0" isn't needed. @@ +41,3 @@ +}; + +static GParamSpec *properties[LAST_PROPERTY] = { NULL, }; given that this will be overwritten in class_init, there's no need to initialise it. ::: panels/printers/pp-job.h @@ +26,3 @@ +G_BEGIN_DECLS + +#define PP_TYPE_JOB (pp_job_get_type ()) Again, G_DECLARE_FINAL_TYPE ? ::: panels/printers/pp-jobs-dialog.c @@ +314,1 @@ + title = g_strdup_printf (_("%s - Active Jobs"), printer_name); This probably needs a translator comment.
Created attachment 312419 [details] [review] printers: redesign the Printer Jobs Dialog Update the Printer Jobs Dialog to match the current designs at https://wiki.gnome.org/Design/SystemSettings/Printers
FWIW I think it would be more natural to use a minus (remove) icon rather than a stop icon for cancel. The first time I opened that dialog, I was confused why there was no button to remove jobs from the list.
Makes sense to me. Allan?
(In reply to Michael Catanzaro from comment #9) > FWIW I think it would be more natural to use a minus (remove) icon rather > than a stop icon for cancel. The first time I opened that dialog, I was > confused why there was no button to remove jobs from the list. The minus sign just looks like a line without a corresponding + icon. An alternative would be to use a trash icon, if edit-delete is insufficient for some reason.
Review of attachment 312419 [details] [review]: Thank you for working on this. I have several comments: ::: panels/printers/jobs-dialog.ui @@ +20,3 @@ + <child> + <object class="GtkButton" id="jobs-clear-all-button"> + <property name="label" translatable="yes">Clear All</property> You should add a comment for translators here. @@ +92,2 @@ <property name="visible">True</property> + <property name="label" translatable="yes">No Active Printer Jobs</property> You should add a comment for translators here. ::: panels/printers/pp-job.c @@ +128,3 @@ + properties[PROP_TITLE] = g_param_spec_string ("title", "title", "title", + NULL, G_PARAM_READWRITE); + properties[PROP_STATE] = g_param_spec_int ("state", "state", "state", Could you give here a short description as the third parameter? Something like "Title of this print job". The same goes for the previous 2 properties. ::: panels/printers/pp-jobs-dialog.c @@ +64,3 @@ +static void +job_stop_cb (GtkButton *button, + gint job_id) Could you pass the PpJob here as in the job_pause_cb() ? @@ +94,3 @@ + "media-playback-pause-symbolic" : "media-playback-start-symbolic", + GTK_ICON_SIZE_SMALL_TOOLBAR)); +} You should maybe change the state of the PpJob here too. @@ +113,3 @@ + + switch (job_state) + { Indent this by another 2 spaces here please. @@ +150,2 @@ + widget = gtk_label_new (""); + g_object_bind_property (job, "title", widget, "label", G_BINDING_SYNC_CREATE); Do we need to bind this property? Does it change? Even the state is set statically on the next lines. @@ +188,1 @@ + stack = (GtkStack*) gtk_builder_get_object (GTK_BUILDER (dialog->builder), "stack"); Use GTK_STACK () macro here. @@ +188,2 @@ + stack = (GtkStack*) gtk_builder_get_object (GTK_BUILDER (dialog->builder), "stack"); + clear_all_button = (GtkWidget*) gtk_builder_get_object (GTK_BUILDER (dialog->builder), "jobs-clear-all-button"); Use GTK_WIDGET () macro here. @@ +191,3 @@ { + gtk_widget_set_sensitive (clear_all_button, TRUE); + gtk_stack_set_visible_child_name (GTK_STACK (stack), "list-jobs-page"); No need for GTK_STACK () here. @@ +196,3 @@ { + gtk_widget_set_sensitive (clear_all_button, FALSE); + gtk_stack_set_visible_child_name (GTK_STACK (stack), "no-jobs-page"); No need for GTK_STACK () here. @@ +219,2 @@ + g_object_unref (job); + g_free (state); I don't see this used anywhere. @@ +233,2 @@ { + if (dialog->printer_name) Compare the pointer against NULL please. @@ +309,3 @@ g_signal_connect (dialog->dialog, "response", G_CALLBACK (jobs_dialog_response_cb), dialog); + clear_all_button = (GtkButton *) gtk_builder_get_object (dialog->builder, "jobs-clear-all-button"); You should use the macro GTK_BUTTON () here. @@ +314,1 @@ + /* Translators: This is the printer name to which we are showing the active jobs */ There should "... for which ..." @@ +317,3 @@ g_free (title); + dialog->listbox = (GtkListBox*) You should use the macro GTK_LIST_BOX () here ::: panels/printers/pp-utils.h @@ -264,1 @@ void job_cancel_purge_async (gint job_id, Since we have the new PpJob class, it would be great to make this function one of its methods. Something like pp_job_cancel_purge_async(). @@ -265,3 @@ gboolean job_purge, GCancellable *cancellable, - JCPCallback callback, Remove definition and each usage of the JCPCallback please. @@ +267,3 @@ gpointer user_data); +void job_cancel_purge_all (cups_job_t *jobs, If you'll be moving the job_cancel_purge_async() and job_cancel_purge_async() into the PpJob then it would be good to move this one to PpCups like e.g. cups_cancel_purge_jobs() which would take GList of PpJob as an input. @@ -270,1 @@ typedef void (*JSHUCallback) (gpointer user_data); Remove this definition of JSHUCallback please. @@ -270,3 @@ typedef void (*JSHUCallback) (gpointer user_data); void job_set_hold_until_async (gint job_id, Similarly to the job_cancel_purge_async(), it would be great to make this function one the methods of PpJob. E.g. pp_job_set_hold_until_async().
Created attachment 317016 [details] [review] printers: redesign the Printer Jobs Dialog Update the Printer Jobs Dialog to match the current designs at https://wiki.gnome.org/Design/SystemSettings/Printers
I'll do the review once I'm back from Christmas holiday. I'm still not sure whether to ask you for *_finish() functions for the pp_job_*_async() functions. But since we just call them and don't need the result because we are connected to CUPS' subscription it shouldn't be needed. But there can an error in which case we would like to know about it and react to it (we don't now - e.g. in the case when root doesn't have permission to cancel other's job).
Review of attachment 317016 [details] [review]: Thank you for the patch. It still needs some changes but once they are in it should be ok to push. ::: panels/printers/pp-cups.c @@ +31,3 @@ + gpointer user_data) +{ + GList *l = NULL; You don't need to initialize this pointer. @@ +35,3 @@ + for (l = job_list; l != NULL; l = l->next) + { + pp_job_cancel_purge_async (l->data, I prefer to have a variable to which you retype the ->data pointer so it is clear what you are passing. panels/printers/pp-job.c ::: panels/printers/pp-cups.h @@ +67,3 @@ GError **error); +void pp_cups_cancel_purge_jobs (PpCups *cups, You'll need to change this to the async:finish pair because it will be needed if the PpCups instance is created just for one occurrence and need to be unreffed in a callback of this call (e.g. in the on_clear_all_button_clicked). ::: panels/printers/pp-job.c @@ +50,3 @@ + +static void +job_cancel_purge_async_dbus_cb (GObject *source_object, Add here the "pp_" prefix please so that the name is pp_job_cancel_purge_async_dbus_cb(). And similarly for all the other functions. @@ +63,3 @@ + g_object_unref (source_object); + + if (output) Check this against NULL please and all the other pointer checks too. @@ +215,3 @@ + + switch (property_id) + { Indent this by another 2 spaces please. @@ +281,3 @@ + + properties[PROP_ID] = g_param_spec_int ("id", "id", "id", + 0, G_MAXINT, 0, G_PARAM_READWRITE); It is usual to use more lines for these calls. At least the name, nick and blurb are usually on its own lines but it would be good to have all the others too. The code is better readable then. The same goes for all the other properties as well. ::: panels/printers/pp-job.h @@ +40,3 @@ + const gchar *job_hold_until, + GCancellable *cancellable, + gpointer user_data); Make this the classic "*_async()", "*_finish()" pairs. The same goes for the "pp_cups_cancel_purge_jobs()". The main reason for this is that when you are cancelling all the jobs you are creating PpCups instance which is not unreffed later. For this you need a callback and the standard way is the async:finish pair. ::: panels/printers/pp-jobs-dialog.c @@ +207,1 @@ + g_object_unref (job); Is the job properly reffed? It seems that the dialog->jobs doesn't get its own reference. @@ +253,3 @@ + cups = pp_cups_new (); + + pp_cups_cancel_purge_jobs (cups, dialog->jobs, dialog->num_jobs, FALSE, NULL, NULL); You'll need to unref the "cups" in callback of the pp_cups_cancel_purge_jobs() once you'll add it.
Created attachment 318966 [details] [review] printers: redesign the Printer Jobs Dialog Update the Printer Jobs Dialog to match the current designs at https://wiki.gnome.org/Design/SystemSettings/Printers
(In reply to Marek Kašík from comment #15) > Review of attachment 317016 [details] [review] [review]: > > Thank you for the patch. It still needs some changes but once they are in it > should be ok to push. Thanks for your review. > ::: panels/printers/pp-cups.h > @@ +67,3 @@ > GError **error); > > +void pp_cups_cancel_purge_jobs (PpCups *cups, > > You'll need to change this to the async:finish pair because it will be > needed if the PpCups instance is created just for one occurrence and need to > be unreffed in a callback of this call (e.g. in the > on_clear_all_button_clicked). As we discussed on IRC, I decided to go for a simpler approach, by doing the loop through jobs on the on_clear_all_button_clicked callback itself as you suggested. Otherwise, it would involve having a gtask spawing another async operation (g_dbus_connection_call), which would add complexity for such a simple operation. Feel free to disagree and request this implementation or better approach. > ::: panels/printers/pp-job.h > @@ +40,3 @@ > + const gchar *job_hold_until, > + GCancellable *cancellable, > + gpointer user_data); > > Make this the classic "*_async()", "*_finish()" pairs. The same goes for the > "pp_cups_cancel_purge_jobs()". The main reason for this is that when you are > cancelling all the jobs you are creating PpCups instance which is not > unreffed later. For this you need a callback and the standard way is the > async:finish pair. Assuming the approach described in my comment above, that wouldn't be necessary. > > ::: panels/printers/pp-jobs-dialog.c > @@ +253,3 @@ > + cups = pp_cups_new (); > + > + pp_cups_cancel_purge_jobs (cups, dialog->jobs, dialog->num_jobs, FALSE, > NULL, NULL); > > You'll need to unref the "cups" in callback of the > pp_cups_cancel_purge_jobs() once you'll add it. Same as above.
Review of attachment 318966 [details] [review]: Thank you for the modifications. Here are some comments: ::: panels/printers/pp-cups.h @@ +25,3 @@ #include <gio/gio.h> #include "pp-utils.h" +#include "pp-job.h" Is this needed? ::: panels/printers/pp-job.h @@ +40,3 @@ + const gchar *job_hold_until, + GCancellable *cancellable, + gpointer user_data); Since we don't use a callback here we can remove the user_data. We are not using the cancellable too so remove also the cancellable. You can remove the definition and usage of the structures JCPData and JSHUData then. @@ +45,3 @@ + gboolean job_purge, + GCancellable *cancellable, + gpointer user_data); Ditto. ::: panels/printers/pp-jobs-dialog.c @@ +57,3 @@ gchar *printer_name; + GList *jobs; Do we need this? Isn't the listbox enough? @@ +249,3 @@ + GList *l; + + for (l = dialog->jobs; l != NULL; l = l->next) We could use gtk_tree_model_foreach() together with the listbox here. Just be sure that an update triggered by this doesn't invalidate the list itself. @@ +338,2 @@ if (dialog->num_jobs > 0) + g_list_free (dialog->jobs); You'll probably remove this in favour of the listbox but I'm curious whether not using g_list_free_full() here is intentional?
Created attachment 319013 [details] [review] printers: redesign the Printer Jobs Dialog Update the Printer Jobs Dialog to match the current designs at https://wiki.gnome.org/Design/SystemSettings/Printers
Review of attachment 319013 [details] [review]: Thank you for the modifications. I have just few other comments. Please push the patch to master once you fix them. ::: panels/printers/pp-job.c @@ +62,3 @@ + else + { + g_error_free (error); You don't need the error variable if you don't use the info in it. Just pass NULL in the g_dbus_connection_call_finish(). @@ +119,3 @@ + else + { + g_error_free (error); You don't need the error variable if you don't use the info in it. Just pass NULL in the g_dbus_connection_call_finish(). @@ +173,3 @@ + + switch (property_id) + { Indent this by another 2 spaces please as you did in the pp_job_set_property().
Thanks. Pushed to master as https://git.gnome.org/browse/gnome-control-center/commit/?id=8baaa81a397d9f261d9f0646f0fcde9991fc9bbb