GNOME Bugzilla – Bug 613926
[patch] Please include the ICC Profile tagged in the screenshot
Last modified: 2011-03-17 22:17:23 UTC
Created attachment 157079 [details] [review] Initial patch for review If you enable a custom color profile to correct your screen, images look perfect when you've used a calibrated camera or scanner to produce them. If you've calibrated your printer, they also match what you want to print. This is called a 100% color managed workflow. When you take a screenshot of your color managed desktop and send it to somebody else, it looks all wrong. This is because the image as been "corrected" for the current display, not some general profile (e.g. sRGB). If you tag the screenshot with your display profile and email it to someone, you can be sure that what you see on your screen is what they are going to see on their screen, assuming they also have a color managed desktop. If they don't (or the viewer they have is crap) then it just ignores the embedded profile, and it's as if the profile was never attached. Of course, if this sounds too good to be true there is one drawback. Each ICC display profile is about 70k in size, and this makes the image size go up a little. Of course, the kind of person who calibrates their monitor is the kind of person who doesn't mind a 70k file size increase for perfect color representation. I've also made this an option for GConf, so power users who just want sRGB tagged images can turn this new functionality off. You'll need a new version (from git master) of gnome-color-manager for the new DBus call, but if you haven't got gcm installed, or it's an old version, gnome-screenshot will fall back to not embedding a profile silently. Comment welcome please. Thanks.
Review of attachment 157079 [details] [review]: thanks! overall, the patch looks good to me - apart from the coding style issue here and there. I'm not entirely sure what happens when you try to take a screenshot of the whole desktop: does the root window have an ICC profile as well? with the fixes proposed it should go after branching gnome-utils for 3.0, pending approval from Cosimo. ::: gnome-screenshot/gnome-screenshot.c @@ +904,3 @@ + icc_profile_filename = get_profile_for_window (window); + if (icc_profile_filename != NULL) { + ret = g_file_get_contents (icc_profile_filename, (gchar **) &icc_profile, &icc_profile_size, &error); the coding style of the block is wrong ::: gnome-screenshot/gnome-screenshot.schemas.in @@ +73,3 @@ + <locale name="C"> + <short>Include ICC Profile</short> + <long>Include the ICC profile in the screenshot</long> "Include the ICC profile of the target in the screenshot file" might be a better description ::: gnome-screenshot/screenshot-save.c @@ +182,1 @@ SaveFunction callback, I'd go for a: screenshot_set_icc_profile (const gchar *icc_profile_base64); function instead of passing the icc profile to the save function; just to keep the screenshot_save_start() function as clean as possible. we might start adding the screenshot format or other options to the save_start() function and if we keep adding them as arguments then it becomes quite hard to call. alternatively, I'd pass the ICC profile to screenshot_dialog_new() and attach it to the screenshot save dialog, since screenshot_save_start() will get passed the dialog as well. @@ +221,3 @@ + NULL); + } else { + ret = gdk_pixbuf_save (pixbuf, tmp_filename, coding style: else and curly brace should go on another indentation level
(In reply to comment #1) > I'm not entirely sure what happens when you try to take a screenshot of the > whole desktop: does the root window have an ICC profile as well? Well, it's a bit of a fudge (as the user could have multiple different profiles for each profile) but in this case the profile of the primary display is used which is probably sane. I'll fix up the coding style stuff next week and re-submit a patch. Thanks for the review.
Created attachment 157740 [details] [review] new patch with comments addressed from review New patch, hopefully addressing all issues. Thanks!
Ping?
Is this a feature you guys want? Is it worth me rebasing my patch, or am I wasting my time? Thanks.
sorry for the lack of feedback - yes, rebasing the patch would be great.
Created attachment 172991 [details] [review] rebased patch New patch, rebased to git master. Thanks.
Review of attachment 172991 [details] [review]: I see that you're using dbus-glib. would it be possible to use gdbus instead?
Review of attachment 172991 [details] [review]: Yes, but when I wrote this patch GDBus didn't exist. I will rewrite with GDBus now.
Created attachment 172994 [details] [review] gdbus version for review
doesn't look like gdbus to me :-)
Comment on attachment 172994 [details] [review] gdbus version for review Marking as needs-work for last Emmanuele's comment.
Created attachment 183612 [details] [review] gdbus version for review (really, really) Sorry, I attached the wrong file, and then got caught up in other stuff. I've rebased my patch to master (after to manually converting it to GSettings!) and attached it here. Please review. Thanks.
Review of attachment 183612 [details] [review]: looks fine to me; you can commit it to master post-3.0 release, given that it adds new strings - unless you want to go through the i18n string break; I'm pretty sure it'll be fine, given that the only strings added are inside the GSettings schema, and nobody sees those.
(In reply to comment #14) > looks fine to me; you can commit it to master post-3.0 release, given that it > adds new strings - unless you want to go through the i18n string break; I'm > pretty sure it'll be fine, given that the only strings added are inside the > GSettings schema, and nobody sees those. I'll ask on the mailing list for an exception. Thanks.
Comment on attachment 183612 [details] [review] gdbus version for review (really, really) Committed as freeze request granted. Thanks.