Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 15, 2013, 9:27 p.m.) Status -- This change has been marked as submitted. 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 - ChangeLog ef6790f src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFile.cpp c327082 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 8f4669a tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 14, 2013, 5:37 p.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 (updated) - ChangeLog ef6790f src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFile.cpp c327082 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 8f4669a tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 14, 2013, 5:39 p.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 (updated) - ChangeLog ef6790f src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFile.cpp c327082 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 8f4669a tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/#review30984 --- This review has been submitted with commit c91a2017116789ecb5c63c7e850cb69cfeb6b6f1 by Matěj Laitl on behalf of Tatjana Gornak to branch master. - Commit Hook On April 9, 2013, 8:24 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 9, 2013, 8:24 p.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 - ChangeLog 7b394ac src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 4a5639e tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 13, 2013, 12:50 p.m.) Status -- This change has been marked as submitted. 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 - ChangeLog 7b394ac src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 4a5639e tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/#review30985 --- Sorry, I errorneously pushed something I didn't want to. However I've hit Ctrl+C during the push, it seems that the commit hooks fired, but the commits might not have hit Amarok master. - Matěj Laitl On April 13, 2013, 12:50 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 13, 2013, 12:50 p.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 - ChangeLog 7b394ac src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 4a5639e tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
On April 11, 2013, 11:58 p.m., Matěj Laitl wrote: I've actually tested this patch, but it doesn't work as expected. First problem is that if I save and .asx playlist and then load it, the tracks don't load and stay grayed-out, probably because of their url is lowercased when the playlist is read. (the urls seem file inside the file) At this point I wonder why the test for .axs playlis passes just fine. Another problem is crash below. Hm, you are right. I knew that problem, but for some reason I thought that url can only be case-insensitive (but, obviously I was wrong: http://www.w3.org/TR/WD-html40-970708/htmlweb.html) - Tatjana --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/#review30945 --- On April 9, 2013, 8:24 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 9, 2013, 8:24 p.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 - ChangeLog 7b394ac src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 4a5639e tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 9, 2013, 8:24 p.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 (updated) - ChangeLog 7b394ac src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 4a5639e tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- (Updated April 7, 2013, 2:54 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 (updated) - ChangeLog 7b394ac src/CMakeLists.txt d667d95 src/MainWindow.cpp 07dca94 src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp e19769d src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 82de3a6 src/core-impl/playlists/types/file/asx/ASXPlaylist.h PRE-CREATION src/core-impl/playlists/types/file/asx/ASXPlaylist.cpp PRE-CREATION src/core/playlists/PlaylistFormat.cpp 6b3cb6b src/playlistmanager/file/PlaylistFileProvider.cpp 4a5639e tests/core-impl/playlists/types/file/CMakeLists.txt ef69236 tests/core-impl/playlists/types/file/asx/TestASXPlaylist.h PRE-CREATION tests/core-impl/playlists/types/file/asx/TestASXPlaylist.cpp PRE-CREATION 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
Re: Review Request 109758: Asx playlist implementation.
--- 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
Review Request 109758: Asx playlist implementation.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109758/ --- 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
