<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>
</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 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>
<br />
<p>- Matěj</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>