<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#ffffff">
Thanks Kevin, I'll get onto those.<br>
<br>
When updating the patch, is it best to start a new one or just add
the changes as a diff to the existing reviewboard entry?<br>
<br>
On 06/02/11 22:17, Kevin Krammer wrote:
<blockquote cite="mid:20110206121757.10773.72438@vidsolbach.de"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<table style="border: 1px solid rgb(201, 195, 153);"
width="100%" bgcolor="#f9f3c9" cellpadding="8">
<tbody>
<tr>
<td> This is an automatically generated e-mail. To reply,
visit: <a moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/">http://svn.reviewboard.kde.org/r/5399/</a>
</td>
</tr>
</tbody>
</table>
<br>
<pre style="white-space: pre-wrap; word-wrap: break-word;">Not a KLines developer so just a few general hints.</pre>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192);
border-collapse: collapse;" width="100%" bgcolor="white"
border="0">
<thead> <tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192,
192, 192); font-size: 9pt; padding: 4px 8px;
text-align: left;" bgcolor="#f0f0f0"> <a
moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38349#file38349line31"
style="color: black; font-weight: bold;
text-decoration: underline;">/trunk/KDE/kdegames/klines/ballitem.cpp</a>
<span style="font-weight: normal;"> (Diff revision 3)
</span> </th>
</tr>
</thead> <tbody style="background-color: rgb(228, 217,
203); padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><br>
</td>
</tr>
</tbody> <tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">29</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"> <span class="o">:</span> <span class="n">QGraphicsPixmapItem</span><span class="p">(</span> <span class="mi">0</span><span class="p">,</span> <span class="n">parent</span> <span class="p">)</span></pre>
</td>
<th style="border-left: 1px solid rgb(192, 192, 192);
border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">31</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"> <span class="o">:</span> <span class="n">KGameRenderedItem</span><span class="p">(</span><span class="n">KLinesRenderer</span><span class="o">::</span><span class="n">renderer</span><span class="p">()</span> <span class="p">,</span> <span class="s">""</span><span class="p">,</span> <span class="nb">NULL</span><span class="p">)</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; word-wrap: break-word;">Use QString() instead of "".
QString() is a very cheap operation, it only creates the handle object, without any data.
</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192);
border-collapse: collapse;" width="100%" bgcolor="white"
border="0">
<thead> <tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192,
192, 192); font-size: 9pt; padding: 4px 8px;
text-align: left;" bgcolor="#f0f0f0"> <a
moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38349#file38349line48"
style="color: black; font-weight: bold;
text-decoration: underline;">/trunk/KDE/kdegames/klines/ballitem.cpp</a>
<span style="font-weight: normal;"> (Diff revision 3)
</span> </th>
</tr>
</thead> <tbody style="background-color: rgb(228, 217,
203); padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><br>
</td>
</tr>
</tbody> <tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">45</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"> <span class="n">set<span class="hl">Pixmap</span></span><span class="p"><span class="hl">(</span></span><span class="hl"> </span><span class="n">KLinesRenderer</span><span class="o">::</span><span class="n"><span class="hl">self</span></span><span class="p"><span class="hl">()</span></span><span class="o"><span class="hl">-></span></span><span class="n">ballPixmap</span><span class="p"><span class="hl">(</span></span><span class="hl"> </span><span class="n">m_color</span> <span class="p"><span class="hl">)</span></span><span class="hl"> </span><span class="p">);</span></pre>
</td>
<th style="border-left: 1px solid rgb(192, 192, 192);
border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">48</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"> <span class="n">setSpriteKey</span><span class="p">(</span><span class="n">KLinesRenderer</span><span class="o">::</span><span class="n">ballPixmapId</span><span class="p">(</span><span class="n">m_color</span><span class="p">));</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; word-wrap: break-word;">Seems to change the coding style regarding whitespace within parentheses.</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192);
border-collapse: collapse;" width="100%" bgcolor="white"
border="0">
<thead> <tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192,
192, 192); font-size: 9pt; padding: 4px 8px;
text-align: left;" bgcolor="#f0f0f0"> <a
moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38352#file38352line38"
style="color: black; font-weight: bold;
text-decoration: underline;">/trunk/KDE/kdegames/klines/renderer.h</a>
<span style="font-weight: normal;"> (Diff revision 3)
</span> </th>
</tr>
</thead> <tbody style="background-color: rgb(228, 217,
203); padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><br>
</td>
</tr>
</tbody> <tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#f0f0f0"><font size="2">38</font></th>
<td width="50%" bgcolor="#ffffff">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"><span class="cm"> * Only one instance of this class exists during a program run.</span></pre>
</td>
<th style="border-left: 1px solid rgb(192, 192, 192);
border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#f0f0f0"><font size="2">35</font></th>
<td width="50%" bgcolor="#ffffff">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"><span class="cm"> * Only one instance of this class exists during a program run.</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; word-wrap: break-word;">Is this part of the comment still applicable after the change to "all members static"?</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192);
border-collapse: collapse;" width="100%" bgcolor="white"
border="0">
<thead> <tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192,
192, 192); font-size: 9pt; padding: 4px 8px;
text-align: left;" bgcolor="#f0f0f0"> <a
moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38353#file38353line71"
style="color: black; font-weight: bold;
text-decoration: underline;">/trunk/KDE/kdegames/klines/renderer.cpp</a>
<span style="font-weight: normal;"> (Diff revision 3)
</span> </th>
</tr>
</thead> <tbody style="background-color: rgb(228, 217,
203); padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><br>
</td>
<td colspan="2">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;">KLinesRenderer *g_KLinesRenderer = NULL;</pre>
</td>
</tr>
</tbody> <tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#b1ebb0"><br>
</th>
<td width="50%" bgcolor="#c5ffc4"><br>
</td>
<th style="border-left: 1px solid rgb(192, 192, 192);
border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#b1ebb0"><font size="2">70</font></th>
<td width="50%" bgcolor="#c5ffc4">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"><span class="n">KLinesRenderer</span> <span class="o">*</span><span class="n">g_KLinesRenderer</span> <span class="o">=</span> <span class="nb">NULL</span><span class="p">;</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; word-wrap: break-word;">Is this needed?</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192);
border-collapse: collapse;" width="100%" bgcolor="white"
border="0">
<thead> <tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192,
192, 192); font-size: 9pt; padding: 4px 8px;
text-align: left;" bgcolor="#f0f0f0"> <a
moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38353#file38353line106"
style="color: black; font-weight: bold;
text-decoration: underline;">/trunk/KDE/kdegames/klines/renderer.cpp</a>
<span style="font-weight: normal;"> (Diff revision 3)
</span> </th>
</tr>
</thead> <tbody style="background-color: rgb(228, 217,
203); padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><br>
</td>
<td colspan="2">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;">KLinesRenderer *g_KLinesRenderer = NULL;</pre>
</td>
</tr>
</tbody> <tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">87</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"><span class="hl"> </span><span class="n"><span class="hl">QString</span></span><span class="hl"> </span><span class="n"><span class="hl">id</span></span><span class="hl"> </span><span class="o"><span class="hl">=</span></span> <span class="n">color2char</span><span class="p">(</span> <span class="n">color</span> <span class="p">)</span><span class="o">+</span><span class="n">QString</span><span class="p">(</span> <span class="s">"_rest"</span> <span class="p">);</span></pre>
</td>
<th style="border-left: 1px solid rgb(192, 192, 192);
border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">101</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"><span class="hl"> </span><span class="k"><span class="hl">return</span></span> <span class="n">color2char</span><span class="p">(</span> <span class="n">color</span> <span class="p">)</span><span class="o">+</span><span class="n">QString</span><span class="p">(</span> <span class="s">"_rest"</span> <span class="p">);</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; word-wrap: break-word;">When converting an ASCII string literal into a QString, using QLatin1String(literal) is preferable.
I don't remember why exactly, I think it makes it faster to convert the 8bit literal into Qt's internal 16bit representation due to not having to consider UTF-8 multi byte sequences</pre>
</div>
<br>
<div>
<table style="border: 1px solid rgb(192, 192, 192);
border-collapse: collapse;" width="100%" bgcolor="white"
border="0">
<thead> <tr>
<th colspan="4" style="border-bottom: 1px solid rgb(192,
192, 192); font-size: 9pt; padding: 4px 8px;
text-align: left;" bgcolor="#f0f0f0"> <a
moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/diff/3/?file=38353#file38353line131"
style="color: black; font-weight: bold;
text-decoration: underline;">/trunk/KDE/kdegames/klines/renderer.cpp</a>
<span style="font-weight: normal;"> (Diff revision 3)
</span> </th>
</tr>
</thead> <tbody style="background-color: rgb(228, 217,
203); padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><br>
</td>
<td colspan="2">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;">KLinesRenderer *g_KLinesRenderer = NULL;</pre>
</td>
</tr>
</tbody> <tbody>
<tr>
<th style="border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">107</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"> <span class="k">return</span> <span class="n"><span class="hl">QPixmap</span></span><span class="p"><span class="hl">()</span>;</span></pre>
</td>
<th style="border-left: 1px solid rgb(192, 192, 192);
border-right: 1px solid rgb(192, 192, 192);"
align="right" bgcolor="#e9eaa8"><font size="2">121</font></th>
<td width="50%" bgcolor="#fdfebc">
<pre style="font-size: 8pt; line-height: 140%; margin: 0pt;"> <span class="k">return</span> <span class="s"><span class="hl">""</span></span><span class="p">;</span></pre>
</td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; word-wrap: break-word;">QString()</pre>
</div>
<br>
<p>- Kevin</p>
<br>
<p>On September 25th, 2010, 12:41 p.m., Lindsay Mathieson wrote:</p>
<table style="background-image:
url("http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png");
background-position: left top; background-repeat: repeat-x;
border: 1px solid black;" width="100%" bgcolor="#fefadf"
cellpadding="8" cellspacing="0">
<tbody>
<tr>
<td>
<div>Review request for KDE Games.</div>
<div>By Lindsay Mathieson.</div>
<p style="color: grey;"><i>Updated Sept. 25, 2010, 12:41
p.m.</i></p>
<h1 style="color: rgb(87, 80, 18); font-size: 10pt;
margin-top: 1.5em;">Description </h1>
<table style="border: 1px solid rgb(184, 181, 160);"
width="100%" bgcolor="#ffffff" cellpadding="10"
cellspacing="0">
<tbody>
<tr>
<td>
<pre style="margin: 0pt; padding: 0pt; white-space: pre-wrap; word-wrap: break-word;">Updates klines to use KGameRenderer and KGameRenderedItem in place of QSvgRenderer and QGraphicsPixmapItem
Removes KPixmapCache usage</pre>
</td>
</tr>
</tbody>
</table>
<h1 style="color: rgb(87, 80, 18); font-size: 10pt;
margin-top: 1.5em;">Testing </h1>
<table style="border: 1px solid rgb(184, 181, 160);"
width="100%" bgcolor="#ffffff" cellpadding="10"
cellspacing="0">
<tbody>
<tr>
<td>
<pre style="margin: 0pt; padding: 0pt; white-space: pre-wrap; word-wrap: break-word;">Playing game
Changing themes
Resizing game
Comparison with original</pre>
</td>
</tr>
</tbody>
</table>
<h1 style="color: rgb(87, 80, 18); font-size: 10pt;
margin-top: 1.5em;">Diffs </h1>
<ul style="margin-left: 3em; padding-left: 0pt;">
<li>/trunk/KDE/kdegames/klines/scene.cpp <span
style="color: grey;">(1177731)</span></li>
<li>/trunk/KDE/kdegames/klines/renderer.cpp <span
style="color: grey;">(1177731)</span></li>
<li>/trunk/KDE/kdegames/klines/renderer.h <span
style="color: grey;">(1177731)</span></li>
<li>/trunk/KDE/kdegames/klines/previewitem.cpp <span
style="color: grey;">(1177731)</span></li>
<li>/trunk/KDE/kdegames/klines/klines.cpp <span
style="color: grey;">(1177731)</span></li>
<li>/trunk/KDE/kdegames/klines/ballitem.cpp <span
style="color: grey;">(1177731)</span></li>
<li>/trunk/KDE/kdegames/klines/ballitem.h <span
style="color: grey;">(1177731)</span></li>
<li>/trunk/KDE/kdegames/klines/animator.cpp <span
style="color: grey;">(1177731)</span></li>
</ul>
<p><a moz-do-not-send="true"
href="http://svn.reviewboard.kde.org/r/5399/diff/"
style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</tbody>
</table>
</div>
</blockquote>
<br>
</body>
</html>