<table><tr><td style="">kossebau added a comment.
</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/D25877">View Revision</a></tr></table><br /><div><div><p>Some comments while flying over the code due to being triggered by phab message, but no in-detail review and analysis of approach done, lack of concentration currently, sorry.</p>

<p>I also propose to change all magic number <tt style="background: #ebebeb; font-size: 13px;">0</tt>s to use some <tt style="background: #ebebeb; font-size: 13px;">defaultThemeRow</tt> from a <tt style="background: #ebebeb; font-size: 13px;">constexpr int defaultThemeRow = 0:</tt> instead,</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/D25877#inline-148002">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcolorschememanager.cpp:159</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; "><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">name</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #aa4000">return</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">model</span><span style="color: #aa2211">-></span><span class="n">index</span><span class="p">(</span><span style="color: #601200">0</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Propose to add a short comment explaining why we return index(0) here for the future code reader starting off at this code before having read all code and docs. E.g.<br />
<tt style="background: #ebebeb; font-size: 13px;">// empty string is mapped to "reset to default/system", which is first item in model</tt><br />
That ensures the intention of this code is clear on high level.</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/D25877#inline-148003">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcolorschememanager.cpp:162</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; ">    <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">int</span> <span class="n">i</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span> <span class="n">i</span> <span style="color: #aa2211"><</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">model</span><span style="color: #aa2211">-></span><span class="n">rowCount</span><span class="p">();</span> <span style="color: #aa2211">++</span><span class="n">i</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; ">        <span class="n">QModelIndex</span> <span class="n">index</span> <span style="color: #aa2211">=</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">model</span><span style="color: #aa2211">-></span><span class="n">index</span><span class="p">(</span><span class="n">i</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Could start off at 1 now, no?</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/D25877#inline-148004">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcolorschememanager.cpp:189</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; ">    <span class="p">}</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright">    </span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">!</span></span><span class="bright"></span><span class="n"><span class="bright">group</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">checkedAction</span></span><span class="bright"></span><span class="p"><span class="bright">())</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">group</span><span style="color: #aa2211">-></span><span class="n">actions</span><span class="p">()[</span><span style="color: #601200">0</span><span class="p">]</span><span style="color: #aa2211">-></span><span class="n">setChecked</span><span class="p">(</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;">also could get a short comment what the highlevel logic is here<br />
<tt style="background: #ebebeb; font-size: 13px;">// no color theme selected? so it's the default one</tt></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/D25877#inline-148006">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcolorschememanager.cpp:221</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 style="color: #74777d">/* hint for plasma-integration to synchronize the color scheme with the window manager/compositor</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d">     * The property needs to be set before the palette change because is is checked upon the </span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Using <tt style="background: #ebebeb; font-size: 13px;">//</tt> for each comment line is more typical for explanation comments, why the different style here?</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/D25877#inline-148001">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kcolorschememanager.h: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; "><span style="color: #74777d"> * schemes to their user. For example it is very common for photo and painting applications to use</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #74777d"> * a dark color scheme even if the default is a light scheme.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * a dark color scheme even if the default is a light scheme.<span class="bright"> It also allows going back to following</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #74777d"> * the system color scheme.</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Here you might also want to state at which version this feature of "allows going back" was added, to make it clear that with older versions of KF this was not possible.<br />
Same with the existing methods where behaviour was changed.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R265 KConfigWidgets</div></div></div><br /><div><strong>BRANCH</strong><div><div>systemthem (branched from master)</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D25877">https://phabricator.kde.org/D25877</a></div></div><br /><div><strong>To: </strong>davidre, Frameworks, ngraham<br /><strong>Cc: </strong>ahmadsamir, asemke, kossebau, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns<br /></div>