Review Request 107473: Changes in processing playlist files

Matěj Laitl matej at laitl.cz
Sat Feb 9 00:55:50 UTC 2013 

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


First, thanks for the hard work and sorry for us being slow with reviewing. I think the latest patch revisions go into right direction. This turned out be be a serious API designing and code refactoring - a job not usual for a newcomer who's just getting into (sometimes very messy) Amarok code. With this in mind, please don't be disappointed that this goes slower/harder/more labourous than expected.

In this review I've focused on PlaylistFile API, because I took this as an opportunity to clean it up so that it as as easy as possible to use and implement new subclasses. (sorry) :)


src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20358>

    nitpick: we prefer <KJob>, <Threadweaver/Job> -style includes when whey exist.



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20359>

    Doesn't seem to be needed outside of PlaylistFile.cpp -> the declaration could be moved to the .cpp or (preferably) to a new file. More descriptive name like PlaylistFileLoaderJob could be better.



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20360>

    Making something QObject has consequences such as thread affinity etc. and I'd prefer not to make Playlists QObjects. This seem to be needed just for loadingDone() slot - perhaps that work could be done in PlaylistFileWorker? (which can be made a friend of PlaylistFile to access protected method)



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20361>

    Are all the constructors needed? In my opinion there should be only one: PlaylistFile( const KUrl &url );  (this is not something introduced by your patch, I'm just abusing it as an opportunity for cleanup)



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20362>

    What is the difference between save() and savePlaylist()? Also, if we don't allow constructor w/out url specified, the url arguments here can go away. (which I'd like very much)
    
    It seems that save() is the public method for other code to use and savePlaylist() is the method for subclasses to implement. If this is the case, make it so:
     * make save() non-virtual
     * make savePlaylist() protected
    
    In each case, both need to be documented.



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20363>

    Why are the 2 very similar methods needed? I'd like if they could be factored into just one, perhaps the one with the QTextStream argument. Why are then public and not protected?



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20364>

    Thinking about it more, I think it is ridiculous to require calling triggerTrackLoad() manually. Just start the loading job in constructor and ditch triggerTrackLoad() altogether.
    
    [you may want to postpone working on this for the case other Amarok developers don't agree]



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20365>

    Please document these more - including their relationship. Does calling setName() change filename? vice versa? (or perhaps setFileName() is redundant and can be removed (+1 from me)?)



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20367>

    While the method names look obvious, we'd really like to have all important classes documented.



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20368>

    Thinking about this, the Provider argument should probably go into the PlaylistFile constructor and this method shouldn't exist - changing providers during playlist life shouldn't be supported.



src/core-impl/playlists/types/file/PlaylistFile.h
<http://git.reviewboard.kde.org/r/107473/#comment20366>

    Oh, this method shouldn't be public and better if it didn't exist at all. Subclasses can modify m_url, users should call set(File)Name() instead.



src/core-impl/playlists/types/file/PlaylistFile.cpp
<http://git.reviewboard.kde.org/r/107473/#comment20369>

    This should probably take some code from Playlists::loadPlaylistFile() so that it supports also remote KUrls.



src/playlist/PlaylistActions.h
<http://git.reviewboard.kde.org/r/107473/#comment20370>

    RestorerPtr not used anywhere. (which is correct, no need for complex memory management for the helper class)



src/playlist/PlaylistActions.h
<http://git.reviewboard.kde.org/r/107473/#comment20372>

    Different memory management than plain-pointer + delete is preferable. Perhaps QScopedPointer<>, making Restorer a QObject and setting parent, or making it a non-pointer attribute?



src/playlist/PlaylistActions.h
<http://git.reviewboard.kde.org/r/107473/#comment20371>

    Nice solution. Please move to separate .h + .cpp file to make the code more structured. Also, the class and the processTracks() methods should be at least basic purpose description.


- Matěj Laitl


On Feb. 7, 2013, 3:57 p.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2013, 3:57 p.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> 
> I've started my changes with an implementation of a ASXPlaylist class,
> due this work I've found that implementation of different playlist file
> types has different logic, as result I end up with a rewriting
> playlist's implementations.
> 
> As example of difference:
>   -- Constructor M3UPlaylist::M3UPlaylist( const KUrl &url ) sets a url, but
>      does not load playlist, therefore loading of playlist are
> posponed till data from playlist are actualy needed (lazy loading)
>      On the other hand constructor XSPFPlaylist::XSPFPlaylist( const
> KUrl &url, bool autoAppend )  loads playlist.
> 
> The main idea of proposed changes is to create a unify code for
> processing playlist files:
>   -- lazy loading through LazyLoadPlaylist
>   -- common functionality was moved to PlaylistFile.
> 
> It is worth to be noticed that this patch changes structure of
> playlist's classes, bit does not changes their behavior, even if this behavior
> is inconsistent in some cases.
> 
> In following patch-requests I plan to submit ASX Playlist parser and
> organize playlists processing in more consistent way.
> 
> 
> This addresses bug 291934.
>     https://bugs.kde.org/show_bug.cgi?id=291934
> 
> 
> Diffs
> -----
> 
>   src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 3176c45 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.h e974539 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 3ba154a 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core/playlists/Playlist.h b4e4f40 
>   src/playlist/PlaylistActions.h e8e541b 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 
>   tests/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION 
>   tests/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION 
>   tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 0f19e1b 
>   tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp f4643ca 
> 
> Diff: http://git.reviewboard.kde.org/r/107473/diff/
> 
> 
> Testing
> -------
> 
> 1) All unit-tests were passed.
> 2) For all playlists I've also checked loading and
>    saving through gui.
> 
> 
> Thanks,
> 
> Tatjana Gornak
> 
>

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

More information about the Amarok-devel mailing list