Bug 112422 - Mark/unmark compilation does not work for multiselection
Summary: Mark/unmark compilation does not work for multiselection
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: general (show other bugs)
Version: 1.3.1
Platform: Debian stable Linux
: NOR normal
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-09-11 15:15 UTC by Joachim Reichel
Modified: 2006-12-17 13:34 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Enable marking/unmarking multiple items as compilation (7.48 KB, patch)
2006-12-06 21:36 UTC, Tobias Knieper
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joachim Reichel 2005-09-11 15:15:20 UTC
Version:           1.3.1 (using KDE KDE 3.3.2)
Installed from:    Debian stable Packages
Compiler:          g++-3.3 
OS:                Linux

Mark/unmaek compilation does not work if I select several albums and call the context menu. The changes are applied only to some (one?) album, the rest is unchanged.

Note:
KDE 3.3.2, Debian packages
amarok 1.3.1, compiled from source
Comment 1 richlv 2006-07-17 15:14:16 UTC
can't reproduce with a recent svn.
if not reproducible by submitter, probably fixed at some point.
Comment 2 Joachim Reichel 2006-08-02 22:08:12 UTC
The bug still exists with current SVN. BTW: I use sqlite, if that matters ...

While one album is updated, I see lots of the following messages:

amarok: BEGIN: void CollectionDB::updateTags(const QString&, const MetaBundle&, bool)
amarok:   [CollectionDB] [ERROR!] Query returned more than 1 song. Aborting updating metadata
amarok: END__: void CollectionDB::updateTags(const QString&, const MetaBundle&, bool) - Took 0.013s


Comment 3 Joachim Reichel 2006-10-29 00:02:41 UTC
The bug is still present in 1.4.3. Any chance that this will get fixed?
Comment 4 Joachim Reichel 2006-11-12 16:59:38 UTC
*sigh* I had to rescan my whole collection for some reason. Now many of my compilations are listed again under "Various Artists" and I have to go through each one in turn and mark it as "Do not show under various artists".

I'm willing to pay 50 EUR (save charges) to the person (or group) who is fixing this bug. What I expect is the following: Choose "group by album", select a subset of the albums, select "Do not show under various artists", choose "group by artists" and all tracks of these albums are no longer shown under "Various Artists", but under their correponding artist. Or some equivalent procedure. In particular, it has to work with a multi-selection of albums. Using the context-menu for each album in turn is not an option. And of course the reverse ("Show under various artists") has to work in the same way.

Fixing the bug in SVN is fine, however, the money is due when an amarok version containing that bug fix is released. (I hope that the patch will not be removed later for whatever reason.)

One more note: My offer is void if the bug should be already fixed in current SVN.
Comment 5 Tobias Knieper 2006-12-06 21:36:35 UTC
Created attachment 18823 [details]
Enable marking/unmarking multiple items as compilation

This patch does three things:
1) Enable marking/unmarking multiple items as compilation. This fixes bug
#112422.
2) Correct the context menu in collection browser in such way that it only
offers entries which the context menu can in fact handle. The entries
BURN_COMPOSER, BURN_ARTIST, BURN_ALBUM can currently handle only one single
item. If there is a multiselection only the item right under the mouse is
processed. As this is not intuitive this patch offers the context menu items
only if just a single item is selected.
3) Fix two unneeded calls of listSelected() as they were already computed
before.
Comment 6 Martin Aumueller 2006-12-06 22:14:19 UTC
SVN commit 611130 by aumuell:

make (Don't) Show Under Various Artists work when multiple albums are selected
- thanks to Tobias Knieper <tobias.knieper@micekiller.de> for the patch!
BUG: 112422


 M  +3 -0      ChangeLog  


--- trunk/extragear/multimedia/amarok/ChangeLog #611129:611130
@@ -57,6 +57,9 @@
     * Amarok now saves playlists with relative paths by default.
 
   BUGFIXES:
+    * (Don't) Show Under Various Artists would not work when multiple albums
+      are selected. Patch by Tobias Knieper <tobias.knieper@micekiller.de>.
+      (BR 112422)
     * Changed temp download location for Magnatune purchases (BR 137912)
     * Fixed potential double payment issues in the Magnatune store
     * Only synchronize already set values to media devices. (BR 138150)
Comment 7 Martin Aumueller 2006-12-06 22:17:24 UTC
SVN commit 611132 by aumuell:

SVN_SILENT actually commit Tobias Knieper's patch
CCBUG: 112422


 M  +71 -27    collectionbrowser.cpp  
 M  +2 -0      collectionbrowser.h  


--- trunk/extragear/multimedia/amarok/src/collectionbrowser.cpp #611131:611132
@@ -1320,6 +1320,7 @@
                        COMPILATION_SET, COMPILATION_UNSET, ORGANIZE, DELETE, TRASH, FILE_MENU  };
         #endif
         KURL::List selection = listSelected();
+        QStringList siblingSelection = listSelectedSiblingsOf( cat, item );
         menu.insertItem( SmallIconSet( Amarok::icon( "files" ) ), i18n( "&Load" ), MAKE );
         menu.insertItem( SmallIconSet( Amarok::icon( "add_playlist" ) ), i18n( "&Append to Playlist" ), APPEND );
         menu.insertItem( SmallIconSet( Amarok::icon( "queue_track" ) ), selection.count() == 1 ? i18n( "&Queue Track" )
@@ -1336,17 +1337,17 @@
         if( cat == IdArtist )
         {
             menu.insertItem( SmallIconSet( Amarok::icon( "burn" ) ), i18n("Burn All Tracks by This Artist"), BURN_ARTIST );
-            menu.setItemEnabled( BURN_ARTIST, K3bExporter::isAvailable() );
+            menu.setItemEnabled( BURN_ARTIST, K3bExporter::isAvailable() && siblingSelection.count() == 1 );
         }
         else if( cat == IdComposer )
         {
             menu.insertItem( SmallIconSet( Amarok::icon( "burn" ) ), i18n("Burn All Tracks by This Composer"), BURN_COMPOSER );
-            menu.setItemEnabled( BURN_COMPOSER, K3bExporter::isAvailable() );
+            menu.setItemEnabled( BURN_COMPOSER, K3bExporter::isAvailable() && siblingSelection.count() == 1 );
         }
-        else if( cat == IdAlbum || cat == IdVisYearAlbum )
+        else if( (cat == IdAlbum || cat == IdVisYearAlbum) )
         {
             menu.insertItem( SmallIconSet( Amarok::icon( "burn" ) ), i18n("Burn This Album"), BURN_ALBUM );
-            menu.setItemEnabled( BURN_ALBUM, K3bExporter::isAvailable() );
+            menu.setItemEnabled( BURN_ALBUM, K3bExporter::isAvailable() && siblingSelection.count() == 1 );
         }
         // !item->isExpandable() in tree mode corresponds to
         // showing tracks in iPod mode
@@ -1377,28 +1378,14 @@
 
         menu.insertItem( SmallIconSet( Amarok::icon( "files" ) ), i18n("Manage Files"), &fileMenu, FILE_MENU );
 
-        if ( cat == IdAlbum || cat == IdVisYearAlbum ) {
+        if ( (cat == IdAlbum || cat == IdVisYearAlbum) && siblingSelection.count() > 0 ) {
             menu.insertSeparator();
             menu.insertItem( SmallIconSet( "ok" ), i18n( "Show under &Various Artists" ), COMPILATION_SET );
             menu.insertItem( SmallIconSet( "cancel" ), i18n( "&Do not Show under Various Artists" ), COMPILATION_UNSET );
         }
 
-        QString trueItemText;
+        QString trueItemText = getTrueItemText( cat, item );
 
-        //Work out the true name of the album ( where Unknown is "" ) , and the
-        if ( dynamic_cast<CollectionItem*>( item ) )
-        {
-            CollectionItem* collectItem = static_cast<CollectionItem*>( item );
-            trueItemText = collectItem->getSQLText( 0 );
-            if ( cat == IdVisYearAlbum && !collectItem->isUnknown() )
-                trueItemText = trueItemText.right( trueItemText.length() - trueItemText.find( i18n( " - " ) ) - i18n( " - " ).length() );
-        }
-        else
-        {
-            trueItemText = item->text( 0 );
-            warning() << "RMB pressed for non-CollectionItem with text '" << trueItemText << '\'' << endl;
-        }
-
         switch( menu.exec( point ) )
         {
             case APPEND:
@@ -1443,20 +1430,23 @@
                 K3bExporter::instance()->exportTracks( selection );
                 break;
             case COMPILATION_SET:
-                setCompilation( trueItemText, true );
+                for ( QStringList::Iterator it = siblingSelection.begin(); it != siblingSelection.end(); ++it ) {
+                    setCompilation( *it, true );
+                }
                 break;
             case COMPILATION_UNSET:
-                setCompilation( trueItemText, false );
+                for ( QStringList::Iterator it = siblingSelection.begin(); it != siblingSelection.end(); ++it ) {
+                    setCompilation( *it, false );
+                }
                 break;
             case ORGANIZE:
-                organizeFiles( listSelected(), i18n( "Organize Collection Files" ), false /* do not add to collection, just move */ );
+                organizeFiles( selection, i18n( "Organize Collection Files" ), false /* do not add to collection, just move */ );
                 break;
             case DELETE:
-                KURL::List files = listSelected();
-                if ( DeleteDialog::showTrashDialog(this, files) )
+                if ( DeleteDialog::showTrashDialog(this, selection) )
                   {
-                    CollectionDB::instance()->removeSongs( files );
-                    foreachType( KURL::List, files )
+                    CollectionDB::instance()->removeSongs( selection );
+                    foreachType( KURL::List, selection )
                       CollectionDB::instance()->emitFileDeleted( (*it).path() );
                   }
                 m_dirty = true;
@@ -2001,7 +1991,61 @@
     d->dragCopy();
 }
 
+QString
+CollectionView::getTrueItemText( int cat, QListViewItem* item ) const
+{
+    //Work out the true name of the album ( where Unknown is "" ) , and the
+    QString trueItemText;
+    if ( item == 0 )
+    {
+        warning() << "getTrueItemText() called for empty CollectionItem" << endl;
+        return QString::null;
+    }
+    if ( dynamic_cast<CollectionItem*>( item ) )
+    {
+        CollectionItem* collectItem = static_cast<CollectionItem*>( item );
+        trueItemText = collectItem->getSQLText( 0 );
+        if ( cat == IdVisYearAlbum && !collectItem->isUnknown() )
+            trueItemText = trueItemText.right( trueItemText.length() - trueItemText.find( i18n( " - " ) ) - i18n( " - " ).length() );
+    }
+    else
+    {
+        trueItemText = item->text( 0 );
+        warning() << "getTrueItemText() called for non-CollectionItem with text '" << trueItemText << '\'' << endl;
+    }
+    return trueItemText;
+}
 
+QStringList
+CollectionView::listSelectedSiblingsOf( int cat, QListViewItem* item )
+{
+    // notice that using the nextSibling()-axis does not work in this case as this
+    // would only select items below the specified item.
+    QStringList list;
+    QString trueItemText;
+    int depth = item->depth();
+
+    // go to top most item
+    while( item && item->itemAbove() )
+    {
+        item = item->itemAbove();
+        //debug() << "walked up to item: " << getTrueItemText( cat, item ) << endl;
+    }
+    // walk down to get all selected items in same depth
+    while( item )
+    {
+        if ( item->isSelected() && item->depth() == depth )
+        {
+            trueItemText = getTrueItemText( cat, item );
+            //debug() << "selected item: " << trueItemText << endl;
+            if( !trueItemText.isEmpty() ) // filter out "Unknown"
+                list << trueItemText;
+        }
+        item = item->itemBelow();
+    }
+    return list;
+}
+
 KURL::List
 CollectionView::listSelected()
 {
--- trunk/extragear/multimedia/amarok/src/collectionbrowser.h #611131:611132
@@ -284,6 +284,8 @@
         void removeDuplicatedHeaders();
 
         void startDrag();
+        QString getTrueItemText( int, QListViewItem* ) const;
+        QStringList listSelectedSiblingsOf( int, QListViewItem* );
         KURL::List listSelected();
 
         void playlistFromURLs( const KURL::List &urls );
Comment 8 Tobias Knieper 2006-12-06 22:36:41 UTC
Joachim, the patch got commited and should be contained in the next release. Feel free to donate the money to the amarok team (have a look at the donate button at http://amarok.kde.org).

regards,
tobias
Comment 9 Joachim Reichel 2006-12-17 13:34:54 UTC
The patch works as expected. So I will donate the money to the amarok team when the next version contained this patch is released. Thanks.