After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788201 - Add shadows and highlights
Add shadows and highlights
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on: 789554 790860
Blocks:
 
 
Reported: 2017-09-26 16:44 UTC by Debarshi Ray
Modified: 2017-12-11 17:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tool-colors: Add gegl:shadows-highlights (10.98 KB, patch)
2017-10-25 12:54 UTC, Umang Jain
none Details | Review
Add shadows-highlights action (4.55 KB, patch)
2017-10-25 12:55 UTC, Umang Jain
none Details | Review
Set application-license as GPL3 for gegl:shadows-highlights (779 bytes, patch)
2017-10-25 12:55 UTC, Umang Jain
none Details | Review
application: Set license as GPL3 for gegl:shadows-highlights (1.24 KB, patch)
2017-10-27 06:24 UTC, Umang Jain
none Details | Review
Add shadows-highlights action (4.64 KB, patch)
2017-10-27 06:24 UTC, Umang Jain
none Details | Review
Absorb gegl:shadows-highlights (38.16 KB, patch)
2017-12-11 17:07 UTC, Debarshi Ray
committed Details | Review
gegl: Set GeglConfig:application-license to GPL3 (889 bytes, patch)
2017-12-11 17:07 UTC, Debarshi Ray
committed Details | Review
Add shadows-highlights action (4.66 KB, patch)
2017-12-11 17:08 UTC, Debarshi Ray
committed Details | Review
tool-colors: Add gegl:shadows-highlights (11.26 KB, patch)
2017-12-11 17:08 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2017-09-26 16:44:49 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. ;)
Comment 2 Umang Jain 2017-10-25 12:54:39 UTC
Created attachment 362265 [details] [review]
tool-colors: Add gegl:shadows-highlights
Comment 3 Umang Jain 2017-10-25 12:55:05 UTC
Created attachment 362266 [details] [review]
Add shadows-highlights action
Comment 4 Umang Jain 2017-10-25 12:55:28 UTC
Created attachment 362267 [details] [review]
Set application-license as GPL3 for gegl:shadows-highlights
Comment 5 Debarshi Ray 2017-10-25 13:53:21 UTC
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.
Comment 6 Debarshi Ray 2017-10-25 13:55:22 UTC
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.
Comment 7 Debarshi Ray 2017-10-25 14:01:41 UTC
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.
Comment 8 Debarshi Ray 2017-10-25 14:06:42 UTC
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 ;)
Comment 9 Umang Jain 2017-10-27 04:40:49 UTC
(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.
Comment 10 Umang Jain 2017-10-27 05:45:28 UTC
(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
Comment 11 Debarshi Ray 2017-10-27 05:59:40 UTC
(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.
Comment 12 Umang Jain 2017-10-27 06:24:22 UTC
Created attachment 362381 [details] [review]
application: Set license as GPL3 for gegl:shadows-highlights
Comment 13 Umang Jain 2017-10-27 06:24:54 UTC
Created attachment 362382 [details] [review]
Add shadows-highlights action
Comment 14 Debarshi Ray 2017-12-11 17:07:13 UTC
Created attachment 365379 [details] [review]
Absorb gegl:shadows-highlights
Comment 15 Debarshi Ray 2017-12-11 17:07:38 UTC
Created attachment 365380 [details] [review]
gegl: Set GeglConfig:application-license to GPL3
Comment 16 Debarshi Ray 2017-12-11 17:08:12 UTC
Created attachment 365381 [details] [review]
Add shadows-highlights action
Comment 17 Debarshi Ray 2017-12-11 17:08:37 UTC
Created attachment 365382 [details] [review]
tool-colors: Add gegl:shadows-highlights