<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/109369/">http://git.reviewboard.kde.org/r/109369/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 11th, 2013, 2:20 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/109369/diff/1/?file=118797#file118797line165" style="color: black; font-weight: bold; text-decoration: underline;">src/core/collections/CollectionLocation.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">class AMAROK_CORE_EXPORT CollectionLocation : public QObject</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">164</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">prepareCopy</span><span class="p">(</span> <span class="k">const</span> <span class="n">Meta</span><span class="o">::</span><span class="n">TrackList</span> <span class="o">&</span><span class="n">tracks</span><span class="p">,</span> <span class="n">CollectionLocation</span> <span class="o">*</span><span class="n">destination</span> <span class="p">);</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">165</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">prepareCopy</span><span class="p">(</span> <span class="k">const</span> <span class="n">Meta</span><span class="o">::</span><span class="n">TrackList</span> <span class="o">&</span><span class="n">tracks</span><span class="p">,</span> <span class="n">CollectionLocation</span> <span class="o">*</span><span class="n">destination</span> <span class="p">);</span></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">166</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">prepareCopy</span><span class="p">(</span> <span class="k">const</span> <span class="n">Meta</span><span class="o">::</span><span class="n">TrackList</span> <span class="o">&</span><span class="n">tracks</span><span class="p">,</span> <span class="n">CollectionLocation</span> <span class="o">*</span><span class="n">destination</span><span class="p">,</span> <span class="k">const</span> <span class="n">QList</span><span class="o"><</span><span class="n">CategoryId</span><span class="o">::</span><span class="n">CatMenuId</span><span class="o">></span> <span class="o">&</span><span class="n">sortLevels</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 is not the right approach. Instead, ensure that all *callers* of prepareCopy() already pass tracks in desired order.</pre>
</blockquote>
<p>On March 11th, 2013, 3:28 p.m. UTC, <b>Anmol Ahuja</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;">But prepareCopy functions are called in around 11 different files! Should I overload these functions in UmsCollectionLocationcpp and sort the tracks there?</pre>
</blockquote>
<p>On March 11th, 2013, 3:33 p.m. UTC, <b>Anmol Ahuja</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;">Oh, I was wrong, sorry, fixing it</pre>
</blockquote>
<p>On March 11th, 2013, 3:34 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;">> Should I overload these functions in UmsCollectionLocationcpp and sort the tracks there?
No. Instead, go though the 11 files and check whether they pass the tracks in desired order. Many of they probably already do.</pre>
</blockquote>
<p>On March 11th, 2013, 3:35 p.m. UTC, <b>Anmol Ahuja</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;">But wouldn't it still be better to overload these in UmsCollectionLocation.cpp? The order doesn't matter for the other CollectionLocations, it's just unnecessary overhead.</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;">Well, the original point was not to *sort* the tracks somewhere, but to *keep them in original sorting*. While it is not possible everywhere, but for example when copying from playlist it is well possible and in fact the only possible way to do it. You cannot achieve correct copy order from playlist by just extending prepareCopy(), in addition to the fact that it is ugly, redundant, and just plain wrong.</pre>
<br />
<p>- Matěj</p>
<br />
<p>On March 9th, 2013, 6:11 p.m. UTC, Anmol Ahuja 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 Anmol Ahuja.</div>
<p style="color: grey;"><i>Updated March 9, 2013, 6:11 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;">1. Made a levelSort() function to sort tracks according to multiple parameters ( Should i just be sorting tracks belonging to the same album? )
2. Modified copyUrlToCollection() to use QList instead of QMap
3. FileView's files are being copied in the order of selection, thanks to using QLists ( is that acceptable? )</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;">Seems to be copying tracks in the correct order now</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/services/mp3tunes/Mp3tunesServiceCollectionLocation.cpp <span style="color: grey">(aa61072)</span></li>
<li>src/core-impl/collections/db/sql/SqlCollectionLocation.cpp <span style="color: grey">(93efe97)</span></li>
<li>src/services/mp3tunes/Mp3tunesServiceCollectionLocation.h <span style="color: grey">(2b06cb4)</span></li>
<li>src/services/ServiceCollectionLocation.cpp <span style="color: grey">(d1cb0d8)</span></li>
<li>src/core/collections/CollectionLocation.h <span style="color: grey">(d37ccfb)</span></li>
<li>src/core/collections/CollectionLocation.cpp <span style="color: grey">(aecc068)</span></li>
<li>src/core-impl/collections/umscollection/UmsCollectionLocation.cpp <span style="color: grey">(e0ba0ac)</span></li>
<li>src/core-impl/collections/umscollection/UmsCollectionLocation.h <span style="color: grey">(45ba596)</span></li>
<li>src/core-impl/collections/umscollection/UmsCollection.cpp <span style="color: grey">(6bebd98)</span></li>
<li>src/core-impl/collections/support/TrashCollectionLocation.cpp <span style="color: grey">(61c2e49)</span></li>
<li>src/core-impl/collections/support/TrashCollectionLocation.h <span style="color: grey">(239a977)</span></li>
<li>src/core-impl/collections/support/PlaylistCollectionLocation.cpp <span style="color: grey">(c885046)</span></li>
<li>src/core-impl/collections/mtpcollection/handler/MtpHandler.cpp <span style="color: grey">(a8d9f52)</span></li>
<li>src/core-impl/collections/support/PlaylistCollectionLocation.h <span style="color: grey">(10a365f)</span></li>
<li>src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.cpp <span style="color: grey">(c1b76f5)</span></li>
<li>src/core-impl/collections/mediadevicecollection/handler/MediaDeviceHandler.h <span style="color: grey">(821f1b0)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.cpp <span style="color: grey">(f60aff6)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceCollectionLocation.h <span style="color: grey">(e40529f)</span></li>
<li>src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.cpp <span style="color: grey">(8a40c6c)</span></li>
<li>src/core-impl/collections/ipodcollection/jobs/IpodCopyTracksJob.h <span style="color: grey">(3c2d9f2)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodCollectionLocation.cpp <span style="color: grey">(f8105f9)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodCollectionLocation.h <span style="color: grey">(cc27e19)</span></li>
<li>src/core-impl/collections/db/sql/SqlCollectionLocation.h <span style="color: grey">(0bcf244)</span></li>
<li>src/core-impl/collections/audiocd/AudioCdCollectionLocation.cpp <span style="color: grey">(be13551)</span></li>
<li>src/browsers/filebrowser/FileView.h <span style="color: grey">(df8fbee)</span></li>
<li>src/browsers/CollectionTreeItemModel.cpp <span style="color: grey">(fa194b1)</span></li>
<li>src/browsers/CollectionTreeView.cpp <span style="color: grey">(fd9fe66)</span></li>
<li>src/browsers/collectionbrowser/CollectionWidget.h <span style="color: grey">(c281f41)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109369/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>