Review Request: Changes in processing playlist files

[Edward Hades Toroshchin]

> On Nov. 26, 2012, 6:16 p.m., Matěj Laitl wrote:
> > Hi, thanks for your work!
> > 
> > First, I must acknowledge that our current file playlist code is a big mess and it certainly needs cleanup and code deduplication, your work is therefore greatly appreciated. I have reviewed just one file when it came to my mind: instead of introducing LazyPlaylistFile, the lazy-loading logic should be incorporated directly to PlaylistFile! There's no reason to have playlist files without lazy loading.
> > 
> > Could you please rework the patch eliminating LazyPlaylistFile? Also please pay attention to code style (e.g. spaces around parameters etc.) and to document methods along the way. (you seem to add methods to PlaylistFile. Please keep the public interface minimal usable - the setName() / setFilename() / setUrl() looks really scary - what does what? Does the file get renamed?) I understand this is mainly existing mess, not your new code. :-|
> 
> Matěj Laitl wrote:
>     Also thanks for writing test-case along with your patch! This is appreciated.
> 
> Tatjana Gornak wrote:
>     Hi,
>     
>     initially I've considered implementation of lazy-loading in playlistfiles, but there is a problem with constant methods. For example: virtual method Playlist::name() const. In case of XSPFPlaylist it calls method XSPFPlaylist::title(), but to get title you need to  load playlist, thereore title() can not be a constant method.
> 
> Matěj Laitl wrote:
>     I see. I've submitted https://git.reviewboard.kde.org/r/107484/ to ease your work. With it, I propose the following:
>     
>      * PlaylistFile subclasses return cached name (initially just name part of the filename/url) in name(), without triggering anything. (also applies to description)
>      * PlaylistFile subclasses return already loaded tracks in tracks(), without triggering anything
>      * only thing that triggers file loading is triggerTrackLoad(), where you start a background job (ThreadWeaver::Job) to load the tracks. If the playlists has metadata in file, you update it and call notifyObserversMetadataChanged()
>      * PlaylistManager::downloadPlaylist() is moved as a helper method to FilePlaylist and incorporated to the backgroundJob that performs the playlist loading.

So you propose not to maintain the current behavior, but just go with it and refactor the playlists?


- Edward Hades


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


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