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 749830 - New printer dialog installs wrong printer
New printer dialog installs wrong printer
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Printers
3.14.x
Other Linux
: Normal normal
: ---
Assigned To: Marek Kašík
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-25 12:28 UTC by Marek Kašík
Modified: 2015-08-03 15:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Install the selected printer (4.76 KB, patch)
2015-05-25 12:30 UTC, Marek Kašík
none Details | Review
Install the selected printer (4.72 KB, patch)
2015-06-17 10:48 UTC, Marek Kašík
none Details | Review
Cache pointer to device-treeview (9.39 KB, patch)
2015-06-30 11:18 UTC, Marek Kašík
none Details | Review
Use GtkListStore for device list (43.03 KB, patch)
2015-06-30 11:20 UTC, Marek Kašík
none Details | Review
Use macro for getting widgets from builder (7.24 KB, patch)
2015-07-17 13:35 UTC, Marek Kašík
committed Details | Review
Cache pointer to device-treeview (8.62 KB, patch)
2015-07-17 13:37 UTC, Marek Kašík
committed Details | Review
Use GtkListStore for device list (38.07 KB, patch)
2015-07-17 13:39 UTC, Marek Kašík
none Details | Review
Use GtkListStore for device list (41.21 KB, patch)
2015-07-20 10:39 UTC, Marek Kašík
none Details | Review
Make PpPrintDevice a regular class (80.55 KB, patch)
2015-07-29 14:17 UTC, Marek Kašík
committed Details | Review
Merge device-class and is-network-device properties (12.76 KB, patch)
2015-07-29 14:18 UTC, Marek Kašík
committed Details | Review
Use GtkListStore for device list (44.23 KB, patch)
2015-07-29 14:19 UTC, Marek Kašík
none Details | Review
Use GtkListStore for device list (44.90 KB, patch)
2015-07-31 16:03 UTC, Marek Kašík
committed Details | Review

Description Marek Kašík 2015-05-25 12:28:25 UTC
If there are several devices of the same name detected during running of the 'New printer dialog' then selecting one of them and clicking 'Add' could lead to installation of different printer than the one which user wanted to install.
This is caused by getting of the PpDevice, which will be installed, based on name because there can be several devices with the same name (different interfaces of a printer, several printers of the same model on one network...).
Comment 1 Marek Kašík 2015-05-25 12:30:36 UTC
Created attachment 303919 [details] [review]
Install the selected printer

This patch gets selected printer according to assigned index and not just a name. We should get this also into 3.14.
Comment 2 Bastien Nocera 2015-06-17 10:14:51 UTC
Review of attachment 303919 [details] [review]:

::: panels/printers/pp-new-printer-dialog.c
@@ +1656,3 @@
   GList                     *item;
   gchar                     *description;
+  gsize                      index = 0;

why gsize if the column is an int64? Use gint64.

@@ +2061,3 @@
 
+      if (index >= 0)
+        device = (PpPrintDevice *) g_list_nth_data (priv->devices, index);

No need for casting here.

Keep separate lists and liststore (priv->devices, ) seems fragile and error-prone to me. For example, aren't you changing the ordering of the list in get_authenticated_samba_devices_cb() when you call remove_device_from_list()?

I would use one single storage, the GtkListStore.
Comment 3 Marek Kašík 2015-06-17 10:48:33 UTC
Created attachment 305470 [details] [review]
Install the selected printer

(In reply to Bastien Nocera from comment #2)
> Review of attachment 303919 [details] [review] [review]:
> 
> ::: panels/printers/pp-new-printer-dialog.c
> @@ +1656,3 @@
>    GList                     *item;
>    gchar                     *description;
> +  gsize                      index = 0;
> 
> why gsize if the column is an int64? Use gint64.

Changed.


> @@ +2061,3 @@
>  
> +      if (index >= 0)
> +        device = (PpPrintDevice *) g_list_nth_data (priv->devices, index);
> 
> No need for casting here.

Changed.


> Keep separate lists and liststore (priv->devices, ) seems fragile and
> error-prone to me. For example, aren't you changing the ordering of the list
> in get_authenticated_samba_devices_cb() when you call
> remove_device_from_list()?

I always regenerate the liststore using actualize_devices_list() once it is edited. 


> I would use one single storage, the GtkListStore.

The GList simplifies the work with the list of devices. E.g. passing it to canonicalize_device_name() is easier than using the liststore. We also have the new_devices GList which has the same format so combining it with the list of devices is simpler.
Comment 4 Marek Kašík 2015-06-17 10:52:00 UTC
Handling e.g. the liststore in group_physical_devices_cb() from https://bugzilla.gnome.org/show_bug.cgi?id=693186#c3 would make it quite challenging...
Comment 5 Marek Kašík 2015-06-30 11:18:30 UTC
Created attachment 306386 [details] [review]
Cache pointer to device-treeview
Comment 6 Marek Kašík 2015-06-30 11:20:34 UTC
Created attachment 306387 [details] [review]
Use GtkListStore for device list

I've removed the GList holding printing devices and used the already present GtkListStore for this. There is quite a lot of inseparable changes because of this in the patch.
Comment 7 Bastien Nocera 2015-07-01 13:44:41 UTC
Review of attachment 306386 [details] [review]:

::: panels/printers/pp-new-printer-dialog.c
@@ +338,3 @@
 
+  if (gtk_tree_selection_get_selected (
+        gtk_tree_view_get_selection (priv->treeview), &model, &iter))

Eek indentation. Our screens are wide, don't linefeed in the middle of a line like that.

@@ +397,3 @@
 
+  priv->treeview = (GtkTreeView *)
+    gtk_builder_get_object (priv->builder, "devices-treeview");

Ditto.

Maybe you should look at using templates instead?

@@ +491,3 @@
   gboolean                   selected;
 
+  selected = gtk_tree_selection_get_selected (

Ditto.

@@ +504,1 @@
+      widget = (GtkWidget*)

Ditto.

@@ +508,1 @@
+      widget = (GtkWidget*)

Ditto.

@@ +512,1 @@
+      notebook = (GtkWidget*)

Ditto.

@@ +2036,3 @@
       g_clear_object (&priv->cancellable);
 
+      if (gtk_tree_selection_get_selected (

Linefeed again.
Comment 8 Bastien Nocera 2015-07-01 13:56:00 UTC
Review of attachment 306387 [details] [review]:

::: panels/printers/pp-new-printer-dialog.c
@@ +406,3 @@
     gtk_builder_get_object (priv->builder, "devices-treeview");
 
+  priv->store = (GtkListStore *)

Line feeds.

@@ +409,3 @@
+    gtk_builder_get_object (priv->builder, "devices-liststore");
+
+  priv->filter = (GtkTreeModelFilter *)

Ditto.

@@ +565,1 @@
+  if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store), &iter))

The idiomatic way to do this is:

cont = gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store), &iter);
while (cont) {
  ...
  cont = gtk_tree_model_iter_next (GTK_TREE_MODEL (priv->store), &iter);
}

@@ +708,3 @@
+  GtkTreeIter    iter;
+
+  if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (device_liststore), &iter))

Ditto about the way to do that loop.

@@ +747,3 @@
+              priv->samba_searching;
+
+  spinner = (GtkWidget *)

No need for line feed.

@@ +828,3 @@
 
+      for (i = 0; device_uris[i] != NULL; i++)
+        g_strfreev (device_uris[i]);

That looks like a separate fix/cleanup.

@@ +937,3 @@
+
+static gsize
+list_store_length (GtkListStore *store)

len = gtk_tree_model_iter_n_children (model, NULL);

@@ +1572,3 @@
       words_length = g_strv_length (words);
 
+      if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store), &titer))

List loop again.

@@ +1619,3 @@
   if (!found && words_length == 1)
     {
+      if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store), &titer))

Ditto.
Comment 9 Marek Kašík 2015-07-17 13:35:34 UTC
Created attachment 307619 [details] [review]
Use macro for getting widgets from builder

(In reply to Bastien Nocera from comment #7)
> Review of attachment 306386 [details] [review] [review]:
> 
> ::: panels/printers/pp-new-printer-dialog.c
> @@ +397,3 @@
>  
> +  priv->treeview = (GtkTreeView *)
> +    gtk_builder_get_object (priv->builder, "devices-treeview");
> 
> Ditto.
> 
> Maybe you should look at using templates instead?

I've added there macro WID() for getting widgets from the builder.
Comment 10 Marek Kašík 2015-07-17 13:37:26 UTC
Created attachment 307620 [details] [review]
Cache pointer to device-treeview

(In reply to Bastien Nocera from comment #7)
> Review of attachment 306386 [details] [review] [review]:
> 
> ::: panels/printers/pp-new-printer-dialog.c
> @@ +338,3 @@
>  
> +  if (gtk_tree_selection_get_selected (
> +        gtk_tree_view_get_selection (priv->treeview), &model, &iter))
> 
> Eek indentation. Our screens are wide, don't linefeed in the middle of a
> line like that.

Done.


> @@ +397,3 @@
>  
> +  priv->treeview = (GtkTreeView *)
> +    gtk_builder_get_object (priv->builder, "devices-treeview");
> 
> Ditto.

Done.


> @@ +491,3 @@
>    gboolean                   selected;
>  
> +  selected = gtk_tree_selection_get_selected (
> 
> Ditto.

Done.


> @@ +504,1 @@
> +      widget = (GtkWidget*)
> 
> Ditto.

Done.


> @@ +508,1 @@
> +      widget = (GtkWidget*)
> 
> Ditto.

Done.


> @@ +512,1 @@
> +      notebook = (GtkWidget*)
> 
> Ditto.

Done.


> @@ +2036,3 @@
>        g_clear_object (&priv->cancellable);
>  
> +      if (gtk_tree_selection_get_selected (
> 
> Linefeed again.

Done.
Comment 11 Marek Kašík 2015-07-17 13:39:36 UTC
Created attachment 307621 [details] [review]
Use GtkListStore for device list

(In reply to Bastien Nocera from comment #8)
> Review of attachment 306387 [details] [review] [review]:
> 
> ::: panels/printers/pp-new-printer-dialog.c
> @@ +406,3 @@
>      gtk_builder_get_object (priv->builder, "devices-treeview");
>  
> +  priv->store = (GtkListStore *)
> 
> Line feeds.

Done.


> @@ +409,3 @@
> +    gtk_builder_get_object (priv->builder, "devices-liststore");
> +
> +  priv->filter = (GtkTreeModelFilter *)
> 
> Ditto.

Done.


> @@ +565,1 @@
> +  if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store), &iter))
> 
> The idiomatic way to do this is:
> 
> cont = gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store), &iter);
> while (cont) {
>   ...
>   cont = gtk_tree_model_iter_next (GTK_TREE_MODEL (priv->store), &iter);
> }

Changed.


> @@ +708,3 @@
> +  GtkTreeIter    iter;
> +
> +  if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (device_liststore),
> &iter))
> 
> Ditto about the way to do that loop.

Changed.


> @@ +747,3 @@
> +              priv->samba_searching;
> +
> +  spinner = (GtkWidget *)
> 
> No need for line feed.

Done.


> @@ +828,3 @@
>  
> +      for (i = 0; device_uris[i] != NULL; i++)
> +        g_strfreev (device_uris[i]);
> 
> That looks like a separate fix/cleanup.

Already pushed now.


> @@ +937,3 @@
> +
> +static gsize
> +list_store_length (GtkListStore *store)
> 
> len = gtk_tree_model_iter_n_children (model, NULL);

Thank you for the tip.


> @@ +1572,3 @@
>        words_length = g_strv_length (words);
>  
> +      if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store),
> &titer))
> 
> List loop again.

Changed.


> @@ +1619,3 @@
>    if (!found && words_length == 1)
>      {
> +      if (gtk_tree_model_get_iter_first (GTK_TREE_MODEL (priv->store),
> &titer))
> 
> Ditto.

Changed.
Comment 12 Bastien Nocera 2015-07-17 14:15:23 UTC
Review of attachment 307619 [details] [review]:

Looks good.
Comment 13 Bastien Nocera 2015-07-17 14:17:27 UTC
Review of attachment 307620 [details] [review]:

Sure.
Comment 14 Bastien Nocera 2015-07-17 14:23:11 UTC
Review of attachment 307621 [details] [review]:

Looks fine otherwise.

::: panels/printers/pp-new-printer-dialog.c
@@ +443,3 @@
+                      -1);
+  pp_print_device_free (device);
+

Remove linefeed here, or add one before the free above.

@@ +586,3 @@
+
+  if (device != NULL)
+    *list = g_list_append (*list, g_strdup (device->device_original_name));

Here you should prepend the names...

@@ +622,3 @@
+          gtk_tree_model_foreach (GTK_TREE_MODEL (priv->store),
+                                  append_original_name,
+                                  &original_names_list);

...And reverse the list after looping through the items.

@@ +1995,3 @@
+          gtk_tree_model_foreach (GTK_TREE_MODEL (priv->store),
+                                  append_original_name,
+                                  &original_names_list);

Ditto here about reversing the list.
Comment 15 Marek Kašík 2015-07-17 16:32:31 UTC
Comment on attachment 307621 [details] [review]
Use GtkListStore for device list

There is something wrong with the patch yet. I'll continue on it at Monday.
Comment 16 Marek Kašík 2015-07-20 10:39:06 UTC
Created attachment 307752 [details] [review]
Use GtkListStore for device list

(In reply to Bastien Nocera from comment #14)
> Review of attachment 307621 [details] [review] [review]:
> 
> Looks fine otherwise.
> 
> ::: panels/printers/pp-new-printer-dialog.c
> @@ +443,3 @@
> +                      -1);
> +  pp_print_device_free (device);
> +
> 
> Remove linefeed here, or add one before the free above.

I've added one before the free.


> @@ +586,3 @@
> +
> +  if (device != NULL)
> +    *list = g_list_append (*list, g_strdup (device->device_original_name));
> 
> Here you should prepend the names...

Done.


> @@ +622,3 @@
> +          gtk_tree_model_foreach (GTK_TREE_MODEL (priv->store),
> +                                  append_original_name,
> +                                  &original_names_list);
> 
> ...And reverse the list after looping through the items.

Done.


> @@ +1995,3 @@
> +          gtk_tree_model_foreach (GTK_TREE_MODEL (priv->store),
> +                                  append_original_name,
> +                                  &original_names_list);
> 
> Ditto here about reversing the list.

Done.



(In reply to Marek Kašík from comment #15)
> Comment on attachment 307621 [details] [review] [review]
> Use GtkListStore for device list
> 
> There is something wrong with the patch yet. I'll continue on it at Monday.

The problem was that I forgot to include the changes from pp-utils.c and pp-utils.h which caused some troubles (they were in the first patch too). I also had to place more update_dialog_state() in the code to update the dialog state correctly.

I'm sorry for another round of review.
Comment 17 Bastien Nocera 2015-07-27 11:30:01 UTC
Review of attachment 307752 [details] [review]:

::: panels/printers/new-printer-dialog.ui
@@ +17,3 @@
+      <column type="gboolean"/>
+      <!-- column-name device -->
+      <column type="gpointer"/>

Any reason why this can't be an object? (even a GBoxed one)

That would remove the need to clear the GtkListStore by hand.

::: panels/printers/pp-utils.c
@@ +4222,2 @@
         {
+          device_original_name = (gchar *) iter->data;

The cast isn't needed.
Comment 18 Marek Kašík 2015-07-27 14:33:37 UTC
(In reply to Bastien Nocera from comment #17)
> Review of attachment 307752 [details] [review] [review]:
> 
> ::: panels/printers/new-printer-dialog.ui
> @@ +17,3 @@
> +      <column type="gboolean"/>
> +      <!-- column-name device -->
> +      <column type="gpointer"/>
> 
> Any reason why this can't be an object? (even a GBoxed one)
>
> That would remove the need to clear the GtkListStore by hand.

I'm going to make it a GObject, it will just take some time.

> ::: panels/printers/pp-utils.c
> @@ +4222,2 @@
>          {
> +          device_original_name = (gchar *) iter->data;
> 
> The cast isn't needed.
Comment 19 Marek Kašík 2015-07-29 14:17:15 UTC
Created attachment 308399 [details] [review]
Make PpPrintDevice a regular class

This patch changes PpPrintDevice into a regular class.
Comment 20 Marek Kašík 2015-07-29 14:18:23 UTC
Created attachment 308400 [details] [review]
Merge device-class and is-network-device properties

'device-class' property of PpPrintDevice can contain "network" or "direct" values. This information can be stored in already present property 'is-network-device' as well. This patch merges the into 'is-network-device'.
Comment 21 Marek Kašík 2015-07-29 14:19:23 UTC
Created attachment 308401 [details] [review]
Use GtkListStore for device list

(In reply to Marek Kašík from comment #18)
> (In reply to Bastien Nocera from comment #17)
> > Review of attachment 307752 [details] [review] [review] [review]:
> > ::: panels/printers/pp-utils.c
> > @@ +4222,2 @@
> >          {
> > +          device_original_name = (gchar *) iter->data;
> > 
> > The cast isn't needed.

Removed.
Comment 22 Bastien Nocera 2015-07-29 15:37:38 UTC
Review of attachment 308399 [details] [review]:

Couple of nitpicks, looks fine otherwise.

::: panels/printers/pp-host.c
@@ +406,3 @@
+                                     "device-uri", device_uri,
+                                     "device-name", dests[i].name,
+                                     "device-location", cupsGetOption ("printer-location",

Split this up so as to assign it to a real variable, make the g_object_new() much nicer.

@@ +775,3 @@
+                                 "device-uri", device_uri,
+                                 /* Translators: The found device is a Line Printer Daemon printer */
+                                 "device-name", _("LPD Printer"),

Could we, in a separate bug, rename that to "Legacy Unix Printer"?

::: panels/printers/pp-new-printer-dialog.c
@@ +562,3 @@
+          g_object_set (store_device,
+                        "device-original-name", pp_print_device_get_device_name (device),
+                        "is-network-device", g_strcmp0 (pp_print_device_get_device_class (device), "network") == 0,

Assign to a separate variable as well.

::: panels/printers/pp-samba.c
@@ +362,3 @@
                 {
+                  device = g_object_new (PP_TYPE_PRINT_DEVICE,
+                                         "host-name", g_str_has_prefix (dirname, "smb://") ? dirname + 6 : dirname,

In a separate variable.

@@ +376,3 @@
             {
+              device = g_object_new (PP_TYPE_PRINT_DEVICE,
+                                     "host-name", g_str_has_prefix (dirname, "smb://") ? dirname + 6 : dirname,

Ditto.
Comment 23 Bastien Nocera 2015-07-29 15:39:28 UTC
Review of attachment 308400 [details] [review]:

Same style problems as earlier, but looks good.

::: panels/printers/pp-host.c
@@ +289,2 @@
           device = g_object_new (PP_TYPE_PRINT_DEVICE,
+                                 "is-network-device", g_strcmp0 (printer_informations[0], "network") == 0,

I prefer that in a separate variable.

::: panels/printers/pp-new-printer-dialog.c
@@ +897,3 @@
+                                                 "{ss}",
+                                                 "device-class",
+                                                 pp_print_device_is_network_device (all_devices[i]) ? "network" : "direct");

In a separate variable as well.

::: panels/printers/pp-utils.c
@@ +3530,2 @@
                       if (g_str_has_prefix (key, "device-class"))
+                        g_object_set (devices[index], "is-network-device", g_strcmp0 (value, "network") == 0, NULL);

With a separate variable as well.
Comment 24 Bastien Nocera 2015-07-29 16:00:39 UTC
Review of attachment 308401 [details] [review]:

Look at the refcounting for gtk_list_store_get() and gtk_list_store_set().

::: panels/printers/new-printer-dialog.ui
@@ +17,3 @@
+      <column type="gboolean"/>
+      <!-- column-name device -->
+      <column type="GObject"/>

Can you try with PpPrintDevice ?

::: panels/printers/pp-new-printer-dialog.c
@@ +532,3 @@
     {
+      gtk_tree_model_get (GTK_TREE_MODEL (priv->store), &iter,
+                          DEVICE_COLUMN, &device,

You're leaking a device here, because this adds a ref (1 for the list store + 1 for the get) and...

@@ +539,2 @@
         {
+          gtk_list_store_remove (priv->store, &iter);

...this remove just the list store ref. So you'll need to remove a ref for each pass of the loop.

@@ +558,3 @@
+
+  gtk_tree_model_get (model, iter,
+                      DEVICE_COLUMN, &device,

Leaking a device again here.

@@ +687,3 @@
+    {
+      gtk_tree_model_get (GTK_TREE_MODEL (device_liststore), &iter,
+                          DEVICE_COLUMN, &device,

Leak.

@@ +927,3 @@
                 {
+                  gtk_tree_model_get (GTK_TREE_MODEL (priv->store), &iter,
+                                      DEVICE_COLUMN, &device,

Leak.

@@ +1501,1 @@
+          if (device != NULL)

Shouldn't it always be != NULL?

And:
if (device == NULL) {
  cont = ...
  continue;
}

avoid another level of depth.

@@ +1543,3 @@
+          next_set = FALSE;
+          gtk_tree_model_get (GTK_TREE_MODEL (priv->store), &iter,
+                              DEVICE_COLUMN, &device,

leak.

@@ +1714,3 @@
                               DEVICE_DESCRIPTION_COLUMN, description,
+                              DEVICE_VISIBLE_COLUMN, TRUE,
+                              DEVICE_COLUMN, device,

Leak here.

@@ +1733,3 @@
                               SERVER_NEEDS_AUTHENTICATION_COLUMN, TRUE,
+                              DEVICE_VISIBLE_COLUMN, TRUE,
+                              DEVICE_COLUMN, device,

Leak.

@@ +1755,3 @@
+        {
+          gtk_tree_model_get (GTK_TREE_MODEL (priv->store), &iter,
+                              DEVICE_COLUMN, &device,

leak.

@@ +2082,3 @@
         {
           gtk_tree_model_get (model, &iter,
+                              DEVICE_COLUMN, &device,

I'm guessing leak as well here.
Comment 25 Marek Kašík 2015-07-30 14:33:46 UTC
Comment on attachment 308399 [details] [review]
Make PpPrintDevice a regular class

Thank you for the reviews, I've pushed the first 2 patches into master.

(In reply to Bastien Nocera from comment #22)
> Review of attachment 308399 [details] [review] [review]:
> @@ +775,3 @@
> +                                 "device-uri", device_uri,
> +                                 /* Translators: The found device is a Line
> Printer Daemon printer */
> +                                 "device-name", _("LPD Printer"),
> 
> Could we, in a separate bug, rename that to "Legacy Unix Printer"?
> 

And what about Line Printer Daemon Printer? I would like to keep the protocol there somehow.
Comment 26 Bastien Nocera 2015-07-30 14:36:19 UTC
(In reply to Marek Kašík from comment #25)
> Comment on attachment 308399 [details] [review] [review]
> Make PpPrintDevice a regular class
> 
> Thank you for the reviews, I've pushed the first 2 patches into master.
> 
> (In reply to Bastien Nocera from comment #22)
> > Review of attachment 308399 [details] [review] [review] [review]:
> > @@ +775,3 @@
> > +                                 "device-uri", device_uri,
> > +                                 /* Translators: The found device is a Line
> > Printer Daemon printer */
> > +                                 "device-name", _("LPD Printer"),
> > 
> > Could we, in a separate bug, rename that to "Legacy Unix Printer"?
> > 
> 
> And what about Line Printer Daemon Printer? I would like to keep the
> protocol there somehow.

That just expands the three-letter-acronym and doesn't explain to the user why the CUPS printer might be a better choice. But we can discuss that in the separate bug.
Comment 27 Marek Kašík 2015-07-31 16:03:20 UTC
Created attachment 308566 [details] [review]
Use GtkListStore for device list

(In reply to Bastien Nocera from comment #24)
> Review of attachment 308401 [details] [review] [review]:
> 
> Look at the refcounting for gtk_list_store_get() and gtk_list_store_set().

Thank you. I've added needed refs/unrefs in to the patch

> ::: panels/printers/new-printer-dialog.ui
> @@ +17,3 @@
> +      <column type="gboolean"/>
> +      <!-- column-name device -->
> +      <column type="GObject"/>
> 
> Can you try with PpPrintDevice ?

Done.

> @@ +1501,1 @@
> +          if (device != NULL)
> 
> Shouldn't it always be != NULL?

It should, I've removed the checks similar to this one.
Comment 28 Bastien Nocera 2015-08-03 14:46:27 UTC
Review of attachment 308566 [details] [review]:

Looks good otherwise.

::: panels/printers/pp-utils.c
@@ +4179,2 @@
         {
+          device_original_name = iter->data;

You can put the declaration here, it's only used in this block of code.
Comment 29 Marek Kašík 2015-08-03 15:00:14 UTC
Comment on attachment 308566 [details] [review]
Use GtkListStore for device list

(In reply to Bastien Nocera from comment #28)
> Review of attachment 308566 [details] [review] [review]:
> 
> Looks good otherwise.
> 
> ::: panels/printers/pp-utils.c
> @@ +4179,2 @@
>          {
> +          device_original_name = iter->data;
> 
> You can put the declaration here, it's only used in this block of code.

I've moved it there.

Thank you for all the reviews, I've put the last patch to master.