<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/108717/">http://git.reviewboard.kde.org/r/108717/</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, 2013, 2:09 p.m. UTC, <b>Matěj Laitl</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/108717/diff/11/?file=145267#file145267line40" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/nepomukcollection/NepomukCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 11)
</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; ">using namespace MemoryMeta;</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace Collections</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">37</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">using</span> <span class="k">namespace</span> <span class="n">MemoryMeta</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">36</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">namespace</span> <span class="n">Collections</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">38</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">using</span> <span class="k">namespace</span> <span class="n">Collections</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">37</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><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;">Okay, personal preferences aside, I'm sure that consistency within a project has more value than preferences of individual developers.
There are about 25 .cpp files in current Amarok source code that embody all the method definitions into namespace XYZ {}. On the other hand, there are about 265 files that use using namespace XYZ; syntax for function definitions. Please improve, rather than fight, consistency.</pre>
</blockquote>
<p>On May 30th, 2013, 6:43 p.m. UTC, <b>Edward Hades Toroshchin</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 not about consistency. Favouring the using-directive is not a question of style or preference.</pre>
</blockquote>
<p>On May 30th, 2013, 7:14 p.m. UTC, <b>Matěj Laitl</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 it is a question of what? What is the (technical, now that you've excluded personal one) rationale for introducing this incosistency? (yes, I still view it as an inconsistency)
I also don't like "issues" being marked are resolved here without ack of the one who opened them.</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;">About marked issues: reviewboard's workflow allows that, and we don't have any policy against it. My view is that this code will be maintained mostly by me, so any opened issues do not have a veto power over the patch, unless the issue creator specifically mentions, that the code with this issue unresolved is unacceptable.
Regarding the using-directive specifically: the semantics of using-directive do not allow it to be a viable replacement for placing the definitions inside the namespace. Therefore consistency is not important.
</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 30th, 2013, 2:09 p.m. UTC, <b>Matěj Laitl</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/108717/diff/11/?file=145268#file145268line46" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/nepomukcollection/NepomukInquirer.h</a>
<span style="font-weight: normal;">
(Diff revision 11)
</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; ">public slots:</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">39</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/**</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">43</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">NepomukInquirer</span><span class="p">(</span> <span class="n">QString</span> <span class="n">query</span><span class="p">,</span> <span class="n">std</span><span class="o">::</span><span class="n">auto_ptr</span><span class="o"><</span><span class="n">NepomukParser</span><span class="o">></span> <span class="n">parser</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;">We prefer Qt equivalents of std classes, if they exist. But reading documentation of std::auto_ptr, it doesn't seem to make sense as a function argument, you doesn't seem to want to delete it as soon as it goes out of scope (and yeah, it may be reset by being on rhs of an assignment, but that is convoluted).</pre>
</blockquote>
<p>On May 30th, 2013, 6:43 p.m. UTC, <b>Edward Hades Toroshchin</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;">std::auto_ptr makes perfect sense as a function argument, because it both clearly shows who is responsible for the pointer deallocation and enforces it.</pre>
</blockquote>
<p>On May 30th, 2013, 7:14 p.m. UTC, <b>Matěj Laitl</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;">Not for me. This isn't usual for other Amarok code and I wouldn't like to see such practice introduced. The current practice it to document pointer ownership properly in docstrings.
I'd like to see this issue reopened. (ditto about closing issues without reviewer's ack)</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;">Self-documenting and self-policing interfaces are good, even if they aren't used often.
(ditto about issues policy)</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 30th, 2013, 2:09 p.m. UTC, <b>Matěj Laitl</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/108717/diff/11/?file=145274#file145274line428" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp</a>
<span style="font-weight: normal;">
(Diff revision 11)
</span>
</th>
</tr>
</thead>
<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">428</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">std</span><span class="o">::</span><span class="n">auto_ptr</span><span class="o"><</span><span class="n">NepomukParser</span><span class="o">></span> <span class="n">parser</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;">QScopedPointer please</pre>
</blockquote>
<p>On May 30th, 2013, 7:14 p.m. UTC, <b>Matěj Laitl</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;">Dropped? Why? Dropping concerns I raise without any comment discourages me from (very time-consuming) reviews.</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;">This is directly linked to the auto_ptr issue above.</pre>
<br />
<p>- Edward Hades</p>
<br />
<p>On May 30th, 2013, 7:12 p.m. UTC, Edward Hades Toroshchin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 Amarok and Vishesh Handa.</div>
<div>By Edward Hades Toroshchin.</div>
<p style="color: grey;"><i>Updated May 30, 2013, 7:12 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;">nepomuk: implement custom QueryMaker</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;">Playing tracks from Nepomuk collection, browsing, filtering, adding/removing labels, statistics sync.</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>src/core-impl/collections/nepomukcollection/CMakeLists.txt <span style="color: grey">(642919bd7b2188c6055308eabc07319ae48e14be)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCache.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCache.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCollection.h <span style="color: grey">(5737cb5861563a8f190a29badc15d9e3ef82faf1)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukCollection.cpp <span style="color: grey">(8c35e3dda0e3dc9a23affe1d18cc69de4d24d58c)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.h <span style="color: grey">(66e2473f16227f462c5a3838c71f311b6594333a)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukConstructMetaJob.cpp <span style="color: grey">(19743fe02b1d0c9dd4785d01909a4231b3a69e90)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukInquirer.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukParser.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukParser.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukQueryMaker.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukQueryMaker.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/NepomukSelectors.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.h <span style="color: grey">(8456ddb8d952e130816d01fc312c8df3146e83d1)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukAlbum.cpp <span style="color: grey">(76c9136f9d4f5ba77cac49b242b3aa3b2d844c0c)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukArtist.h <span style="color: grey">(7cf533e760b0a06e5a2316693e13f6aea0ac3dcd)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukArtist.cpp <span style="color: grey">(34d7c8f9c32f96be0ef5f81de9d58bfedb510c2d)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukComposer.h <span style="color: grey">(837c37fa5dce5c3c10e6840f618cf72430b51b3d)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukComposer.cpp <span style="color: grey">(33ba85ae11100e8ccbb5cc94a2d7d0e2007fdbd1)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukGenre.cpp <span style="color: grey">(c34831ab4812f0241c0000eef844c2ea2397ae97)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukLabel.h <span style="color: grey">(6f2e54c3c6a0d350615cea0e335ed9f5c5a0eedf)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukLabel.cpp <span style="color: grey">(7e2a313e5eac91128d0ce211c6575ac3d8d2b1ab)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukTrack.h <span style="color: grey">(9a534dddac51fc39f9d3705f95b194f9669416ab)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukTrack.cpp <span style="color: grey">(b9beca872cd7c7edc3e6b15662d0a49f42213c6d)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukYear.h <span style="color: grey">(9eca44fbcd3aab77149d864cb6d3e5d266cc8461)</span></li>
<li>src/core-impl/collections/nepomukcollection/meta/NepomukYear.cpp <span style="color: grey">(1d6e3b1066e13fd6adba15ae6b1d81f8ca28a5d9)</span></li>
<li>src/statsyncing/collection/CollectionProvider.cpp <span style="color: grey">(7e3bdd83214796fc37e0aef41225baedabe3fbda)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/108717/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-counting-querymaker.png">nepomuk-counting-querymaker.png</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-queries.png">nepomuk-queries.png</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/02/03/nepomuk-sync.png">nepomuk-sync.png</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>