GNOME Bugzilla – Bug 778689
Fade in night light temperature change
Last modified: 2017-02-16 15:01:58 UTC
If you enable night light in the middle of the night (i.e. when we'd normally be using less blue) then it immediately goes red which the designers find kinda jarring. Spread the temperature adjustment over a few seconds just like f.lux does. I also tried using a logarithmic function for this, but linear light seemed better, probably because of the implicit gamma curve baked into the sRGB profile. Comments welcome.
Created attachment 345844 [details] [review] patch for review
Created attachment 345847 [details] [review] patch for review Urgh, it helps if you save the file first...
Review of attachment 345847 [details] [review]: I think you should also look at whether the screensaver is on, because there's really no point animating if there's no display to show it. ::: plugins/color/gsd-night-light.c @@ +48,3 @@ + guint smooth_percentage; + guint smooth_id; + gdouble smooth_temperature; smooth_target_temperature? @@ +97,3 @@ + gboolean smooth_enabled) +{ + self->smooth_enabled = smooth_enabled; You probably want to cancel the existing transition if any, no? @@ +170,3 @@ + + /* increment for the next step */ + if (++self->smooth_percentage > 100) { >= ? @@ +191,3 @@ + self->smooth_temperature = temperature; + self->smooth_percentage = 1; + self->smooth_id = g_timeout_add (50, gsd_night_light_transition_cb, self); This doesn't answer the question of how long the transition should take. Can you please make "how long does the transition take, and how many steps do we want" into split up constants?
*** Bug 778668 has been marked as a duplicate of this bug. ***
Created attachment 345932 [details] [review] patch for review v2 Fixes all issues in review. Thanks.
Review of attachment 345932 [details] [review]: Looks good. Is there anything calling gsd_night_light_set_smooth_enabled() though? ::: plugins/color/gsd-night-light.c @@ +102,3 @@ + } + if (self->smooth_timer != NULL) { + g_timer_destroy (self->smooth_timer); g_clear_pointer (&self->smooth_timer, g_timer_destroy); @@ +204,3 @@ + self->smooth_target_temperature = temperature; + self->smooth_timer = g_timer_new (); + self->smooth_id = g_timeout_add (50, gsd_night_light_smooth_cb, self); This has pretty much the same problem as mentioned in an earlier review. Do we need to wake up every 50msec when the target temp is very close to the current one? If set_temp() is super cheap to call, and the timers align, this animation would be 100 steps (!). I can't really think of a better way though (apart from doing the transition in colord), so this might have to do.
Comment on attachment 345932 [details] [review] patch for review v2 Committed with fixes, thanks.