D5928: Introducing Night Color - KWin's native blue light filter at nighttime

[Martin Flöser]

graesslin requested changes to this revision.
graesslin added a subscriber: jriddell.
graesslin added a comment.
This revision now requires changes to proceed.


  The general concept looks good to me.

INLINE COMMENTS

> constants.h:34-39
> +/* Whitepoint values for temperatures at 100K intervals.
> + * These will be interpolated for the actual temperature.
> + * This table was provided by Ingo Thies, 2013.
> + * See the following file for more information:
> + * https://github.com/jonls/redshift/blob/master/README-colorramp
> + */

What's the license for this? Looking at the linked readme I do not see any license. If the Redshift license applies it would be GPLv3+ which is something I have not allowed so far in KWin/Core as it would remove the GPLv2 option. But to me this looks like a database so other licensing might apply.

summoning @jriddell for support.

> logging.cpp:20-21
> +*********************************************************************/
> +#include "logging.h"
> +Q_LOGGING_CATEGORY(KWIN_COLORCORRECTION, "kwin_colorcorrection", QtCriticalMsg)

same for this file: use the ecm command.

> logging.h:20-26
> +#ifndef KWIN_COLORCORRECTION_LOGGING_H
> +#define KWIN_COLORCORRECTION_LOGGING_H
> +#include <QDebug>
> +#include <QLoggingCategory>
> +
> +Q_DECLARE_LOGGING_CATEGORY(KWIN_COLORCORRECTION)
> +#endif

please use the ecm command to generate the logging.h file instead of adding a new one.

> subdiff wrote in nightcolor.cpp:52
> Is there a general rule to it? Can I call the class NightColorManager instead and leave the file name untouched?
> 
> Is this about the generic class name or about the class name and the file name not being the same?

I cannot answer for David, but I would say that class name and file name should be the same. I would have called it manager.h and manager.cpp. Though that's a little bit generic - also the class name Manager is rather generic. Granted it's in a namespace, but still.

> nightcolor.cpp:152
> +{
> +    KConfigGroup cfgGroup(kwinApp()->config(), "NightColor");
> +

You could also consider to use a kcfgc here. See https://techbase.kde.org/Development/Tutorials/Using_KConfig_XT

We use it in e.g. all effects and it reduces quite a lot the boiler plate code. Especially for things like mapping to an enum.

> nightcolor.cpp:561-588
> +    QHash<QString, QVariant> ret;
> +    ret["Available"] = kwinApp()->platform()->supportsNightColor();
> +
> +    ret["ActiveEnabled"] = true;
> +    ret["Active"] = m_active;
> +
> +    ret["ModeEnabled"] = true;

Please use initializer list:

  return QHash<QString, QVariant>{
      { QStringLiteral{"Avaliable"}, kwinApp()->platform()->supportsNightColor()},
      {QStringLiterall{"ActiveEnabled"}, true},
     /* and so on */
  };

> nightcolor.h:40-41
> +
> +typedef QPair<QDateTime,QDateTime> DATETIMES;
> +typedef QPair<QTime,QTime> TIMES;
> +

I wouldn't use all uppercase. Just use normal CamelCase: DateTimes, Times. All uppercase is normally reserved for defines or constants.

> subdiff wrote in platform.h:382
> Yes, you're probably right. I left the name because I was a little bit worried, that there are maybe platforms in the future, where the mechanism for changing gamma and changing color temperature is not working the same as in DRM. But probably when a platform can do one of the two things it can do the other one as well.
> 
> I'll wait until we settled on the right name of the whole (see Martin's comment about the older Color Correction / ICC / Kolor Manager code) to find a better method name for it. Otherwise I would have called it "supportsColorCorrection", in case there is a platform using a different mechanism than gamma ramp adjustment. Or should we ignore this possibility for now as well in your opinion?

I agree with David here: just make it gamma. If in the future we have a difference we can still introduce more variants.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D5928

To: subdiff, #kwin, graesslin
Cc: jriddell, ngraham, leezu, behrmann, cfeck, graesslin, davidedmundson, plasma-devel, kwin, bwowk, ZrenBot, alexeymin, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20171021/3c6dc8d7/attachment-0001.html>