<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/123682/">https://git.reviewboard.kde.org/r/123682/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 9th, 2015, 1:18 p.m. UTC, <b>Eike Hein</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regressions just going by the screenshot:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> Doesn't use form layouting (e.g. the font box labels aren't right-aligned).
</em> The font box height doesn't match the button heights anymore.
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> Missing colons in the text labels.
</em> Wonky margin between combo box label and combo box.
* No keyboard accelerators.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">General conceptual problems with this port:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> Poor QStyle support, e.g. drag on empty space in Oxygen/Breeze doesn't work with this toolkit.
</em> IIRC QStyle background support (like Oxygen's radial gradient) doesn't work either, don't remember though.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Additional problems after trying it out:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> Poor load performance - loading the KCM from the System Settings main page is significantly slower than the old KCM and involves some flicker.
</em> The anti-aliasing config dialog has a white/unstyled background and improper dialog margins. There's also a ton of crap margins/positioning around widget in its form.
* The "Hinting style" combo box popup has wonky margins between the radio buttons and the text labels, and a grey background when it's supposed to be white.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What's the motivation behind this port? What user problem does it solve? How does it make this product better? Remember to not just throw code over the fence; you're supposed to argue for why a change is a good idea on review board. Your review request doesn't adequately answer any of these questions. Note that "but other KCMs use Qt Quick and have similar problems" is no argument for making this one worse (but it's an argument for fixing the others by whatever means).</p></pre>
</blockquote>
<p>On May 9th, 2015, 1:20 p.m. UTC, <b>Eike Hein</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Sorry, the above was destroyed by markdown parsing:
Regressions just going by the screenshot:
- Doesn't use form layouting (e.g. the font box labels aren't right-aligned).
- The font box height doesn't match the button heights anymore.
- Missing colons in the text labels.
- Wonky margin between combo box label and combo box.
- No keyboard accelerators.
General conceptual problems with this port:
- Poor QStyle support, e.g. drag on empty space in Oxygen/Breeze doesn't work with this toolkit.
- IIRC QStyle background support (like Oxygen's radial gradient) doesn't work either, don't remember though.
Additional problems after trying it out:
- Poor load performance - loading the KCM from the System Settings main page is significantly slower than the old KCM and involves some flicker.
- The anti-aliasing config dialog has a white/unstyled background and improper dialog margins. There's also a ton of crap margins/positioning around widget in its form.
- The "Hinting style" combo box popup has wonky margins between the radio buttons and the text labels, and a grey background when it's supposed to be white.
What's the motivation behind this port? What user problem does it solve? How does it make this product better? Remember to not just throw code over the fence; you're supposed to argue for why a change is a good idea on review board. Your review request doesn't adequately answer any of these questions. Note that "but other KCMs use Qt Quick and have similar problems" is no argument for making this one worse (but it's an argument for fixing the others by whatever means).</pre>
</blockquote>
<p>On May 11th, 2015, 10:59 a.m. UTC, <b>Marco Martin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">so, we talked a bit about it in hangout, the solution will be:
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> do a new branch of master (called like QmlKCMs)
</em> merge this and the cursortheme one in such branch
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> do any eventual new one in the branch
</em> master stays as it is for the next future
* (even ask distributions to package such branch as an experimental, non default alternative to make testing easy)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">any eventual qml/qtquickcontrols but that comes out from doing that is attempted to be fixed upstream
the one big branch will be merged when good enough.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">^ Sounds good</p></pre>
<br />
<p>- Eike</p>
<br />
<p>On May 8th, 2015, 12:39 p.m. UTC, Antonis Tsiapaliokas wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma.</div>
<div>By Antonis Tsiapaliokas.</div>
<p style="color: grey;"><i>Updated May 8, 2015, 12:39 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-desktop
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch ports the kcm fonts to QML.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Everything works execpt from the ComboBox and the FontDialog ("Configure Font").</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">FontDialog</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you open the kcm inside from the system settings, everything is ok.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you use kcmshell5 fonts, the FontDialog is opening behing the kcm window.
In order to solve this issue we must use the setTransientParent, but how can
i do that in the FontDialog?</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">ComboBox</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you open the kcm with the "kcmshell5 fonts", the dropdown menu renders fine.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">But if you open it inside from the system settings, the dropdown menu, renders
in the left of the ComboBox.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also these two signals (main.qml line 295)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">onDpiChanged
onAliasingChanged </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">are being emitted but the kcm.needsSave doesn't work...</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kcms/fonts/CMakeLists.txt <span style="color: grey">(d73636e)</span></li>
<li>kcms/fonts/fonts.cpp <span style="color: grey">(74da799)</span></li>
<li>kcms/fonts/fonts.desktop <span style="color: grey">(1cdad40)</span></li>
<li>kcms/fonts/fonts.h <span style="color: grey">(d98bbe2)</span></li>
<li>kcms/fonts/kcm_fonts.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>kcms/fonts/package/contents/ui/main.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>kcms/fonts/package/metadata.desktop <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123682/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/05/08/833710e7-9569-4cd3-a02f-3ebe95a1c914__kcm_fonts_qml.png">fonts qml</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>