Review Request 107473: Changes in processing playlist files

Tue Mar 5 00:20:38 UTC 2013

> On March 5, 2013, 12:11 a.m., Matěj Laitl wrote:
> > 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)

To answer your question
> PlaylistFile::triggerTrackLoad implements async loading.
> In some cases triggerTrackLoad uses as if it is synchronous
> (e.g. DirectoryLoader, PlaylistController, PlaylistBrowserModel)
> 
> I see two solutions of this problem:
>    1) For now perform async loading in trigerTackLoad only on request.
>       Thereby loading will be async only if it was requested:
>           playlist->makeLoadingAsync();
>       In the future support of async loading can be added everywhere
>       and code for sync loading support can be removed.
>    2) Add async loading support to each place where triggerTrackLoad
>       is called.
>
> I chose first solution. If someone thinks that second solution
> is better, then I need some additional time to figure out how to 
> implement async loading support in remaining cases.

In long term, the second solution is better. However as you've already implemented stop-gap code for the 1st solution (and it isn't scary at all), keep the first one in this review request. You can always convert more users of file playlists to being async in future review requests and as you've said, once all are async, support for sync loading could be dropped.

BTW: are you a student eligible for Google Summer of Code?

- Matěj

-----------------------------------------------------------
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/055fee48/attachment.html>