<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 5th, 2013, 12:11 a.m. UTC, <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107473/diff/6/?file=116572#file116572line168" style="color: black; font-weight: bold; text-decoration: underline;">src/DirectoryLoader.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">DirectoryLoader::directoryListResults( KIO::Job *job, const KIO::UDSEntryList &list )</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">165</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">subscribeTo</span><span class="p">(</span> <span class="n">Playlists</span><span class="o">::</span><span class="n">PlaylistPtr</span><span class="o">::</span><span class="n">staticCast</span><span class="p">(</span> <span class="n">playlist</span> <span class="p">)</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">139</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">playlist</span><span class="o">-></span><span class="n">triggerTrackLoad</span><span class="p">();</span> <span class="c1">// playlist track loading is on demand.</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">166</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">playlist</span><span class="o">-></span><span class="n">triggerTrackLoad</span><span class="p">();</span> <span class="c1">// playlist track loading is on demand.</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Here you would have to deal with both async-loading and sync-loading playlists (you don't want to add assumption that all file playlists load asyncly), I suggest the following:
(...)
playlist->triggerTrackLoad(); // playlist track loading is on demand.
if( playlist->trackCount() >= 0 )
loadingFinished( playlist ); // fool ourselves because nobody else would trigger this
else
subscribeTo( playlist ); // let loadingFinished() be called by the playlist
I also think you can use playlist.data() instead of the ::staticCast() and let compiler find PlaylistPtr constructor.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hm, I think I do not understand your point.
Current code works for both types of loading, because DirectoryLoader is observer and it requires only
notification that loading is finished (and this notification should be triggered irrespective of loading type).
And it seems to me, that in code above there is a chance that we subscribe to playlist after loading is finished.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 5th, 2013, 12:11 a.m. UTC, <b>Matěj Laitl</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/107473/diff/6/?file=116586#file116586line201" style="color: black; font-weight: bold; text-decoration: underline;">src/core/playlists/Playlist.h</a>
<span style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace Playlists</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">200</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/**</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">201</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * Call this method to assure asynchronously loading.</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">202</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * @note not all playlist implemetations support asynchronous loading</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">203</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> */</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">204</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">makeLoadingAsync</span><span class="p">()</span> <span class="p">{</span> <span class="n">m_async</span> <span class="o">=</span> <span class="nb">true</span><span class="p">;</span> <span class="p">}</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">205</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="cm">/** Allows to check if asynchronously loading is activated*/</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">206</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="nf">isLoadingAsync</span><span class="p">()</span> <span class="p">{</span> <span class="k">return</span> <span class="n">m_async</span><span class="p">;</span> <span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is too specific to warrant 2 new methods and a class variable. Please add "bool asynchronous" optional argument to triggerTrackLoad(), defaulting to true. (and pass it as an argument to playlist loading job too)</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This 2 methods together with sync loading in PlaylistFile::triggerTrackLoad are temporary solution and should be removed in some point in a future.
Optional argument in triggerTrackLoad will affect all inherited from Playlist classes, so it will be harder to remove this code. If it is ok, then I'll add this optional argument to triggerTrackLoad.</pre>
<br />
<p>- Tatjana</p>
<br />
<p>On March 2nd, 2013, 6:06 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 2, 2013, 6:06 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/CMakeLists.txt <span style="color: grey">(92650c3)</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/core-impl/playlists/types/file/PlaylistFile.h <span style="color: grey">(4322da9)</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.cpp <span style="color: grey">(2dcc0cd)</span></li>
<li>src/core-impl/playlists/types/file/TestPlaylistObserver.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/playlists/types/file/TestPlaylistObserver.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/playlists/types/file/m3u/M3UPlaylist.h <span style="color: grey">(3176c45)</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">(e974539)</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/playlists/Playlist.h <span style="color: grey">(b4e4f40)</span></li>
<li>src/core/playlists/Playlist.cpp <span style="color: grey">(2d57e6b)</span></li>
<li>src/playlist/PlaylistActions.h <span style="color: grey">(e8e541b)</span></li>
<li>src/playlist/PlaylistActions.cpp <span style="color: grey">(00bf13a)</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/file/PlaylistFileProvider.cpp <span style="color: grey">(e4e7284)</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.cpp <span style="color: grey">(8e9e47d)</span></li>
<li>tests/core-impl/playlists/types/file/CMakeLists.txt <span style="color: grey">(ef69236)</span></li>
<li>tests/core-impl/playlists/types/file/TestPlaylistObserver.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core-impl/playlists/types/file/TestPlaylistObserver.cpp <span style="color: grey">(PRE-CREATION)</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.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>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107473/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>