GNOME Bugzilla – Bug 693183
GNOME Printer Setup Tool: List of discovered printers should better describe duplicate entries
Last modified: 2014-11-10 11:06:00 UTC
When adding a printer a list of discovered printers is shown. If there are duplicate entries, for example if the same network printer is discovered with different network protocols (LPD, IPP, ...), these entries are usually grouped into one by using the D-Bus service of system-config-printer. system-config-printer groups entries coming from the network connection of the printer and groups entries coming from the USB connection of the printer, but it does not group network results with USB results, as system-config-printer separates them by a tree view. This means that when one connects one printer by both network and USB one gets two entries, one generated from all appearances under the discovered network printers and one from all appearances under the discovered USB printers (and if this printer does Bluetooth there would be a third entry). Now the g-c-c printer tool lists these entries as <model name> <model name>-2 This way the user does not know which is the network printer and which is the USB printer. The results dhould look like <model name> (USB) <model name> (Network) To reproduce connect a printer with both network and USB (or Bluetooth) input by both network and USB, then try to set it up with the preferred connection type. Also reported at Ubuntu: https://bugs.launchpad.net/ubuntu/+source/gnome-control-center/+bug/1115669
Sounds like a broken situation to me to connect the same printer via both network and usb. But I agree that the labels you propose are nicer that -2
Created attachment 267519 [details] [review] Show type of connection of found devices We can at least show connection type for found devices. The attached patch does this. It describes the devices connection types similarly to system-config-printer.
Created attachment 267521 [details] screenshot for showing of types of found devices The screenshot shows some of those types (the dialog was enlarged just for creation of the screenshot - it is not part of the patch).
Created attachment 267650 [details] [review] Set correct hostname for found devices This patch is needed for showing of hostnames of network printers. It parses URI and set hostname of found printers appropriately.
Created attachment 267651 [details] [review] Show type of connection of found devices Update of the patch with a little different wording and comments for translators.
Created attachment 267652 [details] [review] Show type of connection of found devices I sent the original patch by mistake, this is the updated one.
Created attachment 267654 [details] screenshot for showing of types of found devices Updated screenshot.
Created attachment 268910 [details] [review] Show type of connection of found devices Here is another update of the patch. It discovers more details about found devices.
Created attachment 268911 [details] updates screenshot
Created attachment 269006 [details] [review] Show type of connection of found devices I've made those descriptions more compact after discussion with Allan. It shows just name of the protocol used for connection of the device and location or hostname of the device separated by a dash if available. I prefer location specified in the setting of the printer over its hostname since it is usually something more understandable for users and it is usually specified by an administrator (e.g. "1st floor kitchen", "Reception", ...).
Created attachment 269007 [details] updated screenshot
Created attachment 269031 [details] [review] Show type of connection of found devices (In reply to comment #10) > Created an attachment (id=269006) [details] [review] > Show type of connection of found devices > > I've made those descriptions more compact after discussion with Allan. > It shows just name of the protocol used for connection of the device and > location or hostname of the device separated by a dash if available. > I prefer location specified in the setting of the printer over its hostname > since it is usually something more understandable for users and it is usually > specified by an administrator (e.g. "1st floor kitchen", "Reception", ...). I changed it a little yet. It works now so that if there is a location of the printer specified (e.g. Kitchen) then we show just the location and we don't show the protocol because I assume that administrator set this descriptive enough. If there isn't the location specified we show the protocol and depending on whether we have the hostname available also the hostname. The device name is shown in all cases. I've also put the hostname into apostrophes because it can be very cryptical sometimes (automatically generated queues, IP addresses, ...).
Created attachment 269033 [details] updated screenshot
Review of attachment 267650 [details] [review]: ::: panels/printers/pp-utils.c @@ +3597,3 @@ + if (value != NULL) + { + status = httpSeparateURI (HTTP_URI_CODING_ALL, What does this function do? Can you use g_filename_from_uri() instead? tmp = g_filename_from_uri (value, &hostname, NULL); g_free (tmp);
Review of attachment 269031 [details] [review]: The strings could really do with a lot of work and review. Those are the ones I find bad, on top of my head. This needs to go through the designers. ::: panels/printers/pp-new-printer-dialog.c @@ +1631,3 @@ + else if (g_str_has_prefix (device_uri, "hal")) + /* Translators: The found device is a local printer detected by the Hardware Abstraction Layer */ + description = g_strdup (_("HAL")); This really shouldn't be in a visible string. @@ +1634,3 @@ + else if (g_str_has_prefix (device_uri, "hp:")) + /* Translators: The found device is a printer supported by HP Linux Imaging and Printing project */ + description = g_strdup (_("HPLIP")); This isn't really a connection type. @@ +1637,3 @@ + else if (g_str_has_prefix (device_uri, "hpfax:")) + /* Translators: The found device is a fax machine supported by HP Linux Imaging and Printing project */ + description = g_strdup (_("HPLIP fax machine")); Neither is this. @@ +1640,3 @@ + else if (g_str_has_prefix (device_uri, "smb")) + /* Translators: The found device is a printer connected via Samba protocol */ + description = g_strdup (_("Samba")); Samba isn't a protocol name. SMB is. "Windows Printer Sharing" is probably a better name. @@ +1646,3 @@ + if (g_strrstr (device_uri, "._ipp") != NULL) + /* Translators: The found device is an IPP printer connected via DNS-SD protocol */ + description = g_strdup (_("DNS-SD - IPP")); Why do we need to know that it's been discovered via DNS-SD?
(In reply to comment #14) > Review of attachment 267650 [details] [review]: > > ::: panels/printers/pp-utils.c > @@ +3597,3 @@ > + if (value != NULL) > + { > + status = httpSeparateURI (HTTP_URI_CODING_ALL, > > What does this function do? It parses given URI into its components. We use hostname and scheme part of the URI on following lines. > Can you use g_filename_from_uri() instead? > > tmp = g_filename_from_uri (value, &hostname, NULL); > g_free (tmp); No, I can't. g_filename_from_uri() is just for "file:" URIs.
Created attachment 269378 [details] [review] Show type of connection of found devices (In reply to comment #15) > Review of attachment 269031 [details] [review]: > > The strings could really do with a lot of work and review. > > Those are the ones I find bad, on top of my head. This needs to go through the > designers. I'll ask Allan to look at it. > ::: panels/printers/pp-new-printer-dialog.c > @@ +1631,3 @@ > + else if (g_str_has_prefix (device_uri, "hal")) > + /* Translators: The found device is a local printer detected by the > Hardware Abstraction Layer */ > + description = g_strdup (_("HAL")); > > This really shouldn't be in a visible string. I've removed this part together with the one for "file:" type URIs. They shouldn't appear to common user. I added them just for completeness there. > @@ +1634,3 @@ > + else if (g_str_has_prefix (device_uri, "hp:")) > + /* Translators: The found device is a printer supported by HP Linux > Imaging and Printing project */ > + description = g_strdup (_("HPLIP")); > > This isn't really a connection type. I've changed the description to "HPLIP Driven Printer" to indicate that the device is compatible with HPLIP. > @@ +1637,3 @@ > + else if (g_str_has_prefix (device_uri, "hpfax:")) > + /* Translators: The found device is a fax machine supported by HP > Linux Imaging and Printing project */ > + description = g_strdup (_("HPLIP fax machine")); > > Neither is this. I've changed this to "HPLIP Driven Fax Machine". > @@ +1640,3 @@ > + else if (g_str_has_prefix (device_uri, "smb")) > + /* Translators: The found device is a printer connected via Samba > protocol */ > + description = g_strdup (_("Samba")); > > Samba isn't a protocol name. SMB is. "Windows Printer Sharing" is probably a > better name. I've changed it to "Windows Printer Share". > @@ +1646,3 @@ > + if (g_strrstr (device_uri, "._ipp") != NULL) > + /* Translators: The found device is an IPP printer connected via > DNS-SD protocol */ > + description = g_strdup (_("DNS-SD - IPP")); > > Why do we need to know that it's been discovered via DNS-SD? It can help user to distinguish between automatically found printers (browsed on the network) and those which he had to search for specifically. Marek
Created attachment 269379 [details] updated screenshot
Comment on attachment 269378 [details] [review] Show type of connection of found devices We've agreed with Allan that we need to simplify this more.
Created attachment 269805 [details] [review] Set hostname for devices with no hostname (In reply to comment #16) > (In reply to comment #14) > > Review of attachment 267650 [details] [review] [details]: > > > > ::: panels/printers/pp-utils.c > > @@ +3597,3 @@ > > + if (value != NULL) > > + { > > + status = httpSeparateURI (HTTP_URI_CODING_ALL, > > > > What does this function do? > > It parses given URI into its components. We use hostname and scheme part of the > URI on following lines. > > > > Can you use g_filename_from_uri() instead? > > > > tmp = g_filename_from_uri (value, &hostname, NULL); > > g_free (tmp); > > No, I can't. g_filename_from_uri() is just for "file:" URIs. I've changed this patch. It tries to find hostname for all devices for which we don't have hostname yet. We set it during adding of the device to the list of found devices in the new printer dialog. I've also added handling of dnssd, mdns, hp and hpfax URIs there.
Created attachment 269806 [details] [review] Show connection type of found devices I've changed it so that the dialog shows just type of local connection ("USB", "Parallel Port", "Serial Port", Bluetooth) or just location for network printer or just address of network printer on the second line. The fact that a printer is a network printer is shown by the icon. Location takes precedence if we know hostname and also location of a network printer.
Created attachment 269807 [details] updated screenshot
Btw, I'll solve the duplicates by another patch which will try to group duplicate entries better using the method GroupPhysicalDevices.
Created attachment 269811 [details] [review] Show type of connection of found devices (In reply to comment #21) > Created an attachment (id=269806) [details] [review] > Show connection type of found devices > > I've changed it so that the dialog shows just type of local connection ("USB", > "Parallel Port", "Serial Port", Bluetooth) or just location for network printer > or just address of network printer on the second line. The fact that a printer > is a network printer is shown by the icon. > Location takes precedence if we know hostname and also location of a network > printer. This applies better to master.
(In reply to comment #22) > Created an attachment (id=269807) [details] > updated screenshot Looks great! One small niggle - the subtext (eg. "USB") should be white when the row is selected.
(In reply to comment #25) > (In reply to comment #22) > > Created an attachment (id=269807) [details] [details] > > updated screenshot > > Looks great! One small niggle - the subtext (eg. "USB") should be white when > the row is selected. Thank you. I know about the problem of the gray text but I don't know about a simple solution right now. I'll try to solve this later.
Created attachment 270116 [details] [review] Don't set color for selected text (In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #22) > > > Created an attachment (id=269807) [details] [details] [details] > > > updated screenshot > > > > Looks great! One small niggle - the subtext (eg. "USB") should be white when > > the row is selected. > > Thank you. I know about the problem of the gray text but I don't know about a > simple solution right now. I'll try to solve this later. Here is a fix for the color of the description text of the selected device.
Created attachment 270117 [details] updated screenshot
Review of attachment 269805 [details] [review]: Looks fine.
Review of attachment 269811 [details] [review]: ::: panels/printers/pp-new-printer-dialog.c @@ +1771,3 @@ + { + /* Translators: Name of found network printer followed by its location (e.g. Kitchen, Reception) */ + display_string = g_markup_printf_escaped (_("<b>%s</b>\n<small><span foreground=\"#555555\">Location: %s</span></small>"), Split this up in a way that doesn't require translators to edit HTML like strings (or change the colours in the translation) @@ +1779,3 @@ + if (device->host_name != NULL && device->host_name[0] != '\0') + /* Translators: Name of found network printer followed by its network address */ + display_string = g_markup_printf_escaped (_("<b>%s</b>\n<small><span foreground=\"#555555\">Address: %s</span></small>"), Ditto.
Review of attachment 270116 [details] [review]: Looks fine.
Comment on attachment 269805 [details] [review] Set hostname for devices with no hostname (In reply to comment #29) > Review of attachment 269805 [details] [review]: > > Looks fine. Thank you for the review. I've pushed this patch to master. There is no UI change or string change in it, it just fills an internal string for found devices. I did a little modification in it, there should be the string "device->device_uri" checked for the socket, lpd, ipp or smb instead of the "scheme".
Created attachment 270370 [details] [review] Don't set color for selected text (In reply to comment #31) > Review of attachment 270116 [details] [review]: > > Looks fine. I've updated it so that it applies before the next patch.
Created attachment 270372 [details] [review] Show connection type of found devices (In reply to comment #30) > Review of attachment 269811 [details] [review]: > > ::: panels/printers/pp-new-printer-dialog.c > @@ +1771,3 @@ > + { > + /* Translators: Name of found network printer followed by > its location (e.g. Kitchen, Reception) */ > + display_string = g_markup_printf_escaped > (_("<b>%s</b>\n<small><span foreground=\"#555555\">Location: > %s</span></small>"), > > Split this up in a way that doesn't require translators to edit HTML like > strings (or change the colours in the translation) > > @@ +1779,3 @@ > + if (device->host_name != NULL && device->host_name[0] != > '\0') > + /* Translators: Name of found network printer followed by > its network address */ > + display_string = g_markup_printf_escaped > (_("<b>%s</b>\n<small><span foreground=\"#555555\">Address: > %s</span></small>"), > > Ditto. I've moved the markup tags from translated strings in the previous patch.
Review of attachment 270370 [details] [review]: Looks good.
Review of attachment 270372 [details] [review]: Looks good otherwise. ::: panels/printers/pp-new-printer-dialog.c @@ +1704,3 @@ + if (g_str_has_prefix (device_uri, "usb") || + g_str_has_prefix (device_uri, "hp:/usb/") || + g_str_has_prefix (device_uri, "hpfax:/usb/")) Please add braces around the conditional block when it's multiple lines (even if one of those lines is a comment) @@ +1761,3 @@ + if (description == NULL) + { + if (device->device_location != NULL && device->device_location[0] != '\0') Ditto.
(In reply to comment #36) > Review of attachment 270372 [details] [review]: > > Looks good otherwise. > > ::: panels/printers/pp-new-printer-dialog.c > @@ +1704,3 @@ > + if (g_str_has_prefix (device_uri, "usb") || > + g_str_has_prefix (device_uri, "hp:/usb/") || > + g_str_has_prefix (device_uri, "hpfax:/usb/")) > > Please add braces around the conditional block when it's multiple lines (even > if one of those lines is a comment) > > @@ +1761,3 @@ > + if (description == NULL) > + { > + if (device->device_location != NULL && > device->device_location[0] != '\0') > > Ditto. Thank you for the reviews. I'll add the brackets there right before the push.
Created attachment 270375 [details] [review] Show connection type of found devices (In reply to comment #37) > (In reply to comment #36) > > Review of attachment 270372 [details] [review] [details]: > > > > Looks good otherwise. > > > > ::: panels/printers/pp-new-printer-dialog.c > > @@ +1704,3 @@ > > + if (g_str_has_prefix (device_uri, "usb") || > > + g_str_has_prefix (device_uri, "hp:/usb/") || > > + g_str_has_prefix (device_uri, "hpfax:/usb/")) > > > > Please add braces around the conditional block when it's multiple lines (even > > if one of those lines is a comment) > > > > @@ +1761,3 @@ > > + if (description == NULL) > > + { > > + if (device->device_location != NULL && > > device->device_location[0] != '\0') > > > > Ditto. > > Thank you for the reviews. I'll add the brackets there right before the push. I'm attaching the requested change yet because of the break request.
Review of attachment 270375 [details] [review]: Looks good.
Comment on attachment 270375 [details] [review] Show connection type of found devices Thank you for the reviews. I've pushed the patches into master. I'm closing this bug and as a next step I'll try to improve the detection of duplicates in https://bugzilla.gnome.org/show_bug.cgi?id=693186. Marek
*** Bug 728175 has been marked as a duplicate of this bug. ***