Review Request: Save selected tracks to already saved playlist or a new playlist

[Bart Cerneels]

> On 2010-12-01 08:31:06, Bart Cerneels wrote:
> >
> 
> Bart Cerneels wrote:
>     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
> 
> Dennis Francis wrote:
>     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.
> 
> Bart Cerneels wrote:
>     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.
>

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


- Bart


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100181/#review449
-----------------------------------------------------------


On 2010-12-01 01:06:25, Dennis Francis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100181/
> -----------------------------------------------------------
> 
> (Updated 2010-12-01 01:06:25)
> 
> 
> Review request for Amarok.
> 
> 
> Summary
> -------
> 
> 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 )
> 
> 
> This addresses bugs 186545, 211811 and 239950.
>     https://bugs.kde.org/show_bug.cgi?id=186545
>     https://bugs.kde.org/show_bug.cgi?id=211811
>     https://bugs.kde.org/show_bug.cgi?id=239950
> 
> 
> Diffs
> -----
> 
>   src/browsers/playlistbrowser/UserPlaylistModel.cpp 2178d27 
>   src/core-impl/collections/mediadevicecollection/playlist/MediaDeviceUserPlaylistProvider.h e1ae132 
>   src/core-impl/collections/mediadevicecollection/playlist/MediaDeviceUserPlaylistProvider.cpp 12d3b46 
>   src/core-impl/playlists/providers/user/UserPlaylistProvider.h 609e1a8 
>   src/core/playlists/PlaylistFormat.h b93180d 
>   src/core/playlists/PlaylistFormat.cpp 6b3cb6b 
>   src/core/playlists/PlaylistProvider.h 4cc3417 
>   src/core/playlists/PlaylistProvider.cpp 2d37e8b 
>   src/playlist/PlaylistDock.h 6e13e7f 
>   src/playlist/PlaylistModel.h 3ad8030 
>   src/playlist/PlaylistModel.cpp 1acbc9f 
>   src/playlist/view/PlaylistViewCommon.h 9582b1b 
>   src/playlist/view/PlaylistViewCommon.cpp db300a3 
>   src/playlist/view/listview/PrettyListView.h f22a7c8 
>   src/playlist/view/listview/PrettyListView.cpp 062c584 
>   src/playlistmanager/PlaylistManager.h 943fcf1 
>   src/playlistmanager/PlaylistManager.cpp fb13ea7 
>   src/playlistmanager/file/PlaylistFileProvider.h bd19f79 
>   src/playlistmanager/file/PlaylistFileProvider.cpp 22e8abe 
>   src/playlistmanager/sql/SqlUserPlaylistProvider.h 3a5a62e 
>   src/playlistmanager/sql/SqlUserPlaylistProvider.cpp d2ae5b9 
> 
> Diff: http://git.reviewboard.kde.org/r/100181/diff
> 
> 
> Testing
> -------
> 
> It works fine foe me.
> 
> 
> Thanks,
> 
> Dennis
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/amarok-devel/attachments/20101202/b72742bc/attachment-0001.htm