Review Request: The Playdar Collection and related changes

Leo Franchi lfranchi at kde.org
Fri Sep 24 04:32:13 CEST 2010 

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

Ship it!


Looks good to me! Max, feel free to review the CurriedQM stuff, where Andy essentially saves a list of function calls and replays them. Code looks clean. Andy, if you fix the really absolutely minor things i found, merge when you have the chance. Oh, and make sure you add an entry to the ChangeLog :)

Also, for future reference, we discussed on IRC something like the following:

1) On startup, check for playdar. If it exists, show the playdar collection.
2) Show a Playdar entry in Internet Sources. If checked, poll for playdar every ~5 or 10 minutes, with a refresh button available in the settings of the internet service. Though playdar is not technically an internet service, we could write a small stub one to provide a config UI in a logical place. It would be nice to have a "collections config" page to put this, but as all the other collections are auto-detected, this might not make a ton of sense.

Thoughts appreciated.


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

    I'm guessing the & is a copy and paste error here :)



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

    Probably worth calling the superclass constructor with parent() rather than in the constructor body :)



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

    is this string shown to the user? if so, it needs to be wrapped in i18n()



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

    I don't know if you care, but all your license headers are missing a final > in your email address :D


- Leo


On 2010-09-24 01:49:15, Andy Coder wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100001/
> -----------------------------------------------------------
> 
> (Updated 2010-09-24 01:49:15)
> 
> 
> 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
> -----
> 
>   CMakeLists.txt 191b02e 
>   external/CMakeLists.txt PRE-CREATION 
>   external/JsonQt/CMakeLists.txt PRE-CREATION 
>   external/JsonQt/COMPATIBILITY PRE-CREATION 
>   external/JsonQt/COPYING.HEADER PRE-CREATION 
>   external/JsonQt/Doxyfile.cmake PRE-CREATION 
>   external/JsonQt/README.txt PRE-CREATION 
>   external/JsonQt/add-copyright-header.sh PRE-CREATION 
>   external/JsonQt/lib/CMakeLists.txt PRE-CREATION 
>   external/JsonQt/lib/JsonQtExport.h PRE-CREATION 
>   external/JsonQt/lib/JsonRpc.h PRE-CREATION 
>   external/JsonQt/lib/JsonRpc.cpp PRE-CREATION 
>   external/JsonQt/lib/JsonRpcAdaptor.h PRE-CREATION 
>   external/JsonQt/lib/JsonRpcAdaptor.cpp PRE-CREATION 
>   external/JsonQt/lib/JsonRpcAdaptorPrivate.h PRE-CREATION 
>   external/JsonQt/lib/JsonRpcAdaptorPrivate.cpp PRE-CREATION 
>   external/JsonQt/lib/JsonToProperties.h PRE-CREATION 
>   external/JsonQt/lib/JsonToProperties.cpp PRE-CREATION 
>   external/JsonQt/lib/JsonToVariant.h PRE-CREATION 
>   external/JsonQt/lib/JsonToVariant.cpp PRE-CREATION 
>   external/JsonQt/lib/ParseException.h PRE-CREATION 
>   external/JsonQt/lib/ParseException.cpp PRE-CREATION 
>   external/JsonQt/lib/VariantToJson.h PRE-CREATION 
>   external/JsonQt/lib/VariantToJson.cpp PRE-CREATION 
>   external/JsonQt/tests/CMakeLists.txt PRE-CREATION 
>   external/JsonQt/tests/JsonRpc.cpp PRE-CREATION 
>   external/JsonQt/tests/JsonRpcAdaptor.cpp PRE-CREATION 
>   external/JsonQt/tests/JsonToProperties.cpp PRE-CREATION 
>   external/JsonQt/tests/JsonToVariant.cpp PRE-CREATION 
>   external/JsonQt/tests/VariantToJson.cpp PRE-CREATION 
>   external/README.txt PRE-CREATION 
>   src/context/applets/similarartists/ArtistWidget.h a9331f6 
>   src/context/applets/similarartists/ArtistWidget.cpp 702248e 
>   src/context/engines/current/CurrentEngine.cpp bb8e869 
>   src/context/engines/songkick/CMakeLists.txt fab9a28 
>   src/context/engines/songkick/JsonQt/CMakeLists.txt 9978895 
>   src/context/engines/songkick/JsonQt/COMPATIBILITY 1a5b619 
>   src/context/engines/songkick/JsonQt/COPYING.HEADER bf9c30e 
>   src/context/engines/songkick/JsonQt/Doxyfile.cmake 4d2a4bf 
>   src/context/engines/songkick/JsonQt/README.txt 4ebcf73 
>   src/context/engines/songkick/JsonQt/add-copyright-header.sh 8d21c92 
>   src/context/engines/songkick/JsonQt/lib/CMakeLists.txt 3d39457 
>   src/context/engines/songkick/JsonQt/lib/JsonQtExport.h d7024e5 
>   src/context/engines/songkick/JsonQt/lib/JsonRpc.h 8f0024c 
>   src/context/engines/songkick/JsonQt/lib/JsonRpc.cpp cbcd161 
>   src/context/engines/songkick/JsonQt/lib/JsonRpcAdaptor.h 83836c8 
>   src/context/engines/songkick/JsonQt/lib/JsonRpcAdaptor.cpp 18a1504 
>   src/context/engines/songkick/JsonQt/lib/JsonRpcAdaptorPrivate.h 03c990c 
>   src/context/engines/songkick/JsonQt/lib/JsonRpcAdaptorPrivate.cpp c8fddb0 
>   src/context/engines/songkick/JsonQt/lib/JsonToProperties.h f04bd58 
>   src/context/engines/songkick/JsonQt/lib/JsonToProperties.cpp 4859d49 
>   src/context/engines/songkick/JsonQt/lib/JsonToVariant.h 82b62c0 
>   src/context/engines/songkick/JsonQt/lib/JsonToVariant.cpp 323e31a 
>   src/context/engines/songkick/JsonQt/lib/ParseException.h a942d37 
>   src/context/engines/songkick/JsonQt/lib/ParseException.cpp c663179 
>   src/context/engines/songkick/JsonQt/lib/VariantToJson.h 2a1a645 
>   src/context/engines/songkick/JsonQt/lib/VariantToJson.cpp baa7cdd 
>   src/context/engines/songkick/JsonQt/tests/CMakeLists.txt 4954fe7 
>   src/context/engines/songkick/JsonQt/tests/JsonRpc.cpp 1ce4c9c 
>   src/context/engines/songkick/JsonQt/tests/JsonRpcAdaptor.cpp 6e99599 
>   src/context/engines/songkick/JsonQt/tests/JsonToProperties.cpp e1a0707 
>   src/context/engines/songkick/JsonQt/tests/JsonToVariant.cpp 414d1c9 
>   src/context/engines/songkick/JsonQt/tests/VariantToJson.cpp dfad043 
>   src/core-impl/collections/CMakeLists.txt 79046d6 
>   src/core-impl/collections/playdarcollection/CMakeLists.txt PRE-CREATION 
>   src/core-impl/collections/playdarcollection/PlaydarCollection.h PRE-CREATION 
>   src/core-impl/collections/playdarcollection/PlaydarCollection.cpp PRE-CREATION 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.h PRE-CREATION 
>   src/core-impl/collections/playdarcollection/PlaydarMeta.cpp PRE-CREATION 
>   src/core-impl/collections/playdarcollection/PlaydarQueryMaker.h PRE-CREATION 
>   src/core-impl/collections/playdarcollection/PlaydarQueryMaker.cpp PRE-CREATION 
>   src/core-impl/collections/playdarcollection/amarok_collection-playdarcollection.desktop PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/Controller.h PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/Controller.cpp PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/ProxyResolver.h PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/ProxyResolver.cpp PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/QMFunctionTypes.h PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/Query.h PRE-CREATION 
>   src/core-impl/collections/playdarcollection/support/Query.cpp PRE-CREATION 
>   src/core-impl/collections/support/MemoryFilter.cpp e329faa 
>   src/core-impl/collections/support/MemoryQueryMaker.cpp 831601b 
>   src/core-impl/meta/proxy/MetaProxy.cpp 341e076 
>   src/core-impl/meta/proxy/MetaProxy_p.h d2638a8 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp 592bc24 
> 
> 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/20100924/b5d0cfa7/attachment-0001.htm

More information about the Amarok-devel mailing list