GNOME Bugzilla – Bug 779313
Query for number of jobs asynchronously
Last modified: 2017-03-06 15:09:40 UTC
Instead of cupsGetJobs ()
Created attachment 346824 [details] [review] printers: Query for the number of jobs asynchronously
Review of attachment 346824 [details] [review]: Looking at the cups_get_jobs_async() it seems that we should replace it by proper implementation in PpPrinter class. Something like pp_printer_get_jobs_async() and pp_printer_get_jobs_finish() with correct usage of GCancellable. Try insert g_usleep(3000000) into the cups_get_jobs_func(), run printers panel and go quickly to overview. Something bad happens. This wouldn't happen if we would know that the PpPrinterEntry doesn't exist already.
Created attachment 346908 [details] [review] printers: Introduce pp_printer_get_jobs_async () It is a PpPrinter method to query asychronously for printing jobs information. It should replace cups_get_jobs_async ()
Created attachment 346909 [details] [review] printers: Port PpJobsDialog to pp_printer_get_jobs_async
Created attachment 346910 [details] [review] printers: Drop cups_get_jobs_async pp_printer_get_jobs_async does the job with more modern GLib API (GTask).
Review of attachment 346908 [details] [review]: Thank you for the patch. I have several comments on it. ::: panels/printers/pp-printer-entry.c @@ +404,3 @@ gint num_jobs, num_of_jobs; + jobs = pp_printer_get_jobs_finish (printer, result, NULL); Use an error here to check whether it was cancelled. Free the list later. Unref the source_object. @@ +409,1 @@ num_jobs = num_of_jobs < 0 ? 0 : (guint) num_of_jobs; g_list_length returns guint so you can use directly that. @@ +431,3 @@ + + g_object_unref (self->get_jobs_cancellable); + self->get_jobs_cancellable = NULL; Use g_clear_object() here. @@ +447,3 @@ + self->get_jobs_cancellable, + get_jobs_cb, + g_object_ref (self)); Don't ref the entry please, use error returned by the _finish() method to check whether the call was cancelled. ::: panels/printers/pp-printer.c @@ +298,3 @@ +{ + g_slice_free (GetJobsData, get_jobs_data); +} If you use g_new() for allocation then you can use g_free() instead of the new function. @@ +313,3 @@ + gint num_jobs; + + g_object_get (printer, "printer-name", &printer_name, NULL); You are leaking the printer_name. @@ +320,3 @@ + get_jobs_data->which_jobs); + + while (num_jobs > 0) This would work but use for() loop here please. @@ +324,3 @@ + num_jobs--; + + list = g_list_append (list, &jobs[num_jobs]); Lets create PpJob for every such job so the result is usable by other callers. @@ +326,3 @@ + list = g_list_append (list, &jobs[num_jobs]); + } + cupsFreeJobs (num_jobs, jobs); This would make the result unusable but if you return GList of PpJob then it can stay here.
Review of attachment 346909 [details] [review]: Thank you for this patch. It needs some work yet. ::: panels/printers/pp-jobs-dialog.c @@ +63,1 @@ gint ref_count; Since we already have proper async implementation of getting of jobs, please remove the ref_count variable and merge pp_jobs_dialog_free_idle() into pp_jobs_dialog_free() please. @@ +176,3 @@ stack = GTK_STACK (gtk_builder_get_object (GTK_BUILDER (dialog->builder), "stack")); clear_all_button = GTK_WIDGET (gtk_builder_get_object (GTK_BUILDER (dialog->builder), "jobs-clear-all-button")); Check whether the call was cancelled before using any pointer outside of PpPrinter here (via the error) that was original intent for the rewrite. @@ +204,3 @@ + g_object_unref (dialog->get_jobs_cancellable); + dialog->get_jobs_cancellable = NULL; You can use g_clear_object() here.
Review of attachment 346910 [details] [review]: We can remove the CGJCallback now. ::: panels/printers/pp-utils.h @@ +246,2 @@ gint num_of_jobs, gpointer user_data); Remove also the CGJCallback please.
Created attachment 347055 [details] [review] printers: Introduce pp_printer_get_jobs_async () It is a PpPrinter method to query asychronously for printing jobs information. It should replace cups_get_jobs_async ()
Created attachment 347056 [details] [review] printers: Port PpJobsDialog to pp_printer_get_jobs_async
Created attachment 347057 [details] [review] printers: Properly free pp_jobs_dialog Since now we have proper async implementation for getting jobs, we do not need to increment/decrement the objects ref_count manually.
Created attachment 347058 [details] [review] printers: Drop cups_get_jobs_async pp_printer_get_jobs_async does the job with more modern GLib API (GTask).
Created attachment 347059 [details] [review] printers: Introduce pp_printer_get_jobs_async () It is a PpPrinter method to query asychronously for printing jobs information. It should replace cups_get_jobs_async ()
Created attachment 347060 [details] [review] printers: Port PpJobsDialog to pp_printer_get_jobs_async
Created attachment 347061 [details] [review] printers: Properly free pp_jobs_dialog Since now we have proper async implementation for getting jobs, we do not need to increment/decrement the objects ref_count manually.
Created attachment 347062 [details] [review] printers: Drop cups_get_jobs_async pp_printer_get_jobs_async does the job with more modern GLib API (GTask).
Review of attachment 347059 [details] [review]: This need just 2 small changes yet. ::: panels/printers/pp-printer-entry.c @@ +410,2 @@ + g_object_unref (source_object); + g_list_free (jobs); You haven't freed the jobs (use g_list_free_full() for that). @@ +413,3 @@ + if (error != NULL) + { + g_warning ("Could not get jobs: %s\n", error->message); Don't show this when the operation was cancelled (I see a lot of these when I just open the panel).
Review of attachment 347060 [details] [review]: This one too. ::: panels/printers/pp-jobs-dialog.c @@ +181,3 @@ + if (error != NULL) + { + g_message ("Could not get jobs: %s\n", error->message); Don't show this message when the operation was cancelled and maybe g_warning() would be better here. @@ +203,1 @@ } You can g_list_free() the list here.
Review of attachment 347061 [details] [review]: This one looks good to me. Push it once we have all ready.
Review of attachment 347062 [details] [review]: Thank you for the change. Push it once we have all patches ready.
Created attachment 347072 [details] [review] printers: Introduce pp_printer_get_jobs_async () It is a PpPrinter method to query asychronously for printing jobs information. It should replace cups_get_jobs_async ()
Created attachment 347073 [details] [review] printers: Port PpJobsDialog to pp_printer_get_jobs_async
Created attachment 347074 [details] [review] printers: Introduce pp_printer_get_jobs_async () It is a PpPrinter method to query asychronously for printing jobs information. It should replace cups_get_jobs_async ()
Created attachment 347075 [details] [review] printers: Port PpJobsDialog to pp_printer_get_jobs_async
Review of attachment 347075 [details] [review]: I'm sorry but we need one more change. When I was testing the patch I've got a crash. It was caused by not cancelled cancellable. We need to cancel the cancellable if there is any if we are updating the dialog too soon. ::: panels/printers/pp-jobs-dialog.c @@ +222,3 @@ dialog->ref_count++; + + dialog->get_jobs_cancellable = g_cancellable_new (); You have to cancel the cancellable if it is there (and free) before this.
Review of attachment 347074 [details] [review]: Lets do the same in this patch. ::: panels/printers/pp-printer-entry.c @@ +453,3 @@ + PpPrinter *printer; + + self->get_jobs_cancellable = g_cancellable_new (); You have to cancel (and free) the cancellable if it is there before this.
Created attachment 347309 [details] [review] printers: Introduce pp_printer_get_jobs_async () It is a PpPrinter method to query asychronously for printing jobs information. It should replace cups_get_jobs_async ()
Created attachment 347310 [details] [review] printers: Port PpJobsDialog to pp_printer_get_jobs_async
Review of attachment 347309 [details] [review]: Thank you for the change and all the patches. Push it to master please.
Review of attachment 347310 [details] [review]: This one too.
thanks! Attachment 347061 [details] pushed as 2ad93bc - printers: Properly free pp_jobs_dialog Attachment 347062 [details] pushed as 1df4da5 - printers: Drop cups_get_jobs_async Attachment 347309 [details] pushed as 1d0ae49 - printers: Introduce pp_printer_get_jobs_async () Attachment 347310 [details] pushed as c40772a - printers: Port PpJobsDialog to pp_printer_get_jobs_async