----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/#review29958 -----------------------------------------------------------
Hi, looks good & clean (thanks to your previous cleanups), just a couple of tweaks would be nice. (but please concentrate on your first patch first) You'd also have to adapt to yet-unpublished changes on top of your first patch that I have, but that'll be easy. Please add ChangeLog entry under FEATURES too, mentioning relevant bugs, if any. Kudos for providing the test along with the implementation. src/core-impl/playlists/types/file/asx/ASXPlaylist.h <http://git.reviewboard.kde.org/r/109758/#comment22331> PlaylistFile.h already includes/fwd-declares these, no need to mention them here. src/core-impl/playlists/types/file/asx/ASXPlaylist.h <http://git.reviewboard.kde.org/r/109758/#comment22330> Not used anywhere, just ditch these. (I've already done this with other playlistfiles in follow-up to your other patch) src/core-impl/playlists/types/file/asx/ASXPlaylist.h <http://git.reviewboard.kde.org/r/109758/#comment22332> AMAROK_EXPORT_TESTS is gone in master, make it AMAROK_EXPORT; I think that using QDomDocument locally just in saving and loading methods would save memory. I know that XSPF playlist uses this approach too - I'd like to change it, but leave that for future. src/core-impl/playlists/types/file/asx/ASXPlaylist.h <http://git.reviewboard.kde.org/r/109758/#comment22335> nitpick: no need to mention default destructor if superclass already has virtual destructor. src/core-impl/playlists/types/file/asx/ASXPlaylist.h <http://git.reviewboard.kde.org/r/109758/#comment22333> Indentation src/core-impl/playlists/types/file/asx/ASXPlaylist.h <http://git.reviewboard.kde.org/r/109758/#comment22334> This should be "protected". (as private virtual methods make very little sense) src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp <http://git.reviewboard.kde.org/r/109758/#comment22336> Are all these includes needed? I'd guess that not. At least <typeinfo> is not and is somewhat tricky as it uses C++ TRRI which may be supported in varying levels across compilers. src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp <http://git.reviewboard.kde.org/r/109758/#comment22337> nitpick: we should prefer "using namespace Playlist;" instead of wrapping the whole .cpp file into namespace. tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp <http://git.reviewboard.kde.org/r/109758/#comment22338> Amarok code style is to have the "void" on extra line on its own. (more occasions below) - Matěj Laitl On March 27, 2013, 3:26 a.m., Tatjana Gornak wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109758/ > ----------------------------------------------------------- > > (Updated March 27, 2013, 3:26 a.m.) > > > Review request for Amarok. > > > Description > ------- > > Asx playlist implementation. > > P.S. This patch make sense only if https://git.reviewboard.kde.org/r/107473/ > will be accepted > > > This addresses bug 170207. > https://bugs.kde.org/show_bug.cgi?id=170207 > > > Diffs > ----- > > tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION > tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION > tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 > src/core/playlists/PlaylistFormat.cpp 6b3cb6b > src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 > src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION > src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION > src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd > src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d > src/CMakeLists.txt c03541f > src/MainWindow.cpp 66f4f76 > > Diff: http://git.reviewboard.kde.org/r/109758/diff/ > > > Testing > ------- > > Loading and saving works > > > Thanks, > > Tatjana Gornak > >
_______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
