<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/100181/">http://git.reviewboard.kde.org/r/100181/</a>
</td>
</tr>
</table>
<br />
<p>On December 1st, 2010, 12:10 p.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;">Wonderful, looks like my very long comment is not saved, great.
Short version:
Patch has usability problems because of conflict with Playlist design, not meant as a playlist editor.
Partial saving of selected tracks is good but has to work without context menu action. Use the default save button(s) instead (Playlist::Dock::slotSaveCurrentPlaylist())
Bart</pre>
</blockquote>
<p>On December 1st, 2010, 6:56 p.m., <b>Dennis Francis</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'm not still sure of the nature of usability problem. May be your long comment explained everything in detail :)
You mean *moving* the whole functionality of the patch to the save button ? or add similar functionality for save button by adding
additional actions to it ?
Modifying Playlist::Dock::slotSaveCurrentPlaylist() to save only selected tracks would not allow the user to have the normal functionality of the save button.
Not sure how the "save-into-existing-playlist" feature be implemented in save button without usability problems.</pre>
</blockquote>
<p>On December 2nd, 2010, 7:13 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;">The long comment explained, among other things, that we worked hard to reduce the context menu actions for the playlist. These should be no more then 5, so adding 2 more including an menu is not good.
> Modifying Playlist::Dock::slotSaveCurrentPlaylist() to save only selected tracks would not allow the user to have the normal functionality of the save button.
By not selecting any tracks the complete tracklist will be saved. If testing shows this is not intuitive we could add a modal dialog asking the user how the playlist should be saved (all, only selected) or just a message in the statusbar.
</pre>
</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;">> save-into-existing-playlist
The first question is whether we want this functionality at all; because it enables the use of The Playlist (really a playback queue) as a saved playlist manager.
Let's say we do want it, I've been thinking about an elegant solution for a while.
We allow loading a single playlist into The Playlist. This is indicated using a colored widget in the top of the view. Any editing (removing, adding, sorting, etc) of the tracklist will now be applied to the loaded saved playlist. It would also work in the other direction: adding tracks to the saved playlist is reflected in The Playlist. This "connection" can always be broken by clicking on the loaded playlist widget.
This loading-in is not just usable for saved playlists by the way, there are other sub-classes of Playlists::Playlist. Dynamic playlists adding tracks to itself, and by being loaded being an elegant way to implement "dynamic mode". And streams being implemented as a playlist is also possible.</pre>
<br />
<p>- Bart</p>
<br />
<p>On December 1st, 2010, 1:06 a.m., Dennis Francis wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.orgrb/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 Dennis Francis.</div>
<p style="color: grey;"><i>Updated 2010-12-01 01:06:25</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;">Added functionality in the context menu to save the selected tracks ( on the right side )
- to a new playlist ( currently only sqlplaylist and fileplaylist providers are only supported )
- into an already existing playlist
( Duplication is avoided hence feels like a playlist update )</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;">It works fine foe me.</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=186545">186545</a>,
<a href="https://bugs.kde.org/show_bug.cgi?id=211811">211811</a>,
<a href="https://bugs.kde.org/show_bug.cgi?id=239950">239950</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/UserPlaylistModel.cpp <span style="color: grey">(2178d27)</span></li>
<li>src/core-impl/collections/mediadevicecollection/playlist/MediaDeviceUserPlaylistProvider.h <span style="color: grey">(e1ae132)</span></li>
<li>src/core-impl/collections/mediadevicecollection/playlist/MediaDeviceUserPlaylistProvider.cpp <span style="color: grey">(12d3b46)</span></li>
<li>src/core-impl/playlists/providers/user/UserPlaylistProvider.h <span style="color: grey">(609e1a8)</span></li>
<li>src/core/playlists/PlaylistFormat.h <span style="color: grey">(b93180d)</span></li>
<li>src/core/playlists/PlaylistFormat.cpp <span style="color: grey">(6b3cb6b)</span></li>
<li>src/core/playlists/PlaylistProvider.h <span style="color: grey">(4cc3417)</span></li>
<li>src/core/playlists/PlaylistProvider.cpp <span style="color: grey">(2d37e8b)</span></li>
<li>src/playlist/PlaylistDock.h <span style="color: grey">(6e13e7f)</span></li>
<li>src/playlist/PlaylistModel.h <span style="color: grey">(3ad8030)</span></li>
<li>src/playlist/PlaylistModel.cpp <span style="color: grey">(1acbc9f)</span></li>
<li>src/playlist/view/PlaylistViewCommon.h <span style="color: grey">(9582b1b)</span></li>
<li>src/playlist/view/PlaylistViewCommon.cpp <span style="color: grey">(db300a3)</span></li>
<li>src/playlist/view/listview/PrettyListView.h <span style="color: grey">(f22a7c8)</span></li>
<li>src/playlist/view/listview/PrettyListView.cpp <span style="color: grey">(062c584)</span></li>
<li>src/playlistmanager/PlaylistManager.h <span style="color: grey">(943fcf1)</span></li>
<li>src/playlistmanager/PlaylistManager.cpp <span style="color: grey">(fb13ea7)</span></li>
<li>src/playlistmanager/file/PlaylistFileProvider.h <span style="color: grey">(bd19f79)</span></li>
<li>src/playlistmanager/file/PlaylistFileProvider.cpp <span style="color: grey">(22e8abe)</span></li>
<li>src/playlistmanager/sql/SqlUserPlaylistProvider.h <span style="color: grey">(3a5a62e)</span></li>
<li>src/playlistmanager/sql/SqlUserPlaylistProvider.cpp <span style="color: grey">(d2ae5b9)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100181/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>