Skip to content

Commit

Permalink
Make QgsLocator more thread safe
Browse files Browse the repository at this point in the history 
- add a clone() method to filters, and always search using the
clone instead of the original filter
- add a prepare() method to filters, which is always run in the
main thread and can be used to prepare the filter for safe
background execution (e.g. creating feature iterators in advance)
- don't use QtConcurrent to perform searches in background threads,
since it is not safe to use with QObjects
- instead manually create threads and ensure that cloned objects
are always moved to the thread that they will run in, to ensure
that they correctly have thread affinity with the thread in which
they are executed
  • Loading branch information
@nyalldawson
nyalldawson committed Feb 12, 2018
1 parent 5431a2d commit 981afb3
Show file tree
Hide file tree
Showing 12 changed files with 236 additions and 75 deletions.
3 changes: 0 additions & 3 deletions .ci/travis/linux/blacklist.txt
View file
Expand Up @@ -30,9 +30,6 @@ PyQgsSpatialiteProvider
# Flaky, see https://travis-ci.org/qgis/QGIS/jobs/297708174
PyQgsServerAccessControl

# Flaky, see https://dash.orfeo-toolbox.org/testDetails.php?test=61670866&build=297397
PyQgsLocator

# Need a local postgres installation
PyQgsAuthManagerPKIPostgresTest
PyQgsAuthManagerPasswordPostgresTest
Expand Down
22 changes: 22 additions & 0 deletions python/core/locator/qgslocatorfilter.sip.in
View file
Expand Up @@ -72,6 +72,12 @@ Abstract base class for filters which collect locator results.
QgsLocatorFilter( QObject *parent = 0 );
%Docstring
Constructor for QgsLocatorFilter.
%End

virtual QgsLocatorFilter *clone() const = 0 /Factory/;
%Docstring
Creates a clone of the filter. New requests are always executed in a
clone of the original filter.
%End

virtual QString name() const = 0;
Expand Down Expand Up @@ -108,6 +114,20 @@ results from this filter.
be ignored.
%End

virtual void prepare( const QString &string, const QgsLocatorContext &context );
%Docstring
Prepares the filter instance for an upcoming search for the specified ``string``. This method is always called
from the main thread, and individual filter subclasses should perform whatever
tasks are required in order to allow a subsequent search to safely execute
on a background thread.
%End

void executeSearchAndDelete( const QString &string, const QgsLocatorContext &context, QgsFeedback *feedback );
%Docstring
Executes a search for this filter instance, and then deletes the current instance
of the filter.
%End

virtual void fetchResults( const QString &string, const QgsLocatorContext &context, QgsFeedback *feedback ) = 0;
%Docstring
Retrieves the filter results for a specified search ``string``. The ``context``
Expand Down Expand Up @@ -190,6 +210,8 @@ custom configuration widget.

signals:

void finished();

void resultFetched( const QgsLocatorResult &result );
%Docstring
Should be emitted by filters whenever they encounter a matching result
Expand Down
16 changes: 12 additions & 4 deletions python/plugins/processing/gui/AlgorithmLocatorFilter.py
View file
Expand Up @@ -39,6 +39,10 @@ class AlgorithmLocatorFilter(QgsLocatorFilter):

def __init__(self, parent=None):
super(AlgorithmLocatorFilter, self).__init__(parent)
self.found_results = []

def clone(self):
return AlgorithmLocatorFilter()

def name(self):
return 'processing_alg'
Expand All @@ -52,10 +56,10 @@ def priority(self):
def prefix(self):
return 'a'

def fetchResults(self, string, context, feedback):
def prepare(self, string, context):
# collect results in main thread, since this method is inexpensive and
# accessing the processing registry is not thread safe
for a in QgsApplication.processingRegistry().algorithms():
if feedback.isCanceled():
return
if a.flags() & QgsProcessingAlgorithm.FlagHideFromToolbox:
continue

Expand All @@ -69,7 +73,11 @@ def fetchResults(self, string, context, feedback):
result.score = float(len(string)) / len(a.displayName())
else:
result.score = 0
self.resultFetched.emit(result)
self.found_results.append(result)

def fetchResults(self, string, context, feedback):
for result in self.found_results:
self.resultFetched.emit(result)

def triggerResult(self, result):
alg = QgsApplication.processingRegistry().createAlgorithmById(result.userData)
Expand Down
118 changes: 82 additions & 36 deletions src/app/locator/qgsinbuiltlocatorfilters.cpp
View file
Expand Up @@ -31,28 +31,39 @@ QgsLayerTreeLocatorFilter::QgsLayerTreeLocatorFilter( QObject *parent )
: QgsLocatorFilter( parent )
{}

void QgsLayerTreeLocatorFilter::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback *feedback )
QgsLayerTreeLocatorFilter *QgsLayerTreeLocatorFilter::clone() const
{
return new QgsLayerTreeLocatorFilter();
}

void QgsLayerTreeLocatorFilter::prepare( const QString &string, const QgsLocatorContext & )
{
// collect results in main thread, since this method is inexpensive and
// accessing the layer tree root is not thread safe
QgsLayerTree *tree = QgsProject::instance()->layerTreeRoot();
QList<QgsLayerTreeLayer *> layers = tree->findLayers();
Q_FOREACH ( QgsLayerTreeLayer *layer, layers )
const QList<QgsLayerTreeLayer *> layers = tree->findLayers();
for ( QgsLayerTreeLayer *layer : layers )
{
if ( feedback->isCanceled() )
return;

if ( layer->layer() && stringMatches( layer->layer()->name(), string ) )
{
QgsLocatorResult result;
result.filter = this;
result.displayString = layer->layer()->name();
result.userData = layer->layerId();
result.icon = QgsMapLayerModel::iconForLayer( layer->layer() );
result.score = static_cast< double >( string.length() ) / layer->layer()->name().length();
emit resultFetched( result );
mResults.append( result );
}
}
}

void QgsLayerTreeLocatorFilter::fetchResults( const QString &, const QgsLocatorContext &, QgsFeedback * )
{
for ( const QgsLocatorResult &result : qgis::as_const( mResults ) )
{
emit resultFetched( result );
}
}

void QgsLayerTreeLocatorFilter::triggerResult( const QgsLocatorResult &result )
{
QString layerId = result.userData.toString();
Expand All @@ -68,27 +79,39 @@ QgsLayoutLocatorFilter::QgsLayoutLocatorFilter( QObject *parent )
: QgsLocatorFilter( parent )
{}

void QgsLayoutLocatorFilter::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback *feedback )
QgsLayoutLocatorFilter *QgsLayoutLocatorFilter::clone() const
{
return new QgsLayoutLocatorFilter();
}

void QgsLayoutLocatorFilter::prepare( const QString &string, const QgsLocatorContext & )
{
// collect results in main thread, since this method is inexpensive and
// accessing the project layouts is not thread safe

const QList< QgsMasterLayoutInterface * > layouts = QgsProject::instance()->layoutManager()->layouts();
for ( QgsMasterLayoutInterface *layout : layouts )
{
if ( feedback->isCanceled() )
return;

if ( layout && stringMatches( layout->name(), string ) )
{
QgsLocatorResult result;
result.filter = this;
result.displayString = layout->name();
result.userData = layout->name();
//result.icon = QgsMapLayerModel::iconForLayer( layer->layer() );
result.score = static_cast< double >( string.length() ) / layout->name().length();
emit resultFetched( result );
mResults.append( result );
}
}
}

void QgsLayoutLocatorFilter::fetchResults( const QString &, const QgsLocatorContext &, QgsFeedback * )
{
for ( const QgsLocatorResult &result : qgis::as_const( mResults ) )
{
emit resultFetched( result );
}
}

void QgsLayoutLocatorFilter::triggerResult( const QgsLocatorResult &result )
{
QString layoutName = result.userData.toString();
Expand All @@ -109,19 +132,32 @@ QgsActionLocatorFilter::QgsActionLocatorFilter( const QList<QWidget *> &parentOb
setUseWithoutPrefix( false );
}

void QgsActionLocatorFilter::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback *feedback )
QgsActionLocatorFilter *QgsActionLocatorFilter::clone() const
{
return new QgsActionLocatorFilter( mActionParents );
}

void QgsActionLocatorFilter::prepare( const QString &string, const QgsLocatorContext & )
{
// collect results in main thread, since this method is inexpensive and
// accessing the gui actions is not thread safe

QList<QAction *> found;

Q_FOREACH ( QWidget *object, mActionParents )
for ( QWidget *object : qgis::as_const( mActionParents ) )
{
if ( feedback->isCanceled() )
return;

searchActions( string, object, found );
}
}

void QgsActionLocatorFilter::fetchResults( const QString &, const QgsLocatorContext &, QgsFeedback * )
{
for ( const QgsLocatorResult &result : qgis::as_const( mResults ) )
{
emit resultFetched( result );
}
}

void QgsActionLocatorFilter::triggerResult( const QgsLocatorResult &result )
{
QAction *action = qobject_cast< QAction * >( qvariant_cast<QObject *>( result.userData ) );
Expand All @@ -131,8 +167,8 @@ void QgsActionLocatorFilter::triggerResult( const QgsLocatorResult &result )

void QgsActionLocatorFilter::searchActions( const QString &string, QWidget *parent, QList<QAction *> &found )
{
QList< QWidget *> children = parent->findChildren<QWidget *>();
Q_FOREACH ( QWidget *widget, children )
const QList< QWidget *> children = parent->findChildren<QWidget *>();
for ( QWidget *widget : children )
{
searchActions( string, widget, found );
}
Expand All @@ -154,12 +190,11 @@ void QgsActionLocatorFilter::searchActions( const QString &string, QWidget *pare
if ( stringMatches( searchText, string ) )
{
QgsLocatorResult result;
result.filter = this;
result.displayString = searchText;
result.userData = QVariant::fromValue( action );
result.icon = action->icon();
result.score = static_cast< double >( string.length() ) / searchText.length();
emit resultFetched( result );
mResults.append( result );
found << action;
}
}
Expand All @@ -171,7 +206,12 @@ QgsActiveLayerFeaturesLocatorFilter::QgsActiveLayerFeaturesLocatorFilter( QObjec
setUseWithoutPrefix( false );
}

void QgsActiveLayerFeaturesLocatorFilter::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback *feedback )
QgsActiveLayerFeaturesLocatorFilter *QgsActiveLayerFeaturesLocatorFilter::clone() const
{
return new QgsActiveLayerFeaturesLocatorFilter();
}

void QgsActiveLayerFeaturesLocatorFilter::prepare( const QString &string, const QgsLocatorContext & )
{
if ( string.length() < 3 )
return;
Expand All @@ -183,11 +223,9 @@ void QgsActiveLayerFeaturesLocatorFilter::fetchResults( const QString &string, c
if ( !layer )
return;

int found = 0;
QgsExpression dispExpression( layer->displayExpression() );
QgsExpressionContext context;
context.appendScopes( QgsExpressionContextUtils::globalProjectLayerScopes( layer ) );
dispExpression.prepare( &context );
mDispExpression = QgsExpression( layer->displayExpression() );
mContext.appendScopes( QgsExpressionContextUtils::globalProjectLayerScopes( layer ) );
mDispExpression.prepare( &mContext );

// build up request expression
QStringList expressionParts;
Expand All @@ -211,17 +249,25 @@ void QgsActiveLayerFeaturesLocatorFilter::fetchResults( const QString &string, c
req.setFlags( QgsFeatureRequest::NoGeometry );
req.setFilterExpression( expression );
req.setLimit( 30 );
mIterator = layer->getFeatures( req );

mLayerId = layer->id();
mLayerIcon = QgsMapLayerModel::iconForLayer( layer );
}

void QgsActiveLayerFeaturesLocatorFilter::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback *feedback )
{
int found = 0;
QgsFeature f;
QgsFeatureIterator it = layer->getFeatures( req );
while ( it.nextFeature( f ) )

while ( mIterator.nextFeature( f ) )
{
if ( feedback->isCanceled() )
return;

QgsLocatorResult result;
result.filter = this;

context.setFeature( f );
mContext.setFeature( f );

// find matching field content
Q_FOREACH ( const QVariant &var, f.attributes() )
Expand All @@ -236,10 +282,10 @@ void QgsActiveLayerFeaturesLocatorFilter::fetchResults( const QString &string, c
if ( result.displayString.isEmpty() )
continue; //not sure how this result slipped through...

result.description = dispExpression.evaluate( &context ).toString();
result.description = mDispExpression.evaluate( &mContext ).toString();

result.userData = QVariantList() << f.id() << layer->id();
result.icon = QgsMapLayerModel::iconForLayer( layer );
result.userData = QVariantList() << f.id() << mLayerId;
result.icon = mLayerIcon;
result.score = static_cast< double >( string.length() ) / result.displayString.size();
emit resultFetched( result );

Expand Down

0 comments on commit 981afb3

Please sign in to comment.