Review Request: The Playdar Collection and related changes

[Maximilian Kossick]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100001/#review78
-----------------------------------------------------------



src/core-impl/collections/playdarcollection/PlaydarCollection.h
<http://git.reviewboard.kde.org/r/100001/#comment49>

    This should be switched to using QWeakPointer now that we use Qt 4.6. Additionally, you might want to use QWeakPointer::toStrongRef whenever you actually need to work with a Resolver.



src/core-impl/collections/playdarcollection/PlaydarCollection.cpp
<http://git.reviewboard.kde.org/r/100001/#comment51>

    Is this really necessary?



src/core-impl/collections/playdarcollection/PlaydarCollection.cpp
<http://git.reviewboard.kde.org/r/100001/#comment52>

    I'd recommend inverting this check. That way it becomes a lot clearer which lock you are releasing in the (current) else block.



src/core-impl/collections/playdarcollection/PlaydarMeta.h
<http://git.reviewboard.kde.org/r/100001/#comment53>

    can you use QWeakPointer/QSharedPointer?



src/core-impl/collections/playdarcollection/PlaydarMeta.cpp
<http://git.reviewboard.kde.org/r/100001/#comment54>

    Please look at using the already existing statistics provider to provide the statistics related values (e.g. playcount, lastplayed, rating). They will persist the data.



src/core-impl/collections/playdarcollection/PlaydarMeta.cpp
<http://git.reviewboard.kde.org/r/100001/#comment55>

    just return QString();?



src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp
<http://git.reviewboard.kde.org/r/100001/#comment56>

    Why are you reimplementing the autoDelete functionality? QueryMaker already uses provides the autoDeletion upon emitting queryDone(). I cannot find any reason to override that behaviour in this code



src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp
<http://git.reviewboard.kde.org/r/100001/#comment57>

    QString artist; is sufficient (and a bit faster iirc)



src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp
<http://git.reviewboard.kde.org/r/100001/#comment58>

    I'm pretty sure that this is identical to the implementation in QueryMaker itself. If so, please remove it here.



src/core-impl/collections/playdarcollection/support/Controller.h
<http://git.reviewboard.kde.org/r/100001/#comment60>

    Internal behaviour should generally not be put into the API documentaton. In my opinion it is sufficient for users to understand that they have to wait for the playdarReady or playdarError signal before they can use the controller.



src/core-impl/collections/playdarcollection/support/Controller.h
<http://git.reviewboard.kde.org/r/100001/#comment59>

    Should this not be a private? The constructor API comment suggests so.



src/core-impl/collections/playdarcollection/support/Controller.cpp
<http://git.reviewboard.kde.org/r/100001/#comment61>

    It might make sense to create an internal method, e.g. "serverUrl", which returns a KUrl containing everything but the request parameters. That way you write the url only once now and can make ther server configurable in the future.



src/core-impl/collections/playdarcollection/support/Query.h
<http://git.reviewboard.kde.org/r/100001/#comment62>

    ->QWeakPointer


- Maximilian


On 2010-09-25 18:21:06, Andy Coder wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100001/
> -----------------------------------------------------------
> 
> (Updated 2010-09-25 18:21:06)
> 
> 
> Review request for Amarok and Leo Franchi.
> 
> 
> Summary
> -------
> 
> This is what it does, (basically):
> 
> 1) Adds the PlaydarCollection/QueryMaker/Meta/etc. code in src/core-impl/collections/playdar-collection/
> 
> 2) Adds the top-level external/ directory and moves JsonQt into it
> 
> 3) Changes the formerly unused side of the behavior of MetaProxy::Track,
>    (when AwaitLookupNotification == false), so that PlaydarTracks can sit
>    around and eventually get updated without slowing things to a crawl
> 
> 4) Changes CurrentEngine and the SimilarArtists applet' use of
>    CollectionManager::instance()->primaryCollection() to ->queryMaker()
> 
> 5) Adds the 'Add top track to playlist' button to SimilarArtists
> 
> 6) Prevents XSPFPlaylist from killing of MetaProxy::Tracks just
>    because they're not playable yet
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/playdarcollection/PlaydarCollection.cpp PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/ProxyResolver.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100001/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> It works! (for me)
>   http://git.reviewboard.kde.org/r/100001/s/2/
> 
> 
> Thanks,
> 
> Andy
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101009/d63dffbf/attachment-0001.htm