<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/113666/">http://git.reviewboard.kde.org/r/113666/</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 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210110#file210110line52" style="color: black; font-weight: bold; text-decoration: underline;">cmake/modules/FindMtp.cmake</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">46</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">message</span><span class="p">(</span><span class="s">STATUS</span> <span class="s2">"Found MTP but version requirements not met"</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">52</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">message</span><span class="p">(</span><span class="s">STATUS</span> <span class="s2">"Found MTP but version requirements <span class="hl">(${MTP_MIN_VERSION}) </span>not met"</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;">The wording is a bit convoluted.
Probably something like "Found MTP, but we want at least version ${MTP_MIN_VERSION}" would be better.</pre>
</blockquote>
<p>On November 18th, 2013, 8:30 a.m. CET, <b>Myriam Schweingruber</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;">How about: "Found MTP, but version ${MTP_MIN_VERSION} is needed"</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;">Fixed by means of
message(STATUS "Found MTP, but too old. Version ${MTP_MIN_VERSION} or greater is needed")
</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210114#file210114line345" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">345</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">parentFolder</span> <span class="o">==</span> <span class="n">folderId</span> <span class="p">)</span> <span class="c1">// prevent infinite loop when the structure is corrupted</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;">Oh my. I believe this is one of the manifestations of MTP creator's genius?</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 was actually my paranoia (that fires every time when loop "finiteness" depends on external data), but I wouldn't be surprised if there ware crappy MTP devices out there in the wild needing this.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210114#file210114line547" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">547</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">formatsHelp</span> <span class="o">=</span> <span class="n">i18n</span><span class="p">(</span> <span class="s">"<html>This is just an intersection of formats supported "</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">548</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="s">"by both the device and Amarok, not complete list of formats supported by "</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">549</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="s">"the device.</html>"</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">550</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ui</span><span class="o">-></span><span class="n">formatsLabel</span><span class="o">-></span><span class="n">setToolTip</span><span class="p">(</span> <span class="n">formatsHelp</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">551</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ui</span><span class="o">-></span><span class="n">formatsLabel</span><span class="o">-></span><span class="n">setWhatsThis</span><span class="p">(</span> <span class="n">formatsHelp</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">552</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ui</span><span class="o">-></span><span class="n">formatsPlaceholder</span><span class="o">-></span><span class="n">setToolTip</span><span class="p">(</span> <span class="n">formatsHelp</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">553</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ui</span><span class="o">-></span><span class="n">formatsPlaceholder</span><span class="o">-></span><span class="n">setWhatsThis</span><span class="p">(</span> <span class="n">formatsHelp</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;">(same as the above)</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;">Because I want the same string to be at four (two) places and .ui doesn't allow me (AFAIK) to share these. Making the exact change at four places every time gets boring pretty quickly; it is also beneficiary for translators to keep count of unique strings low and this eliminates the possibility of the strings going out of sync.
This is in no way a strong opinion, still this tickles my internal DRY principle [1] alarm. If you know a way how to share a string in a .ui file, I'm all in.
[1] http://www.artima.com/intv/dry.html</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210114#file210114line619" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">619</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QObject</span> <span class="o">*</span><span class="n">dialog</span> <span class="o">=</span> <span class="n">sender</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;">Do you do this just to avoid saving a pointer to the dialog, or is there any other reason?</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;">No other reason, and now I see that I can eliminate sender() entirely by connecting dialog's deleteLater() right to SIGNAL(finished()). (Qt guarantees slot execution order)</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210114#file210114line656" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">656</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="o">!</span><span class="n">job</span><span class="p">.</span><span class="n">data</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">657</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span><span class="p">;</span> <span class="c1">// can happen as the slot can trigger very late after the signal</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;">Potential race condition: job pointee might get deleted after this check.</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;">Not really, with current code. All jobs passed to MtpCollection::registerTransferJob() have QObject thread affinity set to the main thread, are only deleted using QObject::deleteLater() and MtpCollection::slotRegisterTransferJob() always happens in the main thread.
I've extended MtpCollection::registerTransferJob() documentation with requirements imposed on the passed job.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210114#file210114line679" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">679</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_transferJanitors</span><span class="p">.</span><span class="n">insert</span><span class="p">(</span> <span class="n">type</span><span class="p">,</span> <span class="n">janitor</span> <span class="p">);</span> <span class="c1">// replaces</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;">Obscure comment.</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;">Clarified: // replaces any possible leftover null pointer</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210114#file210114line700" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">700</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_mc</span><span class="o">-></span><span class="n">acquireReadLock</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">701</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">typedef</span> <span class="n">KSharedPtr</span><span class="o"><</span><span class="n">MemoryMeta</span><span class="o">::</span><span class="n">Track</span><span class="o">></span> <span class="n">MemoryTrackPtr</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">702</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">MemoryTrackPtr</span> <span class="n">existingTrack</span> <span class="o">=</span> <span class="n">MemoryTrackPtr</span><span class="o">::</span><span class="n">staticCast</span><span class="p">(</span> <span class="n">m_mc</span><span class="o">-></span><span class="n">trackMap</span><span class="p">().</span><span class="n">value</span><span class="p">(</span> <span class="n">trackUidUrl</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">703</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_mc</span><span class="o">-></span><span class="n">releaseLock</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 believe you should acquire a write lock for the whole transaction here. Otherwise you get a race condition, if two threads try to add the same track.</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;">Yes, but addTrack( LIBMTP_track_t *mtpTrack ) is only called by ParseMtpTracksJob, and MtpCollection does its best to only have one ParstTracksJob at a time. I've extended addTrack() documentation to mention assumptions for thread safety.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210114#file210114line737" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollection.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">737</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_mc</span><span class="o">-></span><span class="n">acquireReadLock</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">738</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">Meta</span><span class="o">::</span><span class="n">TrackPtr</span> <span class="n">existingTrack</span> <span class="o">=</span> <span class="n">m_mc</span><span class="o">-></span><span class="n">trackMap</span><span class="p">().</span><span class="n">value</span><span class="p">(</span> <span class="n">uidUrl</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">739</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">m_mc</span><span class="o">-></span><span class="n">releaseLock</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;">(same as above)</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;">MemoryMeta::MapChanger::removeTrack() works fine if you pass a track that is no longer in the collection, so only harm done in this unlikely race condition is a spurious emitted warning.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210116#file210116line101" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/MtpCollectionFactory.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">101</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">MtpCollection</span> <span class="o">*</span><span class="n">collection</span> <span class="o">=</span> <span class="n">qobject_cast</span><span class="o"><</span><span class="n">MtpCollection</span> <span class="o">*></span><span class="p">(</span> <span class="n">sender</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">102</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="o">!</span><span class="n">collection</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">103</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">warning</span><span class="p">()</span> <span class="o"><<</span> <span class="s">"non-MtpCollection connected to slotColectionInitialized()"</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'm ordinarily all for extra paranoid checking, but I am curious, why do you have this check here and nowhere else?</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 rather a check that sender() and qobject_cast suceeded (the error message tries to infer the reason, perhaps it is a bit confusing because of it). Other methods in the factory don't depend on sender() or qobject_cast's.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 18th, 2013, 12:49 a.m. CET, <b>Edward 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/113666/diff/1/?file=210119#file210119line1" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/collections/mtp/TODO</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">1</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> * Error handling around MTP calls - helper RAII class?</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">2</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> * let user specify copied track paths</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;">Are you sure it is a good idea to have per-folder TODO files? I'd personally keep them in bugzilla or in "// TODO" comments.</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 a kind of a left-over from this patch being a GSoC project - the plan is to resolve the 2 remaining TODO's here (ideally before 2.9 (beta?) release) and remove this file then - certainly not to add more items.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On November 21st, 2013, 1:24 p.m. CET, Matěj Laitl 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.</div>
<div>By Matěj Laitl.</div>
<p style="color: grey;"><i>Updated Nov. 21, 2013, 1:24 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
amarok
</div>
<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 my full GSoC project to rewrite MTP collection.
Notes:
* Individual commits can be seen at http://quickgit.kde.org/?p=clones%2Famarok%2Flaitl%2Famarok.git&a=shortlog&h=gsoc
* First commit to remove the old implementation is not included here for brevity
* Project description can be seen at https://google-melange.appspot.com/gsoc/project/google/gsoc2013/strohel/32001
* More info, progress reports and final report available at http://strohel.blogspot.com/search/label/gsoc</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;">Works well with my Samsung Android 4.1 phone; testing with greater variety of devices needed.</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/mtp/meta/MtpTrack.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/MtpDeviceConfiguration.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/MtpHelpers.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/MtpHelpers.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/MtpTranscodeCapability.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/MtpTranscodeCapability.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/MtpTransferJanitor.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/MtpTransferJanitor.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/OneToOneMap.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/OneToOneMap.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/support/forward_declarations.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>cmake/modules/FindMtp.cmake <span style="color: grey">(a6b7a0e)</span></li>
<li>src/core-impl/collections/CMakeLists.txt <span style="color: grey">(4785158)</span></li>
<li>src/core-impl/collections/mtp/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/MtpCollection.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/MtpCollection.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/MtpCollectionFactory.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/MtpCollectionFactory.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/MtpCollectionLocation.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/MtpCollectionLocation.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/TODO <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/amarok-mtp-manage-music.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/amarok_collection-mtp.desktop <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/CopyMtpTracksJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/CopyMtpTracksJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/DeleteMtpTracksJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/DeleteMtpTracksJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/DownloadMtpTrackJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/DownloadMtpTrackJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/InitMtpDeviceJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/InitMtpDeviceJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/ListMtpStorageJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/ListMtpStorageJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/MtpTransferJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/MtpTransferJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/ParseMtpTracksJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/ParseMtpTracksJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/SetMtpDeviceNameJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/SetMtpDeviceNameJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/UpdateMtpTrackJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/UpdateMtpTrackJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/UploadMtpTrackJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/jobs/UploadMtpTrackJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/meta/MtpAlbum.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/meta/MtpAlbum.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/meta/MtpEntity.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/collections/mtp/meta/MtpTrack.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113666/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>