Review Request 107473: Changes in processing playlist files

Tue Mar 5 04:49:00 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.

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.

> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote:
> > src/core/playlists/Playlist.h, lines 201-207
> > <http://git.reviewboard.kde.org/r/107473/diff/6/?file=116586#file116586line201>
> >
> >     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)

This 2 methods together with sync loading in PlaylistFile::triggerTrackLoad are temporary solution and should be removed in some point in a future. 

Optional argument in triggerTrackLoad will affect all inherited from Playlist classes, so it will be harder to remove this code. If it is ok, then I'll add this optional argument to triggerTrackLoad.

- Tatjana

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

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/898ab85e/attachment-0001.html>