<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="http://svn.reviewboard.kde.org/r/5238/">http://svn.reviewboard.kde.org/r/5238/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 6th, 2010, 3:02 a.m., <b>Parker Coates</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;">So first off, I'd just like to say that I've been meaning to port KNetWalk for a while now, but never really found the time. It's probably my favourite game in the entire module. Thanks for giving it a shot. For the most part things look good.
Normally, I would say that this patch is too big. Porting from widgets to a canvas and switching the rendering framework in a single diff means that there's a lot of (not always related) things going on. But due to the coupling between the two frameworks, splitting it up into two diffs would probably mean extra work and wouldn't be worthwhile. In the future, please try to keep unrelated changes (like game pausing) in their own patches. It's much easier to review five ten-line patches than one thirty-line patch.</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;">Hey, thanks for the review and encouragement.
I got a bit carried away while working on this, I think. I'm not so used to working in a patch-submission workflow so I tend to lump changes together, include whitespace and other trivial modifications in unrelated patches, etc. Newbie mistakes that I will try to discipline myself to avoid :)
I do agree with your suggestions and will work on putting together an updated patch, but my available free time dropped considerably shortly after I submitted this review request so it could be a few days before I have anything new to show.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 6th, 2010, 3:02 a.m., <b>Parker Coates</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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="/r/5238/diff/1/?file=35076#file35076line121" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdegames/knetwalk/src/mainwindow.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">114</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QSound</span><span class="o">*</span> <span class="n">clicksound</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">115</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QSound</span><span class="o">*</span> <span class="n">connectsound</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">116</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QSound</span><span class="o">*</span> <span class="n">startsound</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">117</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QSound</span><span class="o">*</span> <span class="n">turnsound</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">118</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QSound</span><span class="o">*</span> <span class="n">winsound</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">119</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">120</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">username</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">121</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">soundpath</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">122</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QAction</span><span class="o">*</span> <span class="n">soundaction</span><span class="p">;</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Where did this stuff go?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">These vars are all unused in the trunk version. This is mostly what I was referring to when in the patch description I mentioned apparent leftovers from the pre-KDE version of the game.</pre>
<br />
<p>- Brian</p>
<br />
<p>On September 2nd, 2010, 10:36 p.m., Brian Croom wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" 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 black solid;">
<tr>
<td>
<div>Review request for KDE Games and Stefan Majewsky.</div>
<div>By Brian Croom.</div>
<p style="color: grey;"><i>Updated 2010-09-02 22:36:16</i></p>
<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;">As KNetWalk's use of QWidget as its graphics stack was marked in red on the porting status wiki page, and because I wanted to see if I had acquired a good enough working understanding of the QGraphicsView framework to be able to accomplish it, I decided to attempt a full port of this game. I referred to the KMines source while restructuring the code, as it has various conceptually similar elements (a grid with square cells that are activated by the user during gameplay, etc.)
Doing this required some rather invasive changes to the GUI code. In addition to switching to QGraphicsView, this patch also:
- Removes the distinction between mouse and keyboard input modes. They now work together nicely, as they should.
- Adds the ability to pause the game, and hide the puzzle while it is paused.
- Cleans up code in various places and removes a number of obsolete class members that I presume to have been vestiges from before the KDE port.
My only disappointment is that switching to QGraphicsView did not get rid of the rendering artifacts that often appear when the cables are being rotated. Particularly on the cells with T-shaped cable junctions, white pixels often appear at the edge of the pixmap during the rotation animation. I would greatly appreciate any suggestions for how to avoid that happening.</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;">I have come to enjoy playing this game quite a lot, and have given it considerable testing. All the usual things seem to work well including resizing, theme changes, changing difficulty level, and pausing/resuming.</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>/trunk/KDE/kdegames/knetwalk/src/fielditem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/fielditem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/cell.cpp <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/CMakeLists.txt <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/abstractgrid.h <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/abstractgrid.cpp <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/cell.h <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/gamewidget.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/gamewidget.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/globals.h <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/knetwalk.kcfg <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/knetwalkui.rc <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/main.cpp <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/mainwindow.h <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/mainwindow.cpp <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/renderer.h <span style="color: grey">(1168585)</span></li>
<li>/trunk/KDE/kdegames/knetwalk/src/renderer.cpp <span style="color: grey">(1168585)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5238/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>