Review Request: Fixes bug 263640

Maximilian Kossick maximilian.kossick at googlemail.com
Mon Aug 22 18:29:59 UTC 2011


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 at 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 at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20110822/5309acc4/attachment.html>


More information about the Amarok-devel mailing list