Review Request 107473: Changes in processing playlist files

Matěj Laitl matej at laitl.cz
Tue Mar 5 00:11:22 UTC 2013 

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


Tatjana, thanks for your continued effort on this review. :-) There seem to be a couple of problematic places that may cause regressions, please take a look at them so this can be merged. (which I hope can be done rather soon, this patch contains incredible amount of clean-ups)


src/DirectoryLoader.cpp
<http://git.reviewboard.kde.org/r/107473/#comment21304>

    I don't think this will work as intended because:
    
     a) metadataChanged() is not suited to inform about finished loading. It can be for example called twice per a single playlist if it for example reads its name asynchronously
     b) medatataChanged() can be triggered in any thread, thus this method needs to be made thread safe (wrt its accesses to m_entities, m_tracks) using a QMutex (which needs to guard all other accesses to m_tracks, m_entires in turn)
    
    To solve the above, I think you would need another PlaylistObserver method: tracksLoaded(). (trackAdded() would seem good, but think of playlists with zero tracks)



src/DirectoryLoader.cpp
<http://git.reviewboard.kde.org/r/107473/#comment21305>

    Here you would have to deal with both async-loading and sync-loading playlists (you don't want to add assumption that all file playlists load asyncly), I suggest the following:
    
    (...)
    playlist->triggerTrackLoad(); // playlist track loading is on demand.
    if( playlist->trackCount() >= 0 )
        loadingFinished( playlist ); // fool ourselves because nobody else would trigger this
    else
        subscribeTo( playlist ); // let loadingFinished() be called by the playlist
    
    I also think you can use playlist.data() instead of the ::staticCast() and let compiler find PlaylistPtr constructor.



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

    This is used just at 1 places, so we might as well consider removing this and making them use addTrack(), but this future consideration doesn't block this patch in any way.



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

    I think you leak PlaylistFileLoaderJob instance here.
    
    Please connect worker's done() signal to its deleteLater() slot here.



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

    run() method should be in fact protected.



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

    Well, this is called from a dedicated thread, so blocking download doesn't matter here and is in fact preferred (as opposed to complicating the class with async handling).



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

    nitpick: error() << "text" << url.toLocalFile(); would work too and is kinda simpler (unimportant remark; I now see you are not the original author of this line)



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

    path.toLocalFile() would seem more appropriate here.



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

    Ouch, this seems to be copy of the file you've added under tests/



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

    Ouch, this seems to be copy of the file you've added under tests/



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

    The last statement now becomes redundant as you've (correctly) added m_tracksLoaded check above.
    
    HOWEVER, I'd just set it to true here, empty m3u playlist is valid too and will prevent possible errors in future edits.



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

    ditto m_trackLoaded.
    
    Setting it to true here right away is even more pronounced as the method has multiple exit points and all set it to true.



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

    This is incorrect change, the old ns was correct, see http://xspf.org/quickstart/



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

    This seems strange, why insertPlaylist() is called *before* actually creating the proxy tracks? This seems that it won't work like this.



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

    Newly added track will be already in tracks(), adding it extra will duplicate it.
    
    In fact, I don't think it is needed to reimplement addTrack() at all - notice that setTrackList() is called in savePlaylist() too.



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

    There should be no need to reimplement, setTrackList() is already called in savePlaylists().



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

    Looks kinda inconsistent wrt other metadata setting methods, I'd rather not touch this. (ditto for other occasions below)



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

    The check seems needed, note that PlaylistFile::save() will currently invent a name if m_url is empty. Ditto for much more methods below. Not touching these has a benefit of making the patch smaller. :-)



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

    This should be removed, right. (just noting it so that I don't confuse you by my previous comment)



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

    This is too specific to warrant 2 new methods and a class variable. Please add "bool asynchronous" optional argument to triggerTrackLoad(), defaulting to true. (and pass it as an argument to playlist loading job too)



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

    Thinking about it more, the Restorer doesn't need to be a member variable of the PlaylistActions at all.
    
    Make it a QObject subclass, allocate it on heap using new and make it auto-delete itself by calling deleteLater() when it finishes its job.



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

    The argument would better be "const KUrl &url"



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

    Playlist::Actions are usually accessed using Actions::instance(), no need to keep pointer to them and to require it in the constructor.



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

    This would be better named m_tracks.



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

    Class-wise this is redundant with m_tracks and should be avoided. Just create local iterator in processTracks()



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

    this will have to be changed to tracksLoaded() or whatever name you'll call the new PlaylistObserver method. This method will also need to be made thread safe as PlaylistObserver methods may get called by any thread at any time.



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

    Just checking m_playlistToRestore == playlist would have the same effect with a bonus of avoiding one class variable.



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

    Hmm, this seems to add first run jingle if there is nothing in the restored playlist. Certainly not what we want.



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

    processTracks() can get called multiple times, but this code doesn't seem to be ready for it.



src/playlistmanager/file/PlaylistFileProvider.cpp
<http://git.reviewboard.kde.org/r/107473/#comment21334>

    There seems to be no equivalent of this removed code. Easiest solution would be to add optional PlaylistProvider * parameter to Playlists::loadPlaylistFile() defaulting to 0.



tests/core-impl/meta/multi/TestMetaMultiTrack.cpp
<http://git.reviewboard.kde.org/r/107473/#comment21335>

    This is actually needed and causes the test to fail here. Perhaps you don't build with debugging assertions enabled? All developers should build with CMAKE_BUILD_TYPE=Debug
    
    Or perhaps this was just incorrectly resolved rebasing conflict?
    
    (I won't repeat this for occasions below)



tests/core-impl/playlists/types/file/TestPlaylistObserver.h
<http://git.reviewboard.kde.org/r/107473/#comment21339>

    This file would be better moved to tests/core/playlists/ (applies to the associated .cpp file, too)



tests/core-impl/playlists/types/file/TestPlaylistObserver.cpp
<http://git.reviewboard.kde.org/r/107473/#comment21338>

    This will need the
        /* Collection manager needs to be instantiated in the main thread, but
         * MetaProxy::Tracks used by playlist may trigger its creation in a different thread.
         * Pre-create it explicitly */
        CollectionManager::instance();
    trick too.


- Matěj Laitl


On March 2, 2013, 6:06 p.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated March 2, 2013, 6:06 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/CMakeLists.txt 92650c3 
>   src/DirectoryLoader.h e53a30d 
>   src/DirectoryLoader.cpp bd7fa34 
>   src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 
>   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.cpp 2dcc0cd 
>   src/core-impl/playlists/types/file/TestPlaylistObserver.h PRE-CREATION 
>   src/core-impl/playlists/types/file/TestPlaylistObserver.cpp PRE-CREATION 
>   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/playlists/Playlist.h b4e4f40 
>   src/core/playlists/Playlist.cpp 2d57e6b 
>   src/playlist/PlaylistActions.h e8e541b 
>   src/playlist/PlaylistActions.cpp 00bf13a 
>   src/playlist/PlaylistRestorer.h PRE-CREATION 
>   src/playlist/PlaylistRestorer.cpp PRE-CREATION 
>   src/playlistmanager/PlaylistManager.h cbf65b0 
>   src/playlistmanager/PlaylistManager.cpp 89c754b 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   tests/TestDirectoryLoader.h 0583a9e 
>   tests/TestDirectoryLoader.cpp b98247d 
>   tests/core-impl/meta/multi/TestMetaMultiTrack.cpp 8e9e47d 
>   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 b25d059 
>   tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 
>   tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 
> 
> 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/20130305/3fc2bbf4/attachment-0001.html>

More information about the Amarok-devel mailing list