Review Request 107473: Changes in processing playlist files

Thu Mar 28 21:05:38 UTC 2013

> On March 28, 2013, 12:16 a.m., Matěj Laitl wrote:
> > Hmmm, I'm apparently able to update the diff, please disregard the attached file.
> > 
> > Please have a look a differences between v11 and v12, most of them are style fixes or cleanups, but it also boasts fixes for a couple of subtle bugs (like mismatched calls to XSPFPlaylist constructor). The patch v12 works rather well here, just one minor problem I've faced - if I drag a playlist from File Browser to play queue (the "main" playlist), the track order is not preserved. Could you please have a look at it? Perhaps it is better to query all tracks at once after trackLoaded() is called instead of using trackAdded() method.

Yes, sure I'll try to fix it.

But there is a problem with v12. When I tried to build it with -DKDE4_BUILD_TESTS=ON, build failed because of undefined reference to symbol 'ThreadWeaver::Weaver::instance()' in testmetamultitrack. It happened because ${KDE4_THREADWEAVER_LIBRARIES} was not added as link library for testmetamultitrack.

- Tatjana

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

On March 27, 2013, 7:35 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, 7:35 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/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
> ----------------
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20130328/3f8be994/attachment-0001.html>