Review Request: Changes in processing playlist files

Tue Nov 27 08:08:57 UTC 2012

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

I think you started with the wrong assumption that lazy loading of the contents of the playlist file is required.
We have some use cases for PlaylistFile:
1) Make playlists found in the collection folders available through the PlaylistBrowser using PlaylistFileProvider. 
2) Restore the session with contents of the playback queue using XSPFPlaylist saved to $KDEHOME/share/apps/amarok/current.xspf.
3) Wherever we need to load or save a playlist file, locally or remote. Like the "Add to playlist..." or "Export playlist..." actions in the menu.

Like you can tell from my inline comments, the support for remote loading needs to be removed from these classes. Fetching the content can be done outside of these classes using load(). The name and other playlist metadata is either in the content (XSPF) or derived from the filename (M3U, PLS). New PlaylistFile derived classes should use whatever the format has available, preferably from the content.

Since you want to add a new Playlist class, what is you use case for it. Is there any need to load remote content in use case 1) or 2) (might be specific about the format)?

I propose you don't refactor PlaylistFile just yet and implement ASXPlaylist so it works as needed for the above use cases. After that we'll have a better understanding for refactoring. I'm sure clean-up and refactor will be needed.

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

    I'm surprised your compiler does not trip up on this. Better remove the ';' to be compatible with the less forgiving compilers out there, like MSVC.

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

    downloadPlaylist just needs to go, it's legacy that I just didn't remove yet. I don't think it's still used. If you do find a use case from it, put the loading outside of the Playlist code and in the user itself.

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

    This comment has not been valid for a while. loading from a stream just makes tracks available in *this* playlist, doesn't add to the current playlist (a.k.a. the Playback Queue).
    Just remove it and write new docs once you know what they need to do.

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

    I don't think we should have load() in the base class. When loading a stream one needs to know what type of playlist is getting loaded, hence need to use one of the specialized PlaylistFile classes (M3U, XSPF, PLS, ...). Polymorphism is not required nor useful here.

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

    This needs to be removed as mentioned earlier. Implement the loading in the user or PlaylistManager (might already be).

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

    This is not good. There should be a default constructor that does minimal setup. Move the DOM creation to the save() function instead or create it on demand.

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

    Here is the real reason why lazy loading is needed. Reading playlist-file contents from disk is too fast to bother, but getting the right Track objects from CollectionManager::trackForUrl() is problematic. It should either block until the track is ready (depends in TrackProviders) or using proxy loading like it is now. The proxy is temporary and gets initialized with metadata we know from the contents of this playlist file.

- Bart Cerneels

On Nov. 26, 2012, 12:10 p.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107473/
> -----------------------------------------------------------
> 
> (Updated Nov. 26, 2012, 12:10 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.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt b70147d 
>   src/core-impl/playlists/types/file/LazyPlaylistFile.h PRE-CREATION 
>   src/core-impl/playlists/types/file/LazyPlaylistFile.cpp PRE-CREATION 
>   src/core-impl/playlists/types/file/PlaylistFile.h 4322da9 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 
>   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 3ba154a 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h ebf3235 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 
>   src/core/playlists/Playlist.h 5cf11f2 
>   src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 
>   tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 
>   tests/core-impl/playlists/types/file/TestLazyPlaylistFile.h PRE-CREATION 
>   tests/core-impl/playlists/types/file/TestLazyPlaylistFile.cpp PRE-CREATION 
>   tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 0f19e1b 
> 
> 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/20121127/336cc569/attachment-0001.html>