Review Request: The Playdar Collection and related changes

[Andy Coder]

> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/PlaydarCollection.h, line 104
> > <http://git.reviewboard.kde.org/r/100001/diff/1/?file=154#file154line104>
> >
> >     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.

Yeah, I noticed the QWeakPointer review, and I'm happy to take care of it in my code, (tomorrow, probably).


> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/PlaydarCollection.cpp, line 183
> > <http://git.reviewboard.kde.org/r/100001/diff/2/?file=286#file286line183>
> >
> >     Is this really necessary?

It was when I just wanted to know what was getting called by user code, but there's probably a good number of these that aren't really helpful when debugging anymore.


> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp, line 39
> > <http://git.reviewboard.kde.org/r/100001/diff/2/?file=290#file290line39>
> >
> >     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

I must have missed that one.  There might still be something needed since PlaydarQueryMaker manages a MemoryQueryMaker, but I'll have a look at it.


> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp, line 377
> > <http://git.reviewboard.kde.org/r/100001/diff/2/?file=290#file290line377>
> >
> >     I'm pretty sure that this is identical to the implementation in QueryMaker itself. If so, please remove it here.

QueryMaker.h:
 160         virtual QueryMaker* addMatch( const Meta::DataPtr &data ) = 0;

Admittedly, I think this is how every other QueryMaker implements it, so it probably should be in QueryMaker itself, but it definitely isn't.


> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/PlaydarCollection.cpp, line 263
> > <http://git.reviewboard.kde.org/r/100001/diff/2/?file=286#file286line263>
> >
> >     I'd recommend inverting this check. That way it becomes a lot clearer which lock you are releasing in the (current) else block.

Seems reasonable, it is an unfortunately long block of code between acquiring and releasing the lock, (even though it happens right away).


> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/PlaydarMeta.cpp, line 252
> > <http://git.reviewboard.kde.org/r/100001/diff/2/?file=288#file288line252>
> >
> >     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.

Will do.


> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/support/Controller.h, line 52
> > <http://git.reviewboard.kde.org/r/100001/diff/2/?file=292#file292line52>
> >
> >     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.

Yeah... The docs here aren't correct, but I'll keep the note about internal behaviour in mind. This should get updated today/tomorrow.


> On 2010-10-09 10:52:35, Maximilian Kossick wrote:
> > src/core-impl/collections/playdarcollection/support/Controller.cpp, line 55
> > <http://git.reviewboard.kde.org/r/100001/diff/2/?file=293#file293line55>
> >
> >     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.

Yep.


- Andy


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


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/c0b95442/attachment-0001.htm