<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/117785/">https://git.reviewboard.kde.org/r/117785/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I ask you favor, stop doing unnecessary changes to the project's coding style. Although I also don't like it, those changes make your patch more difficult to read.
Consider carefully the issues I pointed bellow. If you have any questions or objections, feel free to expose them.
PS: for some reason I do not understand, the following glitches appeared when I tested your patch. See the screenshot: http://imagebin.org/308512</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117785/diff/2/?file=269058#file269058line160" style="color: black; font-weight: bold; text-decoration: underline;">wall.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KBounceWall::KBounceWall( Direction dir, KBounceRenderer* renderer, KBounceBoard* board )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">147</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span> <span class="n">QRect</span><span class="p">(</span> <span class="mi">0</span><span class="p">,</span> <span class="mi">0</span><span class="p">,</span> <span class="n">tileWidth</span><span class="p">,</span> <span class="n">qMin</span><span class="p">(</span> <span class="n">tileHeight</span><span class="p">,</span> <span class="n">boundingRectHeight</span> <span class="p">)</span> <span class="p">),</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">148</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">static</span> <span class="n">QPixmap</span> <span class="n">wallEndLeft</span><span class="p">,</span> <span class="n">wallEndUp</span><span class="p">,</span> <span class="n">wallEndRight</span><span class="p">,</span> <span class="n">wallEndDown</span><span class="p">,</span> <span class="n">wallH</span><span class="p">,</span> <span class="n">wallV</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Why these variables need to be static? If they have to be preserved for future calls of update() then store them as private attributes of KBounceWall. Let's try to keep our code as object-oriented as possible.
Notice that the same argument applies to gs_cachedSize.</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/117785/diff/2/?file=269058#file269058line161" style="color: black; font-weight: bold; text-decoration: underline;">wall.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KBounceWall::KBounceWall( Direction dir, KBounceRenderer* renderer, KBounceBoard* board )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">148</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="tb"> </span><span class="tb"> </span> <span class="n">m_renderer</span><span class="o">-></span><span class="n">spritePixmap</span><span class="p">(</span> <span class="s">"wallEndUp"</span><span class="p">,</span> <span class="n">m_tileSize</span> <span class="p">),</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">149</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">gs_cachedSize</span> <span class="o">!=</span> <span class="n">m_tileSize</span><span class="p">)</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This whole block does not belong here. What you're doing is fetching the sprites from the renderer only if the current tile size differs from the size you cached, that is, we have a resize.
However, we already have a method for handling resize events. That's where this block should be placed in.
The solution I propose:
1. Make wallEndLeft, wallEndUp etc. private attributes of KBounceWall.
2. Turn this block into a new method (fetchSprites() or render(), perhaps) and make a call to it inside KBounceWall::resize(). This way we're making our code more modular and easier to understand.
3. To invalidate the cache when the user select a new theme you don't need to erase gs_cachedSize as is done currently, just call the new fetchSprites() method instead. Actually gs_cachedSize becomes completely unnecessary.
</pre>
</div>
<br />
<p>- Roney Gomes</p>
<br />
<p>On April 28th, 2014, 7:35 p.m. UTC, Thomas Lübking wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Games, Ian Wadham and Roney Gomes.</div>
<div>By Thomas Lübking.</div>
<p style="color: grey;"><i>Updated April 28, 2014, 7:35 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kbounce
</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;">Roney, I hope your the correct addressee - Ian referecend some "Roney" in a private mail.
The patch looks bigger than it is, i had to partially fix coding style in order to read through it, sorry.
The MAJOR issue with the code is that it causes recursions on the eventloop by altering graphicsitems from the paint() call ("setPixmap()"), so i scrathced that.
It seems a cause was that a full repaint was forced by setting some invisible fullsize pixmap item, while the only requirement for this was actually when finishing a wall - so i replaced that with an explicit full update call on occasion.
Also all non-raster graphicssystems HATE QPixmap::fill(Qt::transparent) causing a 24bit -> 32bit change and it's esp. bad on the nvidia driver.
So i reduced that (and allocation) by re-using the old (ARGB) pixmap on a 64x64 alignment.
Ian, you might esp. check that impact on OSX.
Last I cached the sprites, seemed reducing cpu on permanent paint (but i didn't really measure)
I'll also mark & comment the actual changes in the diff.</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;">100% cpu -> 1-2% cpu and more fluid =)
No visible regression spotted.</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>board.h <span style="color: grey">(75f66d4)</span></li>
<li>board.cpp <span style="color: grey">(46b923b)</span></li>
<li>gameobject.h <span style="color: grey">(9fb5788)</span></li>
<li>gamewidget.cpp <span style="color: grey">(23cb6ba)</span></li>
<li>wall.h <span style="color: grey">(c56efa1)</span></li>
<li>wall.cpp <span style="color: grey">(df487a0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117785/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>