[prev in list] [next in list] [prev in thread] [next in thread]
List: amarok-devel
Subject: Re: Review Request: Fixes bug 263640
From: Maximilian Kossick <maximilian.kossick () googlemail ! com>
Date: 2011-08-22 18:29:59
Message-ID: CADufctsv2N0jcGFNtxy4fdR3=v3in8ndXt3sfsMBO5NC_OErpQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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.
I'd recommend making sure that this is not done unnecessarily if you have
not done so already.
Cheers,
Max
On Mon, Aug 22, 2011 at 9:08 AM, Bart Cerneels <bart.cerneels@kde.org>wrote:
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102181/
>
> 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
>
>
> - Bart
>
> On August 2nd, 2011, 2:38 p.m., Sandeep Raghuraman wrote:
> Review request for Amarok.
> By Sandeep Raghuraman.
>
> *Updated Aug. 2, 2011, 2:38 p.m.*
> Description
>
> 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.
> Testing
>
> 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.
> *Bugs: * 263640 <https://bugs.kde.org/show_bug.cgi?id=263640>
> Diffs
>
> - src/core-impl/collections/audiocd/AudioCdMeta.cpp (c4a791e)
> - src/core-impl/collections/daap/CMakeLists.txt (c60b371)
> - src/core-impl/collections/daap/DaapMeta.cpp (b8431dd)
> - src/core-impl/collections/mediadevicecollection/MediaDeviceMeta.cpp
> (5daed6d)
> - src/core-impl/collections/upnpcollection/UpnpMeta.cpp (4d44acf)
> - src/core-impl/meta/file/File.cpp (d8d516e)
> - src/core-impl/meta/stream/Stream.cpp (5a6659c)
> - src/playlist/navigators/StandardTrackNavigator.cpp (9675876)
> - src/playlist/navigators/TrackNavigator.cpp (6fbfc55)
> - src/playlist/view/PlaylistViewCommon.cpp (db300a3)
> - src/playlist/view/listview/PrettyItemDelegate.cpp (08b0724)
> - src/services/ampache/AmpacheMeta.h (fa104ec)
> - src/services/ampache/AmpacheMeta.cpp (2b85a7b)
> - src/services/ampache/CMakeLists.txt (e3d09af)
> - src/services/lastfm/meta/LastFmMeta.cpp (055dfef)
>
> View Diff <http://git.reviewboard.kde.org/r/102181/diff/>
>
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel@kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>
>
[Attachment #5 (text/html)]
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>
_______________________________________________
Amarok-devel mailing list
Amarok-devel@kde.org
https://mail.kde.org/mailman/listinfo/amarok-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic