<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://git.reviewboard.kde.org/r/101887/">http://git.reviewboard.kde.org/r/101887/</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 9th, 2011, 6:06 a.m., <b>Laszlo Papp</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;">"It's a bit long, but Arjen Hiemstra said this would be the best way." -> Why it is not good idea is that I have a very slow internet connection and every time I open this up, takes me ages to load. Second, it really contains more things that should be separated. For instance, I can take more serious technical reviews in my kdeext player and player lib part. Right now I cannot see just them, I need to filter manually which is a bigger overhead than it could be. Furthermore, I could not really take a quality review on this patch since I did not have enough time and I still spent more 1-2 hours with it. It would really help to have separate review requests in order to provide fast and quality feedbacks for snippets.
I would really suggest in the future not providing god/monolythic diffs for review. For me personally, it is really not nice and a lot of overhead meaning I can easily lose my sake for any review.
Next step: please upload the player things in a separate review and I can go through them. </pre>
</blockquote>
<p>On July 9th, 2011, 7:56 a.m., <b>Felix Rohrbach</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;">Sorry, but you told me you wouldn't have time, so I have done it the way Arjen Hiemstra wanted it to make it easier for him to review. I will create a second review only for the player subdirectory as you requested.</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;">"Sorry, but you told me you wouldn't have time, so I have done it the way Arjen Hiemstra wanted it to make it easier for him to review."
1) I have never said I do not have time for review, just that it was going to be postponed a bit. In fact, it was not even postponed.
2) The review is not about in one person in general, anybody can review it and I think splitted way would make sense in general after discussing with more KDE developers.
3) No way, it gets committed in this monolythic form into master. If it gets committed in splitted snippets, why not start asking for review as it is closer to the committing/merging way ?
In general, if this patch is this way, I will make another reviews quite a few times. That is the only way how I can make sure I do not miss something. Do not forget I am just checking the patch format and some smaller programming mistakes. It needs also some review from the design point of view.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 9th, 2011, 6:06 a.m., <b>Laszlo Papp</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="http://git.reviewboard.kde.org/r/101887/diff/1/?file=26490#file26490line98" style="color: black; font-weight: bold; text-decoration: underline;">engine/abstractstatistic.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; ">namespace GluonEngine</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">98</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">91</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">92</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">public</span> <span class="n">Q_SLOTS</span><span class="o">:</span></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;">This breaks the order public/protected/private order. Could you please move back if it is a public slot ?</pre>
</blockquote>
<p>On July 9th, 2011, 7:35 a.m., <b>Felix Rohrbach</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;">hmm, I think you misunderstood the diff here a bit. I just moved some functions from protected to public, the order is still public/public Q_SLOTS/protected/private.</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;">Right, you changed the API, too.
Yeah, well this is something I was talking about. I could not make 100% QA review. With smaller patches, it is not that chancy I skip that one line. In a god/monolythic diff, it is more chancy I make more mistakes with a review.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 9th, 2011, 6:06 a.m., <b>Laszlo Papp</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="http://git.reviewboard.kde.org/r/101887/diff/1/?file=26493#file26493line88" style="color: black; font-weight: bold; text-decoration: underline;">engine/achievementsmanager.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace GluonEngine</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">88</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">bool</span> <span class="n">dependencySatisfied</span><span class="p">(</span> <span class="kt">int</span> <span class="n">index</span> <span class="p">)</span> <span class="k">const</span><span class="p">;</span></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;">hasDependency as a common naming practice for such cases and also as the comment says ?</pre>
</blockquote>
<p>On July 9th, 2011, 7:42 a.m., <b>Felix Rohrbach</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;">huh? hasDependency just checks if there is a dependency, while dependencySatisfied checks whether user has the dependency achievement.</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;">Right, the comment somehow misled me at this point.
/** Has the user the dependency achievement achieved? */ -> I do not still find it feasible though. I would really make it consistent with the comment, as in achieved vs. satisfied. I personally prefer the achieved.</pre>
<br />
<p>- Laszlo</p>
<br />
<p>On July 8th, 2011, 11:17 p.m., Felix Rohrbach wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/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 Gluon.</div>
<div>By Felix Rohrbach.</div>
<p style="color: grey;"><i>Updated July 8, 2011, 11:17 p.m.</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;">This is the second part of my SoK project, bringing statistics and achievements to gluon. While the first part included mainly the statistics and the databases to save them, this part includes the achievements and a way to show them in gluon player applications (atm there's only an implementation for the KDE Extended player), but also brings improvements to the statistics. It's a bit long, but Arjen Hiemstra said this would be the best way. And don't forget the second page of 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;">It compiles and I tried to test every feature I implemented, but I didn't always tried every combination, so there might be still some bugs.</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>creator/plugins/docks/propertiesdock/CMakeLists.txt <span style="color: grey">(05c5242)</span></li>
<li>creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>creator/x-gluon-mimetypes.xml <span style="color: grey">(378876d)</span></li>
<li>engine/CMakeLists.txt <span style="color: grey">(e8764f5)</span></li>
<li>engine/abstractstatistic.h <span style="color: grey">(494032f)</span></li>
<li>engine/achievement.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/achievement.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/achievementsmanager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/achievementsmanager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/assets/CMakeLists.txt <span style="color: grey">(b8c087b)</span></li>
<li>engine/assets/other/achievements/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/assets/other/achievements/achievementsasset.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/assets/other/achievements/achievementsasset.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/assets/other/statistics/statisticsasset.h <span style="color: grey">(a1d1995)</span></li>
<li>engine/assets/other/statistics/statisticsasset.cpp <span style="color: grey">(a8d6372)</span></li>
<li>engine/booleanstatistic.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/booleanstatistic.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/game.cpp <span style="color: grey">(d5fc279)</span></li>
<li>engine/gameproject.h <span style="color: grey">(805d193)</span></li>
<li>engine/gameproject.cpp <span style="color: grey">(8d84024)</span></li>
<li>engine/gameprojectprivate.h <span style="color: grey">(38c442a)</span></li>
<li>engine/gameprojectprivate.cpp <span style="color: grey">(08424b1)</span></li>
<li>engine/gluon_engine_export.h <span style="color: grey">(17fc00c)</span></li>
<li>engine/multiscorestatistic.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/multiscorestatistic.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/projectmetadata.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/projectmetadata.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/statistic.h <span style="color: grey">(e29b2fe)</span></li>
<li>engine/tasksstatistic.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>engine/tasksstatistic.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/examples/apocalypse.gluon/game.gluonmeta <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/examples/ball.gluon/game.gluonmeta <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/examples/invaders.gluon/game.gluonmeta <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/examples/jumpnbump.gluon/game.gluonmeta <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/examples/pong.gluon/game.gluonmeta <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/kdeext/CMakeLists.txt <span style="color: grey">(40da561)</span></li>
<li>player/kdeext/delegates/achievementdelegate.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/kdeext/delegates/achievementdelegate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/kdeext/gamedetailsoverlay.h <span style="color: grey">(99923a2)</span></li>
<li>player/kdeext/gamedetailsoverlay.cpp <span style="color: grey">(b6f525c)</span></li>
<li>player/kdeext/gamesoverlay.cpp <span style="color: grey">(d468186)</span></li>
<li>player/lib/CMakeLists.txt <span style="color: grey">(5957748)</span></li>
<li>player/lib/models/achievementsmodel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/lib/models/achievementsmodel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>player/lib/models/commentsmodel.cpp <span style="color: grey">(5a0bff0)</span></li>
<li>player/lib/models/gameitemsmodel.h <span style="color: grey">(c907f7b)</span></li>
<li>player/lib/models/gameitemsmodel.cpp <span style="color: grey">(82f7202)</span></li>
<li>player/lib/models/gameviewitem.h <span style="color: grey">(6a7f4ca)</span></li>
<li>player/lib/models/gameviewitem.cpp <span style="color: grey">(67f0566)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/101887/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>