<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/100177/">http://git.reviewboard.kde.org/r/100177/</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 28th, 2010, 11:46 p.m., <b>Leo Franchi</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;">Ok, a few comments from using it. Here are issues I think need to be addressed:
* I think the action should be centered like the other actions below the playlist, probable with a separator.
* I think the default size of the dialog is too big, especially given how small the items are
* Lacking icon for 3rd/bottom button
* The list is sort of lacking. It should at least show the Artist as well, maybe like Artist - Title.
* List doesn't update on track progression, but jumps ahead once a button is pressed and tracks have progressed
* Dialog is modal---it shouldn't be, so the user can also add/remove stuff from the queue with the queue manager open. Hand in hand with this is the fact that the playlist should update to reflect any changes made in the dialog immediately
And these are issues I think are nice but not required for version 1:
* Tracks should be re-arrangable by drag-n-drop
* Tracks from the playlist should be draggable onto the Queue Manager to queue them at a specific place
cheers,
leo</pre>
</blockquote>
<p>On November 29th, 2010, 12:18 a.m., <b>Andreas Hartmetz</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;">* Listview item text with too little information: fixedName() looked like it would change the least, but I think you're right: displaying useful information in the common case is more important. Queueing streams that often have no defined end is not a common thing to do.
* List not updating on track progression: I expected the queueChanged() signal from the playlist because track progression effectively dequeues a track. I'll try to fix it in the playlist.
* Modality: Intentional so the playlist doesn't change too much while the dialog exists, possibly causing bugs. I expect that one would usually click on items in the playlist to enqueue them - you will likely discover them there with no dialog open (because it's just annoying to have another window you don't need most of the time). Then you edit the queue. I don't use dynamic playlist though, so if the playlist can change anyway while the dialog is up it could as well be non-modal.
* Updating the playlist: this probably means emitting the right signal at the right place. Any ideas?</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;">Yeah, I think we should optimize for the "usual" use-case of tracks in the playlist. You bring up a good point, as it is super easy to modify the playlist so severely that the queue dialog would basically be reset. so about the modality....hm. In general I think modal dialogs are bad and need a very good reason to be modal---stopping the user from interacting with amarok until he finishes the "queue stuff in the queue manager" action is quite severe. say the user opens the queue manager, changes around the queues a bit and is around halfway there. then the track changes, but the user wants to skip it or something--he has to exit the queue manager (with his partially-completed queue) and and do that, then open the queue manager again. Ok, so this is somewhat obscure. Maybe it depends on how complicated it'll be to keep the manager in sync with the playlist :D
I don't really know about dynamic mode: i've never tried dynamic mode + queueing, as they are almost cross-purpose, so i don't think we should be doing too much work to support that configuration. you get weird behaviours with the dynamic track progression if you suddenly start playing stuff out of order anyway.
hmm, I don't know much about the playlist connection stuff, sorry. you know more than I do now about the playlist :)</pre>
<br />
<p>- Leo</p>
<br />
<p>On November 29th, 2010, midnight, Andreas Hartmetz 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 Andreas Hartmetz.</div>
<p style="color: grey;"><i>Updated 2010-11-29 00:00:22</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;">This adds a Queue Editor, with limited features (move up / move down / clear) like in Amarok 1.
The KAction for it is not very well placed in the main UI, I hope you guys can help me.
A few design changes were necessary, too, as you can see in the patch.</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;">"worksforme"</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=198180">198180</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">(c5ee1e3)</span></li>
<li>src/MainWindow.cpp <span style="color: grey">(f755b6f)</span></li>
<li>src/likeback/LikeBackBar.cpp <span style="color: grey">(6964f88)</span></li>
<li>src/playlist/PlaylistActions.h <span style="color: grey">(a0b2275)</span></li>
<li>src/playlist/PlaylistActions.cpp <span style="color: grey">(d9284e7)</span></li>
<li>src/playlist/PlaylistController.cpp <span style="color: grey">(f05c250)</span></li>
<li>src/playlist/PlaylistDock.h <span style="color: grey">(f7f1231)</span></li>
<li>src/playlist/PlaylistDock.cpp <span style="color: grey">(6170f78)</span></li>
<li>src/playlist/PlaylistItem.h <span style="color: grey">(6bfeb68)</span></li>
<li>src/playlist/PlaylistModel.h <span style="color: grey">(7b7633f)</span></li>
<li>src/playlist/PlaylistModel.cpp <span style="color: grey">(0162321)</span></li>
<li>src/playlist/PlaylistQueueEditor.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/playlist/PlaylistQueueEditor.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/playlist/PlaylistQueueEditor.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/playlist/navigators/DynamicTrackNavigator.cpp <span style="color: grey">(a1c67e7)</span></li>
<li>src/playlist/navigators/TrackNavigator.h <span style="color: grey">(6154f9a)</span></li>
<li>src/playlist/navigators/TrackNavigator.cpp <span style="color: grey">(2a70b08)</span></li>
<li>src/playlist/proxymodels/AbstractModel.h <span style="color: grey">(03ca4d6)</span></li>
<li>src/playlist/proxymodels/ProxyBase.h <span style="color: grey">(2e6f9e7)</span></li>
<li>src/playlist/proxymodels/ProxyBase.cpp <span style="color: grey">(37cb8cb)</span></li>
<li>src/playlist/view/PlaylistViewCommon.cpp <span style="color: grey">(2a5c9f6)</span></li>
<li>src/playlist/view/listview/PrettyItemDelegate.cpp <span style="color: grey">(5494279)</span></li>
<li>src/playlist/view/listview/PrettyListView.cpp <span style="color: grey">(0b2e41e)</span></li>
<li>src/statusbar/StatusBar.cpp <span style="color: grey">(1a67677)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/100177/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>