<table><tr><td style="">graesslin requested changes to this revision.<br />graesslin added a subscriber: jriddell.<br />graesslin added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D5928" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>The general concept looks good to me.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-35758" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">constants.h:34-39</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">/* Whitepoint values for temperatures at 100K intervals.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * These will be interpolated for the actual temperature.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * This table was provided by Ingo Thies, 2013.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * See the following file for more information:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * https://github.com/jonls/redshift/blob/master/README-colorramp</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> */</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">summoning <a href="https://phabricator.kde.org/p/jriddell/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@jriddell</a> for support.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-35760" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">logging.cpp:20-21</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">*********************************************************************/</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf">"logging.h"</span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">same for this file: use the ecm command.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-35759" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">logging.h:20-26</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#ifndef KWIN_COLORCORRECTION_LOGGING_H</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#define KWIN_COLORCORRECTION_LOGGING_H</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><QDebug></span><span style="color: #304a96"></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><QLoggingCategory></span><span style="color: #304a96"></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="n">Q_DECLARE_LOGGING_CATEGORY</span><span class="p">(</span><span class="n">KWIN_COLORCORRECTION</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please use the ecm command to generate the logging.h file instead of adding a new one.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-24568" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">subdiff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">nightcolor.cpp:52</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Is there a general rule to it? Can I call the class NightColorManager instead and leave the file name untouched?</p>
<p style="padding: 0; margin: 8px;">Is this about the generic class name or about the class name and the file name not being the same?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-35763" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">nightcolor.cpp:152</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">KConfigGroup</span> <span class="n">cfgGroup</span><span class="p">(</span><span class="n">kwinApp</span><span class="p">()</span><span style="color: #aa2211">-></span><span class="n">config</span><span class="p">(),</span> <span style="color: #766510">"NightColor"</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You could also consider to use a kcfgc here. See <a href="https://techbase.kde.org/Development/Tutorials/Using_KConfig_XT" class="remarkup-link" target="_blank" rel="noreferrer">https://techbase.kde.org/Development/Tutorials/Using_KConfig_XT</a></p>
<p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-35764" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">nightcolor.cpp:561-588</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QHash</span><span style="color: #aa2211"><</span><span class="n">QString</span><span class="p">,</span> <span class="n">QVariant</span><span style="color: #aa2211">></span> <span class="n">ret</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">ret</span><span class="p">[</span><span style="color: #766510">"Available"</span><span class="p">]</span> <span style="color: #aa2211">=</span> <span class="n">kwinApp</span><span class="p">()</span><span style="color: #aa2211">-></span><span class="n">platform</span><span class="p">()</span><span style="color: #aa2211">-></span><span class="n">supportsNightColor</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">ret</span><span class="p">[</span><span style="color: #766510">"ActiveEnabled"</span><span class="p">]</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">true</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">ret</span><span class="p">[</span><span style="color: #766510">"Active"</span><span class="p">]</span> <span style="color: #aa2211">=</span> <span class="n">m_active</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">ret</span><span class="p">[</span><span style="color: #766510">"ModeEnabled"</span><span class="p">]</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">true</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Please use initializer list:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">return QHash<QString, QVariant>{
{ QStringLiteral{"Avaliable"}, kwinApp()->platform()->supportsNightColor()},
{QStringLiterall{"ActiveEnabled"}, true},
/* and so on */
};</pre></div></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-35761" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">nightcolor.h:40-41</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #aa4000">typedef</span> <span class="n">QPair</span><span style="color: #aa2211"><</span><span class="n">QDateTime</span><span class="p">,</span><span class="n">QDateTime</span><span style="color: #aa2211">></span> <span class="n">DATETIMES</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #aa4000">typedef</span> <span class="n">QPair</span><span style="color: #aa2211"><</span><span class="n">QTime</span><span class="p">,</span><span class="n">QTime</span><span style="color: #aa2211">></span> <span class="n">TIMES</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I wouldn't use all uppercase. Just use normal CamelCase: DateTimes, Times. All uppercase is normally reserved for defines or constants.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D5928#inline-24572" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">subdiff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">platform.h:382</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">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.</p>
<p style="padding: 0; margin: 8px;">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?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I agree with David here: just make it gamma. If in the future we have a difference we can still introduce more variants.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R108 KWin</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D5928" rel="noreferrer">https://phabricator.kde.org/D5928</a></div></div><br /><div><strong>To: </strong>subdiff, KWin, graesslin<br /><strong>Cc: </strong>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<br /></div>