GNOME Bugzilla – Bug 778039
Please support natural light functionality
Last modified: 2017-02-09 21:06:37 UTC
Natural light mode is functionality that exists naively in iOS and is available for both Windows and Linux using various other 3rd party applications like redshift and f.lux. Adding support to gnome-settings-daemon allows this to just work, and also integrate with the color management functionality. Further patches will be submitted to the gnome-control-center that add the UI as designed in https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/displays/displays-v4.png I'm happy for these patches to be torn to pieces (or even be told they should be put into their own separate plugin) but they work for me, and are good enough for a first cut for review. Thanks! Richard.
Created attachment 344718 [details] [review] trivial patch
Created attachment 344719 [details] [review] trivial patch 2
Created attachment 344721 [details] [review] patch for review
Review of attachment 344718 [details] [review]: ::: plugins/color/gsd-color-state.c @@ +179,3 @@ g_return_if_fail (GSD_IS_COLOR_STATE (state)); + + /* same */ You can remove the comment, I think we're alright reading that ;)
Review of attachment 344719 [details] [review]: That's probably fine, but who's going to take care of making sure that we're going to call it when ->state_screen is available? If the code's already there, please add that information to the comment.
Review of attachment 344721 [details] [review]: I would also advise you to export the sunrise/sunset hours in a read-only D-Bus property so that you can show those in the UI without 1) having to contact geoclue 2) don't have to duplicate that code. Could you also please split off the gsd-natural-light-common.[ch] and tests in a separate patch? ::: data/org.gnome.settings-daemon.plugins.color.gschema.xml.in.in @@ +29,3 @@ + <default>16.00</default> + <_summary>The start time</_summary> + <_description>When automatic is not enabled then use this start time in hours from midnight.</_description> > "When automatic" When automatic scheduling is disabled, or better: when "natural-light-schedule-automatic" is disabled Make sure to use the new fancy quotes as done in other places. @@ +34,3 @@ + <default>8.00</default> + <_summary>The end time</_summary> + <_description>When automatic is not enabled then use this end time in hours from midnight.</_description> Ditto. @@ +39,3 @@ + <default>181</default> + <_summary>The last latitude position</_summary> + <_description>When location services are available this represents the latitude of the last detected location. Positive numbers indicate North. Numbers greater than 180 represent an invalid location.</_description> You can remove the explanation of latitude, it's a known term. @@ +44,3 @@ + <default>181</default> + <_summary>The last latitude position</_summary> + <_description>When location services are available this represents the longitude of the last detected location. Positive numbers indicate East. Numbers greater than 180 represent an invalid location.</_description> Ditto for longitude. "d" will trim that value, I'd rather a single value to store both longitude and latitude. ::: plugins/color/gsd-color-manager.c @@ +43,3 @@ #define GSD_COLOR_DBUS_INTERFACE GSD_DBUS_BASE_INTERFACE ".Color" +#define GSD_NATURAL_LIGHT_SCHEDULE_TIMEOUT 5 /* s */ You can write that in full. @@ +44,3 @@ +#define GSD_NATURAL_LIGHT_SCHEDULE_TIMEOUT 5 /* s */ +#define GSD_NATURAL_LIGHT_POLL_TIMEOUT 60 /* s */ Align the comments. @@ +70,3 @@ + /* natural light */ + GSettings *settings; + gboolean disabled_until_tmw; "tomorrow". We have wide monitors. @@ +547,3 @@ + natural_light_recheck (manager); + priv->natural_light_timer_id = + g_timeout_add_seconds (GSD_NATURAL_LIGHT_POLL_TIMEOUT, This is the wrong thing to use. 1) g_timeout_add_seconds() uses monotonic time, you want wall-clock time 2) You're launching every minute even though you probably want to be woken up every minute *during the night* (or the scheduled hours) GnomeDateTime in gnome-desktop is what you want to use instead. @@ +572,3 @@ + GsdColorManager *manager = GSD_COLOR_MANAGER (user_data); + GsdColorManagerPrivate *priv = manager->priv; + GClueLocation *location; const, or leaked? ::: plugins/color/gsd-natural-light-common.c @@ +38,3 @@ + +/* + * Formulas taken from https://www.esrl.noaa.gov/gmd/grad/solcalc/calcdetails.html Got a link to the actual code? @@ +39,3 @@ +/* + * Formulas taken from https://www.esrl.noaa.gov/gmd/grad/solcalc/calcdetails.html + * @latitude is positive if North Yeah, that's pretty useless. Remove this and the one below. Addint information about the units used both for those and sunrise/sunset would be better. @@ +50,3 @@ + GTimeSpan ts = g_date_time_difference (dt, dt_zero); + + if (pos_lat > 180.f || pos_lat < -180.f) Why shouldn't those g_return_val_if_fail() instead? @@ +55,3 @@ + return FALSE; + + gdouble tz_offset = g_date_time_get_utc_offset (dt) / G_USEC_PER_SEC / 60 / 60; // B5 It would be even less readable if we didn't use C99. You'll need to update configure.ac to check for C99 support, and we'll roll with it. @@ +97,3 @@ + + /* allow me to debug this */ + if (g_getenv ("GSD_NATURAL_LIGHT_DEBUG") != NULL) { No, use g_debug() on its own, and change the log domain for this file.
Comment on attachment 344719 [details] [review] trivial patch 2 Fixed, and pushed, thanks.
Comment on attachment 344718 [details] [review] trivial patch Fixed, and pushed, thanks.
(In reply to Bastien Nocera from comment #6) > Review of attachment 344721 [details] [review] [review]: > > I would also advise you to export the sunrise/sunset hours in a read-only > D-Bus property so that you can show those in the UI without 1) having to > contact geoclue 2) don't have to duplicate that code. Allan confirmed just now that we don't want the sunrise/sunset defaults in the time spinners, so unless I'm missing something I don't think we need this. I'm happy to add the properties if you think they're still a good idea. > Could you also please split off the gsd-natural-light-common.[ch] and tests > in a separate patch? Done. > Make sure to use the new fancy quotes as done in other places. Done. > "tomorrow". We have wide monitors. Fixed. > GnomeDateTime in gnome-desktop is what you want to use instead. Added, thanks. > @@ +572,3 @@ > + GsdColorManager *manager = GSD_COLOR_MANAGER (user_data); > + GsdColorManagerPrivate *priv = manager->priv; > + GClueLocation *location; > > const, or leaked? Neither, I checked it's just a unreffed object. > ::: plugins/color/gsd-natural-light-common.c > @@ +38,3 @@ > + > +/* > + * Formulas taken from > https://www.esrl.noaa.gov/gmd/grad/solcalc/calcdetails.html > > Got a link to the actual code? No, I actually just copied the formulas from the excel spreadsheet and turned them into C. :) > Addint information about the units used both for those and sunrise/sunset > would be better. Done. > @@ +50,3 @@ > + GTimeSpan ts = g_date_time_difference (dt, dt_zero); > + > + if (pos_lat > 180.f || pos_lat < -180.f) > > Why shouldn't those g_return_val_if_fail() instead? Sure, that works too, fixed. > It would be even less readable if we didn't use C99. You'll need to update > configure.ac to check for C99 support, and we'll roll with it. Sure, patch coming. > @@ +97,3 @@ > + > + /* allow me to debug this */ > + if (g_getenv ("GSD_NATURAL_LIGHT_DEBUG") != NULL) { > > No, use g_debug() on its own, and change the log domain for this file. I removed the entire block; I can trivially plumb in the lat/long into the spreadsheet for debugging, those debugs were more to validate that my code matched the spreadsheet values. Richard.
Created attachment 344780 [details] [review] require c99
Created attachment 344781 [details] [review] split out -common and tests
Created attachment 344782 [details] [review] add c&p GDateTimeSource
Created attachment 344784 [details] [review] patch for review v2
Created attachment 344795 [details] [review] patch for review v3
Review of attachment 344780 [details] [review]: Explain why in the commit message, and prefix the commit subject with build:
Review of attachment 344781 [details] [review]: What is "common" functionality. I'd rather you explained what is in this patch. ::: plugins/color/gsd-natural-light-common.h @@ +1,3 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 8 -*- + * + * Copyright (C) 2007 William Jon McCann <mccann@jhu.edu> I doubt Jon wrote any of it.
Review of attachment 344782 [details] [review]: > Subject: [PATCH 3/4] color: Add GDateTimeSource from the gnome-desktop project s/project/module/ GNOME is the project. > We'll need this for future functionality. Again, this is terse.
Created attachment 344849 [details] [review] split out -common and tests v2
Created attachment 344850 [details] [review] add c&p GDateTimeSource v2
Review of attachment 344795 [details] [review]: ::: plugins/color/Makefile.am @@ +42,3 @@ gsd-color-state.c \ gsd-color-state.h \ + gsd-natural-light-common.c \ This should be in the same commit as when you add those files, even if unused. ::: plugins/color/gsd-color-manager.c @@ +503,3 @@ + g_autoptr(GDateTime) dt = g_date_time_new_now_local (); + priv->disabled_until_tomorrow = g_variant_get_boolean (value); + priv->disabled_day_of_month = g_date_time_get_day_of_month (dt); Shouldn't this disable the colour change straight away, rather than wait for the next timeout tick? @@ +565,3 @@ +poll_timeout_destroy (GsdColorManager *manager) +{ + GsdColorManagerPrivate *priv = manager->priv; Line feed after that. @@ +647,3 @@ + + /* recheck the levels if the location changed significantly */ + if (update_cached_sunrise_sunset (manager)) You also need to run this update when you get past the sunset or sunrise. For example: - during daytime of day 1, the sunset corresponds to that evening's sunset, the sunrise should correspond to day 2's sunrise - after day 1's sunset, the sunset should be day 2's sunset. If you don't do this, and leave your desktop machine running for a week, the location won't change, those values will skew compared to values that would be calculated. I would advise moving this code to a helper function in your new helper sources, eg. changed = gsd_natural_light_update_sun_times(&priv->cached_sunrise, &priv->cached_sunset);
Review of attachment 344849 [details] [review]: Can you please add a mention, above gsd_natural_light_get_sunrise_sunset(), of how broken this could be for multi-day days or nights, as in the north of Lapland for example? ::: plugins/color/Makefile.am @@ +17,3 @@ gcm-edid.h \ + gsd-natural-light-common.c \ + gsd-natural-light-common.h \ As mentioned, add this to the gsd-color SOURCES as well.
Review of attachment 344849 [details] [review]: Also, if it's super broken, we might want to disable the availability of the functionality in g-c-c, also make a mention of it in the comment.
Review of attachment 344850 [details] [review]: Yep. Don't forget to add the bug URL in the commit message. > thing when the time is changed backwards or forwards manually. You can remove the "manually", it also happens if the time is changed automatically, suspending, changing timezones, or applying daylight savings.
Created attachment 345142 [details] [review] fixups for v3 (In reply to Bastien Nocera from comment #20) > Shouldn't this disable the colour change straight away, rather than wait for > the next timeout tick? Agreed, fixed. > @@ +565,3 @@ > +poll_timeout_destroy (GsdColorManager *manager) > +{ > + GsdColorManagerPrivate *priv = manager->priv; > > Line feed after that. Fixed. > - during daytime of day 1, the sunset corresponds to that evening's sunset, > the sunrise should correspond to day 2's sunrise > - after day 1's sunset, the sunset should be day 2's sunset. Correct, I fixed this and added a test. > I would advise moving this code to a helper function in your new helper > sources, eg. > changed = gsd_natural_light_update_sun_times(&priv->cached_sunrise, > &priv->cached_sunset); I think calling update_cached_sunrise_sunset() from the timer is much clearer and most understatable. I've attached a new patch with just the fixes, but let me know if you actually want a fixed up complete patch.
(In reply to Richard Hughes from comment #24) > > I would advise moving this code to a helper function in your new helper > > sources, eg. > > changed = gsd_natural_light_update_sun_times(&priv->cached_sunrise, > > &priv->cached_sunset); > > I think calling update_cached_sunrise_sunset() from the timer is much > clearer and most understatable. This code needs to throw some debug when the sunset/sunrise times change. You'll also want to add some debug output in a human readable (and possibly machine readable) format of when the timeouts are scheduled, and when they're emitted. The human readable format would be for us to read and debug, the machine readable format would be for the test suite.
Review of attachment 345142 [details] [review]: That looks fine overall, but I think that natural_light_recheck() does far too many checks when we usually know what changed, eg. we shouldn't need to check whether "disable until tomorrow" changed when we could set up a timer to handle it, etc. ::: plugins/color/gcm-self-test.c @@ +126,3 @@ + + /* test rollover to the previous day */ + g_assert (gsd_natural_light_frac_day_is_between (5, 16, 8)); This needs to be in a separate patch for the tests. ::: plugins/color/gsd-color-manager.c @@ +570,3 @@ { GsdColorManagerPrivate *priv = manager->priv; + white space change. ::: plugins/color/gsd-natural-light-common.c @@ +122,3 @@ end += 24; + /* wraparound to the previous day */ In that same separate patch.
Created attachment 345268 [details] [review] patch for review v4 I've refactored this a bit, and added a lot of self tests, which uncovered a few bugs and fixed all the momentary quirks I was experiencing. There's a lot less going on when the various keys change, and this makes it a lot more efficient. Re-review welcome, sorry.
Review of attachment 345268 [details] [review]: > Resolves: https://bugzilla.gnome.org/show_bug.cgi?id=778039 You can remove the "Resolves: " Looks very good otherwise. ::: data/org.gnome.settings-daemon.plugins.color.gschema.xml.in.in @@ +19,3 @@ + <default>4000</default> + <_summary>Temperature of the display when enabled</_summary> + <_description>This temperature in Kelvin is used to modify the screen tones when natural light mode is enabled. Higher values are more blue, lower more red.</_description> bluer, redder. @@ +24,3 @@ + <default>true</default> + <_summary>Use the sunrise and sunset</_summary> + <_description>Calculate the sunrise and sunset times automatically.</_description> automatically, from the current location. @@ +29,3 @@ + <default>16.00</default> + <_summary>The start time</_summary> + <_description>When “natural-light-schedule-automatic” is disabled then use this start time in hours from midnight.</_description> s/then/,/ @@ +34,3 @@ + <default>8.00</default> + <_summary>The end time</_summary> + <_description>When “natural-light-schedule-automatic” is disabled then use this end time in hours from midnight.</_description> Ditto. @@ +39,3 @@ + <default>(181,181)</default> + <_summary>The last detected position</_summary> + <_description>When location services are available this represents the last detected location.</_description> Add a comment explaining that the default value is an invalid value to force update it. ::: plugins/color/gsd-natural-light.h @@ +27,3 @@ + +#define GSD_TYPE_NATURAL_LIGHT (gsd_natural_light_get_type ()) +G_DECLARE_DERIVABLE_TYPE (GsdNaturalLight, gsd_natural_light, GSD, NATURAL_LIGHT, GObject) Why not FINAL? @@ +40,3 @@ +gdouble gsd_natural_light_get_sunset (GsdNaturalLight *nlight); +gdouble gsd_natural_light_get_temperature (GsdNaturalLight *nlight); +gboolean gsd_natural_light_get_disabled_until_tmw (GsdNaturalLight *nlight); Linefeed just before this line, so we can see the get/set pair.
Fixed up all issues, and made into FINAL. Thanks for all the reviews!
Valid latitude range is between -90 and +90, not -180 and + 180. By the way, the Windows 10 Creators Update includes "Night Light" which is their implementation of this feature. But GNOME 3.24 will be released first.
(In reply to Jeremy Bicha from comment #30) > Valid latitude range is between -90 and +90, not -180 and + 180. Ooops, sorry about that! I've learned something. :) commit d1e0b294c9aa701d0ec704630333893f2a1a1e8f Author: Richard Hughes <richard@hughsie.com> Date: Thu Feb 9 19:21:31 2017 +0000 color: Valid latitude range is between -90 and +90
I'm reopening so I remember to check it again tomorrow. commit d1e0b294c9aa701d0ec704630333893f2a1a1e8f is broken as well, as it caps both longitude and latitude to 90 degrees. It should only change the latter. Also make sure that the ranges are inclusive of the min and max values.
Fixed, sorry for the git noise.