GNOME Bugzilla – Bug 788201
Add shadows and highlights
Last modified: 2017-12-11 17:17:16 UTC
An initial port of Darktable's shadows and highlights tool (darktable/src/iop/shadhi.c) has been added to GEGL. Expose it in Photos will lead to enhanced dog-fooding. It's something that I can't live without. ;)
Some helper links: * http://gegl.org/operations/gegl-shadows-highlights.html * https://git.gnome.org/browse/gegl/tree/operations/workshop/shadows-highlights-correction.c * https://git.gnome.org/browse/gegl/tree/operations/workshop/shadows-highlights-correction.c Might be helpful for future reference.
Created attachment 362265 [details] [review] tool-colors: Add gegl:shadows-highlights
Created attachment 362266 [details] [review] Add shadows-highlights action
Created attachment 362267 [details] [review] Set application-license as GPL3 for gegl:shadows-highlights
Review of attachment 362267 [details] [review]: Thanks for the patches! Nitpick: would be nice if we could phrase the commit message with the "application: " prefix. ::: src/photos-application.c @@ +1863,3 @@ gegl_init (NULL, NULL); + g_object_set (gegl_config (), "application-license", "GPL3", NULL); Nitpick: please move the gegl_config () to a separate statement. More importantly, gegl:shadows-highlights should be added to REQUIRED_GEGL_OPS.
Review of attachment 362267 [details] [review]: Oh, I forgot. We should bump the GEGL requirement in configure.ac to a version where gegl:shadows-highlights was introduced.
Review of attachment 362266 [details] [review]: Looks pretty good, other than some minor things: ::: src/photos-application.c @@ +1930,3 @@ g_variant_type_free (parameter_type); parameter_type = g_variant_type_new ("a{sd}"); This is actually an anti-pattern in the code, that we shouldn't perpetuate. Sorry about that. We can directly use G_VARIANT_TYPE ("a{sd}") in the following statement. The current GVariantType implementation is just a glorified C string. @@ +1932,2 @@ parameter_type = g_variant_type_new ("a{sd}"); + self->shadows_highlights_action = g_simple_action_new ("shadows-highlights-current", parameter_type); Nitpick: move this below just above "share-current" to retain the order.
Review of attachment 362265 [details] [review]: I only took a quick look, and it looks ok. Will try it out later. ::: src/photos-tool-colors.c @@ +2,3 @@ * Photos - access, organize and share your photos on GNOME * Copyright © 2015 – 2017 Red Hat, Inc. + * Copyright © 2017 Umang Jain 2016 – 2017, I think ;)
(In reply to Debarshi Ray from comment #6) > Review of attachment 362267 [details] [review] [review]: > > Oh, I forgot. We should bump the GEGL requirement in configure.ac to a > version where gegl:shadows-highlights was introduced. Oh yes. What about --enable-workshop flag in gegl? What would a workshop op be accessed by Photos? I guess something would be required for that too.
(In reply to Debarshi Ray from comment #6) > Review of attachment 362267 [details] [review] [review]: > > Oh, I forgot. We should bump the GEGL requirement in configure.ac to a > version where gegl:shadows-highlights was introduced. gegl:shadows-highlights was introduced in GEGL-0.3.12 [1] The current minimum version in master 0.3.15 ( see commit 53edc6f7eabf) Hence, no need to bump versions. [1] http://gegl.org/NEWS.html
(In reply to Umang Jain from comment #9) > (In reply to Debarshi Ray from comment #6) > > Review of attachment 362267 [details] [review] [review] [review]: > > > > Oh, I forgot. We should bump the GEGL requirement in configure.ac to a > > version where gegl:shadows-highlights was introduced. > > Oh yes. What about --enable-workshop flag in gegl? What would a workshop op > be accessed by Photos? I guess something would be required for that too. We need to move it from workshop to common-gpl3, however, I'd add the fast-path for the NOP case before doing that. (In reply to Umang Jain from comment #10) > (In reply to Debarshi Ray from comment #6) > > Review of attachment 362267 [details] [review] [review] [review]: > > > > Oh, I forgot. We should bump the GEGL requirement in configure.ac to a > > version where gegl:shadows-highlights was introduced. > > gegl:shadows-highlights was introduced in GEGL-0.3.12 [1] > > The current minimum version in master 0.3.15 ( see commit 53edc6f7eabf) > > Hence, no need to bump versions. That's nice. I lost track of the version numbers. Thanks for tracking that down.
Created attachment 362381 [details] [review] application: Set license as GPL3 for gegl:shadows-highlights
Created attachment 362382 [details] [review] Add shadows-highlights action
Created attachment 365379 [details] [review] Absorb gegl:shadows-highlights
Created attachment 365380 [details] [review] gegl: Set GeglConfig:application-license to GPL3
Created attachment 365381 [details] [review] Add shadows-highlights action
Created attachment 365382 [details] [review] tool-colors: Add gegl:shadows-highlights