Review Request 109879: Small cleanups in playlists

[Matěj Laitl]

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


Thanks, we just love code deduplication & cleanups. :-) I have just one thing, rather a question than a remark.


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

    I see you've just deduplicated code from subclasses, but reading the implementation I have absolutely no clue what it actually does. :-)
    
    The method would also benefit from a more specific name, "process" can be really anything and passed url doesn't seem to need to already relative (as the name could suggest).
    
    Could you please document it like:
    /**
     * If the passed url is relative, this method does *something and something* with it, for example "relative/path/file.mp3" gets converted to "something/other/file.mp3"
     * it also sets m_relative to true if it ecounters a relative url (this serves to preserve playlist "relativity" across reads & saves)
     */
    
    Also, please convert this from parameter-modifying to take and return method [1]. While the latter it slightly less efficient, it makes it possible to write processRelativeUrl( methodReturningUrl() ) and is much more prevalent in Amarok code, thus serving consistency.
    
    [1] i.e. KUrl processRelativeUrl( const KUrl &url );


- Matěj Laitl


On April 6, 2013, 6:57 a.m., Tatjana Gornak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109879/
> -----------------------------------------------------------
> 
> (Updated April 6, 2013, 6:57 a.m.)
> 
> 
> Review request for Amarok.
> 
> 
> Description
> -------
> 
> In review request 109758 Mat?j has pointed on some issues which appear not only in new asx implementation.
> 
> - number of includes was minimalized
> - common routine for relative urls processing
> - trackLocation is const method now
> 
> 
> Diffs
> -----
> 
>   src/core-impl/playlists/types/file/PlaylistFile.h d3bc640 
>   src/core-impl/playlists/types/file/PlaylistFile.cpp 36987c6 
>   src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 6280442 
>   src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 225dc0b 
>   src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp 6ec7bfd 
> 
> Diff: http://git.reviewboard.kde.org/r/109879/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tatjana Gornak
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130406/946177a4/attachment.html>