Bug 318782

Summary:	Too many saved playlist tracks/podcasts are associated with a performed (context menu/button) action
Product:	[Applications] amarok	Reporter:	Daniel Faust <hessijames>
Component:	Playlists/Saved Playlists	Assignee:	Amarok Developers <amarok-bugs-dist>
Status:	RESOLVED FIXED
Severity:	critical	CC:	bart.cerneels, grosser.meister.morti, matej, maxime.haselbauer, myriam, ralf-engels
Priority:	VHI	Keywords:	regression, release_blocker
Version:	2.7-git
Target Milestone:	2.8
Platform:	Other
OS:	Linux
Latest Commit:	http://commits.kde.org/amarok/d3982dc6c2fa2953c37d4864bca61e73aff19a52	Version Fixed In:	2.8
Sentry Crash Report:

Description Daniel Faust 2013-04-23 21:11:26 UTC

I tried to fix the bug myself but this goes a little bit too deep for me.
It's like this:
When right-clicking on a playlist or playlist track, PlaylistBrowserNS::PlaylistBrowserView::actionsFor( QModelIndexList indexes ) lists all actions (QAction) for the selected items. The actions have associated playlists and tracks as a QMultiMap (Playlists::PlaylistList) saved in the data member.
The data gets filled in by PlaylistBrowserModel::actionsFor( const QModelIndex &idx ) and the respective PlaylistProvider::playlistActions( Playlists::PlaylistPtr playlist ) and PlaylistProvider::trackActions( Playlists::PlaylistPtr playlist, int trackIndex ) functions.
These three functions however only grow the data of the actions. See: m_appendAction, m_deleteAction and m_removeTrackAction (last two in Playlists::UserPlaylistProvider). So when opening another context menu for different items, the previously selected items are sill associated with the action.

This leads to confusing and possibly dangerous behavior of the actions. Appending too many tracks to the playlist or deleting too many tracks or playlists.

You can also see this bug by just right-clicking on a saved playlist tracks and look at the delete entry. For me it sometimes shows wrong numbers in the delete text, also when selecting delete in a playlist context menu, the appearing warning dialog shows wrong numbers as well.

It seems to have been introduced by commit 7c3a4313 although the commit is rather old and I didn't notice the bug until recently.

I hope my description makes it in fact easier to understand and not more complicated ;)

Reproducible: Always

Comment 1 Matěj Laitl 2013-04-24 11:28:42 UTC

Ralf, please act!

Myriam Schweingruber wrote:
> As of Amarok v2.7.0-108-g6729d98 when marking one podcast channel or episode
> as read this applies to all channels. Only if all channels are makred as read, setting
> one episode as unread will only apply to this episode.

Matěj Laitl wrote:
> Ralf, this is caused by your recent cleanups of Playlist, Podcast and Collection views,
> models and delegates, especially the removal of weird static actions handling. Note
> that this is reproducible also in Saved Playlists, not just podcasts.

Comment 2 Matěj Laitl 2013-04-24 11:49:29 UTC

*** Bug 315896 has been marked as a duplicate of this bug. ***

Comment 3 Matěj Laitl 2013-04-24 11:49:35 UTC

*** Bug 316551 has been marked as a duplicate of this bug. ***

Comment 4 Myriam Schweingruber 2013-05-06 12:18:53 UTC

Currently, double-clicking on a saved playlist creates an infinite loop and tries to repeatedly add all tracks from the saved playlist, making Amarok totally unresponsive.

Double-clicking on one Podcast episode causes all episodes for the subscription to be added multiple times, resulting in playlists as big as 120.00 tracks!

Really, this is one of the worst bugs I have ever seen...

Comment 5 Matěj Laitl 2013-05-06 12:23:10 UTC

Just correcting this is not actually an infinite loop (which would crash/stall Amarok indefinitely). Ralf, gimme a day or 2, I may have something.

Comment 6 Matěj Laitl 2013-05-06 21:53:01 UTC

Git commit d3982dc6c2fa2953c37d4864bca61e73aff19a52 by Matěj Laitl.
Committed on 06/05/2013 at 22:48.
Pushed by laitl into branch 'master'.

Fix Saved Playlists and Podcasts actions

After 2 failed attempts to solve this non-intrusively, I realized the
obvious: the actions belong to the view, not the model! (Qt has no
Model/View/Controller, it has Model/View+Controller)

After this it was "easy", just a fair bit of work to move relevant
actions to PlaylistBrowserModel and ensure they work correctly. Some
playlist (and podcast) actions weren't really appropriate for
PlaylistBrowserModel. Methods to get actions from these needed to be
changed, because the old way simply didn't allow reliable action
execution.

When I was at it, I cleaned up a fair bit of things in podcasts (and
playlist providers) - I haven't seen such code bloat in a while -
100-line-long unused function isn't anything surprising there. I'm
pointing at you, Bart. :-)

This regression exists since Ralf's cleanups, although Ralf certainly
didn't introduce it - he just broke the delicate equilibrium of hacks
that kept it working. (and I still think it was working just by chance)

This doesn't go into ChangeLog as it was introduced post-2.7. The code
is not perfect yet (we have lost "Create Empty Playlist" if you clink
into empty space), but the bug this fixes is much more critical.
FIXED-IN: 2.8
CCMAIL: amarok-devel@kde.org

M  +12   -103  src/browsers/playlistbrowser/PlaylistBrowserModel.cpp
M  +6    -14   src/browsers/playlistbrowser/PlaylistBrowserModel.h
M  +351  -69   src/browsers/playlistbrowser/PlaylistBrowserView.cpp
M  +36   -21   src/browsers/playlistbrowser/PlaylistBrowserView.h
M  +13   -68   src/browsers/playlistbrowser/PodcastModel.cpp
M  +2    -9    src/browsers/playlistbrowser/PodcastModel.h
M  +13   -30   src/core-impl/collections/ipodcollection/IpodPlaylistProvider.cpp
M  +4    -5    src/core-impl/collections/ipodcollection/IpodPlaylistProvider.h
M  +2    -62   src/core-impl/collections/mediadevicecollection/playlist/MediaDeviceUserPlaylistProvider.cpp
M  +3    -18   src/core-impl/collections/mediadevicecollection/playlist/MediaDeviceUserPlaylistProvider.h
M  +48   -46   src/core-impl/collections/umscollection/podcasts/UmsPodcastProvider.cpp
M  +4    -6    src/core-impl/collections/umscollection/podcasts/UmsPodcastProvider.h
M  +1    -218  src/core-impl/playlists/providers/user/UserPlaylistProvider.cpp
M  +17   -64   src/core-impl/playlists/providers/user/UserPlaylistProvider.h
M  +1    -1    src/core-impl/playlists/types/file/PlaylistFile.h
M  +83   -283  src/core-impl/podcasts/sql/SqlPodcastProvider.cpp
M  +3    -6    src/core-impl/podcasts/sql/SqlPodcastProvider.h
M  +0    -17   src/core/playlists/Playlist.cpp
M  +0    -12   src/core/playlists/Playlist.h
M  +43   -4    src/core/playlists/PlaylistProvider.cpp
M  +50   -19   src/core/playlists/PlaylistProvider.h
M  +1    -26   src/core/podcasts/PodcastProvider.h
M  +1    -1    src/playlistmanager/PlaylistManager.cpp
M  +0    -18   src/playlistmanager/SyncedPlaylist.cpp
M  +0    -3    src/playlistmanager/SyncedPlaylist.h
M  +7    -2    src/playlistmanager/file/PlaylistFileProvider.cpp
M  +3    -9    src/playlistmanager/file/PlaylistFileProvider.h
M  +2    -34   src/playlistmanager/sql/SqlUserPlaylistProvider.cpp
M  +2    -8    src/playlistmanager/sql/SqlUserPlaylistProvider.h
M  +17   -29   src/services/gpodder/GpodderProvider.cpp
M  +3    -2    src/services/gpodder/GpodderProvider.h
M  +1    -1    tests/playlistmanager/file/TestPlaylistFileProvider.cpp
M  +1    -1    tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp

http://commits.kde.org/amarok/d3982dc6c2fa2953c37d4864bca61e73aff19a52

Comment 7 Myriam Schweingruber 2013-06-11 09:14:14 UTC

*** Bug 321007 has been marked as a duplicate of this bug. ***