Bug 218527 - Delete key must remove only selected items in saved playlists.
Summary: Delete key must remove only selected items in saved playlists.
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: Playlists/Saved Playlists (show other bugs)
Version: 2.3-GIT
Platform: Ubuntu Linux
: NOR major (vote)
Target Milestone: 2.2.2
Assignee: Amarok Developers
URL:
Keywords: release_blocker
Depends on:
Blocks:
 
Reported: 2009-12-13 15:50 UTC by Maxim Dikun
Modified: 2009-12-22 11:03 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maxim Dikun 2009-12-13 15:50:13 UTC
Version:           2.2.1 (using KDE 4.3.4)
OS:                Linux
Installed from:    Ubuntu Packages

Expand some playlist in saved playlists tree.
Select more than 1 item.
Press Delete key.
We have 1 deleted item instead of all selected.
And also we have wrong selection.
Comment 1 Myriam Schweingruber 2009-12-13 15:58:53 UTC
Indeed, it asks for confirmation before deleting, and it deletes the last items in the list instead of the selected ones.
Comment 2 Maxim Dikun 2009-12-13 16:19:39 UTC
I mean pressing Delete on concrete items (songs) in playlist.
In this case in doesn't asks anything and deletes first item.
Comment 3 Myriam Schweingruber 2009-12-13 19:12:14 UTC
(In reply to comment #2)
> I mean pressing Delete on concrete items (songs) in playlist.
> In this case in doesn't asks anything and deletes first item.

No, this works well in Amarok 2.2.-git already, it's not working when one selects more than one saved playlist, then it starts removing the latest in the list.
Comment 4 Bart Cerneels 2009-12-22 11:02:03 UTC
commit 484d99240ba76fcf072e94031b88a5eb5996b848
Author: Bart Cerneels <bart.cerneels@kde.org>
Date:   Tue Dec 22 10:40:14 2009 +0100

    Fix removing multiple tracks from a playlist.
    
    Also might solve deletion of multiple playlists.
    BUG:218527

diff --git a/ChangeLog b/ChangeLog
index de0942e..f46a6aa 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -77,6 +77,8 @@ VERSION 2.2.2
       PowerTOP when using the Xine backend.
 
   BUGFIXES:
+    * Fixed removing multiple tracks from a saved playlist only removing the first track.
+      (BR 218527)
     * Fixed cancel button not responding when loading thumbnails in the cover 
       manager. (BR 204882)
     * Fixed crash when using the inline playlist editor to resize items containing
diff --git a/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp b/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp
index 110a4e7..59217ba 100644
--- a/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp
+++ b/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp
@@ -42,10 +42,11 @@ bool
 PlaylistsInGroupsProxy::removeRows( int row, int count, const QModelIndex &parent )
 {
     DEBUG_BLOCK
+    bool result;
     debug() << "in parent " << parent << "remove " << count << " starting at row " << row;
     if( !parent.isValid() )
     {
-        QModelIndex folderIdx = index( row, 0, parent );
+        QModelIndex folderIdx = index( row, 0, QModelIndex() );
         if( isGroup( folderIdx ) )
         {
             deleteFolder( folderIdx );
@@ -53,27 +54,35 @@ PlaylistsInGroupsProxy::removeRows( int row, int count, const QModelIndex &paren
         }
 
         //is a playlist not in a folder
-        QModelIndex childIdx = mapToSource( index( row, 0, parent ) );
-        return m_model->removeRow( childIdx.row(), QModelIndex() );
+        //FIXME: before confirming deletion of a playlist it already dissapears from the
+        //view. The beginRemoveRows should not be called here but in the source model.
+        QModelIndex childIdx = mapToSource( index( row, 0, QModelIndex() ) );
+        beginRemoveRows( QModelIndex(), row, row + count );
+        result = m_model->removeRows( childIdx.row(), count, QModelIndex() );
+        endRemoveRows();
+        return result;
     }
 
     if( isGroup( parent ) )
     {
-        bool success = true;
+        result = true;
         for( int i = row; i < row + count; i++ )
         {
             //individually remove all children of this group in the source model
             QModelIndex childIdx = mapToSource( index( i, 0, parent ) );
             //set success to false if removeRows returns false
-            success =
-                m_model->removeRow( childIdx.row(), QModelIndex() ) ? success : false;
+            result =
+                m_model->removeRow( childIdx.row(), QModelIndex() ) ? result : false;
         }
-        return success;
+        return result;
     }
 
     //removing a track from a playlist
+    beginRemoveRows( parent, row, row + count );
     QModelIndex originalIdx = mapToSource( parent );
-    return m_model->removeRows( row, count, originalIdx );
+    result = m_model->removeRows( row, count, originalIdx );
+    endRemoveRows();
+    return result;
 }
 
 QStringList
diff --git a/src/browsers/playlistbrowser/QtGroupingProxy.cpp b/src/browsers/playlistbrowser/QtGroupingProxy.cpp
index e5ac775..40b71d6 100755
--- a/src/browsers/playlistbrowser/QtGroupingProxy.cpp
+++ b/src/browsers/playlistbrowser/QtGroupingProxy.cpp
@@ -85,8 +85,10 @@ QtGroupingProxy::belongsTo( const QModelIndex &idx )
 }
 
 /* m_groupHash layout
-*  group index in m_groupMaps: originalRow in m_model of child Playlists (no sub-groups yet)
-*  group index = -1  for non-grouped playlists
+*  key : index of the group in m_groupMaps
+*  values : original row in m_model of children of this group
+*
+*  key = -1  contains non-grouped indexes
 *
 * TODO: sub-groups
 */
@@ -548,7 +550,9 @@ QtGroupingProxy::modelRowsRemoved( const QModelIndex& index, int start, int end
     Q_UNUSED( index )
     Q_UNUSED( start )
     Q_UNUSED( end )
-    buildTree();
+    //don't call buildTree() because it will mess up with removing multiple rows
+    //individually using removeRow().
+    //TODO: this breaks the view when rows are not deleted manually.
 }
 
 void
Comment 5 Bart Cerneels 2009-12-22 11:03:11 UTC
commit 484d99240ba76fcf072e94031b88a5eb5996b848
Author: Bart Cerneels <bart.cerneels@kde.org>
Date:   Tue Dec 22 10:40:14 2009 +0100

    Fix removing multiple tracks from a playlist.
    
    Also might solve deletion of multiple playlists.
    BUG:218527

diff --git a/ChangeLog b/ChangeLog
index de0942e..f46a6aa 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -77,6 +77,8 @@ VERSION 2.2.2
       PowerTOP when using the Xine backend.
 
   BUGFIXES:
+    * Fixed removing multiple tracks from a saved playlist only removing the first track.
+      (BR 218527)
     * Fixed cancel button not responding when loading thumbnails in the cover 
       manager. (BR 204882)
     * Fixed crash when using the inline playlist editor to resize items containing
diff --git a/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp b/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp
index 110a4e7..59217ba 100644
--- a/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp
+++ b/src/browsers/playlistbrowser/PlaylistsInGroupsProxy.cpp
@@ -42,10 +42,11 @@ bool
 PlaylistsInGroupsProxy::removeRows( int row, int count, const QModelIndex &parent )
 {
     DEBUG_BLOCK
+    bool result;
     debug() << "in parent " << parent << "remove " << count << " starting at row " << row;
     if( !parent.isValid() )
     {
-        QModelIndex folderIdx = index( row, 0, parent );
+        QModelIndex folderIdx = index( row, 0, QModelIndex() );
         if( isGroup( folderIdx ) )
         {
             deleteFolder( folderIdx );
@@ -53,27 +54,35 @@ PlaylistsInGroupsProxy::removeRows( int row, int count, const QModelIndex &paren
         }
 
         //is a playlist not in a folder
-        QModelIndex childIdx = mapToSource( index( row, 0, parent ) );
-        return m_model->removeRow( childIdx.row(), QModelIndex() );
+        //FIXME: before confirming deletion of a playlist it already dissapears from the
+        //view. The beginRemoveRows should not be called here but in the source model.
+        QModelIndex childIdx = mapToSource( index( row, 0, QModelIndex() ) );
+        beginRemoveRows( QModelIndex(), row, row + count );
+        result = m_model->removeRows( childIdx.row(), count, QModelIndex() );
+        endRemoveRows();
+        return result;
     }
 
     if( isGroup( parent ) )
     {
-        bool success = true;
+        result = true;
         for( int i = row; i < row + count; i++ )
         {
             //individually remove all children of this group in the source model
             QModelIndex childIdx = mapToSource( index( i, 0, parent ) );
             //set success to false if removeRows returns false
-            success =
-                m_model->removeRow( childIdx.row(), QModelIndex() ) ? success : false;
+            result =
+                m_model->removeRow( childIdx.row(), QModelIndex() ) ? result : false;
         }
-        return success;
+        return result;
     }
 
     //removing a track from a playlist
+    beginRemoveRows( parent, row, row + count );
     QModelIndex originalIdx = mapToSource( parent );
-    return m_model->removeRows( row, count, originalIdx );
+    result = m_model->removeRows( row, count, originalIdx );
+    endRemoveRows();
+    return result;
 }
 
 QStringList
diff --git a/src/browsers/playlistbrowser/QtGroupingProxy.cpp b/src/browsers/playlistbrowser/QtGroupingProxy.cpp
index e5ac775..40b71d6 100755
--- a/src/browsers/playlistbrowser/QtGroupingProxy.cpp
+++ b/src/browsers/playlistbrowser/QtGroupingProxy.cpp
@@ -85,8 +85,10 @@ QtGroupingProxy::belongsTo( const QModelIndex &idx )
 }
 
 /* m_groupHash layout
-*  group index in m_groupMaps: originalRow in m_model of child Playlists (no sub-groups yet)
-*  group index = -1  for non-grouped playlists
+*  key : index of the group in m_groupMaps
+*  values : original row in m_model of children of this group
+*
+*  key = -1  contains non-grouped indexes
 *
 * TODO: sub-groups
 */
@@ -548,7 +550,9 @@ QtGroupingProxy::modelRowsRemoved( const QModelIndex& index, int start, int end
     Q_UNUSED( index )
     Q_UNUSED( start )
     Q_UNUSED( end )
-    buildTree();
+    //don't call buildTree() because it will mess up with removing multiple rows
+    //individually using removeRow().
+    //TODO: this breaks the view when rows are not deleted manually.
 }
 
 void