Review Request: Changes in processing playlist files

[Matěj Laitl]

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

Also thanks for writing test-case along with your patch! This is appreciated.


- Matěj


-----------------------------------------------------------
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/20121126/00e43c11/attachment.html>