Review Request: Changes in processing playlist files

Tue Nov 27 10:44:32 UTC 2012

> On Nov. 27, 2012, 8:08 a.m., Bart Cerneels wrote:
> > 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.

> I think you started with the wrong assumption that lazy loading of the contents of the playlist file is required.

I don't agree, Myriam doesn't agree. Remember we have a bug "Amarok starts up several minutes when playlists on NFS..."

> Like you can tell from my inline comments, the support for remote loading needs to be removed from these classes.

Why? I would do it differently, (No need to call to playlistcontroller) but why not have it?

> Fetching the content can be done outside of these classes using load().

Yes, but no reason to do that from my PoV.

> 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.

Yes.

> 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)?

This was just a helper class. If you read my comments, I already proposed doing it in PlaylistFile directly and I'm working on a patch that will enable it.

> 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.

Who not deduplicating code first? Your approach seems like a wase of time to me. Remember, this is about deduplicating code, which I always find beneficial.

Tatjana, plese wait for me to finish my preparatory patch. (today hopefully)

- Matěj

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

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/3f5e1d57/attachment-0001.html>