Commit d0bd4c43 authored by Matěj Laitl's avatar Matěj Laitl
Browse files

[Single]CollectionTreeItemModel[Base]: first look at cleanups/corrections

This is the first part of much-needed revisiting of the Collection Browser
and associated models. The final goal is to get rid of the unfortunate
expanding/scrolling behaviour and of the caching bug. That will involve
more code floating around this commit just brings some fixes,
deduplications and corrections.

This should fix bug 262504, but needs confirmation from folks that were
able to reproduce this bug. Reporters, please reopen if you can reproduce
this with recent git or 2.8 Beta (to be released really soon)

BUG: 262504
FIXED-IN: 2.8
parent 08efd243
......@@ -63,6 +63,7 @@ VERSION 2.8-Beta 1
not to fool users.
BUGFIXES:
* Make Collection Browser update listing correctly when metadata change. (BR 262504)
* Fix crash when disabling the Free Music Charts script. (BR 321329)
* Albums having same name but different album artist won't be mixed together on the
playlist if sorting by album is enabled.
......
......@@ -40,61 +40,19 @@
CollectionTreeItemModel::CollectionTreeItemModel( const QList<CategoryId::CatMenuId> &levelType )
: CollectionTreeItemModelBase()
{
CollectionManager* collMgr = CollectionManager::instance();
m_rootItem = new CollectionTreeItem( this );
CollectionManager *collMgr = CollectionManager::instance();
connect( collMgr, SIGNAL(collectionAdded(Collections::Collection*)), this, SLOT(collectionAdded(Collections::Collection*)), Qt::QueuedConnection );
connect( collMgr, SIGNAL(collectionRemoved(QString)), this, SLOT(collectionRemoved(QString)) );
//delete m_rootItem; //clears the whole tree!
m_rootItem = new CollectionTreeItem( this );
m_collections.clear();
QList<Collections::Collection*> collections = CollectionManager::instance()->viewableCollections();
foreach( Collections::Collection *coll, collections )
{
connect( coll, SIGNAL(updated()), this, SLOT(slotFilter()) ) ;
m_collections.insert( coll->collectionId(), CollectionRoot( coll, new CollectionTreeItem( coll, m_rootItem, this ) ) );
}
//m_rootItem->setChildrenLoaded( true ); //children of the root item are the collection items
updateHeaderText();
setLevels( levelType );
debug() << "Collection root has " << m_rootItem->childCount() << " children";
}
CollectionTreeItemModel::~CollectionTreeItemModel()
{
DEBUG_BLOCK
KConfigGroup config = Amarok::config( "Collection Browser" );
QList<int> levelNumbers;
foreach( CategoryId::CatMenuId category, levels() )
levelNumbers.append( category );
config.writeEntry( "TreeCategory", levelNumbers );
}
void
CollectionTreeItemModel::setLevels( const QList<CategoryId::CatMenuId> &levelType )
{
if( m_levelType == levelType && m_rootItem )
return;
m_levelType = levelType;
delete m_rootItem; //clears the whole tree!
m_rootItem = new CollectionTreeItem( this );
m_collections.clear();
QList<Collections::Collection*> collections = CollectionManager::instance()->viewableCollections();
QList<Collections::Collection *> collections = CollectionManager::instance()->viewableCollections();
foreach( Collections::Collection *coll, collections )
{
connect( coll, SIGNAL(updated()), this, SLOT(slotFilter()) ) ;
m_collections.insert( coll->collectionId(), CollectionRoot( coll, new CollectionTreeItem( coll, m_rootItem, this ) ) );
}
m_rootItem->setRequiresUpdate( false ); //all collections have been loaded already
updateHeaderText();
m_expandedItems.clear();
m_expandedSpecialNodes.clear();
m_runningQueries.clear();
m_childQueries.clear();
m_compilationQueries.clear();
reset();
if( m_collections.count() == 1 )
QTimer::singleShot( 0, this, SLOT(requestCollectionsExpansion()) );
setLevels( levelType );
}
Qt::ItemFlags
......@@ -230,8 +188,6 @@ CollectionTreeItemModel::supportedDropActions() const
void
CollectionTreeItemModel::collectionAdded( Collections::Collection *newCollection )
{
DEBUG_BLOCK
if( !newCollection )
return;
......@@ -241,8 +197,6 @@ CollectionTreeItemModel::collectionAdded( Collections::Collection *newCollection
if( m_collections.contains( collectionId ) )
return;
debug() << "Added collection id:" << collectionId;
//inserts new collection at the end.
beginInsertRows( QModelIndex(), m_rootItem->childCount(), m_rootItem->childCount() );
m_collections.insert( collectionId, CollectionRoot( newCollection, new CollectionTreeItem( newCollection, m_rootItem, this ) ) );
......@@ -255,10 +209,6 @@ CollectionTreeItemModel::collectionAdded( Collections::Collection *newCollection
void
CollectionTreeItemModel::collectionRemoved( const QString &collectionId )
{
DEBUG_BLOCK
debug() << "Removed collection id:" << collectionId;
int count = m_rootItem->childCount();
for( int i = 0; i < count; i++ )
{
......@@ -280,9 +230,6 @@ CollectionTreeItemModel::filterChildren()
int count = m_rootItem->childCount();
for ( int i = 0; i < count; i++ )
{
//CollectionTreeItem *item = m_rootItem->child( i );
//if( item )
// item->setChildrenLoaded( false );
markSubTreeAsDirty( m_rootItem->child( i ) );
ensureChildrenLoaded( m_rootItem->child( i ) );
}
......@@ -291,22 +238,10 @@ CollectionTreeItemModel::filterChildren()
void
CollectionTreeItemModel::requestCollectionsExpansion()
{
DEBUG_BLOCK
for( int i = 0, count = m_rootItem->childCount(); i < count; i++ )
{
emit expandIndex( createIndex( i, 0, m_rootItem->child( i ) ) );
}
}
void CollectionTreeItemModel::update()
{
for( int i = 0; i < m_rootItem->childCount(); i++ )
{
markSubTreeAsDirty( m_rootItem->child( i ) );
ensureChildrenLoaded( m_rootItem->child( i ) );
}
}
#include "CollectionTreeItemModel.moc"
......@@ -35,7 +35,6 @@ class CollectionTreeItemModel: public CollectionTreeItemModelBase
public:
CollectionTreeItemModel( const QList<CategoryId::CatMenuId> &levelType );
~CollectionTreeItemModel();
/* QAbstractItemModel methods */
virtual Qt::ItemFlags flags( const QModelIndex &index ) const;
......@@ -46,9 +45,6 @@ class CollectionTreeItemModel: public CollectionTreeItemModelBase
virtual void fetchMore( const QModelIndex &parent );
virtual Qt::DropActions supportedDropActions() const;
/* CollectionTreeItemModelBase methods */
virtual void setLevels( const QList<CategoryId::CatMenuId> &levelType );
public slots:
virtual void collectionAdded( Collections::Collection *newCollection );
virtual void collectionRemoved( const QString &collectionId );
......@@ -58,9 +54,7 @@ class CollectionTreeItemModel: public CollectionTreeItemModelBase
virtual int levelModifier() const { return 0; }
private slots:
virtual void requestCollectionsExpansion();
void update();
void requestCollectionsExpansion();
};
#endif
......@@ -71,7 +71,14 @@ CollectionTreeItemModelBase::CollectionTreeItemModelBase( )
CollectionTreeItemModelBase::~CollectionTreeItemModelBase()
{
delete m_rootItem;
KConfigGroup config = Amarok::config( "Collection Browser" );
QList<int> levelNumbers;
foreach( CategoryId::CatMenuId category, levels() )
levelNumbers.append( category );
config.writeEntry( "TreeCategory", levelNumbers );
if( m_rootItem )
m_rootItem->deleteLater();
}
Qt::ItemFlags CollectionTreeItemModelBase::flags(const QModelIndex & index) const
......@@ -479,7 +486,7 @@ CollectionTreeItemModelBase::ensureChildrenLoaded( CollectionTreeItem *item )
QIcon
CollectionTreeItemModelBase::iconForLevel(int level) const
{
switch( m_levelType[level] )
switch( m_levelType.value( level ) )
{
case CategoryId::Album :
return KIcon( "media-optical-amarok" );
......@@ -509,7 +516,7 @@ void CollectionTreeItemModelBase::listForLevel(int level, Collections::QueryMake
if( m_runningQueries.contains( parent ) )
return;
// - check if we are finished
// following special cases are handled extra - right when the parent is added
if( level > m_levelType.count() ||
parent->isVariousArtistItem() ||
parent->isNoLabelItem() )
......@@ -529,39 +536,26 @@ void CollectionTreeItemModelBase::listForLevel(int level, Collections::QueryMake
if( level + 1 >= m_levelType.count() )
nextLevel = Collections::QueryMaker::Track;
else
nextLevel = mapCategoryToQueryType( m_levelType [ level + 1 ] );
nextLevel = mapCategoryToQueryType( m_levelType.value( level + 1 ) );
qm->setQueryType( mapCategoryToQueryType( m_levelType[ level ] ) );
qm->setQueryType( mapCategoryToQueryType( m_levelType.value( level ) ) );
switch( m_levelType[level] )
switch( m_levelType.value( level ) )
{
case CategoryId::Album :
//restrict query to normal albums if the previous level
//was the artist category. in that case we handle compilations below
if( level > 0 &&
( m_levelType[level-1] == CategoryId::Artist ||
m_levelType[level-1] == CategoryId::AlbumArtist
) &&
!parent->isVariousArtistItem() )
{
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::Artist :
case CategoryId::AlbumArtist:
//handle compilations only if the next level ist CategoryId::Album
if( nextLevel == Collections::QueryMaker::Album )
{
handleCompilations( parent );
qm->setAlbumQueryMode( Collections::QueryMaker::OnlyNormalAlbums );
}
// 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;
}
......@@ -580,6 +574,22 @@ void CollectionTreeItemModelBase::listForLevel(int level, Collections::QueryMake
}
}
void
CollectionTreeItemModelBase::setLevels( const QList<CategoryId::CatMenuId> &levelType )
{
if( m_levelType == levelType )
return;
m_levelType = levelType;
updateHeaderText();
m_expandedItems.clear();
m_expandedSpecialNodes.clear();
m_runningQueries.clear();
m_childQueries.clear();
m_compilationQueries.clear();
filterChildren();
}
Collections::QueryMaker::QueryType
CollectionTreeItemModelBase::mapCategoryToQueryType( int levelType ) const
{
......@@ -743,8 +753,6 @@ CollectionTreeItemModelBase::newResultReady( Meta::DataList data )
void
CollectionTreeItemModelBase::handleSpecialQueryResult( CollectionTreeItem::Type type, Collections::QueryMaker *qm, const Meta::DataList &dataList )
{
DEBUG_BLOCK
debug() << "Received special data: " << dataList.count();
CollectionTreeItem *parent = 0;
if( type == CollectionTreeItem::VariousArtist )
......@@ -876,76 +884,81 @@ CollectionTreeItemModelBase::handleNormalQueryResult( Collections::QueryMaker *q
}
void
CollectionTreeItemModelBase::populateChildren(const DataList & dataList, CollectionTreeItem * parent, const QModelIndex &parentIndex )
CollectionTreeItemModelBase::populateChildren( const DataList &dataList, CollectionTreeItem *parent, const QModelIndex &parentIndex )
{
//add new rows after existing ones here (which means all artists nodes
//will be inserted after the "Various Artists" node)
CategoryId::CatMenuId childCategory = levelCategory( parent->level() );
// add new rows after existing ones here (which means all artists nodes
// will be inserted after the "Various Artists" node)
// figure out which children of parent have to be removed,
// which new children have to be added, and preemptively emit dataChanged for the rest
// have to check how that influences performance...
const QSet<Meta::DataPtr> dataSet = dataList.toSet();
QSet<Meta::DataPtr> childrenSet;
foreach( CollectionTreeItem *child, parent->children() )
{
// we don't add null children, these are special-cased below
if( !child->data() )
continue;
childrenSet.insert( child->data() );
}
const QSet<Meta::DataPtr> dataToBeAdded = dataSet - childrenSet;
const QSet<Meta::DataPtr> dataToBeRemoved = childrenSet - dataSet;
// first remove all rows that have to be removed
// walking through the cildren in reverse order does not screw up the order
for( int i = parent->childCount() - 1; i >= 0; i-- )
{
//figure out which children of parent have to be removed,
//which new children have to be added, and preemptively emit dataChanged for the rest
//have to check how that influences performance...
QHash<Meta::DataPtr, int> dataToIndex;
QSet<Meta::DataPtr> childrenSet;
foreach( CollectionTreeItem *child, parent->children() )
CollectionTreeItem *child = parent->child( i );
// should this child be removed?
bool toBeRemoved;
if( child->isDataItem() )
toBeRemoved = dataToBeRemoved.contains( child->data() );
else if( child->isVariousArtistItem() )
toBeRemoved = childCategory != CategoryId::AlbumArtist;
else if( child->isNoLabelItem() )
toBeRemoved = childCategory != CategoryId::Label;
else
{
if( child->isVariousArtistItem() )
continue;
childrenSet.insert( child->data() );
dataToIndex.insert( child->data(), child->row() );
warning() << "Unknown child type encountered in populateChildren(), removing";
toBeRemoved = true;
}
QSet<Meta::DataPtr> dataSet = dataList.toSet();
QSet<Meta::DataPtr> dataToBeAdded = dataSet - childrenSet;
QSet<Meta::DataPtr> dataToBeRemoved = childrenSet - dataSet;
QList<int> currentIndices;
//first remove all rows that have to be removed
//walking through the cildren in reverse order does not screw up the order
for( int i = parent->childCount() - 1; i >= 0; i-- )
{
CollectionTreeItem *child = parent->child( i );
bool toBeRemoved = child->isDataItem() && !child->isVariousArtistItem() && dataToBeRemoved.contains( child->data() ) ;
if( toBeRemoved )
{
currentIndices.append( i );
}
//make sure we remove the rows if the first row (we are using reverse order) has to be removed too!
if( ( !toBeRemoved || i == 0 ) && !currentIndices.isEmpty() )
{
//be careful in which order you insert the rows above!!
beginRemoveRows( parentIndex, currentIndices.last(), currentIndices.first() );
foreach( int i, currentIndices )
{
parent->removeChild( i );
}
endRemoveRows();
currentIndices.clear();
}
}
//hopefully the view has figured out that we've removed rows yet!
int lastRow = parent->childCount() - 1;
if( lastRow >= 0 )
if( toBeRemoved )
{
emit dataChanged( createIndex( 0, 0, parent->child( 0 ) ), createIndex( lastRow, 0, parent->child( lastRow ) ) );
beginRemoveRows( parentIndex, i, i );
parent->removeChild( i );
endRemoveRows();
}
//the remainging child items may be dirty, so refresh them
foreach( CollectionTreeItem *item, parent->children() )
else
{
if( item->isDataItem() && item->data() && m_expandedItems.contains( item->data() ) )
ensureChildrenLoaded( item );
// the remainging child items may be dirty, so refresh them
if( child->isDataItem() && child->data() && m_expandedItems.contains( child->data() ) )
ensureChildrenLoaded( child );
// tell the view that the existing children may have changed
QModelIndex idx = index( i, 0, parentIndex );
emit dataChanged( idx, idx );
}
//add the new rows
if( !dataToBeAdded.isEmpty() )
}
// add the new rows
if( !dataToBeAdded.isEmpty() )
{
int lastRow = parent->childCount() - 1;
//the above check ensures that Qt does not crash on beginInsertRows ( because lastRow+1 > lastRow+0)
beginInsertRows( parentIndex, lastRow + 1, lastRow + dataToBeAdded.count() );
foreach( Meta::DataPtr data, dataToBeAdded )
{
//the above check ensures that Qt does not crash on beginInsertRows ( because lastRow+1 > lastRow+0)
beginInsertRows( parentIndex, lastRow + 1, lastRow + dataToBeAdded.count() );
foreach( Meta::DataPtr data, dataToBeAdded )
{
new CollectionTreeItem( data, parent, this );
}
endInsertRows();
new CollectionTreeItem( data, parent, this );
}
parent->setRequiresUpdate( false );
endInsertRows();
}
parent->setRequiresUpdate( false );
}
void
......@@ -959,31 +972,38 @@ CollectionTreeItemModelBase::updateHeaderText()
}
QString
CollectionTreeItemModelBase::nameForLevel(int level) const
CollectionTreeItemModelBase::nameForLevel( int level ) const
{
switch( m_levelType[level] )
switch( m_levelType.value( level ) )
{
case CategoryId::Album : return AmarokConfig::showYears() ? i18n( "Year - Album" ) : i18n( "Album" );
case CategoryId::Artist : return i18n( "Artist" );
case CategoryId::AlbumArtist: return i18n( "Album Artist" );
case CategoryId::Composer : return i18n( "Composer" );
case CategoryId::Genre : return i18n( "Genre" );
case CategoryId::Year : return i18n( "Year" );
case CategoryId::Label : return i18n( "Label" );
default: return QString();
case CategoryId::Album:
return AmarokConfig::showYears() ? i18n( "Year - Album" ) : i18n( "Album" );
case CategoryId::Artist:
return i18n( "Artist" );
case CategoryId::AlbumArtist:
return i18n( "Album Artist" );
case CategoryId::Composer:
return i18n( "Composer" );
case CategoryId::Genre:
return i18n( "Genre" );
case CategoryId::Year:
return i18n( "Year" );
case CategoryId::Label:
return i18n( "Label" );
default:
return QString();
}
}
void
CollectionTreeItemModelBase::handleCompilations( CollectionTreeItem *parent ) const
CollectionTreeItemModelBase::handleCompilations( Collections::QueryMaker::QueryType queryType, CollectionTreeItem *parent ) const
{
//this method will be called when we retrieve a list of artists from the database.
//we have to query for all compilations, and then add a "Various Artists" node if at least
//one compilation exists
// this method will be called when we retrieve a list of artists from the database.
// we have to query for all compilations, and then add a "Various Artists" node if at least
// one compilation exists
Collections::QueryMaker *qm = parent->queryMaker();
qm->setQueryType( queryType );
qm->setAlbumQueryMode( Collections::QueryMaker::OnlyCompilations );
qm->setQueryType( Collections::QueryMaker::Album );
for( CollectionTreeItem *tmp = parent; tmp; tmp = tmp->parent() )
tmp->addMatch( qm, levelCategory( tmp->level() - 1 ) );
......@@ -996,7 +1016,6 @@ CollectionTreeItemModelBase::handleCompilations( CollectionTreeItem *parent ) co
void
CollectionTreeItemModelBase::handleTracksWithoutLabels( Collections::QueryMaker::QueryType queryType, CollectionTreeItem *parent ) const
{
DEBUG_BLOCK
Collections::QueryMaker *qm = parent->queryMaker();
qm->setQueryType( queryType );
qm->setLabelQueryMode( Collections::QueryMaker::OnlyWithoutLabels );
......@@ -1108,11 +1127,6 @@ CollectionTreeItemModelBase::slotExpanded( const QModelIndex &index )
}
}
void CollectionTreeItemModelBase::update()
{
reset();
}
void CollectionTreeItemModelBase::markSubTreeAsDirty( CollectionTreeItem *item )
{
//tracks are the leaves so they are never dirty
......@@ -1163,8 +1177,9 @@ CollectionTreeItemModelBase::hasRunningQueries() const
CategoryId::CatMenuId
CollectionTreeItemModelBase::levelCategory( const int level ) const
{
if( level >= 0 && level + levelModifier() < m_levelType.count() )
return m_levelType[ level + levelModifier() ];
const int actualLevel = level + levelModifier();
if( actualLevel >= 0 && actualLevel < m_levelType.count() )
return m_levelType.at( actualLevel );
return CategoryId::None;
}
......
......@@ -72,8 +72,7 @@ class AMAROK_EXPORT CollectionTreeItemModelBase : public QAbstractItemModel
virtual QIcon iconForLevel( int level ) const;
virtual void listForLevel( int level, Collections::QueryMaker *qm, CollectionTreeItem* parent );
virtual void setLevels( const QList<CategoryId::CatMenuId> &levelType ) = 0;
virtual void setLevels( const QList<CategoryId::CatMenuId> &levelType );
virtual QList<CategoryId::CatMenuId> levels() const { return m_levelType; }
virtual CategoryId::CatMenuId levelCategory( const int level ) const;
......@@ -118,7 +117,6 @@ class AMAROK_EXPORT CollectionTreeItemModelBase : public QAbstractItemModel
void slotFilter();
void slotCollapsed( const QModelIndex &index );
void slotExpanded( const QModelIndex &index );
private:
......@@ -140,7 +138,7 @@ class AMAROK_EXPORT CollectionTreeItemModelBase : public QAbstractItemModel
void markSubTreeAsDirty( CollectionTreeItem *item );
/** Initiates a special search for albums without artists */
void handleCompilations( CollectionTreeItem *parent ) const;
void handleCompilations( Collections::QueryMaker::QueryType queryType, CollectionTreeItem *parent ) const;
/** Initiates a special search for tracks without label */
void handleTracksWithoutLabels( Collections::QueryMaker::QueryType queryType, CollectionTreeItem *parent ) const;
......@@ -174,8 +172,6 @@ class AMAROK_EXPORT CollectionTreeItemModelBase : public QAbstractItemModel
protected slots:
void startAnimationTick();
void loadingAnimationTick();
void update();
};
......
......@@ -31,35 +31,15 @@
SingleCollectionTreeItemModel::SingleCollectionTreeItemModel( Collections::Collection *collection,
const QList<CategoryId::CatMenuId> &levelType )
:CollectionTreeItemModelBase( )
: m_collection( collection )
{
m_collection = collection;
//we only have one collection that, by its very nature, is always expanded
m_expandedCollections.insert( m_collection );
setLevels( levelType );
connect( collection, SIGNAL(updated()), this, SLOT(slotFilter()) ) ;
}
void
SingleCollectionTreeItemModel::setLevels( const QList<CategoryId::CatMenuId> &levelType )
{
if( m_levelType == levelType && m_rootItem )
return;
delete m_rootItem; //clears the whole tree!
m_levelType = levelType;
m_rootItem = new CollectionTreeItem( m_collection, 0, this );
connect( collection, SIGNAL(updated()), this, SLOT(slotFilter()) ) ;
m_collections.insert( m_collection->collectionId(), CollectionRoot( m_collection, m_rootItem ) );
//we only have one collection that, by its very nature, is always expanded
m_expandedCollections.insert( m_collection );
updateHeaderText();
m_expandedItems.clear();
m_expandedSpecialNodes.clear();
m_runningQueries.clear();
m_childQueries.clear();
m_compilationQueries.clear();
reset(); //resets the whole model, as the data changed
setLevels( levelType );
}
QVariant
......@@ -109,4 +89,3 @@ SingleCollectionTreeItemModel::filterChildren()
}
#include "SingleCollectionTreeItemModel.moc"
......@@ -43,7 +43,6 @@ class AMAROK_EXPORT SingleCollectionTreeItemModel: public CollectionTreeItemMode
virtual QVariant data(const QModelIndex &index, int role) const;
virtual bool canFetchMore( const QModelIndex &parent ) const;
virtual void fetchMore( const QModelIndex &parent );
virtual void setLevels( const QList<CategoryId::CatMenuId> &levelType );
virtual Qt::ItemFlags flags(const QModelIndex &index) const;
protected:
......