Bug 250750 - Cancel delete of playlist initialised by 'Delete' key not clean
Summary: Cancel delete of playlist initialised by 'Delete' key not clean
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: Playlists/Saved Playlists (show other bugs)
Version: 2.4-GIT
Platform: Compiled Sources Linux
: NOR major
Target Milestone: 2.4.0
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-10 10:02 UTC by James Duncan
Modified: 2010-11-22 10:42 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.4


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description James Duncan 2010-09-10 10:02:49 UTC
Version:           2.3.2-GIT (using KDE 4.4.2) 
OS:                Linux

When I tried to delete two playlists by selecting the first and then Shift-selecting the second (note, the first is now expanded so the tracks of the first are also included in the selection, but the second it not expanded), then pressing 'Delete' on the keyboard, several things happened.  First, many of the visible tracks under the first playlist disappeared and several blank rows were added beneath the remaining ones.  Second, the delete playlist confirmation dialog appeared, but instead of asking to confirm the delete of two playlists, it asks in the singular (this counting issue is a separately reported bug).  When I clicked 'Cancel', the tracklist for the first playlist did not revert.  Upon restarting Amarok, I discovered that every third track, starting with the first, was missing (i.e., the 1st, 4th, 7th, 10th, etc.).  Trying the process again, every third track (again, starting with the first) of the remaining tracks were missing.  I discovered that this also happens when the playlist plus some of its tracks beneath it are selected and deleted in the same manner.  When only part of the tracklist is selected, the number of tracks deleted is the max of the number of tracks selected or the number of tracks in the tracklist / 3 rounded up (and they are removed in order: 1, 4, 7, etc...).

Reproducible: Always

Steps to Reproduce:
1. In a fresh start, open 'Saved Playlists' then click on either of the two categories, opening it up.
2. Click on any of the playlists, then shift-click one after it (the first playlist, plus its tracklist, plus all of the playlists (unopened) up to the second one clicked should be selected).
3. Press 'Delete' (on the keyboard).
4. Click 'Cancel'.

Actual Results:  
Tracks from the first selected playlist disappear (and the remaining ones are now accompanied by empty rows), and then, upon restarting Amarok, every third track starting from, and including, the first should be removed from the first selected playlist.

Expected Results:  
Nothing should change.  Since the delete was canceled, the state of the database/playlist files should not change.
Comment 1 James Duncan 2010-09-10 10:03:34 UTC
I don't know how "major" this is, but it does cause unintended data loss, so I marked it as such...
Comment 2 Myriam Schweingruber 2010-09-10 15:10:22 UTC
Major is OK.
For the GUI row issues, see bug 250432
Comment 3 Dennis 2010-11-19 22:09:09 UTC
Hello, I have submitted a patch for this issue in the review-board.
Comment 4 Bart Cerneels 2010-11-21 11:50:58 UTC
commit 22c4f116fbb9ab10650884d181dbe4e25ae2221e
branch master
Author: Bart Cerneels <bart.cerneels@kde.org>
Date:   Sun Nov 21 11:46:59 2010 +0100

    Fix playlsit delete bugs.
    
    Using the playlist actions instead of removeRow().
    
    Patch by Dennis Francis.
    
    Removed the expandToDepth( 0 ) hack because comment #6 in BR 245646.
    
    BUG:250746
    BUG:250750

diff --git a/src/browsers/playlistbrowser/PlaylistBrowserView.cpp b/src/browsers/playlistbrowser/PlaylistBrowserView.cpp
index 2603e2f..45be175 100644
--- a/src/browsers/playlistbrowser/PlaylistBrowserView.cpp
+++ b/src/browsers/playlistbrowser/PlaylistBrowserView.cpp
@@ -263,8 +263,21 @@ PlaylistBrowserNS::PlaylistBrowserView::keyPressEvent( QKeyEvent *event )
     {
         case Qt::Key_Delete:
         {
-            foreach( const QModelIndex &selectedIdx, selectedIndexes() )
-                model()->removeRow( selectedIdx.row(), selectedIdx.parent() );
+            QModelIndexList indices = selectedIndexes();
+            QActionList actions = actionsFor( indices );
+
+            if( actions.isEmpty() )
+            {
+                debug() <<"No actions !";
+                return;
+            }
+            foreach( QAction *actn, actions )
+                if( actn )
+                    if( actn->objectName() == "deleteAction" )
+                    {
+                        actn->trigger();
+                        actn->setData( QVariant() );
+                    }
             return;
         }
      }
diff --git a/src/core-impl/podcasts/sql/SqlPodcastProvider.cpp b/src/core-impl/podcasts/sql/SqlPodcastProvider.cpp
index ae61504..bf679fc 100644
--- a/src/core-impl/podcasts/sql/SqlPodcastProvider.cpp
+++ b/src/core-impl/podcasts/sql/SqlPodcastProvider.cpp
@@ -336,6 +336,7 @@ SqlPodcastProvider::playlistActions( Playlists::PlaylistPtr playlist )
     {
         actionChannels = m_removeAction->data().value<Podcasts::SqlPodcastChannelList>();
     }
+    m_removeAction->setObjectName( "deleteAction" );
 
     actionChannels << sqlChannel;
     m_removeAction->setData( QVariant::fromValue( actionChannels ) );
@@ -392,6 +393,7 @@ SqlPodcastProvider::trackActions( Playlists::PlaylistPtr playlist, int trackInde
         m_deleteAction->setProperty( "popupdropper_svg_id", "delete" );
         connect( m_deleteAction, SIGNAL( triggered() ), SLOT( slotDeleteDownloadedEpisodes() ) );
     }
+    m_deleteAction->setObjectName( "deleteAction" );
 
     if( m_writeTagsAction == 0 )
     {
diff --git a/src/playlistmanager/file/PlaylistFileProvider.cpp b/src/playlistmanager/file/PlaylistFileProvider.cpp
index 573cdff..22e8abe 100644
--- a/src/playlistmanager/file/PlaylistFileProvider.cpp
+++ b/src/playlistmanager/file/PlaylistFileProvider.cpp
@@ -138,6 +138,7 @@ PlaylistFileProvider::playlistActions( Playlists::PlaylistPtr playlist )
         m_deleteAction->setProperty( "popupdropper_svg_id", "delete" );
         connect( m_deleteAction, SIGNAL( triggered() ), SLOT( slotDelete() ) );
     }
+    m_deleteAction->setObjectName( "deleteAction" );
 
     Playlists::PlaylistFileList actionList =
             m_deleteAction->data().value<Playlists::PlaylistFileList>();
@@ -181,6 +182,7 @@ PlaylistFileProvider::trackActions( Playlists::PlaylistPtr playlist, int trackIn
                                              "Remove From \"%1\"", playlist->name() ) );
     }
 
+    m_removeTrackAction->setObjectName( "deleteAction" );
     //Add the playlist/track combination to a QMultiMap that is stored in the action.
     //In the slot we use this data to remove that track from the playlist.
     PlaylistTrackMap playlistMap = m_removeTrackAction->data().value<PlaylistTrackMap>();
diff --git a/src/playlistmanager/sql/SqlUserPlaylistProvider.cpp b/src/playlistmanager/sql/SqlUserPlaylistProvider.cpp
index 12da6ed..d2ae5b9 100644
--- a/src/playlistmanager/sql/SqlUserPlaylistProvider.cpp
+++ b/src/playlistmanager/sql/SqlUserPlaylistProvider.cpp
@@ -178,6 +178,7 @@ SqlUserPlaylistProvider::playlistActions( Playlists::PlaylistPtr playlist )
         m_deleteAction->setProperty( "popupdropper_svg_id", "delete" );
         connect( m_deleteAction, SIGNAL( triggered() ), SLOT( slotDelete() ) );
     }
+    m_deleteAction->setObjectName( "deleteAction" );
 
     Playlists::SqlPlaylistList actionList = m_deleteAction->data().value<Playlists::SqlPlaylistList>();
     actionList << sqlPlaylist;
@@ -204,6 +205,8 @@ SqlUserPlaylistProvider::trackActions( Playlists::PlaylistPtr playlist, int trac
         m_removeTrackAction->setProperty( "popupdropper_svg_id", "delete" );
         connect( m_removeTrackAction, SIGNAL( triggered() ), SLOT( slotRemove() ) );
     }
+    m_removeTrackAction->setObjectName( "deleteAction" );
+
     //Add the playlist/track combination to a QMultiMap that is stored in the action.
     //In the slot we use this data to remove that track from the playlist.
     PlaylistTrackMap playlistMap = m_removeTrackAction->data().value<PlaylistTrackMap>();