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
