<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107473/">http://git.reviewboard.kde.org/r/107473/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 28th, 2013, 12:16 a.m. UTC, <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On March 28th, 2013, 9:05 p.m. UTC, <b>Tatjana Gornak</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On March 28th, 2013, 9:36 p.m. UTC, <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Oh, please fix that link error. It is strange but known that something on my box links ThreadWeaver even though I don't explicitly mention it, which then causes build failures on other boxes.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I want to ask for an advice about sorting. The current DirectoryLoader behaves like this: if you provide it with a playlist it just loads playlist without sorting its content, if you provide it with a directory, then it loads directory and sorts content of directory, so if there was a playlist its content will appear sorted.
New DirectoryLoader sorts content in all cases, that as you pointed is wrong in case if just one playlist was loaded.
But maybe it is better to preserve order of tracks in playlist in all cases?
</pre>
<br />
<p>- Tatjana</p>
<br />
<p>On March 27th, 2013, 7:35 p.m. UTC, Tatjana Gornak wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Amarok.</div>
<div>By Tatjana Gornak.</div>
<p style="color: grey;"><i>Updated March 27, 2013, 7:35 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">
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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) All unit-tests were passed.
2) For all playlists I've also checked loading and
saving through gui.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=291934">291934</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/browsers/playlistbrowser/PlaylistBrowserModel.cpp <span style="color: grey">(213af3b)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(4cdee5e)</span></li>
<li>src/DirectoryLoader.h <span style="color: grey">(e53a30d)</span></li>
<li>src/DirectoryLoader.cpp <span style="color: grey">(bd7fa34)</span></li>
<li>src/browsers/playlistbrowser/PlaylistBrowserModel.h <span style="color: grey">(1b7a44d)</span></li>
<li>src/context/applets/info/InfoApplet.cpp <span style="color: grey">(f215885)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h <span style="color: grey">(4dd79f3)</span></li>
<li>src/core-impl/collections/umscollection/podcasts/UmsPodcastProvider.cpp <span style="color: grey">(1a4a934)</span></li>
<li>src/core-impl/playlists/types/file/PlaylistFile.h <span style="color: grey">(43dfdeb)</span></li>
<li>src/core-impl/playlists/types/file/PlaylistFile.cpp <span style="color: grey">(2730354)</span></li>
<li>src/core-impl/playlists/types/file/PlaylistFileLoaderJob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/playlists/types/file/PlaylistFileLoaderJob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/playlists/types/file/PlaylistFileSupport.h <span style="color: grey">(b1a1c26)</span></li>
<li>src/core-impl/playlists/types/file/PlaylistFileSupport.cpp <span style="color: grey">(2dcc0cd)</span></li>
<li>src/core-impl/playlists/types/file/m3u/M3UPlaylist.h <span style="color: grey">(7b982fe)</span></li>
<li>src/core-impl/playlists/types/file/m3u/M3UPlaylist.cpp <span style="color: grey">(2395260)</span></li>
<li>src/core-impl/playlists/types/file/pls/PLSPlaylist.h <span style="color: grey">(e69e133)</span></li>
<li>src/core-impl/playlists/types/file/pls/PLSPlaylist.cpp <span style="color: grey">(8fe4b51)</span></li>
<li>src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h <span style="color: grey">(6cf33e9)</span></li>
<li>src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp <span style="color: grey">(bbcecc3)</span></li>
<li>src/core-impl/podcasts/sql/SqlPodcastMeta.cpp <span style="color: grey">(1a025ce)</span></li>
<li>src/core-impl/podcasts/sql/SqlPodcastProvider.cpp <span style="color: grey">(0da9b51)</span></li>
<li>src/core/playlists/Playlist.h <span style="color: grey">(018d2af)</span></li>
<li>src/core/playlists/Playlist.cpp <span style="color: grey">(36879a1)</span></li>
<li>src/core/playlists/PlaylistProvider.h <span style="color: grey">(0fb5818)</span></li>
<li>src/playlist/PlaylistActions.cpp <span style="color: grey">(00bf13a)</span></li>
<li>src/playlist/PlaylistController.cpp <span style="color: grey">(e5bde8b)</span></li>
<li>src/playlist/PlaylistRestorer.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/playlist/PlaylistRestorer.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/playlistmanager/PlaylistManager.h <span style="color: grey">(cbf65b0)</span></li>
<li>src/playlistmanager/PlaylistManager.cpp <span style="color: grey">(89c754b)</span></li>
<li>src/playlistmanager/SyncedPlaylist.h <span style="color: grey">(fce3d14)</span></li>
<li>src/playlistmanager/SyncedPlaylist.cpp <span style="color: grey">(4f7c0b7)</span></li>
<li>src/playlistmanager/file/PlaylistFileProvider.cpp <span style="color: grey">(e4e7284)</span></li>
<li>src/playlistmanager/sql/SqlPlaylist.cpp <span style="color: grey">(88e685b)</span></li>
<li>src/playlistmanager/sql/SqlUserPlaylistProvider.cpp <span style="color: grey">(5eb6987)</span></li>
<li>src/services/gpodder/GpodderProvider.h <span style="color: grey">(8e856e3)</span></li>
<li>tests/TestDirectoryLoader.h <span style="color: grey">(0583a9e)</span></li>
<li>tests/TestDirectoryLoader.cpp <span style="color: grey">(b98247d)</span></li>
<li>tests/core-impl/meta/multi/TestMetaMultiTrack.h <span style="color: grey">(8379bf4)</span></li>
<li>tests/core-impl/meta/multi/TestMetaMultiTrack.cpp <span style="color: grey">(8e9e47d)</span></li>
<li>tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp <span style="color: grey">(b25d059)</span></li>
<li>tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.h <span style="color: grey">(1183599)</span></li>
<li>tests/core-impl/playlists/types/file/pls/TestPLSPlaylist.cpp <span style="color: grey">(5eab2f3)</span></li>
<li>tests/core-impl/playlists/types/file/xspf/TestXSPFPlaylist.cpp <span style="color: grey">(5ea3a41)</span></li>
<li>tests/core/playlists/CMakeLists.txt <span style="color: grey">(e2e0ce4)</span></li>
<li>tests/core/playlists/TestPlaylistObserver.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core/playlists/TestPlaylistObserver.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/playlistmanager/file/TestPlaylistFileProvider.cpp <span style="color: grey">(7d0971a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107473/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/03/27/0001-Cleanups-and-fixes-for-Tatjana-s-changes_1.patch">Cleanups and fixes; please rebase v11 on top of current master and apply this</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>