<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/100098/">http://git.reviewboard.kde.org/r/100098/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 28th, 2010, 6:52 p.m., <b>Ian Monroe</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/100098/diff/3/?file=2522#file2522line198" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/sqlcollection/SqlMeta.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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; ">SqlTrack::updateData( const QStringList &result, bool forceUpdates )</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">198</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="n">artistId</span> <span class="o">></span> <span class="mi">0</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;">Could you document why you added these checks? I'm guessing its just a sanity check, but just say so here.</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;">Actually it's not only sanity.
It's just being defensive. The only part of the code that guarantees that those ids are set is the SqlRegistry and the ScanResultProcessor. If one of those pieces is changed then you will start getting in trouble.
However a short "// sanity" shouldn't be so bad.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 28th, 2010, 6:52 p.m., <b>Ian Monroe</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/100098/diff/3/?file=2524#file2524line327" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/sqlcollection/SqlRegistry.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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; ">SqlRegistry::getAlbum( int id )</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">327</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">query</span> <span class="o">=</span> <span class="n">QString</span><span class="p">(</span> <span class="s">"SELECT name, artist FROM albums WHERE id = %1"</span> <span class="p">).</span><span class="n">arg</span><span class="p">(</span> <span class="n">id</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;">This should maybe be a QLatin1String instead. I say maybe because obviously sql queries that include end-user strings should *not* be a qlatin1string. So I can't say that sqlregistry using just QString is a bad idea.</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;">id is an int.
No problem there.</pre>
<br />
<p>- Ralf</p>
<br />
<p>On October 25th, 2010, 4:55 p.m., Ralf Engels wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.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 Amarok.</div>
<div>By Ralf Engels.</div>
<p style="color: grey;"><i>Updated 2010-10-25 16:55:56</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 patch improves the SqlMeta thread safety.
It has several parts:
- Use QReadWriteLock for SqlTrack.
For this I also needed to pull the read accessor functions into the .cpp
- SqlCollection is a const pointer.
All metas are directly dependent on the SqlCollection and it could and should never change.
Using weak pointers here is not needed.
Note that now the access to m_collection is also thread safe as it's const
- SqlAlbum::setCompilation did modify the old album
This was changed an a new album will be created.
The positive side effect is that the UI will now update directly if setting a compilation
- SqlAlbum::setImage will now copy embedded images directly when set
If the track-uid is set (maybe from another thread) the cover image will still be found.
Note that the image cache is and was not connected to the track uid.</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;">setting and unsetting compilation
changing album name
scanning and fully scanning collection</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=254631">254631</a>
</div>
<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/sqlcollection/SqlCollection.cpp <span style="color: grey">(b88d257)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlMeta.h <span style="color: grey">(b92e351)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlMeta.cpp <span style="color: grey">(6b29b6e)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlRegistry.h <span style="color: grey">(66ba632)</span></li>
<li>src/core-impl/collections/sqlcollection/SqlRegistry.cpp <span style="color: grey">(e1b3571)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100098/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>