GNOME Bugzilla – Bug 669596
Port printer panel to GDBus
Last modified: 2012-02-29 22:40:26 UTC
Created attachment 207013 [details] [review] printers: port to GDBus Patch from Robert Ancell following
Comment on attachment 207013 [details] [review] printers: port to GDBus Hi, this is something I wanted to fix already for quite some time but haven't get to it yet. So thank you for working on it. Unfortunately, the patch doesn't apply any more. There were committed some changes to g-c-c's GIT recently. These changed error handling and usage of floating references to GVariant. >diff --git a/panels/printers/pp-new-printer-dialog.c b/panels/printers/pp-new-printer-dialog.c >index bbe1f91..92753bf 100644 >--- a/panels/printers/pp-new-printer-dialog.c >+++ b/panels/printers/pp-new-printer-dialog.c >@@ -486,7 +485,6 @@ devices_get_cb (GObject *source_object, > { > GVariantBuilder device_list; > GVariantBuilder device_hash; >- GVariant *input = NULL; > GVariant *output = NULL; > GVariant *array = NULL; > GVariant *subarray = NULL; >@@ -524,11 +522,9 @@ devices_get_cb (GObject *source_object, > } > } > >- input = g_variant_new ("(v)", g_variant_builder_end (&device_list)); >- > output = g_dbus_proxy_call_sync (proxy, > "GroupPhysicalDevices", >- input, >+ g_variant_new ("(v)", g_variant_builder_end (&device_list)), > G_DBUS_CALL_FLAGS_NONE, > 60000, > NULL, >@@ -560,7 +556,6 @@ devices_get_cb (GObject *source_object, > > if (output) > g_variant_unref (output); >- g_variant_unref (input); > g_object_unref (proxy); > } > Matthias already committed a fix for all these floating references. >--- a/panels/printers/pp-utils.h >+++ b/panels/printers/pp-utils.h ... >-DBusGProxy *get_dbus_proxy (const gchar *name, >+GDBusProxy *get_dbus_proxy (const gchar *name, This function is not needed any more. Just remove it and use g_dbus_proxy_new_for_bus_sync() together with error handling as in current service_disable(). Thank you Marek
Created attachment 208387 [details] [review] printers: port to GDBus Updated as requested. Note that I've used g_dbus_connection_call instead of using proxies as we never reuse the proxies so they just make the code more complex.
Created attachment 208388 [details] [review] printers: port to GDBus Removed some checks that aren't necessary when using g_dbus_connection_call
Comment on attachment 208388 [details] [review] printers: port to GDBus Hi Robert, thank you for the quick modification of the patch. I have some comments on it. >--- a/panels/printers/pp-new-printer-dialog.c >+++ b/panels/printers/pp-new-printer-dialog.c >@@ -1554,44 +1553,59 @@ new_printer_add_button_cb (GtkButton *button, ... >+ bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &error); >+ if (!bus) >+ { >+ g_warning ("Failed to get system bus: %s", error->message); >+ g_error_free (error); >+ } >+ >+ if (bus) Maybe it would be better to move the body of "if (!bus)" to else branch of the "if (bus)" (or move if (bus) to else branch of if (!bus)). >--- a/panels/printers/pp-utils.c >+++ b/panels/printers/pp-utils.c >@@ -1749,12 +1714,14 @@ printer_rename (const gchar *old_name, ... >+ bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &error); >+ if (!bus) >+ { >+ g_warning ("Failed to get system bus: %s", error->message); >+ g_error_free (error); >+ } > >- if (proxy) >+ if (bus) > { The same case. >@@ -1919,96 +1917,119 @@ gboolean ... > gboolean > printer_delete (const gchar *printer_name) > { ... >+ output = g_dbus_connection_call_sync (bus, >+ MECHANISM_BUS, >+ "/", >+ MECHANISM_BUS, >+ "PrinterSetDefault", You should call "PrinterDelete" method here. >@@ -2024,32 +2045,45 @@ printer_set_default (const gchar *printer_name) ... >+ bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &error); >+ if (!bus) >+ { >+ g_warning ("Failed to get system bus: %s", error->message); >+ g_error_free (error); >+ } > >- if (proxy) >+ if (bus) > { The "if (bus)" thing again. >@@ -2263,43 +2355,56 @@ gboolean > class_add_printer (const gchar *class_name, > const gchar *printer_name) ... >+ if (output) >+ { >+ const gchar *ret_error; > >- result = dbus_g_proxy_call (proxy, "ClassAddPrinter", &error, >- G_TYPE_STRING, class_name, >- G_TYPE_STRING, printer_name, >- G_TYPE_INVALID, >- G_TYPE_STRING, &ret_error, >- G_TYPE_INVALID); >+ g_variant_get (output, "(&s)", &ret_error); >+ if (ret_error[0] != '\0') >+ g_warning ("%s", ret_error); >+ else >+ result = TRUE; > >- if (!result) >+ g_variant_unref (output); >+ result = TRUE; >+ } >+ else There shouldn't be the last "result = TRUE;". >+ g_variant_builder_init (&array_builder, G_VARIANT_TYPE ("as")); >+ for (i = users; i; i++) >+ g_variant_builder_add (&array_builder, "s", *i); This crashes the Printer panel. I usually use this: gint i; ... for (i = 0; users[i]; i++) g_variant_builder_add (&array_builder, "s", users[i]); Also, there should be something like this: g_variant_new ("(s)", string ? string : ""); for all strings for which we are not sure that they are not NULL. Regards Marek
Created attachment 208644 [details] [review] printers: port to GDBus Updated as requested. I've manually checked which strings could be NULL, but please check again.
Created attachment 208646 [details] [review] printers: Convert unnecessary use of GDBusProxy to using GDBusConnection directly This patch updates the existing GDBus code that was using GDBusProxy to the same method used in the gbus-glib patch for consistency. This is a better method in this case as the code is not using any of the signals/properties that the proxies provide and these features require additional D-Bus communication that can be avoided.
Comment on attachment 208644 [details] [review] printers: port to GDBus Hi Robert, thank you for those modifications. I have just final comments on it. >--- a/panels/printers/pp-new-printer-dialog.c >+++ b/panels/printers/pp-new-printer-dialog.c >@@ -1554,44 +1553,58 @@ new_printer_add_button_cb (GtkButton *button, ... >+ output = g_dbus_connection_call_sync (bus, >+ MECHANISM_BUS, >+ "/", >+ MECHANISM_BUS, >+ "PrinterAddWithPpdFile", >+ g_variant_new ("(sssss)", >+ pp->devices[device_id].display_name, >+ pp->devices[device_id].device_uri, >+ ppd_file_name, >+ pp->devices[device_id].device_info, >+ pp->devices[device_id].device_location ? pp->devices[device_id].device_location : ""), >+ G_VARIANT_TYPE ("(s)"), >+ G_DBUS_CALL_FLAGS_NONE, >+ -1, >+ NULL, >+ &error); The "pp->devices[device_id].device_info" can be NULL. Maybe just add check for all those "pp->devices[device_id].*". >@@ -1665,49 +1685,61 @@ new_printer_add_button_cb (GtkButton *button, ... >+ output = g_dbus_connection_call_sync (bus, >+ MECHANISM_BUS, >+ "/", >+ MECHANISM_BUS, >+ "PrinterAdd", >+ g_variant_new ("(sssss)", >+ pp->devices[device_id].display_name, >+ pp->devices[device_id].device_uri, >+ ppd_name->ppd_name, >+ pp->devices[device_id].device_info, >+ pp->devices[device_id].device_location ? pp->devices[device_id].device_location : ""), >+ G_VARIANT_TYPE ("(s)"), >+ G_DBUS_CALL_FLAGS_NONE, >+ -1, >+ NULL, >+ &error); The same here. >@@ -1806,28 +1837,45 @@ new_printer_add_button_cb (GtkButton *button, ... >+ output = g_dbus_connection_call_sync (bus, >+ MECHANISM_BUS, >+ "/", >+ MECHANISM_BUS, >+ "PrinterAddOptionDefault", >+ g_variant_new ("(ssas)", >+ pp->devices[device_id].display_name, >+ "PageSize", >+ &array_builder), >+ G_VARIANT_TYPE ("(s)"), >+ G_DBUS_CALL_FLAGS_NONE, >+ -1, >+ NULL, >+ &error); And here. Feel free to commit this patch after you add those checks. Thank you Marek
Comment on attachment 208646 [details] [review] printers: Convert unnecessary use of GDBusProxy to using GDBusConnection directly Hi, thank you for converting those calls to GDBus. >--- a/panels/printers/pp-new-printer-dialog.c >+++ b/panels/printers/pp-new-printer-dialog.c >@@ -530,13 +522,17 @@ devices_get_cb (GObject *source_object, ... >+ output = g_dbus_connection_call_sync (bus, >+ SCP_BUS, >+ SCP_PATH, >+ SCP_IFACE, >+ "GroupPhysicalDevices", >+ g_variant_new ("(v)", g_variant_builder_end (&device_list)), >+ G_VARIANT_TYPE ("()"), >+ G_DBUS_CALL_FLAGS_NONE, >+ 60000, >+ NULL, >+ &error); There should be NULL or G_VARIANT_TYPE ("(aas)") instead of G_VARIANT_TYPE ("()"). Otherwise than that it is ok so you can commit it to master after the change. Regards Marek
Thanks Marek, I've pushed these changes (7dd5ae6b94d1b294032d0f0fae18df12322eb1c2 and the two following). You pointed to three cases in the review of using pp->devices[device_id].device_info but the third case is using .display_name so assuming that was in error. I've made the device_info change in a separate commit as I'm pretty sure device_class, device_uri, device_make_and_model, device_info and display_name are always non-NULL: ... printer_informations = line_split (standard_output); length = g_strv_length (printer_informations); if (length >= 4) { ... pp->devices[pp->num_devices].device_class = g_strdup (printer_informations[0]); pp->devices[pp->num_devices].device_uri = g_strdup (printer_informations[1]); pp->devices[pp->num_devices].device_make_and_model = g_strdup (printer_informations[2]); pp->devices[pp->num_devices].device_info = g_strdup (printer_informations[3]); pp->devices[pp->num_devices].display_name = g_strdup (printer_informations[3]); ...