Review of attachment 195976 [details] [review]:

You're leaking memory in various places, and the coding style doesn't match what's already there.

I would also make the get_missing_executables() and find_packages_for_executables() use GHashTables to avoid the duplicates in the first place (so you don't need to sort and uniq the GLists).

::: plugins/print-notifications/gsd-printer.c
@@ +398,3 @@
+                        name_index++;
+                } else
+

missing { and it doesn't line up.

@@ +454,3 @@
+                                                 input,
+                                                 G_DBUS_CALL_FLAGS_NONE,
+                }

magic values should use defines.

@@ +511,3 @@
+                        if (error) {
+                                g_warning ("%s", error->message);
+                                               NULL,

g_error_free() actually.

@@ +562,3 @@
+                        if (error) {
+                                g_warning ("%s", error->message);
+                                                tuple = g_variant_get_child_value (array, i);

Ditto.

@@ +616,3 @@
+                fd = g_file_open_tmp ("ccXXXXXX", &file_name, &error);
+
+static gchar *

No need to check for !error.
And you're leaking error.

@@ +719,3 @@
+                                               MECHANISM_BUS,
+                                               NULL,
+                      new_name = g_strdup (name);

You're leaking the error again.

@@ +721,3 @@
+                                               &error);
+
+

Turn this condition around, and exit early instead of creating gigantic if branches.

@@ +784,3 @@
+                                if (error) {
+                                        g_warning ("%s", error->message);
+        gint         i;

g_error_free() again.

Created attachment 196006 [details] [review]
find best matching ppd modified

Hi Bastien,

thank you for your review. I've modified the patch according to it. The patch is attached.

Marek

Review of attachment 196006 [details] [review]:

Avoid the giant branches, it makes reading the code really hard.

Also do not check for "error != NULL", check for the error retval for the particular function.

::: plugins/print-notifications/gsd-printer.c
@@ +87,3 @@
+        GHashTable *executables = NULL;
+
+        if (ppd_file_name) {

if !ppd_file_name.

@@ +137,3 @@
+                }
+
+                                                         "MissingExecutables",

That's incorrect. You need to check for proxy == NULL, not for error != NULL.

@@ +151,3 @@
+        GHashTable *packages = NULL;
+
+                        if (output && g_variant_n_children (output) == 1) {

Giant branch!

@@ +238,3 @@
+                                                       &error);
+
+                                                       NULL,

Giant branch!

@@ +309,3 @@
+                                               &error);
+
+                                }

Again, exit out of the loop if you have !proxy instead of creating a giant branch.

@@ +340,3 @@
+                                                                ppd_name = g_strdup (name);
+
+

Why do you strdup() match and name to free them a couple of lines below?

@@ +456,3 @@
+                                               &error);
+
+                        g_object_unref (proxy);

if !proxy

@@ +502,3 @@
+        GError     *error = NULL;
+
+        gchar       *match = NULL;

if !printer_name return;

@@ +552,3 @@
+        GError     *error = NULL;
+
+                                                 "GetBestDrivers",

Ditto.

@@ +608,3 @@
+                                   cupsEncryption ());
+
+                }

if !http.

@@ +702,3 @@
+        ipp_t *response = NULL;
+
+        gchar       *new_name = NULL;

if !printer_name

@@ +753,3 @@
+                locale = setlocale (LC_MESSAGES, NULL);
+
+                                already_present = TRUE;

if !locale...

@@ +839,3 @@
+        printer_name = create_name (device_id);
+
+                                       ppd_name ? ppd_name : "",

Again.

@@ +959,3 @@
+
+                        ppd_file_name = cupsGetPPD (name);
+                        if (ppd_file_name) {

If !ppf_file_name.

@@ +960,3 @@
+                        ppd_file_name = cupsGetPPD (name);
+                        if (ppd_file_name) {
+                                GHashTable *executables = NULL;

Why do you initialise them when you set them 2 lines below?

Created attachment 196084 [details] [review]
modified patch

I have change the patch according to your comment except one thing:

> 959	 
> 960	ppd_file_name = cupsGetPPD (name);
> 961	if (ppd_file_name) {
> 	
> If !ppf_file_name.

Changing this would insert more code.

Marek

Review of attachment 196084 [details] [review]:

::: plugins/print-notifications/gsd-printer.c
@@ +725,3 @@
+        commands_lowercase = g_ascii_strdown (commands, -1);
+
+                        name = g_strcanon (name, ALLOWED_CHARACTERS, '-');

lower case isn't going to match upper case letters.

Committed with a few bug/style fixes in separate commits.

Hi,

thank you for the commit and those fixes.

Marek