<table><tr><td style="">broulik 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/D11064">View Revision</a></tr></table><br /><div><div><p>Thanks a lot for your patch! This makes the effect of those settings a lot easier to understand. I have a couple of nitpicks below.</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/D11064#inline-53534">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.qml:22</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: #304a96">import</span> <span style="color: #004012">QtQuick</span><span class="p">.</span><span style="color: #004012">Layouts</span> <span style="color: #601200">1.1</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">import</span> <span style="color: #004012">QtQuick</span><span class="p">.</span><span style="color: #004012">Controls</span> <span style="color: #601200">2.<span class="bright">0</span></span> <span style="color: #004012">as</span> <span style="color: #004012">QtControls</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">import</span> <span style="color: #004012">QtQuick</span><span class="p">.</span><span style="color: #004012">Controls</span> <span style="color: #601200">2.<span class="bright">3</span></span> <span style="color: #004012">as</span> <span style="color: #004012">QtControls</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span style="color: #304a96">import</span> <span style="color: #004012">QtQuick</span><span class="p">.</span><span style="color: #004012">Dialogs</span> <span style="color: #601200">1.2</span> <span style="color: #004012">as</span> <span style="color: #004012">QtDialogs</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Is <tt style="background: #ebebeb; font-size: 13px;">2.2</tt> also sufficient (Qt 5.9)?</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/D11064#inline-53526">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.qml:143</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">id: subPixelDelegate</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">width:</span> <span style="color: #004012">subPixelComboImage</span><span class="p">.</span><span style="color: #004012">implicitWidth</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">contentItem:</span> <span style="color: #004012">ColumnLayout</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The popup width is too small, the text cut off. Either make it larger or at least elide the text on the right</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/D11064#inline-53525">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.qml:147</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">width:</span> <span style="color: #004012">subPixelComboImage</span><span class="p">.</span><span style="color: #004012">implicitWidth</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #004012">Text</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">id: subPixelComboText</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Use QtQuick Controls <tt style="background: #ebebeb; font-size: 13px;">Label</tt> instead of plain <tt style="background: #ebebeb; font-size: 13px;">Text</tt> for proper rendering and text color</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/D11064#inline-53528">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.qml:153</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">id: subPixelComboImage</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">fillMode:</span> <span style="color: #004012">Image</span><span class="p">.</span><span style="color: #004012">Pad</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">sourceSize:</span> <span style="color: #000a65">undefined</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is the default, no need to explicitly specify</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/D11064#inline-53529">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.qml:154</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">fillMode:</span> <span style="color: #004012">Image</span><span class="p">.</span><span style="color: #004012">Pad</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">sourceSize:</span> <span style="color: #000a65">undefined</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">source:</span> <span style="color: #766510">"image://preview/"</span><span style="color: #aa2211">+</span><span style="color: #004012">model</span><span class="p">.</span><span style="color: #004012">index</span><span style="color: #aa2211">+</span><span style="color: #766510">"_"</span><span style="color: #aa2211">+</span><span style="color: #004012">kcm</span><span class="p">.</span><span style="color: #004012">fontAASettings</span><span class="p">.</span><span style="color: #004012">hintingCurrentIndex</span><span style="color: #aa2211">+</span><span style="color: #766510">".png"</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Also not needed</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/D11064#inline-53530">View Inline</a><span style="color: #4b4d51; font-weight: bold;">main.qml:155</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">sourceSize:</span> <span style="color: #000a65">undefined</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">source:</span> <span style="color: #766510">"image://preview/"</span><span style="color: #aa2211">+</span><span style="color: #004012">model</span><span class="p">.</span><span style="color: #004012">index</span><span style="color: #aa2211">+</span><span style="color: #766510">"_"</span><span style="color: #aa2211">+</span><span style="color: #004012">kcm</span><span class="p">.</span><span style="color: #004012">fontAASettings</span><span class="p">.</span><span style="color: #004012">hintingCurrentIndex</span><span style="color: #aa2211">+</span><span style="color: #766510">".png"</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Coding style, add spaces between:</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);">source: "image://preview/" + model.index + "_" + kcm.fontAASettings.hintCurrentIndex + ".png"</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/D11064#inline-53532">View Inline</a><span style="color: #4b4d51; font-weight: bold;">previewimageprovider.cpp:35</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: #aa4000">int</span> <span class="n">subPixelIndex</span> <span style="color: #aa2211">=</span> <span class="n">id</span><span class="p">.</span><span class="n">split</span><span class="p">(</span><span style="color: #766510">'_'</span><span class="p">)[</span><span style="color: #601200">0</span><span class="p">].</span><span class="n">toInt</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">int</span> <span class="n">hintingIndex</span> <span style="color: #aa2211">=</span> <span class="n">id</span><span class="p">.</span><span class="n">split</span><span class="p">(</span><span style="color: #766510">'_'</span><span class="p">)[</span><span style="color: #601200">1</span><span class="p">].</span><span class="n">toInt</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">avoid splitting twice, I suggest storing</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);">const auto sections = id.splitRef(QLatin1Char('_'));</pre></div>
<p style="padding: 0; margin: 8px;">Also do a bounds check</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/D11064#inline-53533">View Inline</a><span style="color: #4b4d51; font-weight: bold;">previewimageprovider.cpp:66</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">PreviewRenderEngine</span> <span style="color: #004012">eng</span><span class="p">(</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">QImage</span> <span class="n">img</span> <span style="color: #aa2211">=</span> <span class="n">eng</span><span class="p">.</span><span class="n">drawAutoSize</span><span class="p">(</span><span class="n">m_font</span><span class="p">,</span> <span class="n">text</span><span class="p">,</span> <span class="n">bgnd</span><span class="p">,</span> <span class="n">eng</span><span class="p">.</span><span class="n">getDefaultPreviewString</span><span class="p">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Your image does not take into account <tt style="background: #ebebeb; font-size: 13px;">devicePixelRatio</tt> which makes it blurred on my high dpi screen.</p>
<p style="padding: 0; margin: 8px;">I'm not sure how to exactly fix that, perhaps <tt style="background: #ebebeb; font-size: 13px;">@2x</tt> works for the image provider, or you can manually implement this, to test run</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);">QT_SCREEN_SCALE_FACTORS=2 kcmshell5 kcm_fonts</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/D11064#inline-53531">View Inline</a><span style="color: #4b4d51; font-weight: bold;">previewimageprovider.h:1</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 __PREVIEW_IMAGE_PROVIDER_H__</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#define __PREVIEW_IMAGE_PROVIDER_H__</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Include guard typically goes below copyright, you can also use <tt style="background: #ebebeb; font-size: 13px;">#pragma once</tt> in plasma-desktop</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R119 Plasma Desktop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D11064">https://phabricator.kde.org/D11064</a></div></div><br /><div><strong>To: </strong>progwolff, Plasma, harmathy, mart<br /><strong>Cc: </strong>broulik, plasma-devel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>