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

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> nightcolor.cpp:52
> +
> +Manager::Manager(Platform *platform)
> +    : QObject(platform),

rename the class or the file

> nightcolor.cpp:111
> +        if (reply.isValid()) {
> +            comingFromSuspend = reply.value().toBool();
> +        } else {

I don't understand.

we're coming from suspend if PreparingForSleep is true?

> nightcolor.h:109
> +
> +    Platform *m_platform;
> +    ColorCorrectDBusInterface *m_iface;

use kwinApp()->platform()  when you need it and remove it from the ctor

> org.kde.kwin.ColorCorrect.xml:17
> +    </method>
> +    <signal name="nightColorConfigChangeSignal">
> +        <annotation name="org.qtproject.QtDBus.QtTypeName.Out0" value="QHash<QString,QVariant>"/>

don't include signal in a signal name.

I assume you're doing it to avoid the conflict with the method?
(fun fact, that's valid DBus, just invalid QtDBus)

typical convention is to have the set method called "setNightModeConfig"

> platform.h:382
>      }
> +    void setSupportsNightColor(bool set) {
> +        m_supportsNightColor = set;

ok, if this is staying in KWin I still want it layered better. Otherwise you're going to find it hard to extend it.

Nothing in Platform and subclasses should mention nightmode. 
They should all refer to supportsGammaCorrection or something.

> drm_object_crtc.cpp:115
> +    }
> +    bool isError = drmModeCrtcSetGamma(m_backend->fd(), m_id, m_gammaSize,
> +                                gammaRamp,                           // red

so we have 3 arrays of size m_gammaSize

we can pass the GammaRamp object and do:

gammaRamp.red,
gammaRamp.green,
gammaRamp.blue,

also it'll be safer to use the gammaSize from the gammaramp. otherwise if it changes, you're going to crash.

if you are trying for separation pass 3 uint*s instead of doing pointer maths.

REPOSITORY
  R108 KWin

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

To: subdiff, #kwin
Cc: graesslin, davidedmundson, plasma-devel, kwin, ZrenBot, spstarr, progwolff, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, eliasp, sebas, apol, mart, hein, lukas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20170526/67a59de2/attachment-0001.html>