I agree with Bart, it looks pretty good. One thing to be careful of though: you are quite often calling playableUrl() to determine whether the url is accessible. If I remember correctly there is one case, MtpCollection, that has to copy the file to a temporary location before or within playableUrl() as the Mtp device is not accessible directly.<div>
<br></div><div>I'd recommend making sure that this is not done unnecessarily if you have not done so already.</div><div><br></div><div>Cheers,</div><div>Max<br><br><div class="gmail_quote">On Mon, Aug 22, 2011 at 9:08 AM, Bart Cerneels <span dir="ltr"><<a href="mailto:bart.cerneels@kde.org">bart.cerneels@kde.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div>
<div style="font-family:Verdana, Arial, Helvetica, Sans-Serif"><div class="im">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border:1px #c9c399 solid">
<tbody><tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/102181/" target="_blank">http://git.reviewboard.kde.org/r/102181/</a>
</td>
</tr>
</tbody></table>
<br>
</div><pre style="white-space:pre-wrap;white-space:-moz-pre-wrap;white-space:-pre-wrap;white-space:-o-pre-wrap;word-wrap:break-word">Hi Sandeep,
The patch looks really good, coding style, logic constructs, inlie docs, all top notch.
Sorry for not reviewing sooner, the Desktop Summit and post conference catchup with work ate my time.
I'll try it out later today and perhaps commit if it's functional, or would you like to push it instead?
Bart</pre>
<br>
<p>- Bart</p><div class="im">
<br>
<p>On August 2nd, 2011, 2:38 p.m., Sandeep Raghuraman wrote:</p>
</div><table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-repeat:repeat-x;border:1px black solid">
<tbody><tr>
<td><div class="im">
<div>Review request for Amarok.</div>
<div>By Sandeep Raghuraman.</div>
</div><p style="color:grey"><i>Updated Aug. 2, 2011, 2:38 p.m.</i></p><div class="im">
<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">
<tbody><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">Improved isPlayable functions for some classes derived from Meta::Track. TrackNavigator will not queue unplayable tracks. StandardTrackNavigator::chooseNextTrack now skips unplayable tracks and returns the next playable track in the list. Unplayable tracks are grayed in the playlist to show that they are unavailable.</pre>
</td>
</tr>
</tbody></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">
<tbody><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">Tested with local files only. Cannot test with audio CD's since mine doesn't work. Going to test Upnp and Ampache tracks and Daap tracks if possible.</pre>
</td>
</tr>
</tbody></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=263640" target="_blank">263640</a>
</div>
<h1 style="color:#575012;font-size:10pt;margin-top:1.5em">Diffs </h1>
<ul style="margin-left:3em;padding-left:0">
<li>src/core-impl/collections/audiocd/AudioCdMeta.cpp <span style="color:grey">(c4a791e)</span></li>
<li>src/core-impl/collections/daap/CMakeLists.txt <span style="color:grey">(c60b371)</span></li>
<li>src/core-impl/collections/daap/DaapMeta.cpp <span style="color:grey">(b8431dd)</span></li>
<li>src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp <span style="color:grey">(5daed6d)</span></li>
<li>src/core-impl/collections/upnpcollection/UpnpMeta.cpp <span style="color:grey">(4d44acf)</span></li>
<li>src/core-impl/meta/file/File.cpp <span style="color:grey">(d8d516e)</span></li>
<li>src/core-impl/meta/stream/Stream.cpp <span style="color:grey">(5a6659c)</span></li>
<li>src/playlist/navigators/StandardTrackNavigator.cpp <span style="color:grey">(9675876)</span></li>
<li>src/playlist/navigators/TrackNavigator.cpp <span style="color:grey">(6fbfc55)</span></li>
<li>src/playlist/view/PlaylistViewCommon.cpp <span style="color:grey">(db300a3)</span></li>
<li>src/playlist/view/listview/PrettyItemDelegate.cpp <span style="color:grey">(08b0724)</span></li>
<li>src/services/ampache/AmpacheMeta.h <span style="color:grey">(fa104ec)</span></li>
<li>src/services/ampache/AmpacheMeta.cpp <span style="color:grey">(2b85a7b)</span></li>
<li>src/services/ampache/CMakeLists.txt <span style="color:grey">(e3d09af)</span></li>
<li>src/services/lastfm/meta/LastFmMeta.cpp <span style="color:grey">(055dfef)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/102181/diff/" style="margin-left:3em" target="_blank">View Diff</a></p>
</div></td>
</tr>
</tbody></table>
</div>
</div>
<br>_______________________________________________<br>
Amarok-devel mailing list<br>
<a href="mailto:Amarok-devel@kde.org">Amarok-devel@kde.org</a><br>
<a href="https://mail.kde.org/mailman/listinfo/amarok-devel" target="_blank">https://mail.kde.org/mailman/listinfo/amarok-devel</a><br>
<br></blockquote></div><br></div>