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 760581 - Port pp_cups_get_dests* to GTask
Port pp_cups_get_dests* to GTask
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: 2016-01-13 14:48 UTC by Felipe Borges
Modified: 2016-01-15 14:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
printers: port pp_cups_get_dests* to GTask (3.46 KB, patch)
2016-01-13 14:48 UTC, Felipe Borges
none Details | Review
printers: port pp_cups_get_dests* to GTask (3.66 KB, patch)
2016-01-14 14:05 UTC, Felipe Borges
none Details | Review
printers: port pp_cups_get_dests* to GTask (3.58 KB, patch)
2016-01-15 10:39 UTC, Felipe Borges
none Details | Review
printers: port pp_cups_get_dests* to GTask (3.49 KB, patch)
2016-01-15 10:51 UTC, Felipe Borges
committed Details | Review

Description Felipe Borges 2016-01-13 14:48:13 UTC
.
Comment 1 Felipe Borges 2016-01-13 14:48:41 UTC
Created attachment 318973 [details] [review]
printers: port pp_cups_get_dests* to GTask

As of GLib 2.46, GSimpleAsyncResult is deprecated
in favor of GTask, which provides a simpler API.
Comment 2 Marek Kašík 2016-01-14 12:58:40 UTC
Review of attachment 318973 [details] [review]:

Thank you for converting this to GTask. I have few comments.

::: panels/printers/pp-cups.c
@@ +81,3 @@
+
+  if (data->dests->num_of_dests)
+    g_task_return_pointer (task, data->dests, (GDestroyNotify) cgd_data_free);

The cgd_data_free() is for freeing of CGDData structure not for freeing of PpCupsDests.

@@ +83,3 @@
+    g_task_return_pointer (task, data->dests, (GDestroyNotify) cgd_data_free);
+  else
+    g_task_return_error (task, error);

Having 0 dests is not an error (according to me not according to CUPS). If I remember correctly this was the reason to have the PpCupsDests inside of the CGDData instead of just PpCupsDests so we know that the NULL was set by the cupsGetDests().
Looking into CUPS source code, they consider 0 dests as an error so we can not detect whether there was an error or not. So lets just return the PpCupsDests and remove the whole CGDData structure (not using task data then) and don't set error when there is 0 dests, just return NULL.
Comment 3 Felipe Borges 2016-01-14 14:05:36 UTC
Created attachment 319009 [details] [review]
printers: port pp_cups_get_dests* to GTask

As of GLib 2.46, GSimpleAsyncResult is deprecated
in favor of GTask, which provides a simpler API.
Comment 4 Marek Kašík 2016-01-15 10:30:05 UTC
Review of attachment 319009 [details] [review]:

::: panels/printers/pp-cups.c
@@ +61,2 @@
 {
+  PpCupsDests *dests = task_data;

You set the pointer on next line so the initialization is not used.

@@ +82,2 @@
+  task = g_task_new (cups, cancellable, callback, user_data);
+  g_task_set_task_data (task, dests, (GDestroyNotify) pp_cups_dests_free);

You don't need the task data at all.
Comment 5 Felipe Borges 2016-01-15 10:39:59 UTC
Created attachment 319085 [details] [review]
printers: port pp_cups_get_dests* to GTask

As of GLib 2.46, GSimpleAsyncResult is deprecated
in favor of GTask, which provides a simpler API.
Comment 6 Marek Kašík 2016-01-15 10:44:05 UTC
Review of attachment 319085 [details] [review]:

::: panels/printers/pp-cups.c
@@ +79,2 @@
+  dests = g_new0 (PpCupsDests, 1);
+  dests->dests = NULL;

I'm sorry, I did not mention it specifically in the previous review, but you also don't need the dests pointer here because you set it in the _pp_cups_get_dests_thread(). So remove "PpCupsDests *dests;" and its initialization.
Comment 7 Felipe Borges 2016-01-15 10:51:29 UTC
Created attachment 319086 [details] [review]
printers: port pp_cups_get_dests* to GTask

As of GLib 2.46, GSimpleAsyncResult is deprecated
in favor of GTask, which provides a simpler API.
Comment 8 Marek Kašík 2016-01-15 14:10:50 UTC
Review of attachment 319086 [details] [review]:

Thank you for the changes. The patch looks good to me now.
Comment 9 Felipe Borges 2016-01-15 14:22:21 UTC
Comment on attachment 319086 [details] [review]
printers: port pp_cups_get_dests* to GTask

Thanks. Pushed to master as https://git.gnome.org/browse/gnome-control-center/commit/?id=7cd6800da0b343aaa44bdafb7e1f2348995aa414