GNOME Bugzilla – Bug 693182
GNOME Printer Setup Tool: Printers are discovered only through a hard-coded list of CUPS backends
Last modified: 2013-03-14 14:47:33 UTC
Created attachment 235195 [details] /usr/lib/cups/backend/testbackend For development of printing infrastructure I use a test CUPS backend (attached, to be installed in /usr/lib/cups/backend/ and to be made executable) reporting printers as detected which in reality do not exist, so that I do not need the actual printer models for the test. Other users use manufacturer-supplied printer drivers which bring their own CUPS backends, and Ubuntu also ships the "hpfax" backend which is not in the hard-coded list. The source code of gnome-control-center, file panels/printers/pp-utils.c contains ---------- [...] static void get_device_attributes_async_scb (GHashTable *result, gpointer user_data) { [...] const gchar *backends[] = {"hpfax", "ncp", "beh", "bluetooth", "snmp", "dnssd", "hp", "ipp", "lpd", "parallel", "serial", "socket", "usb", NULL}; [...] ---------- and tests show that the tool is actually only discovering printers through these backends. It is easy to make CUPS using all backends for detecting printers, one can do it by a single function call. Other printer setup tools do it this way and I expect gnome-control-center to also use all CUPS backends, so that users who use special backends can set up their printers.
See also https://bugs.launchpad.net/gnome-control-center/+bug/1115653
hpfax _is_ in that list...
Sorry, hpfax was a bad example, but manufacturer-supoplied backends are still a problem.
Right. So, I think the first step here would be to find out why this list is there in the first place. Marek, why do we have that ? Are there manufacturer-specific backends that cause us problems ?
The list is there because of a time optimization of getting devices/getting attributes of a device. The Printers panel calls DevicesGet() for each backend separately and serially. This allow as to specify order of backends in which they are called so we can sort it according to response time of those backends (e.g. usb backend is much faster than snmp backend). The only thing we can improve here is the creation of the list of backends (and comments :) ). CUPS doesn't have a function which would return a list of those backends so we would need to list files of the directory which contains backends (see main() in http://svn.cups.org/public/cups/trunk/scheduler/cups-deviced.c). I didn't want to do this in g-c-c but I'll look whether there is another way. Marek
Created attachment 238776 [details] [review] detect devices on all backends Hi Till, the attached patch implements similar solution to the one you've mentioned in https://bugzilla.gnome.org/show_bug.cgi?id=695286#c11. It adds special search to the end of all searches so that it includes all backends but excludes those which are listed in the hard-coded list. Regards Marek
Review of attachment 238776 [details] [review]: ::: panels/printers/pp-utils.c @@ +2276,3 @@ +const gchar *cups_backends[] = + {"COMPLEMENT", "hpfax", "ncp", "beh", Please line this up, one per line and sort (or don't sort and explain the ordering with "synced with system-config-printer's blah.py"). @@ +2418,3 @@ if (data->backend_list && !g_cancellable_is_cancelled (data->cancellable)) { + if (g_strcmp0 (data->backend_list->data, "COMPLEMENT") != 0) Can you explain what the COMPLEMENT backend is? Given the amount of special handling you have in this, I would have removed it from cups_backends[] and added it manually when necessary. You could also remove the terminating NULL from cups_backends[] and use G_N_ELEMENTS() all around.
(In reply to comment #7) > Review of attachment 238776 [details] [review]: > > ::: panels/printers/pp-utils.c > @@ +2418,3 @@ > if (data->backend_list && !g_cancellable_is_cancelled > (data->cancellable)) > { > + if (g_strcmp0 (data->backend_list->data, "COMPLEMENT") != 0) > > Can you explain what the COMPLEMENT backend is? > > Given the amount of special handling you have in this, I would have removed it > from cups_backends[] and added it manually when necessary. The list contains cups backends sorted according to their response time, the first one is the slowest and hence it is executed as the last one. The special item "COMPLEMENT" means that we want to execute all other backends which were not on the list before at once (it is on the end of the list because it is low priority). This list is not used in system-config-printer. Since we remove the processed item from the list "data->backend_list", we are finished when the list is empty. That is why I just added the special item "COMPLEMENT", we are still finished when the list is empty. If we remove this item and add special handling to this we would probably need a special flag in a data structure which would say that we already processed the "complementary" list of backends and we can finish the whole operation. I would prefer to keep the "COMPLEMENT" there.
(In reply to comment #8) > (In reply to comment #7) > > Review of attachment 238776 [details] [review] [details]: > > > > ::: panels/printers/pp-utils.c > > @@ +2418,3 @@ > > if (data->backend_list && !g_cancellable_is_cancelled > > (data->cancellable)) > > { > > + if (g_strcmp0 (data->backend_list->data, "COMPLEMENT") != 0) > > > > Can you explain what the COMPLEMENT backend is? > > > > Given the amount of special handling you have in this, I would have removed it > > from cups_backends[] and added it manually when necessary. > > The list contains cups backends sorted according to their response time, the > first one is the slowest and hence it is executed as the last one. The special > item "COMPLEMENT" means that we want to execute all other backends which were > not on the list before at once (it is on the end of the list because it is low > priority). It's not really on the end of the list though. Either mention that the list is in reverse order, or use g_list_reverse() once you're done adding items to it. The list generation could also be moved to a separate function. > This list is not used in system-config-printer. Then please sort it, or explain why it's sorted that way and fix the indentation. > Since we remove the processed item from the list "data->backend_list", we are > finished when the list is empty. That is why I just added the special item > "COMPLEMENT", we are still finished when the list is empty. If we remove this > item and add special handling to this we would probably need a special flag in > a data structure which would say that we already processed the "complementary" > list of backends and we can finish the whole operation. > I would prefer to keep the "COMPLEMENT" there. s/COMPLEMENT/OTHER_BACKENDS/ would make this clearer. You'd also use a #define for it with a comment that it represents all the backends not in the cups_backends[] array. You can still use G_N_ELEMENTS()
Created attachment 238869 [details] [review] detect devices on all backends - modified Hi, attached is patch modified according to your comments. It uses g_list_append() so the list doesn't need to be in reverse order now. Regards Marek
Review of attachment 238869 [details] [review]: Looks good to commit after that. ::: panels/printers/pp-utils.c @@ +2287,3 @@ + */ +const gchar *cups_backends[] = + {"usb", curly brace on the line above. @@ +2300,3 @@ + "ncp", + "hpfax", + OTHER_BACKENDS}; Curly brace on the line below. @@ +2309,3 @@ + + for (i = 0; i < G_N_ELEMENTS (cups_backends); i++) + list = g_list_append (list, g_strdup (cups_backends[i])); g_list_prepend() and then g_list_reverse() is the way to avoid going through the list every time. @@ +2463,2 @@ if (data->backend_list && !g_cancellable_is_cancelled (data->cancellable)) { const char *backend_name; backend_name = data->backend_list->data; ... @@ +3637,3 @@ data->user_data); + if (g_strcmp0 (data->backend_list->data, OTHER_BACKENDS) != 0) Ditto.
I've pushed patch modified according to your comments to gnome-3-8 and master. Thank you for the review.
*** Bug 695286 has been marked as a duplicate of this bug. ***