----------------------------------------------------------- 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 > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
