<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>
<p>On March 5th, 2013, 4:48 a.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;">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>
</blockquote>
<p>On March 6th, 2013, 2:26 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;">> 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).
If "this notification should be triggered irrespective of loading type" is true, then your code is correct. Discussion whether this assumption holds below:
a) if we limit ourselves to PlaylistFiles (which is okay as the code explicitly uses PlaylistFilePtr), this is nearly true with a couple of exceptions:
1. if someone calls triggerTrackLoad() twice, he might expect that tracksLoaded() observer method is triggered twice, too. I suggest PlaylistFile::triggerTrackLoad() is modified to call tracksLoaded() before returning in case m_trackLoaded is true. This is not a problem of this call site, but an API design problem for places that take Playlist pointer and don't know whether triggerTrackLoad() is already called.
2. PlaylistFileLoaderJob::run() has multiple (failure) return points that don't call m_playlist->notifyObserversTracksLoaded(). This will cause current DirectoryLoader code to hang infinitely. I'd be much safer if it were triggered even in error cases. I suggest creating PlaylistFileLoaderJob::slotDone() private slot and connecting it to its done() signal in its constructor and calling notifyObserversTracksLoaded() in its slotDone() signal.
b) while this piece of code would work with just above changes, I suggest that we enforce the same behaviour for all Playlists. I suggest we add notifyObserversTracksLoaded() to default implementation of triggerTrackLoad() (and document it so) so that most other playlists will behave well and tweaking other implementations of triggerTrackLoad().
In short, I'd like if every call to triggerTrackLoad() would trigger tracksLoaded() notification, sooner or later. This way it will make playlist handling much less error prone in corner cases. [OTOH, triggering tracksLoaded() every time even when already may be suboptimal especially when there are multiple observers of the playlist; what is your opinion on that matter?]</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;">I like this solution, despite its sub-optimality, because handling different corner case more likely will lead to an error in the future.</pre>
<br />
<p>- Tatjana</p>
<br />
<p>On March 11th, 2013, 1:57 a.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 11, 2013, 1:57 a.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">(c03541f)</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/browsers/playlistbrowser/PlaylistBrowserModel.cpp <span style="color: grey">(213af3b)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h <span style="color: grey">(4dd79f3)</span></li>
<li>src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp <span style="color: grey">(abf0fae)</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">(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-impl/podcasts/sql/SqlPodcastMeta.cpp <span style="color: grey">(1a025ce)</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/playlist/PlaylistActions.h <span style="color: grey">(75b71fd)</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>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/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>
<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>
</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>