<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/107348/">http://git.reviewboard.kde.org/r/107348/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 17th, 2012, 7:59 p.m., <b>Edward Hades Toroshchin</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/107348/diff/1/?file=94955#file94955line38" style="color: black; font-weight: bold; text-decoration: underline;">src/services/lastfm/SynchronizationAdapter.cpp</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; ">SynchronizationAdapter::SynchronizationAdapter( const LastFmServiceConfig *config )</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">38</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span> <span class="k">this</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">startArtistSearch</span><span class="p">(</span><span class="kt">int</span><span class="p">)),</span></pre></td>
</tr>
<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">39</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SLOT</span><span class="p">(</span><span class="n">slotStartArtistSearch</span><span class="p">(</span><span class="kt">int</span><span class="p">)),</span> <span class="n">Qt</span><span class="o">::</span><span class="n">QueuedConnection</span> <span class="p">);</span></pre></td>
</tr>
<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">40</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span> <span class="k">this</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">startTrackSearch</span><span class="p">(</span><span class="n">QString</span><span class="p">,</span><span class="kt">int</span><span class="p">)),</span></pre></td>
</tr>
<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">41</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SLOT</span><span class="p">(</span><span class="n">slotStartTrackSearch</span><span class="p">(</span><span class="n">QString</span><span class="p">,</span><span class="kt">int</span><span class="p">)),</span> <span class="n">Qt</span><span class="o">::</span><span class="n">QueuedConnection</span> <span class="p">);</span></pre></td>
</tr>
<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">42</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span> <span class="k">this</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">startTagSearch</span><span class="p">(</span><span class="n">QString</span><span class="p">,</span><span class="n">QString</span><span class="p">)),</span></pre></td>
</tr>
<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">43</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SLOT</span><span class="p">(</span><span class="n">slotStartTagSearch</span><span class="p">(</span><span class="n">QString</span><span class="p">,</span><span class="n">QString</span><span class="p">)),</span> <span class="n">Qt</span><span class="o">::</span><span class="n">QueuedConnection</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 be Qt::AutoConnection. Otherwise it will deadlock if artists() or artistTracks() are called from the main thread.</pre>
</blockquote>
<p>On November 18th, 2012, 5:07 p.m., <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;">Right concern, but, from Provider.h:
/**
* (...)
*
* This method is guaranteed to be called in non-main thread and is allowed
* to block for a longer time; it must be implemented in a reentrant manner.
*/
virtual QSet<QString> artists() = 0;
/**
* (...)
*
* This method is guaranteed to be called in non-main thread and is allowed
* to block for a longer time; it must be implemented in a reentrant manner.
*/
virtual TrackList artistTracks( const QString &artistName ) = 0;
So this is not the case unless used wrong. Calling from the main thread is therefore unsupported, more things would break (more deadlocks), no need to support it.
</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;">"is guaranteed to be called" is incorrect sentence then. Since you're describing an API, you should write "must be called". You can't guarantee that the users of your API will even read your docs, can you? :)
Also, since it doesn't matter which kind of connection exactly you use here, just drop QueuedConnection. Or add it to all the other providers, for consistency.
</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 17th, 2012, 7:59 p.m., <b>Edward Hades Toroshchin</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/107348/diff/1/?file=94955#file94955line98" style="color: black; font-weight: bold; text-decoration: underline;">src/services/lastfm/SynchronizationAdapter.cpp</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; ">SynchronizationAdapter::SynchronizationAdapter( const LastFmServiceConfig *config )</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">98</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Q_ASSERT</span><span class="p">(</span> <span class="n">m_semaphore</span><span class="p">.</span><span class="n">available</span><span class="p">()</span> <span class="o">==</span> <span class="mi">0</span> <span class="p">);</span></pre></td>
</tr>
<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">99</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">emit</span> <span class="n">startArtistSearch</span><span class="p">(</span> <span class="mi">1</span> <span class="p">);</span> <span class="c1">// Last.fm indexes from 1</span></pre></td>
</tr>
<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">100</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<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">101</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_semaphore</span><span class="p">.</span><span class="n">acquire</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;">You go to quite some lengths to convert the non-blocking QNetworkReply approach, to a blocking one.
Why don't you just readAll() from it?
</pre>
</blockquote>
<p>On November 18th, 2012, 5:07 p.m., <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;">> You go to quite some lengths to convert the non-blocking QNetworkReply approach, to a blocking one.
Yes, I do. The reason why I do this is that the non-blocking (e.g. event-based) code would be way more ugly (IMHO) on the StatSyncing::Provider level, plus event-based approach has no advantage for a batch process like statistics synchronization.
> Why don't you just readAll() from it?
I do, in slot{Artists,Tracks,Tags}Received().</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;">I didn't suggest you use non-blocking approach. I just suggested that you drop the QSemaphore and his crazy friends in favor of just calling readAll() in a loop with waitForReadyRead() and isFinished().</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 17th, 2012, 7:59 p.m., <b>Edward Hades Toroshchin</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/107348/diff/1/?file=94975#file94975line126" style="color: black; font-weight: bold; text-decoration: underline;">src/statsyncing/collection/CollectionProvider.cpp</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; ">CollectionProvider::slotStartArtistSearch()</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">126</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Collections</span><span class="o">::</span><span class="n">QueryMaker</span> <span class="o">*</span><span class="n">qm</span> <span class="o">=</span> <span class="n">m_coll</span><span class="p">.</span><span class="n">data</span><span class="p">()</span><span class="o">-></span><span class="n">queryMaker</span><span class="p">();</span></pre></td>
</tr>
<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">127</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qm</span><span class="o">-></span><span class="n">setAutoDelete</span><span class="p">(</span> <span class="kc">true</span> <span class="p">);</span></pre></td>
</tr>
<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">128</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qm</span><span class="o">-></span><span class="n">setQueryType</span><span class="p">(</span> <span class="n">Collections</span><span class="o">::</span><span class="n">QueryMaker</span><span class="o">::</span><span class="n">Artist</span> <span class="p">);</span></pre></td>
</tr>
<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">129</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span> <span class="n">qm</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">newResultReady</span><span class="p">(</span><span class="n">Meta</span><span class="o">::</span><span class="n">ArtistList</span><span class="p">)),</span></pre></td>
</tr>
<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">130</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SLOT</span><span class="p">(</span><span class="n">slotNewResultReady</span><span class="p">(</span><span class="n">Meta</span><span class="o">::</span><span class="n">ArtistList</span><span class="p">))</span> <span class="p">);</span></pre></td>
</tr>
<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">131</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span> <span class="n">qm</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">queryDone</span><span class="p">()),</span> <span class="n">SLOT</span><span class="p">(</span><span class="n">slotQueryDone</span><span class="p">())</span> <span class="p">);</span></pre></td>
</tr>
<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">132</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qm</span><span class="o">-></span><span class="n">run</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;">I think this should be implemented in QueryMaker as a blocking API instead.
Also, SqlQueryMaker supports blocking API (more or less), so it could be unified.</pre>
</blockquote>
<p>On November 18th, 2012, 5:07 p.m., <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;">> I think this should be implemented in QueryMaker as a blocking API instead.
Perhaps so, but I view it as a long-term goal and not something that blocks StatSyncing merging. Also see my comments about QueryMakers not working when created in non-eventloop threads. As blocking QueryMakers simply have only sense in non-main threads, BlockingQueryMaker would be hard to implement, because it doesn't have a comfort of an QObject available in the main thread as StatSyncing::CollectionProvider does.
> Also, SqlQueryMaker supports blocking API (more or less), so it could be unified.
The SqlQUeryMaker::setBlocking() is UGLY AS HELL IMHO and would lead to more complexity implementing QueryMakers. We really want to lower the barrier for implementing QueryMakers, see Phalgun's hard time (or fear of so) implemeting NepomukQueryMaker. If ever, I would propose adding BlockingQueryMaker wrapper, which would be currently also tricky, so I'd prefer not to depend on it.</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;">Can you just create a BlockingQueryMaker wrapper out of your code in the CollectionProvider?</pre>
<br />
<p>- Edward Hades</p>
<br />
<p>On November 18th, 2012, 5:12 p.m., Matěj Laitl 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 Amarok and Myriam Schweingruber.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated Nov. 18, 2012, 5: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;">This is a final review request for my whole summer project: Statistics Syncrhonization.
This is BIG and I don't expect you reading through it completely, rather please just skim the parts outside src/statsyncing. Even outside src/statsyncing, you may just check whether the interface and design is okay, I'm confident the implementation has low bug rate/divergence from documented behaviour.
Only TODO for 2.7 release: even more GUI polishing as suggested on the usability review request
Individual commits are available for your reviewing pleasure at http://quickgit.kde.org/?p=clones%2Famarok%2Flaitl%2Famarok.git&a=shortlog&h=gsoc
ChangeLog entries:
FEATURES:
* Track dragging support in Unique Tracks tab of the Synchronize Statistics action;
allows you to do a "diff" between collections and transfer missing tracks.
* Amarok now scrobbles tracks in streams if the stream correctly updates
meta-data.
* When scrobbling to Last.fm, Amarok announces suggested tag corrections.
* Ability to scrobble recently played tracks from iPod (and the like) to Last.fm.
* Synchronization of labels and rating between Last.fm and Amarok collections;
play count can be synchronized one-way from Last.fm to Amarok.
* Statistics synchronization between collections, supports rating, first / last
played time, play count and labels.
CHANGES:
* Configure Amarok dialog gets new Metadata tab to grab some weight from the
Collection tab and to configure statistics synchronization.
BUGFIXES:
* Better Last.fm scrobbling behaviour and error reporting due to rewrite,
should fix long-standing problems.
* Update stream meta-data correctly even with phonon-gstreamer back-end.
FEATURE: 240732
FEATURE: 309697
FEATURE: 206249
BUG: 293320
BUG: 285820
FIXED-IN: 2.7
DIGEST: Amarok statistics synchronization GSoC project merged with many features
and improvements, please see http://strohel.blogspot.com/search/label/gsoc
for more info and a bunch of screen shots.
GUI: Added statistics synchronization dialogs, split and one more config dialog tab</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;">Most of the code is tested months by me, OTOH the Last.fm features like syncing and or scrobbling from iPods is rather new. (weeks, days)</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=many">many</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>ChangeLog <span style="color: grey">(608de6861f931808e2a435a1e9c502d871993027)</span></li>
<li>src/App.cpp <span style="color: grey">(7915861609564a2b0475d0530751a31f04bb58bd)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(575cd11f51ef53474837696b31538bfe4490a1da)</span></li>
<li>src/EngineController.h <span style="color: grey">(b3a228196d7aa322d001a337d040546546e02f9e)</span></li>
<li>src/EngineController.cpp <span style="color: grey">(23e16d82869d6ff0c1201a432da25a1e143a0727)</span></li>
<li>src/MainWindow.h <span style="color: grey">(7e83f09343f0640afb6b18d60109133e4ef262a7)</span></li>
<li>src/MainWindow.cpp <span style="color: grey">(d691951c09af3e7581c7b82920c708b8dd1268de)</span></li>
<li>src/configdialog/ConfigDialog.cpp <span style="color: grey">(263f77db0932e700f4ebc68f8b9f6b6c51fa6381)</span></li>
<li>src/configdialog/dialogs/ExcludedLabelsDialog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/configdialog/dialogs/ExcludedLabelsDialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/configdialog/dialogs/ExcludedLabelsDialog.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/configdialog/dialogs/MetadataConfig.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/configdialog/dialogs/MetadataConfig.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/configdialog/dialogs/MetadataConfig.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/capabilities/multisource/MultiSourceCapabilityImpl.h <span style="color: grey">(4a472c2072193f809b2c9b6dd06f7caa6df2b991)</span></li>
<li>src/core-impl/capabilities/multisource/MultiSourceCapabilityImpl.cpp <span style="color: grey">(28152e61c93ce3cdd2d5a26f8adf33c87571e245)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodMeta.h <span style="color: grey">(c74b7238eceb40e8968bdd1a6af93139ed808040)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodMeta.cpp <span style="color: grey">(572805c46692c3659884c7ceae77da288c111b1d)</span></li>
<li>src/core-impl/collections/umscollection/UmsCollection.h <span style="color: grey">(cc11b09270a58e11609fc0a975d752289dae7f65)</span></li>
<li>src/core-impl/collections/umscollection/UmsCollection.cpp <span style="color: grey">(fb326fcdfd474c7340a9f13f80e211b49a23b339)</span></li>
<li>src/core-impl/meta/multi/MultiTrack.h <span style="color: grey">(336fbca20996e32a063302d22a0e688fc13482cc)</span></li>
<li>src/core-impl/meta/multi/MultiTrack.cpp <span style="color: grey">(3138805d6427291143d5059b56745269a360b69e)</span></li>
<li>src/core-impl/meta/stream/Stream.h <span style="color: grey">(253384d0b48c6b773116d12553e1976da6bdd0ec)</span></li>
<li>src/core-impl/meta/stream/Stream.cpp <span style="color: grey">(26aed5e9894ad17ce9e27344acbdfcd0d5a8def1)</span></li>
<li>src/core/capabilities/MultiSourceCapability.h <span style="color: grey">(819a24ff79f8d741db30c34ed42a38610885bf4c)</span></li>
<li>src/core/capabilities/MultiSourceCapability.cpp <span style="color: grey">(4c46ff45f7f55c3460704f7058c9323a947529cc)</span></li>
<li>src/core/meta/Statistics.h <span style="color: grey">(159b5ffcf3906f215d7327aef4ba47a527d77030)</span></li>
<li>src/core/meta/Statistics.cpp <span style="color: grey">(d991d8b1eb88f69a6fbd10255f961eda63b4f520)</span></li>
<li>src/core/support/Components.h <span style="color: grey">(87fabb035842cb1ab5d5ba63358728cf2fbf311f)</span></li>
<li>src/core/support/Components.cpp <span style="color: grey">(22e5de0d0d2ce9303fdca31839f186bae5fe866d)</span></li>
<li>src/dialogs/CollectionSetup.h <span style="color: grey">(6e3d4808373df7f0584de7c59e84f0d0f0bd463f)</span></li>
<li>src/dialogs/CollectionSetup.cpp <span style="color: grey">(eea8c792ebb8169adf21e710a5dba993761eef14)</span></li>
<li>src/services/lastfm/CMakeLists.txt <span style="color: grey">(61042a13522adb711c93a1b91c3f509b64af07f7)</span></li>
<li>src/services/lastfm/LastFmConfigWidget.ui <span style="color: grey">(60b5bfd401c0b6e40d3251010b7c76473d916b00)</span></li>
<li>src/services/lastfm/LastFmService.h <span style="color: grey">(48d26a3678fe583d6008d940f3e8810b1c0234b1)</span></li>
<li>src/services/lastfm/LastFmService.cpp <span style="color: grey">(26739ba2e812f94dbf62c9c1fd81db90089d7567)</span></li>
<li>src/services/lastfm/LastFmServiceConfig.h <span style="color: grey">(07f79d074ca1c08cd92dfe55ad5fb0435182c4d5)</span></li>
<li>src/services/lastfm/LastFmServiceConfig.cpp <span style="color: grey">(a07cbdf8e6deeb5082520e94cc0bb66de97c05dd)</span></li>
<li>src/services/lastfm/LastFmServiceSettings.cpp <span style="color: grey">(81c1d0c3e5fd44d5e49abb1dee4ce457bf6ffc8d)</span></li>
<li>src/services/lastfm/ScrobblerAdapter.h <span style="color: grey">(5d55ea0d075022b8d657fc54c01a55e07a150821)</span></li>
<li>src/services/lastfm/ScrobblerAdapter.cpp <span style="color: grey">(8d119657bba1fb212d548c322f058e88b6dc8a7d)</span></li>
<li>src/services/lastfm/SynchronizationAdapter.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/services/lastfm/SynchronizationAdapter.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/services/lastfm/SynchronizationTrack.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/services/lastfm/SynchronizationTrack.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Config.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Config.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Controller.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Controller.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Options.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Options.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Process.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Process.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Provider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Provider.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ScrobblingService.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ScrobblingService.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Track.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/Track.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/TrackTuple.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/TrackTuple.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/collection/CollectionProvider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/collection/CollectionProvider.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/collection/CollectionTrack.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/collection/CollectionTrack.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/jobs/MatchTracksJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/jobs/MatchTracksJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/jobs/SynchronizeTracksJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/jobs/SynchronizeTracksJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/CommonModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/CommonModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/MatchedTracksModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/MatchedTracksModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/ProvidersModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/ProvidersModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/SingleTracksModel.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/models/SingleTracksModel.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/ChooseProvidersPage.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/ChooseProvidersPage.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/ChooseProvidersPage.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/MatchedTracksPage.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/MatchedTracksPage.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/MatchedTracksPage.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/TrackDelegate.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/statsyncing/ui/TrackDelegate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/support/QSharedDataPointerMisc.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core-impl/meta/multi/TestMetaMultiTrack.h <span style="color: grey">(61e37627351163a7f2bbae729779900d2abb6ca9)</span></li>
<li>tests/core-impl/meta/multi/TestMetaMultiTrack.cpp <span style="color: grey">(b9d64dee9f8f0880981550aab29b882c4b4bbc31)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107348/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>