Skip to content
Permalink
Browse files

expand browser items in threads

  • Loading branch information
blazek committed Nov 12, 2014
1 parent 078a3ab commit 40bb180d9aefc14bd989bae7ba2e33f5aecdc314
@@ -79,12 +79,13 @@ class QgsDataItem : QObject
QgsDataItem* parent() const;
void setParent( QgsDataItem* parent );
QVector<QgsDataItem*> children() const;
QIcon icon() const;
QIcon icon();
QString name() const;
QString path() const;
void setPath( const QString );

void setIcon( QIcon icon );
void setIconName( const QString & icon );

void setToolTip( QString msg );
QString toolTip() const;
@@ -429,9 +429,9 @@ void QgsBrowserDockWidget::removeFavourite()

void QgsBrowserDockWidget::refresh()
{
QApplication::setOverrideCursor( Qt::WaitCursor );
//QApplication::setOverrideCursor( Qt::WaitCursor );
refreshModel( QModelIndex() );
QApplication::restoreOverrideCursor();
//QApplication::restoreOverrideCursor();
}

void QgsBrowserDockWidget::refreshModel( const QModelIndex& index )
@@ -488,6 +488,7 @@ void QgsBrowserDockWidget::addLayer( QgsLayerItem *layerItem )

void QgsBrowserDockWidget::addLayerAtIndex( const QModelIndex& index )
{
QgsDebugMsg( QString( "rowCount() = %1" ).arg( mModel->rowCount( mProxyModel->mapToSource( index ) ) ) );
QgsDataItem *item = mModel->dataItem( mProxyModel->mapToSource( index ) );

if ( item != NULL && item->type() == QgsDataItem::Layer )
@@ -708,6 +709,7 @@ void QgsBrowserDockWidget::itemExpanded( const QModelIndex& index )

void QgsBrowserDockWidget::expandPath( QString path )
{
return; // debug
QgsDebugMsg( "path = " + path );

if ( !mModel || !mProxyModel )
@@ -15,6 +15,7 @@
#include <QDir>
#include <QApplication>
#include <QStyle>
#include <QtConcurrentMap>

#include "qgis.h"
#include "qgsapplication.h"
@@ -28,6 +29,30 @@

#include <QSettings>

QgsBrowserWatcher::QgsBrowserWatcher( QgsDataItem * item )
: mFinished( false )
, mItem( item )
{
connect( &mFutureWatcher, SIGNAL( finished() ), SLOT( finished() ) );
}

QgsBrowserWatcher::~QgsBrowserWatcher()
{
mFutureWatcher.cancel();
}

void QgsBrowserWatcher::setFuture( QFuture<QVector <QgsDataItem*> > future )
{
mFutureWatcher.setFuture( future );
}

void QgsBrowserWatcher::finished()
{
QgsDebugMsg( "Entered" );
emit finished( mItem, mFutureWatcher.result() );
mFinished = true;
}

// sort function for QList<QgsDataItem*>, e.g. sorted/grouped provider listings
static bool cmpByDataItemName_( QgsDataItem* a, QgsDataItem* b )
{
@@ -41,6 +66,9 @@ QgsBrowserModel::QgsBrowserModel( QObject *parent )
{
connect( QgsProject::instance(), SIGNAL( readProject( const QDomDocument & ) ), this, SLOT( updateProjectHome() ) );
connect( QgsProject::instance(), SIGNAL( writeProject( QDomDocument & ) ), this, SLOT( updateProjectHome() ) );
mLoadingMovie.setFileName( QgsApplication::iconPath( "/mIconLoading.gif" ) );
mLoadingMovie.setCacheMode( QMovie::CacheAll );
connect( &mLoadingMovie, SIGNAL( frameChanged( int ) ), SLOT( loadingFrameChanged() ) );
addRootItems();
}

@@ -219,6 +247,10 @@ QVariant QgsBrowserModel::data( const QModelIndex &index, int role ) const
}
else if ( role == Qt::DecorationRole && index.column() == 0 )
{
if ( fetching( item ) )
{
return mLoadingIcon;
}
return item->icon();
}
else
@@ -241,7 +273,7 @@ QVariant QgsBrowserModel::headerData( int section, Qt::Orientation orientation,

int QgsBrowserModel::rowCount( const QModelIndex &parent ) const
{
//qDebug("rowCount: idx: (valid %d) %d %d", parent.isValid(), parent.row(), parent.column());
//QgsDebugMsg(QString("isValid = %1 row = %2 column = %3").arg(parent.isValid()).arg(parent.row()).arg(parent.column()));

if ( !parent.isValid() )
{
@@ -252,6 +284,7 @@ int QgsBrowserModel::rowCount( const QModelIndex &parent ) const
{
// ordinary item: number of its children
QgsDataItem *item = dataItem( parent );
//if ( item ) QgsDebugMsg(QString("path = %1 rowCount = %2").arg(item->path()).arg(item->rowCount()) );
return item ? item->rowCount() : 0;
}
}
@@ -323,18 +356,6 @@ void QgsBrowserModel::reload()
endResetModel();
}

/* Refresh dir path */
void QgsBrowserModel::refresh( QString path )
{
QModelIndex idx = findPath( path );
if ( idx.isValid() )
{
QgsDataItem* item = dataItem( idx );
if ( item )
item->refresh();
}
}

QModelIndex QgsBrowserModel::index( int row, int column, const QModelIndex &parent ) const
{
QgsDataItem *p = dataItem( parent );
@@ -369,43 +390,32 @@ QModelIndex QgsBrowserModel::findItem( QgsDataItem *item, QgsDataItem *parent )
return QModelIndex();
}

/* Refresh item */
void QgsBrowserModel::refresh( const QModelIndex& theIndex )
{
QgsDataItem *item = dataItem( theIndex );
if ( !item )
return;

QgsDebugMsg( "Refresh " + item->path() );
item->refresh();
}

void QgsBrowserModel::beginInsertItems( QgsDataItem *parent, int first, int last )
{
QgsDebugMsg( "parent mPath = " + parent->path() );
QgsDebugMsgLevel( "parent mPath = " + parent->path(), 3 );
QModelIndex idx = findItem( parent );
if ( !idx.isValid() )
return;
QgsDebugMsg( "valid" );
QgsDebugMsgLevel( "valid", 3 );
beginInsertRows( idx, first, last );
QgsDebugMsg( "end" );
QgsDebugMsgLevel( "end", 3 );
}
void QgsBrowserModel::endInsertItems()
{
QgsDebugMsg( "Entered" );
QgsDebugMsgLevel( "Entered", 3 );
endInsertRows();
}
void QgsBrowserModel::beginRemoveItems( QgsDataItem *parent, int first, int last )
{
QgsDebugMsg( "parent mPath = " + parent->path() );
QgsDebugMsgLevel( "parent mPath = " + parent->path(), 3 );
QModelIndex idx = findItem( parent );
if ( !idx.isValid() )
return;
beginRemoveRows( idx, first, last );
}
void QgsBrowserModel::endRemoveItems()
{
QgsDebugMsg( "Entered" );
QgsDebugMsgLevel( "Entered", 3 );
endRemoveRows();
}
void QgsBrowserModel::connectItem( QgsDataItem* item )
@@ -478,10 +488,122 @@ bool QgsBrowserModel::canFetchMore( const QModelIndex & parent ) const

void QgsBrowserModel::fetchMore( const QModelIndex & parent )
{
QgsDebugMsg( "Entered" );
QgsDataItem* item = dataItem( parent );
if ( item )
item->populate();

if ( !item || fetching( item ) )
return;

QgsDebugMsg( "path = " + item->path() );

if ( item->isPopulated() )
return;

QList<QgsDataItem*> itemList;
itemList << item;
QgsBrowserWatcher * watcher = new QgsBrowserWatcher( item );
connect( watcher, SIGNAL( finished( QgsDataItem*, QVector <QgsDataItem*> ) ), SLOT( childrenCreated( QgsDataItem*, QVector <QgsDataItem*> ) ) );
watcher->setFuture( QtConcurrent::mapped( itemList, QgsBrowserModel::createChildren ) );
mWatchers.append( watcher );
mLoadingMovie.setPaused( false );
emit dataChanged( parent, parent );
}

/* Refresh dir path */
void QgsBrowserModel::refresh( QString path )
{
QModelIndex index = findPath( path );
refresh( index );
}

/* Refresh item */
void QgsBrowserModel::refresh( const QModelIndex& theIndex )
{
QgsDataItem *item = dataItem( theIndex );
if ( !item )
return;

QgsDebugMsg( "Refresh " + item->path() );
//item->refresh();

QList<QgsDataItem*> itemList;
itemList << item;
QgsBrowserWatcher * watcher = new QgsBrowserWatcher( item );
connect( watcher, SIGNAL( finished( QgsDataItem*, QVector <QgsDataItem*> ) ), SLOT( refreshChildrenCreated( QgsDataItem*, QVector <QgsDataItem*> ) ) );
watcher->setFuture( QtConcurrent::mapped( itemList, QgsBrowserModel::createChildren ) );
mWatchers.append( watcher );
mLoadingMovie.setPaused( false );
emit dataChanged( theIndex, theIndex );
}

// This is expected to be run in a separate thread
QVector<QgsDataItem*> QgsBrowserModel::createChildren( QgsDataItem* item )
{
QgsDebugMsg( "Entered" );
QVector <QgsDataItem*> children = item->createChildren();
// Children objects must be pushed to main thread.
foreach ( QgsDataItem* child, children )
{
if ( !child ) // should not happen
continue;
// However it seems to work without resetting parent, the Qt doc says that
// "The object cannot be moved if it has a parent."
QgsDebugMsg( "moveToThread child" + child->path() );
child->setParent( 0 );
child->moveToThread( QApplication::instance()->thread() );
child->setParent( item );
}
return children;
}

void QgsBrowserModel::childrenCreated( QgsDataItem* item, QVector <QgsDataItem*> children )
{
QgsDebugMsg( QString( "children.size() = %1" ).arg( children.size() ) );
QModelIndex index = findItem( item );
if ( !index.isValid() ) // check if item still exists
return;
item->populate( children );
emit dataChanged( index, index );
}

void QgsBrowserModel::refreshChildrenCreated( QgsDataItem* item, QVector <QgsDataItem*> children )
{
QgsDebugMsg( QString( "path = %1 children.size() = %2" ).arg( item->path() ).arg( children.size() ) );
QModelIndex index = findItem( item );
if ( !index.isValid() ) // check if item still exists
return;
item->refresh( children );
emit dataChanged( index, index );
}

bool QgsBrowserModel::fetching( QgsDataItem* item ) const
{
foreach ( QgsBrowserWatcher * watcher, mWatchers )
{
if ( !watcher->isFinished() && watcher->item() == item )
return true;
}
return false;
}

void QgsBrowserModel::loadingFrameChanged()
{
mLoadingIcon = QIcon( mLoadingMovie.currentPixmap() );
int notFinished = 0;
foreach ( QgsBrowserWatcher * watcher, mWatchers )
{
if ( watcher->isFinished() )
{
mWatchers.removeOne( watcher );
continue;
}
QModelIndex index = findItem( watcher->item() );
QgsDebugMsg( QString( "path = %1 not finished" ).arg( watcher->item()->path() ) );
emit dataChanged( index, index );
notFinished++;
}
if ( notFinished == 0 )
mLoadingMovie.setPaused( true );
}

void QgsBrowserModel::addFavouriteDirectory( QString favDir )
@@ -18,9 +18,36 @@
#include <QAbstractItemModel>
#include <QIcon>
#include <QMimeData>
#include <QMovie>
#include <QFuture>
#include <QFutureWatcher>

#include "qgsdataitem.h"

class CORE_EXPORT QgsBrowserWatcher : public QObject
{
Q_OBJECT

public:
QgsBrowserWatcher( QgsDataItem * item );
~QgsBrowserWatcher();

void setFuture( QFuture<QVector <QgsDataItem*> > future );
bool isFinished() { return mFinished; }
QgsDataItem* item() const { return mItem; }

signals:
void finished( QgsDataItem* item, QVector <QgsDataItem*> items );

public slots:
void finished();

private:
bool mFinished;
QgsDataItem *mItem;
QFutureWatcher<QVector <QgsDataItem*> > mFutureWatcher;
};

class CORE_EXPORT QgsBrowserModel : public QAbstractItemModel
{
Q_OBJECT
@@ -92,6 +119,8 @@ class CORE_EXPORT QgsBrowserModel : public QAbstractItemModel

bool canFetchMore( const QModelIndex & parent ) const;
void fetchMore( const QModelIndex & parent );
static QVector<QgsDataItem*> createChildren( QgsDataItem *item );
bool fetching( QgsDataItem *item ) const;

public slots:
// Reload the whole model
@@ -105,6 +134,9 @@ class CORE_EXPORT QgsBrowserModel : public QAbstractItemModel
void removeFavourite( const QModelIndex &index );

void updateProjectHome();
void childrenCreated( QgsDataItem* item, QVector <QgsDataItem*> items );
void refreshChildrenCreated( QgsDataItem* item, QVector <QgsDataItem*> items );
void loadingFrameChanged();

protected:
// populates the model
@@ -114,6 +146,11 @@ class CORE_EXPORT QgsBrowserModel : public QAbstractItemModel
QVector<QgsDataItem*> mRootItems;
QgsFavouritesItem *mFavourites;
QgsDirectoryItem *mProjectHome;

private:
QList<QgsBrowserWatcher *> mWatchers;
QMovie mLoadingMovie;
QIcon mLoadingIcon;
};

#endif // QGSBROWSERMODEL_H

9 comments on commit 40bb180

@nyalldawson

This comment has been minimized.

Copy link
Contributor

@nyalldawson nyalldawson replied Nov 16, 2014

@blazek This is a nice change, but I've noticed that expanding out schemas from a PostGIS database in the browser is now very slow. Previously schemas would expand instantly, it now seems like the whole database is re-queried every time a schema is expanded...

@blazek

This comment has been minimized.

Copy link
Member Author

@blazek blazek replied Nov 18, 2014

Originally all schemas were populated with layers when a database was expanded. It did not seem optimal to me because there may be many schemas which user does not want to expand. I changed it so that when the database is expanded it only creates list of schemas and schema layers are populated when the schema is expanded.

So yes, it takes more time to expand a schema, but it should never take significantly (yes one query is repeated, but it is single query which should be fast) more time to expand database + expand schema than it took before and usually (if there are other schemas) it should take less time (because slow is the layer type query which is launched for every layer). Can you post approximate times (before/after the commit database / schema) ? Now you can also find "Children created in ### ms" in debug output.

BTW, I know that it is crashing on refresh of WMS with sublayers, I hope to fix that soon.

@nyalldawson

This comment has been minimized.

Copy link
Contributor

@nyalldawson nyalldawson replied Nov 18, 2014

@blazek - hmmm, that does sound like the correct approach. I'll do some benchmarks and see if there's actually a problem here.

@nyalldawson

This comment has been minimized.

Copy link
Contributor

@nyalldawson nyalldawson replied Dec 15, 2014

@blazek I don't think this is working correctly - looking at the code, QgsPostgresConn::supportedLayers always queries every layer in the database, it's not limited to a specific schema. This means that QgsPGConnectionItem::createChildren() first iterates over every table to get a unique list of schemas in the database, and then QgsPGSchemaItem::createChildren() also refetches every table and then subsequently filters out any which aren't in the current schema. So for both the initial listing of schemas and for every schema expansion we are querying every database table.

This could be dramatically improved if QgsPGConnectionItem::createChildren() only queried the pg_catalog.pg_namespace table, and then QgsPostgresConn::getTableInfo was modified to accept a schema and only fetched info for tables under that particular schema.

@nyalldawson

This comment has been minimized.

Copy link
Contributor

@nyalldawson nyalldawson replied Dec 16, 2014

@blazek - here's a (broken) proof of concept fix: nyalldawson@3987e6d

@blazek

This comment has been minimized.

Copy link
Member Author

@blazek blazek replied Dec 16, 2014

@nyalldawson QgsPostgresConn::supportedLayers() makes list of all tables but in single query. The slow QgsPostgresConn::retrieveLayerTypes() is called from QgsPGSchemaItem::createChildren(), but only for layers in current schema ( if ( layerProperty.schemaName != mName ) continue; ).

Does your proof (why broken?) make really difference in time? But it is surely improvement to add scheme support to QgsPostgresConn::getTableInfo(), you mean const QString schema = QString() to keep compatibility? Just push when ready please, thanks.

Anyway, we should maybe add all schemas to connection (even those without (geometry) layers) to allow to drag layers to an empty schema?

BTW, there are other interesting issues which I don't know how to solve, for example, if a server is slow and connection fails and the user deletes the connection during createChildren(), QgsPostgresConn will continue to open credentials dialog. I have no idea how to stop QgsPostgresConnPool::instance()->acquireConnection() once it was called.

@nyalldawson

This comment has been minimized.

Copy link
Contributor

@nyalldawson nyalldawson replied Dec 16, 2014

@blazek my "fix" causes a crash when adding layers from the browser. I'm not familiar with that code area, so it makes me nervous that there's not unintended side effects of this change (are QgsPGSchemaItem and QgsPGLayerItem only used for the browser?). But... it's a dramatic difference on my server. Without this change the initial population of schemas takes around 10 seconds, and expanding each schema takes about the same time again. With this change both operations are nearly instant. I'd imagine that this change would also make a huge difference for remote postgres connections too...

@blazek

This comment has been minimized.

Copy link
Member Author

@blazek blazek replied Dec 16, 2014

@nyalldawson It's crashing without the fix as well, you can push and I'll fix the crash in browser later.

@blazek

This comment has been minimized.

Copy link
Member Author

@blazek blazek replied Dec 16, 2014

@nyalldawson Quick fix in 9fbcc7e, hopefully works. I have to review items hierarchy once more.

Please sign in to comment.
You can’t perform that action at this time.