Review Request 107473: Changes in processing playlist files

Matěj Laitl matej at laitl.cz
Mon Mar 11 12:28:25 UTC 2013 

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


Good work! The remarks below are all just nitpicks or typo/style fixes. I'll then start testing this patch on my box in daily use and then this can be finally merged! Yay! :-)


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

    nitpick: "const KUrl &url" is better as it avoids copying



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

    Nitpick: "const Meta::Tracklist &tracks" is better as it save a copy of the list. (which is cheap because it is implicitly shared, but better get accustomed to pass every non-basic type (even KSharedPtrs benefit from being passed by const reference) by constant reference)



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

    Even here const reference would be better.



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

    Taking playlist by const reference would save one copying



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

    Should be "private slots:", as people outside aren't supposed to call this.



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

    Please tweak include order:
    own include
    --
    other amarok includes
    --
    kde incudes
    --
    qt includes
    --
    other includes
    
    Also I don't think amarokconfig.h is needed here. Note that <KIO/Job> (if it exists) is preferred to <kio/job.h>



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

    Constructor is a much better place to do the connect(). Also please use signature normalization inside SIGNAL/SLOT macros per http://marcmutz.wordpress.com/effective-qt/prefer-to-use-normalised-signalslot-signatures/ forming the only exception from Amarok coding style (e.g. you write SLOT(done(int)) instead of SLOT( done( int ) ) )



src/core/playlists/Playlist.h
<http://git.reviewboard.kde.org/r/107473/#comment21658>

    Better:
    
    This method is called after loading of playlist is finished (which was started by triggerTrackLoad()) and all tracks are already added.



src/core/playlists/Playlist.h
<http://git.reviewboard.kde.org/r/107473/#comment21659>

    This is not accurate, as whole point of async loading is that this function returns fast and notifyObservers...() is called after it.
    
    Better:
    (...) as appropriate. It is guaranteed that tracksLoaded() observer method will be called exactly once, either sooner (before returning from this method) or later (asynchronously perhaps from a different thread). You should also use (...)



src/core/playlists/Playlist.h
<http://git.reviewboard.kde.org/r/107473/#comment21660>

    Better:
    
    Default implementation just calls notifyObserversTracksLoaded().
    
    (+ no need for extra blank line)



src/core/playlists/Playlist.h
<http://git.reviewboard.kde.org/r/107473/#comment21662>

    better: (...) playlist loading started by trriggerTrackLoad() is finished and all tracks are added.



src/core/playlists/Playlist.h
<http://git.reviewboard.kde.org/r/107473/#comment21663>

    I think you're down to a very small amount of subclasses, as affected are ones that implement triggerTrackLoad() themselves, namely just PlaylistFile (which you've obviously already adapted) SqlPlaylist (which you've adapted) and SqlPodcastChannel.
    
    Please adapt SqlPodcastChannel too, then this TODO can be removed.



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

    I think you don't have to touch this file anymore.



src/playlist/PlaylistRestorer.cpp
<http://git.reviewboard.kde.org/r/107473/#comment21664>

    Hmm...
    a) you can safely assume that playlist is not null (as you do in second part of the method)
    b) currently, if tracks().count() is <= 0, it fails to emit restoreFinished(). While you could add "else emit restoreFinished();" I think you should go even further and don't check tracks().count() at all, processTracks() as currently coded below should be able to cope with empty m_tracks, m_position just fine.


- Matěj Laitl


On March 11, 2013, 1:57 a.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated March 11, 2013, 1:57 a.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/CMakeLists.txt c03541f 
>   src/DirectoryLoader.h e53a30d 
>   src/DirectoryLoader.cpp bd7fa34 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d 
>   src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b 
>   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 
>   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp abf0fae 
>   src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 
>   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 8fe4b51 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce 
>   src/core/playlists/Playlist.h 018d2af 
>   src/core/playlists/Playlist.cpp 36879a1 
>   src/playlist/PlaylistActions.h 75b71fd 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlist/PlaylistController.cpp e5bde8b 
>   src/playlist/PlaylistRestorer.h PRE-CREATION 
>   src/playlist/PlaylistRestorer.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/SyncedPlaylist.h fce3d14 
>   src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   src/playlistmanager/sql/SqlPlaylist.cpp 88e685b 
>   tests/TestDirectoryLoader.h 0583a9e 
>   tests/TestDirectoryLoader.cpp b98247d 
>   tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 
>   tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 
>   tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 
>   tests/core/playlists/CMakeLists.txt e2e0ce4 
>   tests/core/playlists/TestPlaylistObserver.h PRE-CREATION 
>   tests/core/playlists/TestPlaylistObserver.cpp PRE-CREATION 
> 
> 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/20130311/8a1b9391/attachment-0001.html>

More information about the Amarok-devel mailing list