<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/121411/">https://git.reviewboard.kde.org/r/121411/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 9th, 2014, 6:11 p.m. UTC, <b>Kai Uwe Broulik</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;">Wasn't that part of the idea? Having it scale up the pixmap first when resizing and then re-rendering it later?</p></pre>
</blockquote>
<p>On December 9th, 2014, 7:49 p.m. UTC, <b>Xuetian Weng</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;"><ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">icon size (the widget size) doesn't change frequently. Usually it only happens when user resize the UI or changes the settings.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">the animation IMHO is for smooth transition between different icons/different icon effect (check volume icon, media player play/pause, hovering with opacity changes), I don't see we need to have animated transition from a scaled icon to the actual correct size icon.</li>
</ol></pre>
</blockquote>
<p>On December 10th, 2014, 2:23 p.m. UTC, <b>David Edmundson</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;">Icon size changes may be infrequent but they do happen. It's important that when we resize the panel we don't re-render a tonne of SVGs constantly, we need resizing applets to be smooth.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't understand where this blurriness is coming from; that implies we're loading it at a wrong size then resizing; if that's the case, lets fix that rather than hide it.</p></pre>
</blockquote>
<p>On December 10th, 2014, 2:32 p.m. UTC, <b>Sebastian Kügler</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;">During resize, we're rescaling a pixmap instead of re-rendering the SVG for every frame. That can induce blur.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, rendering an SVG at random sizes van induce blur, since not everything can be aligned to pixels all the time, our SVGs are optimized for "standard sizes".</p></pre>
</blockquote>
<p>On December 10th, 2014, 4:18 p.m. UTC, <b>Xuetian Weng</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;">The problem is not loading it at wrong size. It is the widget is not resized to correct size yet.
The most significant one on plasma here is Device Notifier (When it's popped up at the first time), I print out some debug message at ::geometryChanged
ICON QSizeF(1, 86) QSizeF(1, 1) QVariant(QString, "device-notifier")
ICON QSizeF(129, 86) QSizeF(1, 86) QVariant(QString, "device-notifier")</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As you can see, the size changed from 1x1 to 1x86 then to 129x86. So they are valid size, and the blurriness is exposed by the transition animation.</p></pre>
</blockquote>
<p>On December 10th, 2014, 4:54 p.m. UTC, <b>David Edmundson</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;">Thanks. I can reproduce with the Device Notifier + debug</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What I'm trying to argue is that it should go directly to 129x86. (or at least be an invalid size till then)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Otherwise we're loading an SVG 3 times and not using two of them this patch will hide the visual artifacts of that, but we'll still be wasting a lot of CPU cycles doing something silly.</p></pre>
</blockquote>
<p>On December 11th, 2014, 3:51 a.m. UTC, <b>Xuetian Weng</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;">emm, seems the 1x1 size comes from lines 67 and 68: http://quickgit.kde.org/?p=plasma-workspace.git&a=blob&h=071705a85aee9ee00a3c88657c93d20544eb5c0f&hb=8b670015582e6b501a2a81a23f38eb5772a36e85&f=applets%2Fsystemtray%2Fplugin%2Fprotocols%2Fplasmoid%2Fplasmoidtask.cpp</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">from current code I don't see an easy way to avoid that... I think there's one resize event can be avoided by call setSize(), but anyway there need to one extra resize.</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;">Good find. We could try setting that to 0x0 and see what happens. We'll save a load of cycles on all sorts of other code that way.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">if it goes from 0 to something sensible, it wont' animate as the old m_oldIconPixmap will be null</p></pre>
<br />
<p>- David</p>
<br />
<p>On December 11th, 2014, 7:49 p.m. UTC, Xuetian Weng 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 KDE Frameworks, Plasma, Aleix Pol Gonzalez, and Kai Uwe Broulik.</div>
<div>By Xuetian Weng.</div>
<p style="color: grey;"><i>Updated Dec. 11, 2014, 7:49 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-framework
</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;">Making transition between two different size doesn't make much sense, since repainting is usually happens at that time and it could take some time to finish. And animation need to be stopped if m_animValue is set manually.</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;">Looks fine on tray icon and lock screen, less blurry transition.</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>src/declarativeimports/core/iconitem.cpp <span style="color: grey">(145a7cd)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121411/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>