Review Request 107473: Changes in processing playlist files

Tatjana Gornak t.gornak at yandex.ru
Mon Mar 11 01:57:17 UTC 2013 


> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote:
> > src/DirectoryLoader.cpp, lines 168-169
> > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116572#file116572line168>
> >
> >     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.
> 
> Tatjana Gornak wrote:
>     Hm, I think I do not understand your point.
>     
>     Current code works for both types of loading, because DirectoryLoader is observer and it requires only
>     notification that loading is finished (and this notification should be triggered irrespective of loading type). 
>     
>     And it seems to me, that in code above there is a chance that we subscribe to playlist after loading is finished.
> 
> Matěj Laitl wrote:
>     > Current code works for both types of loading, because DirectoryLoader is observer and it requires only
>     > notification that loading is finished (and this notification should be triggered irrespective of loading type).
>     
>     If "this notification should be triggered irrespective of loading type" is true, then your code is correct. Discussion whether this assumption holds below:
>     
>     a) if we limit ourselves to PlaylistFiles (which is okay as the code explicitly uses PlaylistFilePtr), this is nearly true with a couple of exceptions:
>      1. if someone calls triggerTrackLoad() twice, he might expect that tracksLoaded() observer method is triggered twice, too. I suggest PlaylistFile::triggerTrackLoad() is modified to call tracksLoaded() before returning in case m_trackLoaded is true. This is not a problem of this call site, but an API design problem for places that take Playlist pointer and don't know whether triggerTrackLoad() is already called.
>      2. PlaylistFileLoaderJob::run() has multiple (failure) return points that don't call m_playlist->notifyObserversTracksLoaded(). This will cause current DirectoryLoader code to hang infinitely. I'd be much safer if it were triggered even in error cases. I suggest creating PlaylistFileLoaderJob::slotDone() private slot and connecting it to its done() signal in its constructor and calling notifyObserversTracksLoaded() in its slotDone() signal.
>     
>     b) while this piece of code would work with just above changes, I suggest that we enforce the same behaviour for all Playlists. I suggest we add notifyObserversTracksLoaded() to default implementation of triggerTrackLoad() (and document it so) so that most other playlists will behave well and tweaking other implementations of triggerTrackLoad().
>     
>     In short, I'd like if every call to triggerTrackLoad() would trigger tracksLoaded() notification, sooner or later. This way it will make playlist handling much less error prone in corner cases. [OTOH, triggering tracksLoaded() every time even when already may be suboptimal especially when there are multiple observers of the playlist; what is your opinion on that matter?]

I like this solution, despite its sub-optimality, because handling different corner case more likely will lead to an error in the future.


- Tatjana


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


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/2e401b7f/attachment-0001.html>

More information about the Amarok-devel mailing list