<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/119336/">https://git.reviewboard.kde.org/r/119336/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 17th, 2014, 4:54 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;">Unless I'm missing something, this approach seems to defeat the purpose of the exercise: The goal of the original change is to use actually use smaller textures that fit the QSG's "this can use the atlas" heuristic on most platforms. You have one node per patch element here, but they're all calling createTextureFromImage() on a big honking QImage that's not going to be atlased for most frames.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Can you explain your pixel-perfect quality concerns better - you've said the competing approach breaks things there but not mentioned what or why?</p></pre>
</blockquote>
<p>On July 17th, 2014, 5:15 p.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;">That's a button that uses composeoverborders: see the corners/sides?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
http://wstaw.org/m/2014/07/17/plasma-desktopvc1725.png<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
(overlays would give a similar problem, not only the internal is painted over the borders, but it's also cutted with an alpha mask to match)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
That's why i'm using framesvg to paint with the local qpainter instead of just taking the side image.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As for defeating the purpose yeah, it does (what actually does is just being faster duting animated resizes), but in simple cases (no composeoverborders, no overlays) the timer may be disabled altogether.</p></pre>
</blockquote>
<p>On July 17th, 2014, 5:37 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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">composeoverborders</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Right, but doesn't have David's patch a fallback codepath for that? Obviously I'd heavily -1 everything causing visual regressions too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About the symbol exports: I don't think they're a major issue. We're not installing the header, things aren't exposed to developers above the lib, and it's certainly not an uncommon pattern in Qt (there's data() methods all over the place, even in public APIs).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On the whole I think David's patch has some compelling advantages in the ideal case that may warrant the complexity (I don't think complexity in low-level components is a big deal if it's Worth It[TM] and just needs to be debugged once; this isn't code that changes often).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How do we progress here?</p></pre>
</blockquote>
<p>On July 17th, 2014, 5:45 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;">To credit Marco, I've only just added that to the fallback path; thanks for pointing it out.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
We can then recommend themers avoid it; am I right in thinking it was just for QPainter optimisations?</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;">the screenshot i posted is with david's patch, and you can see the button background one pixel outside its corners.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
the fallback here is to yes rerender the scaled svg, but with Svg::image(), that only returns the rendered element id, not the composition with alpha masks and all that jazz. Also since it immediately rerenders the svg in that case it loses the advantage when the resize it's animated.</p></pre>
<br />
<p>- Marco</p>
<br />
<p>On July 17th, 2014, 12:47 p.m. UTC, Marco Martin 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 and Plasma.</div>
<div>By Marco Martin.</div>
<p style="color: grey;"><i>Updated July 17, 2014, 12:47 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;">This is a derivative of https://git.reviewboard.kde.org/r/119330/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It has a much simpler codebase and it doesn't touch framesvg in the library (and doesn't make things public)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the important differences are two, that are imo 100% necessary to maintain a pixel perfect rendering (sacrificing that is a regression simply not acceptable in any case, even for non default themes, ever). At the same time avoids duplication of framesvg code in framesvgitem.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">potential issues:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> e037203748 support for tiling still need porting<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
</em> yes, it uses a qpainter, that means another copy: but again is necessary as framesvg knows how to render the end result pixel perfect, since for many themes the end piece is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> the simple rendered element id.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
* yes, the final scaled texture is still uploaded as a whole, it only avoids to do it too often (like in animations) with event compression, again, only way to be sure it's correctly rendered all the time.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The latter two points can have the following optimizations:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Iff the frame does not have an overlay and does not have composeoverborders set, the following can be done:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;"> use Svg::image() to fetch the pixmap of the piece, since in that case would be valid<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
</em> resize the framesvg one single time, at a fixed size , like something > 256x256 (256 is not random thing, since we have 8 bits per channel, usually gradients will have no more than 256 stops) and disable the resize timer, this way we are even sure that only one image per prefix will be stored in cache</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I should add regarding the last two optimization points: i would like to see some real benchmarks about them, or i would not consider then necessary until then.</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>examples/applets/widgetgallery/contents/ui/Buttons.qml <span style="color: grey">(379585f)</span></li>
<li>examples/applets/widgetgallery/contents/ui/Menu.qml <span style="color: grey">(1336c42)</span></li>
<li>examples/applets/widgetgallery/contents/ui/standalonemain.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/declarativeimports/core/framesvgitem.h <span style="color: grey">(e155f6a)</span></li>
<li>src/declarativeimports/core/framesvgitem.cpp <span style="color: grey">(8320212)</span></li>
<li>src/declarativeimports/core/svgitem.cpp <span style="color: grey">(1ed0631)</span></li>
<li>tests/dialog.qml <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/testborders.qml <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119336/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>