<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/127866/">https://git.reviewboard.kde.org/r/127866/</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 30th, 2016, 11:34 a.m. UTC, <b>Hugo Pereira Da Costa</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;">Ship It!</pre>
</blockquote>
<p>On May 30th, 2016, 11:39 a.m. UTC, <b>Hugo Pereira Da Costa</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;">err. Wait ...
There are rendering issues here once the patch is applied.
See http://wstaw.org/m/2016/05/30/plasma-desktopY12228.png
(left is "before", right is "after").
So something seems wrong with the background gradient.
I'll investigate a bit ...</p></pre>
</blockquote>
<p>On May 30th, 2016, 12:39 p.m. UTC, <b>Hugo Pereira Da Costa</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;">Hi again,
So, thinking more about it, and actually answering the questions raised in the review:</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;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">do we track public API for this part of Oxygen? Does anything in a different library or application link to this?
No we don't this is supposed to be an "internal" (as in private) library, used only by oxygen style and decoration. No ABI/API guarantee. </p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QColor: can we change the return type. I would say yes, (from reference to value), and return to using QCache</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Tilesets: I would be inclined to changing the return type here too, using values, and assuming the copy constructor does not cost much, based on the implicit-shareness nature of pixmaps, and keep a built-in QCache here (which should be more efficient than the (otherwise nice) FIFO. </li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now I understand that this is a lot of going back and forth, and a job I should rather do myself, as the maintainer of the code.
So I would propose to "postpone" this RR for now, and for me to locally
- take the QPixmap change from this patch
- implement something similar for QColor and TileSet
- test.
Then if I manage to do that in a reasonable time (e.g. this week), drop the review and commit my change instead (with proper credits where due). Otherwise (because of me being too busy with other stuff), just commit this review (once the problem mentionned above is fixed, though I could not investigate yet).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What do you think ? </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(also: I need to sanitize this run-time changing of the cache use and max-cost)</p></pre>
</blockquote>
<p>On May 30th, 2016, 3:17 p.m. UTC, <b>Michael Pyne</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;">Hugo,
That all sounds fair. And if it's all too much difficulty, then we might be able to lean on the hinted-at-but-not-quite-documented QCache behavior that ::insert() only fails if the item being inserted has a cost higher than maxCost. In that case I could ignore the Coverity issues (since Coverity can't statically prove that items are always < maxCost) and then drop the RR (and perhaps ask Qt to document more stringently that guarantee for QCache::insert).
Still though, I think the QColor return type change would make sense even independent of this RR.</pre>
</blockquote>
<p>On June 6th, 2016, 7:52 p.m. UTC, <b>Hugo Pereira Da Costa</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;">Hi again Michael,</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So, I have a working implementation here that
- keeps using QCache wherever possible, passing QColor and TileSet as values rather than as refs (in a way that is similar to the change you did for QPixmaps)
- uses your FIFOCache for the implementation of Oxygen::Cache, which is a "cache of caches", and thus cannot possibly use values.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Seems to work (though I would like to test a bit more), and should fix all the issues with Coverty.
Since I have blatently copied your code for the home-made FiFOCache, i have added you as a copyright holder for oxygenhelper.h
is that ok with you ? Should i put your name in other places too ? </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Best, </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hugo</p></pre>
</blockquote>
<p>On June 8th, 2016, 2:49 a.m. UTC, <b>Michael Pyne</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;">Since I have blatently copied your code for the home-made FiFOCache, i have added you as a copyright holder for oxygenhelper.h
is that ok with you ? Should i put your name in other places too ?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's perfectly fine. You don't need to mention me elsewhere, as long as this RR is mentioned in the commit log I think that would cover any needed attribution of work.</p></pre>
</blockquote>
<p>On February 25th, 2017, 10:33 p.m. UTC, <b>Albert Astals Cid</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;">What's the status of this, was commited? Needs further work? Should be discarded?</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;">yes.Code was committed,review should be discarded</p></pre>
<br />
<p>- Hugo</p>
<br />
<p>On May 22nd, 2016, 4:20 a.m. UTC, Michael Pyne 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-workspace and Hugo Pereira Da Costa.</div>
<div>By Michael Pyne.</div>
<p style="color: grey;"><i>Updated May 22, 2016, 4:20 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
oxygen
</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 should mostly complete the QCache fixes I kicked off in a previous RR, 127837. Hugo noted there were many other similar usages, and boy he wasn't kidding! ;) The long story short is that these usages can theoretically cause use-after-free behavior (which can lead to crashes and even undefined behavior if the compiler ever gets smart).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">NOTE</em> It is -much- easier to review if you download the diff to your git repository for oxygen and then run "git diff -b" to ignore whitespace changes, particularly for the QPixmap changes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For QPixmaps we return values instead of pointers, so we simply make a separate copy to be cached when we do insert. For QColor we return references to values so we <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">must</em> return pointers, and those have to be owned by a QCache to avoid memleaks. So I added a helper function to loop until the cache accepts the new entry. TileSets are a similar concern, except those have manual loops since I was uncertain about whether TileSet's copy constructor was the best idea or not.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This fixes a ton of Coverity issues (59717 - 259733, 259739, 259742 - 259752, 1336154, 1336155) and might be associated with Qt bug 38142 and KDE bug 219055 (which doesn't actually appear to be a dupe of a different bug to me...).</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;">Compiled without warnings, installed and ran <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">oxygen-demo5 -style oxygen</code>. Used the GUI Benchmark feature to automatically cycle through all the listed features -- no crashes or obvious rendering errors.</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>kdecoration/oxygendecohelper.cpp <span style="color: grey">(aa75eca)</span></li>
<li>kstyle/oxygenstyle.cpp <span style="color: grey">(e428606)</span></li>
<li>kstyle/oxygenstylehelper.h <span style="color: grey">(9510a60)</span></li>
<li>kstyle/oxygenstylehelper.cpp <span style="color: grey">(612ba37)</span></li>
<li>liboxygen/oxygenhelper.h <span style="color: grey">(a6453a0)</span></li>
<li>liboxygen/oxygenhelper.cpp <span style="color: grey">(4843604)</span></li>
<li>liboxygen/oxygenshadowcache.cpp <span style="color: grey">(907e586)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127866/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>