Skip to content

Commit

Permalink
expand browser items in threads
Browse files Browse the repository at this point in the history
  • Loading branch information
@blazek
blazek committed Nov 12, 2014
1 parent 078a3ab commit 40bb180
Show file tree
Hide file tree
Showing 17 changed files with 394 additions and 191 deletions.
3 changes: 2 additions & 1 deletion python/core/qgsdataitem.sip
View file
Expand Up @@ -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;
Expand Down
6 changes: 4 additions & 2 deletions src/app/qgsbrowserdockwidget.cpp
View file
Expand Up @@ -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 )
Expand Down Expand Up @@ -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 )
Expand Down Expand Up @@ -708,6 +709,7 @@ void QgsBrowserDockWidget::itemExpanded( const QModelIndex& index )

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

if ( !mModel || !mProxyModel )
Expand Down
186 changes: 154 additions & 32 deletions src/core/qgsbrowsermodel.cpp
View file
Expand Up @@ -15,6 +15,7 @@
#include <QDir>
#include <QApplication>
#include <QStyle>
#include <QtConcurrentMap>

#include "qgis.h"
#include "qgsapplication.h"
Expand All @@ -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 )
{
Expand All @@ -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();
}

Expand Down Expand Up @@ -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
Expand All @@ -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() )
{
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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 )
Expand Down Expand Up @@ -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 )
Expand Down
37 changes: 37 additions & 0 deletions src/core/qgsbrowsermodel.h
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

@blazek blazek commented on 40bb180 Nov 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nyalldawson
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@blazek
Copy link
Member Author

@blazek blazek commented on 40bb180 Dec 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

@blazek blazek commented on 40bb180 Dec 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@blazek
Copy link
Member Author

@blazek blazek commented on 40bb180 Dec 16, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please sign in to comment.