Review Request: GSoC report: Integrate Spotify into Amarok #4 (squashed commits, recent on top)

Mon Aug 13 16:43:05 UTC 2012

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


src/core-impl/collections/spotifycollection/SpotifyCollection.h
<http://git.reviewboard.kde.org/r/105285/#comment13496>

    The naming is a bit inconsistent:
    
    checkStatus is in "doSomething" format, whereas all the other slots are in "somethingHappened" format,
    spotifyReady is in nounVerb format, which is rather odd,
    slotSpotifyError contains "slot" in the name, whereas all the other slots do not.
    

src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13495>

    I'd rather you checked here, if m_controller is 0.


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13497>

    CollectionManager adds every collection to track providers, so you shouldn't do this yourself.


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13500>

    Why do you create collection both in init() and in spotifyReady()? I don't see you removing the collection anywhere.


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13498>

    Ditch the if()


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13501>

    magic number


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13502>

    magic number


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13503>

    Ditch the if(), delete 0 is a safe operation.


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13504>

    DONE TODO? Does it mean you can remove the comment now? :)


src/core-impl/collections/spotifycollection/SpotifyCollection.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13505>

    same here


src/core-impl/collections/spotifycollection/SpotifyConfig.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13506>

    It's a yes-no message box, it doesn't have a cancel.


src/core-impl/collections/spotifycollection/SpotifyPlaylist.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13507>

    :QObject(0) is the default behavior, do not be that verbose


src/core-impl/collections/spotifycollection/SpotifyPlaylist.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13509>

    double comma? have you compiled the code? :)


src/core-impl/collections/spotifycollection/SpotifyPlaylist.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13508>

    same here


src/core-impl/collections/spotifycollection/SpotifySettings.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13510>

    I don't like the formatting here.
    
    Remember that you can carry the strings in C like this:
    
     "Spotify resolver blah-blah. "
     "It's missing or not installed blah blah"
    
    Also, don't use the contractions, and don't separate sentences with a comma.
    
    Try like this:
    
    "Spotify resolver is missing or not installed correctly on your system. "
    "This program is required by Spotify collection. "
    "Do you want to download and install it now?"


src/core-impl/collections/spotifycollection/SpotifySettings.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13511>

    magic number?


src/core-impl/collections/spotifycollection/SpotifySettings.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13512>

    Jesus.
    
    QFile f("somewhere.something");
    f.setPermissions(f.permissions() | QFile::ExeOwner);
    
    Do. Not. Use. system().


src/core-impl/collections/spotifycollection/amarok_collection-spotifycollection.desktop
<http://git.reviewboard.kde.org/r/105285/#comment13513>

    Are you supposed to fill the translations with untranslated strings?


src/core-impl/collections/spotifycollection/amarok_collection-spotifycollection.desktop
<http://git.reviewboard.kde.org/r/105285/#comment13514>

    Don't forget to bump this.


src/core-impl/collections/spotifycollection/support/Controller.h
<http://git.reviewboard.kde.org/r/105285/#comment13515>

    extern is meaningless for functions.


src/core-impl/collections/spotifycollection/support/Controller.h
<http://git.reviewboard.kde.org/r/105285/#comment13516>

    I'd rather see "explicit" here.


src/core-impl/collections/spotifycollection/support/Controller.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13517>

    Please sort the headers according to the style guide.


src/core-impl/collections/spotifycollection/support/Controller.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13518>

    I'd rather see an inline function here.


src/core-impl/collections/spotifycollection/support/Controller.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13519>

    I haven't found the place where query is deleted.


src/core-impl/collections/spotifycollection/support/Controller.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13520>

    I think there should be a restart delay here. Imagine that the resolver quits immediately after startup.


src/core-impl/collections/spotifycollection/support/Controller.cpp
<http://git.reviewboard.kde.org/r/105285/#comment13521>

    Is this _really_ needed? Can spotify resolver be sometimes ".exe" and sometimes python script or ".doc"? :)


- Edward Hades Toroshchin


On Aug. 13, 2012, 11:24 a.m., Zhengliang Feng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105285/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2012, 11:24 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> Things I've done this week:
> * Added a new playlist provider SpotifyPlaylistProvider
> * Added playlist class SpotifyPlaylist
> * Clear all extra whitespaces
> * Figured out how Collection, QueryMaker and Playlist worked
> 
> Things need to be done next week:
> * Clean ScriptResolver's interfaces, move all query related interfaces into Query class
> * Replace Controller class with ScriptResolver
> * Test SpotifyCollection
> * Finish playlist and playlist provider
> 
> 
> Diffs
> -----
> 
>   src/core-impl/collections/CMakeLists.txt c78b920 
>   src/core-impl/collections/spotifycollection/CMakeLists.txt PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyCollection.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyCollection.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyConfig.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyConfig.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyConfigWidget.ui PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyMeta.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyMeta.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyPlaylist.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyPlaylist.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyPlaylistProvider.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyQueryMaker.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifyQueryMaker.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifySettings.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/SpotifySettings.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/amarok_collection-spotifycollection.desktop PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/Controller.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/Controller.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/QMFunctionTypes.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/Query.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/Query.cpp PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/TrackProxy.h PRE-CREATION 
>   src/core-impl/collections/spotifycollection/support/TrackProxy.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/105285/diff/
> 
> 
> Testing
> -------
> 
> No test done this week. 
> 
> 
> Thanks,
> 
> Zhengliang Feng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120813/e9448055/attachment-0001.html>