Review Request: Changes in processing playlist files

[Matěj Laitl]

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


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


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

    Nitpick: also doable after the class declaration without the fwd-declare + a bit too much newlines for my linking, but this is REALLY minor.



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

    I think that the lazy-loading logic should be added directly to PlaylistFile, without this looks-like-optional but used-all-the-time bridge in form of LazyPlaylistFile.



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

    Needs documentation. I understand passing url, but why passing PlaylistFilePtr?



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

    nitpick: please respect Amarok coding style formatting. (HACKING/ folder) Also applies to method definitions, format is:
    
    ReturnType
    Class::method( type param )
    {
        return something;
    }
    
    and
    
    LazyPlaylistFile::~LazyPlaylistFile()
    {
    }
    



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

    This looks really convoluted. Is this copied from some existing code? Also, I think m_tracksLoaded() should be set just in one method, not in both loadReadlPlaylist() and triggerTrackLoad().



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

    Please don't introduce whitespace errors.


- Matěj Laitl


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/53ab0196/attachment-0001.html>