Jenkins build is back to normal : amarok_master #308
See http://build.kde.org/job/amarok_master/308/changes ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 107473: Changes in processing playlist files
On March 27, 2013, 12:35 p.m., Matěj Laitl wrote: Yeah, nice change, you go even further with code deduplication. I have a load of changes that should be applied on top of your patch (which will include fixes to issues below), please wait for it along with some comments. You also accidentally forgot to include 6 newly added files in the patch v10, please reinclude them ASAP (so that I can rebase my changes on top of it). - Matěj --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review29948 --- On March 27, 2013, 1:13 a.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated March 27, 2013, 1:13 a.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. This addresses bug 291934. https://bugs.kde.org/show_bug.cgi?id=291934 Diffs - src/CMakeLists.txt c03541f src/DirectoryLoader.h e53a30d src/DirectoryLoader.cpp bd7fa34 src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp abf0fae src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 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 8fe4b51 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce src/core/playlists/Playlist.h 018d2af src/core/playlists/Playlist.cpp 36879a1 src/playlist/PlaylistActions.cpp 00bf13a src/playlist/PlaylistController.cpp e5bde8b src/playlistmanager/PlaylistManager.h cbf65b0 src/playlistmanager/PlaylistManager.cpp 89c754b src/playlistmanager/SyncedPlaylist.h fce3d14 src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 src/playlistmanager/sql/SqlPlaylist.cpp 88e685b tests/TestDirectoryLoader.h 0583a9e tests/TestDirectoryLoader.cpp b98247d tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.h 1183599 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 tests/core/playlists/CMakeLists.txt e2e0ce4 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 ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 109585: FIX UI doesn't say I have to run Moodbar generator manually. (BR 289483)
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109585/#review29951 --- Nice change, thanks. Kudos for including the ChangeLog entry. Just please do the minor tweaks and this will be Ship It! ChangeLog http://git.reviewboard.kde.org/r/109585/#comment22320 ChangeLog entries are added on top of the relevant categories. (please rebase your patch on top of current git master first) src/configdialog/dialogs/GeneralConfig.ui http://git.reviewboard.kde.org/r/109585/#comment22319 I don't think this needs mouseTracking, please revert it to its default state using Qt Designer. src/configdialog/dialogs/GeneralConfig.ui http://git.reviewboard.kde.org/r/109585/#comment22318 Just please make the link text '... visit a href=...related article on KDE UserBase/a.' - Matěj Laitl On March 19, 2013, 5:58 p.m., Harsh Gupta wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109585/ --- (Updated March 19, 2013, 5:58 p.m.) Review request for Amarok and Myriam Schweingruber. Description --- Added a label in moodbar options saying moodbar files must be generated manually. This addresses bug 289483. https://bugs.kde.org/show_bug.cgi?id=289483 Diffs - ChangeLog 284f188 src/configdialog/dialogs/GeneralConfig.ui 4e33f64 Diff: http://git.reviewboard.kde.org/r/109585/diff/ Testing --- All test case passed. File Attachments Screenshot http://git.reviewboard.kde.org/media/uploaded/files/2013/03/19/moodbar.png Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 109470: A patch which allows us to distinguish between cached lyrics and lyrics newly downloaded from the internet.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109470/#review29952 --- Review Board says you've uploaded a malformed patch, please fix it. src/context/engines/lyrics/LyricsEngine.h http://git.reviewboard.kde.org/r/109470/#comment22321 The method name is badly choosen and it lacks documentation. - Matěj Laitl On March 23, 2013, 5:33 p.m., mayank jha wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109470/ --- (Updated March 23, 2013, 5:33 p.m.) Review request for Amarok. Description --- It required modifications, when there is no change in the lyrics downloaded and lyrics retrieved from cache the title display of the lyrics browser changes to Cached Lyrics from Lyrics so we can tell the difference between old and new. Diffs - src/context/applets/lyrics/LyricsApplet.cpp 2394964 src/context/engines/lyrics/LyricsEngine.h b187b73 src/context/engines/lyrics/LyricsEngine.cpp 2befa91 Diff: http://git.reviewboard.kde.org/r/109470/diff/ Testing --- Its working fine! Thanks, mayank jha ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 109695: JJ#241066: Added a prepareToQuit() signal to amarokWindowScript
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109695/#review29953 --- Looks good, just please resolve the remarks and please add entry to ChangeLog under CHANGES section so that Script developers know and the bug is mentioned. You may want to for example wait a couple of seconds in your example script to check that scripts can postpone Amarok exit. src/App.cpp http://git.reviewboard.kde.org/r/109695/#comment22322 No. Adding arbitrary waits is simply wrong. Scripts have an opportunity to clean up in prepareToQuit() (note that signal-slot delivery is blocking except when the receiver is in another thread) src/scriptengine/AmarokWindowScript.h http://git.reviewboard.kde.org/r/109695/#comment22323 Please document this signal so that script developer know its use and semantics. - Matěj Laitl On March 24, 2013, 8:56 p.m., Anmol Ahuja wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109695/ --- (Updated March 24, 2013, 8:56 p.m.) Review request for Amarok. Description --- Bug 241066 - JJ: Add Signals trackStop and amarokShutdown to Amarok scripting interface Changes: 1. Added a prepareToQuit() signal to amarokWindowScript 2. Delayed the app's exit to 1 second after the signal is emitted 2. Replaced kapp macro calls with App::instance() because quit() is not virtual Note: Signal trackFinished()[trackStop] already exists, so only added prepareToQuit()[amarokShutdown] Diffs - src/App.h 97dfdf2 src/App.cpp fdb4255 src/MainWindow.cpp 66f4f76 src/dbus/mpris1/RootHandler.cpp e60eb1b src/dbus/mpris2/MediaPlayer2.cpp f86ccb3 src/scriptengine/AmarokScript.cpp 922e71d src/scriptengine/AmarokWindowScript.h 5407579 src/scriptengine/AmarokWindowScript.cpp 897e2da Diff: http://git.reviewboard.kde.org/r/109695/diff/ Testing --- Tested the new signal in a script Thanks, Anmol Ahuja ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 109752: JJ 316128: Handle Data CDs in Amarok
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109752/#review29956 --- Looks rather good, please resolve minor issues below. Also please add ChangeLog entry under CHANGES (on top) mentioning the bug number. WRT: to testing: what have you tested with, udisks1 or udisks2? Which KDE version? What happens with mixed audio CD with both CDDA tracks and data track? src/core-impl/collections/umscollection/UmsCollection.cpp http://git.reviewboard.kde.org/r/109752/#comment22328 I think this check should be moved up above the while loop, as the StoregeAccess device should be directly the OpticalDisc device. src/core-impl/collections/umscollection/UmsCollection.cpp http://git.reviewboard.kde.org/r/109752/#comment22325 nitpick: Amarok conding style says OpticalDisc *opt rather than OpticalDisc * opt. Also opt sounds like option to me, disc would be a better variable name. src/core-impl/collections/umscollection/UmsCollection.cpp http://git.reviewboard.kde.org/r/109752/#comment22326 if ( - if( src/core-impl/collections/umscollection/UmsCollection.cpp http://git.reviewboard.kde.org/r/109752/#comment22327 This debugging statement is only useful when developing please remove it. (along with the braces) - Matěj Laitl On March 26, 2013, 10:57 p.m., Anmol Ahuja wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109752/ --- (Updated March 26, 2013, 10:57 p.m.) Review request for Amarok. Description --- Added a check for data CDs in UmsCollection.cpp so they can be handled by UmsCollection in Amarok Diffs - src/core-impl/collections/umscollection/UmsCollection.cpp 2b5c53b Diff: http://git.reviewboard.kde.org/r/109752/diff/ Testing --- Tested a data CD to see if it plays Thanks, Anmol Ahuja ___ 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
Re: Review Request 108995: FIX Pre-amplifier in equalizer doesn't work
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108995/#review29959 --- Looks good, thanks! Just a remark or 2 below. Thanks also for the variable naming fixes. We also encourage commits to be focused on just one thing. Could you please split this patch into 2, first one just changing the variable names of existing variables, second implementing the actual pre-amp change? I suggest you use git add -p (interactive adding files to git index) along with its s: split function and e: editing hunks to be added by hand. src/EngineController.h http://git.reviewboard.kde.org/r/108995/#comment22340 Hmm, please call it EQUALIZER_BANDS_COUNT and set it to 10, rather than 11. Then adapt the code. Or rather, instead of macros, in C++ const variables are preferred. You should be able to achieve the same with: static const s_equalizerBandsCount = 10; src/EngineController.cpp http://git.reviewboard.kde.org/r/108995/#comment22339 The equalizerParameters.isEmpty() is not needed here, the code would be no-op even if it is empty. - Matěj Laitl On March 14, 2013, 5:30 p.m., Harsh Gupta wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108995/ --- (Updated March 14, 2013, 5:30 p.m.) Review request for Amarok. Description --- 1. Disabled Pre-amplifier in equalizer if it is not supported by phonon. 2. Fixed top and bottom labels of first slider. Earlier band name label was at the top and slider value label was at the bottom for first slider. 3. Removed an extra semicolon EqualizerDialog.h . Note : I have made an assumption that if at all preamp is present then it will the first element of Effect Parameter list. This addresses bug 301311. https://bugs.kde.org/show_bug.cgi?id=301311 Diffs - src/EngineController.h 5de4beb src/EngineController.cpp 58d7360 src/dialogs/EqualizerDialog.h fd9032b src/dialogs/EqualizerDialog.cpp 7d62e10 src/dialogs/EqualizerDialog.ui 43b0187 Diff: http://git.reviewboard.kde.org/r/108995/diff/ Testing --- All unit test cases passed. Note : I have tested it with gstreamer only. Xine phonon keep crashing on my PC. File Attachments Equalizer snapshot http://git.reviewboard.kde.org/media/uploaded/files/2013/02/17/equalizer.png Thanks, Harsh Gupta ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 108906: Add ability to drag titles to re-arrange them in queue manager
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/108906/ --- (Updated March 27, 2013, 2:45 p.m.) Status -- This change has been discarded. Review request for Amarok. Description --- The new ListWidget (PlaylistQueueEditorListWidget - feeling enterprisey today) supports drag'n'drop of self-contained ListItems to itself. On successful dropEvent a signal is emitted, which is intercepted by a new slot reloadQueue() in PlaylistQueueEditor. This slot in turn recreates the queue via The::playlistActions(). Things to consider in future: - dragging from the playlist into the queue editor - making the whole operation more efficient than clearing and recreating whole queue on each dropEvent - possibly less hacky way of implementing things? maybe some backend interface would need to be added/exposed for the queue editor? any advice from a seasoned Amarok developer would be very valuable If this isn't going to be accepted as-is into Amarok codebase then I'm willing to put some time into reworking the solution. This addresses bug 263563. https://bugs.kde.org/show_bug.cgi?id=263563 Diffs - src/CMakeLists.txt 043dc64 src/playlist/PlaylistQueueEditor.h 40b8cdf src/playlist/PlaylistQueueEditor.cpp f647e37 Diff: http://git.reviewboard.kde.org/r/108906/diff/ Testing --- Tested on two different installations, no bugs, regressions or instabilities noticed. Thanks, Bartosz Szreder ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 107473: Changes in processing playlist files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated March 27, 2013, 3:41 p.m.) Review request for Amarok. Changes --- Patch with forgotten files. 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. This addresses bug 291934. https://bugs.kde.org/show_bug.cgi?id=291934 Diffs (updated) - src/CMakeLists.txt c03541f src/DirectoryLoader.h e53a30d src/DirectoryLoader.cpp bd7fa34 src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp abf0fae src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 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 8fe4b51 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce src/core/playlists/Playlist.h 018d2af src/core/playlists/Playlist.cpp 36879a1 src/playlist/PlaylistActions.cpp 00bf13a src/playlist/PlaylistController.cpp e5bde8b src/playlist/PlaylistRestorer.h PRE-CREATION src/playlist/PlaylistRestorer.cpp PRE-CREATION src/playlistmanager/PlaylistManager.h cbf65b0 src/playlistmanager/PlaylistManager.cpp 89c754b src/playlistmanager/SyncedPlaylist.h fce3d14 src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 src/playlistmanager/sql/SqlPlaylist.cpp 88e685b tests/TestDirectoryLoader.h 0583a9e tests/TestDirectoryLoader.cpp b98247d tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.h 1183599 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 tests/core/playlists/CMakeLists.txt e2e0ce4 tests/core/playlists/TestPlaylistObserver.h PRE-CREATION tests/core/playlists/TestPlaylistObserver.cpp PRE-CREATION 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 ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 107473: Changes in processing playlist files
On March 27, 2013, 12:35 p.m., Matěj Laitl wrote: src/core-impl/playlists/types/file/m3u/M3UPlaylist.h, lines 49-52 http://git.reviewboard.kde.org/r/107473/diff/9-10/?file=120161#file120161line49 Private? Should be protected. Sorry, originally I've planned to make PlaylistFile::savePlaylist private instead (but forgot to change it). My point is that if we do not need to invoke virtual function from derived classes, but only customize the behaviour, then function can be private. But ok, protected will work as well. - Tatjana --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/#review29948 --- On March 27, 2013, 3:41 p.m., Tatjana Gornak wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated March 27, 2013, 3:41 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. This addresses bug 291934. https://bugs.kde.org/show_bug.cgi?id=291934 Diffs - src/CMakeLists.txt c03541f src/DirectoryLoader.h e53a30d src/DirectoryLoader.cpp bd7fa34 src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp abf0fae src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 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 8fe4b51 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce src/core/playlists/Playlist.h 018d2af src/core/playlists/Playlist.cpp 36879a1 src/playlist/PlaylistActions.cpp 00bf13a src/playlist/PlaylistController.cpp e5bde8b src/playlist/PlaylistRestorer.h PRE-CREATION src/playlist/PlaylistRestorer.cpp PRE-CREATION src/playlistmanager/PlaylistManager.h cbf65b0 src/playlistmanager/PlaylistManager.cpp 89c754b src/playlistmanager/SyncedPlaylist.h fce3d14 src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 src/playlistmanager/sql/SqlPlaylist.cpp 88e685b tests/TestDirectoryLoader.h 0583a9e tests/TestDirectoryLoader.cpp b98247d tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.h 1183599 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 tests/core/playlists/CMakeLists.txt e2e0ce4 tests/core/playlists/TestPlaylistObserver.h PRE-CREATION tests/core/playlists/TestPlaylistObserver.cpp PRE-CREATION 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
Re: Review Request 109470: A patch which allows us to distinguish between cached lyrics and lyrics newly downloaded from the internet.
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109470/ --- (Updated March 27, 2013, 7:22 p.m.) Review request for Amarok. Changes --- updated! Description --- It required modifications, when there is no change in the lyrics downloaded and lyrics retrieved from cache the title display of the lyrics browser changes to Cached Lyrics from Lyrics so we can tell the difference between old and new. Diffs (updated) - src/context/engines/lyrics/LyricsEngine.cpp 2befa91 src/context/applets/lyrics/LyricsApplet.cpp 2394964 src/context/engines/lyrics/LyricsEngine.h b187b73 Diff: http://git.reviewboard.kde.org/r/109470/diff/ Testing --- Its working fine! Thanks, mayank jha ___ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel
Re: Review Request 107473: Changes in processing playlist files
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/107473/ --- (Updated March 27, 2013, 7:35 p.m.) Review request for Amarok. Changes --- Added patch to apply on top of v11 (which should be itself rebased on top of current master first) since I cannot apparently update the diff directly. Tatjana, please post the above as v12, I'll comment then. 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. This addresses bug 291934. https://bugs.kde.org/show_bug.cgi?id=291934 Diffs (updated) - src/browsers/playlistbrowser/PlaylistBrowserModel.cpp 213af3b src/CMakeLists.txt 4cdee5e src/DirectoryLoader.h e53a30d src/DirectoryLoader.cpp bd7fa34 src/browsers/playlistbrowser/PlaylistBrowserModel.h 1b7a44d src/context/applets/info/InfoApplet.cpp f215885 src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h 4dd79f3 src/core-impl/collections/umscollection/podcasts/UmsPodcastProvider.cpp 1a4a934 src/core-impl/playlists/types/file/PlaylistFile.h 43dfdeb src/core-impl/playlists/types/file/PlaylistFile.cpp 2730354 src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp PRE-CREATION src/core-impl/playlists/types/file/PlaylistFileSupport.h b1a1c26 src/core-impl/playlists/types/file/PlaylistFileSupport.cpp 2dcc0cd src/core-impl/playlists/types/file/m3u/M3UPlaylist.h 7b982fe src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp 2395260 src/core-impl/playlists/types/file/pls/PLSPlaylist.h e69e133 src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp 8fe4b51 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h 6cf33e9 src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp bbcecc3 src/core-impl/podcasts/sql/SqlPodcastMeta.cpp 1a025ce src/core-impl/podcasts/sql/SqlPodcastProvider.cpp 0da9b51 src/core/playlists/Playlist.h 018d2af src/core/playlists/Playlist.cpp 36879a1 src/core/playlists/PlaylistProvider.h 0fb5818 src/playlist/PlaylistActions.cpp 00bf13a src/playlist/PlaylistController.cpp e5bde8b src/playlist/PlaylistRestorer.h PRE-CREATION src/playlist/PlaylistRestorer.cpp PRE-CREATION src/playlistmanager/PlaylistManager.h cbf65b0 src/playlistmanager/PlaylistManager.cpp 89c754b src/playlistmanager/SyncedPlaylist.h fce3d14 src/playlistmanager/SyncedPlaylist.cpp 4f7c0b7 src/playlistmanager/file/PlaylistFileProvider.cpp e4e7284 src/playlistmanager/sql/SqlPlaylist.cpp 88e685b src/playlistmanager/sql/SqlUserPlaylistProvider.cpp 5eb6987 src/services/gpodder/GpodderProvider.h 8e856e3 tests/TestDirectoryLoader.h 0583a9e tests/TestDirectoryLoader.cpp b98247d tests/core-impl/meta/multi/TestMetaMultiTrack.h 8379bf4 tests/core-impl/meta/multi/TestMetaMultiTrack.cpp 8e9e47d tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp b25d059 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.h 1183599 tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp 5eab2f3 tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp 5ea3a41 tests/core/playlists/CMakeLists.txt e2e0ce4 tests/core/playlists/TestPlaylistObserver.h PRE-CREATION tests/core/playlists/TestPlaylistObserver.cpp PRE-CREATION tests/playlistmanager/file/TestPlaylistFileProvider.cpp 7d0971a 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. File Attachments (updated) Cleanups and fixes; please rebase v11 on top of current master and apply this http://git.reviewboard.kde.org/media/uploaded/files/2013/03/27/0001-Cleanups-and-fixes-for-Tatjana-s-changes_1.patch Thanks, Tatjana Gornak
