Review Request: The Playdar Collection and related changes
Ben Cooksley
bcooksley at kde.org
Tue Oct 12 05:09:49 CEST 2010
On Tue, Oct 12, 2010 at 10:12 AM, Andy Coder <andrew.coder at gmail.com> wrote:
> (Just an aside in response to the issues Max raised)
>
> As far as getting this up soon, it seems I was being way too optimistic.
> I've got papers/exams right now, and I'll be on the road
> Wednesday-Saturday. Even if I did the code now, I'm even having read access
> denied by git, and definitely don't have time to deal with that and make
> changes, (and everything external).
You likely need to update your Git remote path.
Edit .git/config in your Amarok Git clone, and ensure the url is
git at git.kde.org:amarok.git
Regards,
Ben
> If anyone feels like doing it themselves, go for it, but this will all
> otherwise have to wait until next week.
>
> - Andy Coder
>
>
> On Sat, Oct 9, 2010 at 1:26 PM, Andy Coder <andrew.coder at gmail.com> wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> http://git.reviewboard.kde.org/r/100001/
>>
>> On October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/PlaydarCollection.h<http://git.reviewboard.kde.org/r/100001/diff/1/?file=154#file154line104> (Diff
>> revision 1)
>>
>> namespace Collections
>>
>> 104
>>
>> QList< QPointer< Playdar::ProxyResolver > > m_proxyResolverList;
>>
>> 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 October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/PlaydarCollection.cpp<http://git.reviewboard.kde.org/r/100001/diff/2/?file=286#file286line183> (Diff
>> revision 2)
>>
>> namespace Collections
>>
>> 183
>>
>> DEBUG_BLOCK
>>
>> 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 October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/PlaydarCollection.cpp<http://git.reviewboard.kde.org/r/100001/diff/2/?file=286#file286line263> (Diff
>> revision 2)
>>
>> namespace Collections
>>
>> 263
>>
>> if( !m_memoryCollection->trackMap().contains( track->uidUrl() ) )
>>
>> 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 October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/PlaydarMeta.cpp<http://git.reviewboard.kde.org/r/100001/diff/2/?file=288#file288line252> (Diff
>> revision 2)
>>
>> uint
>>
>> 252
>>
>> Meta::PlaydarTrack::lastPlayed() const
>>
>> 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 October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp<http://git.reviewboard.kde.org/r/100001/diff/2/?file=290#file290line39> (Diff
>> revision 2)
>>
>> namespace Collections
>>
>> 39
>>
>> , m_autoDelete( false )
>>
>> 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 October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp<http://git.reviewboard.kde.org/r/100001/diff/2/?file=290#file290line377> (Diff
>> revision 2)
>>
>> namespace Collections
>>
>> 377
>>
>> PlaydarQueryMaker::addMatch( const Meta::DataPtr &data )
>>
>> 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 October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/support/Controller.h<http://git.reviewboard.kde.org/r/100001/diff/2/?file=292#file292line52> (Diff
>> revision 2)
>>
>> namespace Playdar
>>
>> 52
>>
>> * We invoke the private function status() here, return immediately,
>>
>> 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 October 9th, 2010, 10:52 a.m., *Maximilian Kossick* wrote:
>>
>> src/core-impl/collections/playdarcollection/support/Controller.cpp<http://git.reviewboard.kde.org/r/100001/diff/2/?file=293#file293line55> (Diff
>> revision 2)
>>
>> namespace Playdar {
>>
>> 55
>>
>> const QString baseUrl( "http://localhost:60210/api/?method=resolve" );
>>
>> 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
>>
>> On September 25th, 2010, 6:21 p.m., Andy Coder wrote:
>> Review request for Amarok and Leo Franchi.
>> By Andy Coder.
>>
>> *Updated 2010-09-25 18:21:06*
>> Description
>>
>> 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)
>>
>> View Diff <http://git.reviewboard.kde.org/r/100001/diff/>
>> Screenshots
>> [image: It works! (for me)]<http://git.reviewboard.kde.org/r/100001/s/2/>
>>
>
>
> _______________________________________________
> 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/20101012/1a7406ea/attachment-0001.htm
More information about the Amarok-devel
mailing list