<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 November 27th, 2012, 8:08 a.m., <b>Bart Cerneels</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;">I think you started with the wrong assumption that lazy loading of the contents of the playlist file is required.
We have some use cases for PlaylistFile:
1) Make playlists found in the collection folders available through the PlaylistBrowser using PlaylistFileProvider.
2) Restore the session with contents of the playback queue using XSPFPlaylist saved to $KDEHOME/share/apps/amarok/current.xspf.
3) Wherever we need to load or save a playlist file, locally or remote. Like the "Add to playlist..." or "Export playlist..." actions in the menu.
Like you can tell from my inline comments, the support for remote loading needs to be removed from these classes. Fetching the content can be done outside of these classes using load(). The name and other playlist metadata is either in the content (XSPF) or derived from the filename (M3U, PLS). New PlaylistFile derived classes should use whatever the format has available, preferably from the content.
Since you want to add a new Playlist class, what is you use case for it. Is there any need to load remote content in use case 1) or 2) (might be specific about the format)?
I propose you don't refactor PlaylistFile just yet and implement ASXPlaylist so it works as needed for the above use cases. After that we'll have a better understanding for refactoring. I'm sure clean-up and refactor will be needed.</pre>
</blockquote>
<p>On November 27th, 2012, 10:44 a.m., <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;">> I think you started with the wrong assumption that lazy loading of the contents of the playlist file is required.
I don't agree, Myriam doesn't agree. Remember we have a bug "Amarok starts up several minutes when playlists on NFS..."
> Like you can tell from my inline comments, the support for remote loading needs to be removed from these classes.
Why? I would do it differently, (No need to call to playlistcontroller) but why not have it?
> Fetching the content can be done outside of these classes using load().
Yes, but no reason to do that from my PoV.
> The name and other playlist metadata is either in the content (XSPF) or derived from the filename (M3U, PLS). New PlaylistFile derived classes should use whatever the format has available, preferably from the content.
Yes.
> Since you want to add a new Playlist class, what is you use case for it. Is there any need to load remote content in use case 1) or 2) (might be specific about the format)?
This was just a helper class. If you read my comments, I already proposed doing it in PlaylistFile directly and I'm working on a patch that will enable it.
> I propose you don't refactor PlaylistFile just yet and implement ASXPlaylist so it works as needed for the above use cases. After that we'll have a better understanding for refactoring. I'm sure clean-up and refactor will be needed.
Who not deduplicating code first? Your approach seems like a wase of time to me. Remember, this is about deduplicating code, which I always find beneficial.
Tatjana, plese wait for me to finish my preparatory patch. (today hopefully)</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;">> Who not deduplicating code first? Your approach seems like a wase of time to me. Remember, this is about deduplicating code, which I always find beneficial.
Experience has taught me otherwise. It's faster to implement and then refactor for code reuse and consistency then to assume possible refactoring beforehand. With limited time/problem-space understanding/experience/etc smaller functional patches are better than attempting large refactorings. It's better for the review process as well.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 27th, 2012, 8:08 a.m., <b>Bart Cerneels</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/1/?file=96233#file96233line218" style="color: black; font-weight: bold; text-decoration: underline;">src/core-impl/playlists/types/file/xspf/XSPFPlaylist.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">XSPFPlaylist::loadXSPF( QByteArray &content )</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">150</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">foreach</span><span class="p">(</span> <span class="k">const</span> <span class="n">XSPFTrack</span> <span class="o">&</span><span class="n">track</span><span class="p">,</span> <span class="n">xspfTracks</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;">Here is the real reason why lazy loading is needed. Reading playlist-file contents from disk is too fast to bother, but getting the right Track objects from CollectionManager::trackForUrl() is problematic. It should either block until the track is ready (depends in TrackProviders) or using proxy loading like it is now. The proxy is temporary and gets initialized with metadata we know from the contents of this playlist file.</pre>
</blockquote>
<p>On November 27th, 2012, 10:36 a.m., <b>Myriam Schweingruber</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;">That is a totally wrong assumption, don't you remember that we introduced it because reading playlists on start-up is a PITA? Please don't assume that all users have their collection and playlists on an SSD.
Playlists should never be read unless you actually load it in the play queue, not a second earlier. We reduced start up time by over a minute, and some users reported start up delays of several minutes because playlist were read on start. See also https://bugs.kde.org/show_bug.cgi?id=245654, you introduced that yourself :)</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;">The delay is coming from loading the tracks from NFS, not the text content of the playlist file. I guess I needed to clarify that lazy loading of tracks using ProxyTrack is what we currently have in SQL and XSPF playlists and is what all playlist implementations need to do.</pre>
<br />
<p>- Bart</p>
<br />
<p>On November 26th, 2012, 12:10 p.m., Tatjana Gornak wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.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 Nov. 26, 2012, 12:10 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>
<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">(b70147d)</span></li>
<li>src/core-impl/playlists/types/file/LazyPlaylistFile.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core-impl/playlists/types/file/LazyPlaylistFile.cpp <span style="color: grey">(PRE-CREATION)</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/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">(3ba154a)</span></li>
<li>src/core-impl/playlists/types/file/xspf/XSPFPlaylist.h <span style="color: grey">(ebf3235)</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">(5cf11f2)</span></li>
<li>src/playlistmanager/file/PlaylistFileProvider.cpp <span style="color: grey">(e4e7284)</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/TestLazyPlaylistFile.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core-impl/playlists/types/file/TestLazyPlaylistFile.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp <span style="color: grey">(0f19e1b)</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>