<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>
<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>
<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>