[amarok] src/browsers: Collection Browser: revert to showing Various Artists even under Artist level [RFC]

[Matěj Laitl]

Git commit c336d3777393a177c340f487d14508a6788e519a by Matěj Laitl.
Committed on 26/06/2013 at 19:54.
Pushed by laitl into branch 'master'.

Collection Browser: revert to showing Various Artists even under Artist level [RFC]

I don't like this, because it is illogical and shows some of your tracks twice:
http://picpaste.com/amarok-artist-level-shows-track-twice.png

...but the previous change alone was too disruptive for our users.

I suggest the following:
 a) show showing "Various Artists" under "Artist" level and therefore
    stop showing some tracks twice. Show "Various Artists" only under the
    "Album Artist" level where it logically belongs
 b) tweak the default presets to use "Album Artist" where they previously
    used "Arist", except for "Album / Artist" where it doesn't make sense
 c) (optional) adapt user configs treat "artist" as "album artist" for
    users migrating from older Amarok releases
 d) (optional) rename "Artist" level to "Track Artist" make the
    distinction more clear
 e) (optional) find better icon for "Album Artist" items/use "Artist"
    icon, Sentynel finds the current one way too fugly

^^^ I think that the points above combined would mean nearly no
disruption in user experience and would make the interface more logical
and correct.

What do you think? Do you have some objections?

CCMAIL: amarok-devel at kde.org

M  +15   -19   src/browsers/CollectionTreeItemModelBase.cpp

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

diff --git a/src/browsers/CollectionTreeItemModelBase.cpp b/src/browsers/CollectionTreeItemModelBase.cpp
index ad78c96..b811c34 100644
--- a/src/browsers/CollectionTreeItemModelBase.cpp
+++ b/src/browsers/CollectionTreeItemModelBase.cpp
@@ -54,6 +54,8 @@ inline uint qHash( const Meta::DataPtr &data )
     return qHash( data.data() );
 }
 
+static const QSet<CategoryId::CatMenuId> variousArtistCategories =
+        QSet<CategoryId::CatMenuId>() << CategoryId::AlbumArtist << CategoryId::Artist;
 
 CollectionTreeItemModelBase::CollectionTreeItemModelBase( )
     : QAbstractItemModel()
@@ -540,25 +542,20 @@ void CollectionTreeItemModelBase::listForLevel(int level, Collections::QueryMake
 
             qm->setQueryType( mapCategoryToQueryType( m_levelType.value( level ) ) );
 
-            switch( m_levelType.value( level ) )
+            CategoryId::CatMenuId category = m_levelType.value( level );
+            if( category == CategoryId::Album )
             {
-                case CategoryId::Album:
-                    // restrict query to normal albums if the previous level
-                    // was the AlbumArtist category. In that case we handle compilations below
-                    if( levelCategory( level - 1 ) == CategoryId::AlbumArtist )
-                        qm->setAlbumQueryMode( Collections::QueryMaker::OnlyNormalAlbums );
-                    break;
-                case CategoryId::AlbumArtist:
-                    // we used to handleCompilations() only if nextLevel is Album, but I cannot
-                    // tell any reason why we should have done this --- strohel
-                    handleCompilations( nextLevel, parent );
-                    break;
-                case CategoryId::Label:
-                    handleTracksWithoutLabels( nextLevel, parent );
-                    break;
-                default : //TODO handle error condition. return tracks?
-                    break;
+                // restrict query to normal albums if the previous level
+                // was the AlbumArtist category. In that case we handle compilations below
+                if( levelCategory( level - 1 ) == CategoryId::AlbumArtist )
+                    qm->setAlbumQueryMode( Collections::QueryMaker::OnlyNormalAlbums );
             }
+            else if( variousArtistCategories.contains( category ) )
+                // we used to handleCompilations() only if nextLevel is Album, but I cannot
+                // tell any reason why we should have done this --- strohel
+                handleCompilations( nextLevel, parent );
+            else if( category == CategoryId::Label )
+                handleTracksWithoutLabels( nextLevel, parent );
         }
 
         for( CollectionTreeItem *tmp = parent; tmp; tmp = tmp->parent() )
@@ -641,7 +638,6 @@ CollectionTreeItemModelBase::addQueryMaker( CollectionTreeItem* item,
     m_runningQueries.insert( item, qm );
 }
 
-
 void
 CollectionTreeItemModelBase::queryDone()
 {
@@ -918,7 +914,7 @@ CollectionTreeItemModelBase::populateChildren( const DataList &dataList, Collect
         if( child->isDataItem() )
             toBeRemoved = dataToBeRemoved.contains( child->data() );
         else if( child->isVariousArtistItem() )
-            toBeRemoved = childCategory != CategoryId::AlbumArtist;
+            toBeRemoved = !variousArtistCategories.contains( childCategory );
         else if( child->isNoLabelItem() )
             toBeRemoved = childCategory != CategoryId::Label;
         else