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.

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.


- 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/f0105dd6/attachment.htm