GNOME Bugzilla – Bug 651972
[PATCH] Add optional colord support to the UNIX print dialog
Last modified: 2011-06-09 17:32:15 UTC
Created attachment 189300 [details] [review] patch for initial comments This functionality adds a new 'Printer Profile' entry to the 'Color' page in the UNIX print dialog if colord support is enabled. This shows the user what color profile will be used for the settings they have selected, and if no profile or the default profile is going to be used. We are deliberately not allowing the user to _change_ the selected profile, as the ICC profile is an implementation detail, and we should not change the other print settings based on the characterization state. The OpenICC group broadly recommend showing the profile that is used, so that power users can be sure the correct profile is being used at the right time. Normal users won't care, as they don't know how horrible the color match is without profiling the printer and media. --- This is part of the ThreePointOne feature for integrated color management in GNOME : https://live.gnome.org/ThreePointOne/Features/ColorManagement I don't mind changing / adding to the colord API at this stage, and obviously all of the patch is up for debate. If colord isn't available or isn't being used, then no UI is shown. Thanks for any review, Richard.
Created attachment 189315 [details] A screenshot
Review of attachment 189300 [details] [review]: ::: configure.ac @@ +1548,3 @@ + AC_DEFINE(HAVE_COLORD, 1, [define if we have colord]) + fi +fi In these kinds of configure checks, always make it so that explicit --enable + missing dep -> error ::: gtk/gtkprintunixdialog.c @@ +927,3 @@ +} +#endif + Can I say that this looks a tad excessive for showing a simple label ? Why 500 lines for that ? I kinda would have expected a simple dbus call every time we switch to the color page, to get the current label, and thats it...
(In reply to comment #2) > Review of attachment 189300 [details] [review]: > > ::: configure.ac > @@ +1548,3 @@ > + AC_DEFINE(HAVE_COLORD, 1, [define if we have colord]) > + fi > +fi > > In these kinds of configure checks, always make it so that > explicit --enable + missing dep -> error Do you mean in the case that "if test "os_win32" == "yes"? > Can I say that this looks a tad excessive for showing a simple label ? > > Why 500 lines for that ? > > I kinda would have expected a simple dbus call every time we switch to the > color page, to get the current label, and thats it... Right, it does look quite unwieldy. The colord API is quite low level, as it's not subsystem dependent. So initially we connect to the daemon, and then get the correct virtual device for the GTK printer. We then connect to the device, and perform a query on it from the GTK print dialog options (the qualifier). From the result, we get the properties of the returned profile and set the label and tooltip based on properties from the profile. The alternative would be a method like: char * GetProfileTitleForDeviceByQualifier(char *device_id, char *format) Which would make the GTK print dialog code much smaller; but in doing so makes the colord API specific to the thing we're trying to do. And if we want to show the calibration date in the tooltip, we need to also add: char * GetProfileCreatedForDeviceByQualifierAsString(char *device_id, char *format) Which really bakes in the presentation to the system API. I would also say that since making all 3 _sync methods _async, the code has grown in size threefold. I'm not saying the _sync calls are better (otherwise we block the main thread waiting for colord) just that all those async calls mean quite a bit of boilerplate. That said, I'm open for ideas on how to make this easier to use. Maybe GTK should call into something like org.gnome.settings-daemon.GetProfileTitleForDeviceByQualifier() method, although then the GTK printing dialog would runtime-depend on gnome-settings-daemon which probably is even less palatable. Ideas welcome. Richard.
(In reply to comment #3) > > In these kinds of configure checks, always make it so that > > explicit --enable + missing dep -> error > > Do you mean in the case that "if test "os_win32" == "yes"? I mean that if colord is not installed, I want ./configure --enable-colord to error out > > That said, I'm open for ideas on how to make this easier to use. Maybe GTK > should call into something like > org.gnome.settings-daemon.GetProfileTitleForDeviceByQualifier() method, > although then the GTK printing dialog would runtime-depend on > gnome-settings-daemon which probably is even less palatable. No, I think thats a nonstarter > Ideas welcome. I would have kinda expected this information to come from cups. After all, it knows everything about the device, presumably also what color profile is going to be used with it....
Created attachment 189368 [details] [review] [patch] patch for review (In reply to comment #4) > I mean that if colord is not installed, I want > > ./configure --enable-colord > > to error out Fixed in the new patch. > > That said, I'm open for ideas on how to make this easier to use. Maybe GTK > > should call into something like > > org.gnome.settings-daemon.GetProfileTitleForDeviceByQualifier() method, > > although then the GTK printing dialog would runtime-depend on > > gnome-settings-daemon which probably is even less palatable. > > No, I think thats a nonstarter Agreed. > > Ideas welcome. > > I would have kinda expected this information to come from cups. After all, it > knows everything about the device, presumably also what color profile is going > to be used with it.... Yes, at the moment CUPS is registering devices with colord and also assigning profiles from the PPD using qualifiers specified my the PPD vendor. This was consciously done out of band with cups, as the cups developers did not want the color registration stuff inside cupsd itself. The way it works on OSX is using the ColorSync framework. In the colorsync model CUPS creates permanent devices at queue add time, and the OSX printing system queries ColorSync for the profile to be used for each job. The difference with colord, is that ColorSync actually does the color conversion, but on Linux it's being done by ghostscript, ultimately using lcms2. Although the model looks a little clunky (out of band with cupsd) it seems to work well for OSX, and it's no coincidence it's the design I'm using with colord. It also makes it possible to do the display and printer device registration without relying on a cups daemon being installed.
Review of attachment 189368 [details] [review]: Ok, done some more review on this, but I think I am going to propose that we rework this a little more radically. Can we move all the code dealing with colord interaction to the cups print backend, and get this into the print dialog by adding a printer option for the color profile ? That would be my preferred way of organizing this. You may have to slightly enhance the printeroptionwidget machinery to handle with purely informational strings, but that should be minimal. ::: gtk/gtkprintunixdialog.c @@ +1772,3 @@ +#ifdef HAVE_COLORD +static void +create_colord_color_page (GtkPrintUnixDialog *dialog) Would be better to find a function name that doesn't insinuate it was creating a new page when it is just adding to an existing page, but I'm blanking atm. Also, can we move this function up to where the other ifdef HAVE_COLORD functions live ? @@ +1787,3 @@ + 0, 1, rows - 1 , rows, GTK_FILL, 0, 0, 0); + + /* TRANSLATORS: this is when colord is unavailable */ Probably better to say 'this is when color profile information is unavailable'. Don't assume translators can make any sense of 'colord'... @@ +1855,3 @@ +#ifdef HAVE_COLORD + /* update the colord UI on the 'Color' page */ Comment seems misleading and unnecessary. Its creating, not updating. But the function name already says so. Just drop the comment.
(In reply to comment #6) > Can we move all the code dealing with colord interaction to the cups print > backend, and get this into the print dialog by adding a printer option for the > color profile ? That would be my preferred way of organizing this. Yes, I think you might be right with this. It's quite a bit of work to do that, but it's probably the right thing to do. > You may have to slightly enhance the printeroptionwidget machinery to handle > with purely informational strings, but that should be minimal. Sure, should be pretty easy. I'll hack something up today. Thanks, Richard.
Created attachment 189483 [details] [review] test patch (In reply to comment #7) > Sure, should be pretty easy. I'll hack something up today. I've uploaded a preliminary patch. It's not polished, and there are a couple of bugs still remaining, but I wanted to know if this thing is the kind of approach you wanted. I'm not sure if I should be hooking into GtkPrinterCups rather than doing the gtk_colord_setup_monitored_printer() which looks a little fragile. Ideas welcome. Thanks.
Review of attachment 189483 [details] [review]: I don't think this can quite work as it currently is. You create a single colord object for the print backend, that is fine. But then you try to use it for the printer options of multiple printers, even though it only associates with a single device/profile. Also, I don't think it works to have the colord object hold on to the printeroptionset. If you look at the get_options implementation in the cups backend, it creates a new optionset each time. So, what I think needs to happen here is that you need to - move the device/profile to be stored with the printer (just like the colord instance is with the backend) - don't hold on to printer option set, but instead keep your own private copies of the relevant bits of information, and inject those into the provided option set when necessary ::: modules/printbackends/cups/gtkcolord.c @@ +130,3 @@ + + /* find what we've got */ + gtk_printer_option_set_foreach (priv->set, colord_print_option_print_cb, NULL); This is debug-only ? Could be inside #ifdef G_ENABLE_DEBUG, I guess. @@ +145,3 @@ + /* TRANSLATORS: when we're running an old CUPS, and + * it hasn't registered the device with colord */ + title = _("Device unavailable"); The text strikes me as not very good. After all, the device is available...we're printing on it ! What is not available is color management on this device. @@ +157,3 @@ + { + /* TRANSLATORS: when there is no profile available */ + title = _("No profile available"); Should probably say 'color profile' in the translator comment to make the context clear. Find to keep the string itself as it is, since the label will make it clear. @@ +165,3 @@ + goto out; + + /* get the filename of the profile */ A little too much trivial commenting here, no need to comment every line... Also, the comments tend to get out of sync - this is not getting a filename, but a title now, it seems... Lets just cut down on the obvious comments. @@ +168,3 @@ + title = cd_profile_get_title (priv->profile); + if (title == NULL) { + g_warning ("profile has no title!"); I don't think the warning is useful - should be debug printf if anything. What really want to do is set title to some placeholder text though, since you are going to out:, where it is used. @@ +196,3 @@ + error->message); + g_error_free (error); + } Does it make sense to still update the ui here after you got an error ? Just wondering if a return is missing in the if. @@ +221,3 @@ + error->message); + g_error_free (error); + goto out; Same question here: does it make sense to update the ui after you got an error ? Perhaps it does, I really don't know. @@ +279,3 @@ + format[2] = "*"; + + /* get profile for qualifier */ should this say 'get qualifier for profile' ? @@ +285,3 @@ + format[0]); + + /* has the optionset changed in a way that will not affect colord */ Seems another misleading comment - this is about whether _we_ care, not colord, right ?
Created attachment 189556 [details] [review] test patch (In reply to comment #9) > So, what I think needs to happen here is that you need to > - move the device/profile to be stored with the printer (just like the colord > instance is with the backend) Done in the attached patch. I'm much happier with the structure of this patch compared to the last one. It's certainly way more efficient and clean. > ::: modules/printbackends/cups/gtkcolord.c > @@ +130,3 @@ > + > + /* find what we've got */ > + gtk_printer_option_set_foreach (priv->set, colord_print_option_print_cb, > NULL); > > This is debug-only ? Could be inside #ifdef G_ENABLE_DEBUG, I guess. I've removed this for now. > @@ +145,3 @@ > + /* TRANSLATORS: when we're running an old CUPS, and > + * it hasn't registered the device with colord */ > + title = _("Device unavailable"); > > The text strikes me as not very good. After all, the device is > available...we're printing on it ! What is not available is color management on > this device. Fixed. > @@ +157,3 @@ > + { > + /* TRANSLATORS: when there is no profile available */ > + title = _("No profile available"); > > Should probably say 'color profile' in the translator comment to make the > context clear. Find to keep the string itself as it is, since the label will > make it clear. Fixed. > @@ +165,3 @@ > + goto out; > + > + /* get the filename of the profile */ > > A little too much trivial commenting here, no need to comment every line... > Also, the comments tend to get out of sync - this is not getting a filename, > but a title now, it seems... > Lets just cut down on the obvious comments. Fixed. > @@ +168,3 @@ > + title = cd_profile_get_title (priv->profile); > + if (title == NULL) { > + g_warning ("profile has no title!"); > > I don't think the warning is useful - should be debug printf if anything. > What really want to do is set title to some placeholder text though, since you > are going to out:, where it is used. Fixed. > @@ +196,3 @@ > + error->message); > + g_error_free (error); > + } > > Does it make sense to still update the ui here after you got an error ? Just > wondering if a return is missing in the if. Yup, I'm intentionally updating the UI in failure so that old-correct-but-now-incorrect profile gets cleared. I'm wondering whether hiding the GtkPrinterOption completely if colord fails at runtime. Ideas welcome. > @@ +221,3 @@ > + error->message); > + g_error_free (error); > + goto out; > > Same question here: does it make sense to update the ui after you got an error > ? Perhaps it does, I really don't know. Same answer. > @@ +279,3 @@ > + format[2] = "*"; > + > + /* get profile for qualifier */ > > should this say 'get qualifier for profile' ? No, but I've made the comment better. > @@ +285,3 @@ > + format[0]); > + > + /* has the optionset changed in a way that will not affect colord */ > > Seems another misleading comment - this is about whether _we_ care, not colord, > right ? Rewritten to be clearer.
Review of attachment 189556 [details] [review]: I agree this patch looks much better. Except for the lingering unease about the api changing ifdef, it looks good. ::: modules/printbackends/cups/gtkprintbackendcups.c @@ +3547,3 @@ + helper->settings, + set); +} Ah, tricky. So this is necessary because changing other printer settings (e.g. resolution) can change what color profile is selected ? ::: modules/printbackends/cups/gtkprintercups.h @@ +94,3 @@ +GtkPrinterCups *gtk_printer_cups_new (const char *name, + GtkPrintBackend *backend); +#endif Not loving how this makes the (internal) api depends on configure switches. We could just make the client a gpointer and just pass NULL, maybe ? Or maybe it doesn't matter much...
Created attachment 189569 [details] [review] patch for review (In reply to comment #11) > Ah, tricky. So this is necessary because changing other printer settings (e.g. > resolution) can change what color profile is selected ? Yup. > Not loving how this makes the (internal) api depends on configure switches. We > could just make the client a gpointer > and just pass NULL, maybe ? Fixed in this new patch. Thanks. Richard.
Review of attachment 189569 [details] [review]: Looks good to commit with that change. I assume you tested building with --disable-colord, too ? ::: modules/printbackends/cups/gtkprintercups.c @@ +531,3 @@ +#ifdef HAVE_COLORD + /* connect to colord */ + printer->colord_cancellable = g_cancellable_new (); Just to be on the safe side, might want to wrap this into an if (colord_client != NULL), maybe ? After all, we _are_ passing NULL to this from the backend side sometimes...
(In reply to comment #13) > Review of attachment 189569 [details] [review]: > > Looks good to commit with that change. > I assume you tested building with --disable-colord, too ? Yup, that works fine. > ::: modules/printbackends/cups/gtkprintercups.c > @@ +531,3 @@ > +#ifdef HAVE_COLORD > + /* connect to colord */ > + printer->colord_cancellable = g_cancellable_new (); > > Just to be on the safe side, might want to wrap this into an > if (colord_client != NULL), maybe ? After all, we _are_ passing NULL to this > from the backend side sometimes... Done. Thanks for all the review. I've removed a couple of g_debug lines that snuck in, and now pushed this to master.