Merge pull request #6316 from nyalldawson/locator_thread

Make QgsLocator more thread safe

Author: nyalldawson

.ci/travis/linux/blacklist.txt

@@ -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

python/core/locator/qgslocatorfilter.sip.in

@@ -69,9 +69,22 @@ Abstract base class for filters which collect locator results.
       Lowest
     };
 
+    enum Flag
+    {
+      FlagFast,
+    };
+    typedef QFlags<QgsLocatorFilter::Flag> Flags;
+
+
     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;
@@ -86,6 +99,11 @@ Returns the unique name for the filter. This should be an untranslated string id
 Returns a translated, user-friendly name for the filter.
 
 .. seealso:: :py:func:`name`
+%End
+
+    virtual QgsLocatorFilter::Flags flags() const;
+%Docstring
+Returns flags which specify the filter's behavior.
 %End
 
     virtual Priority priority() const;
@@ -108,6 +126,14 @@ 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
+
     virtual void fetchResults( const QString &string, const QgsLocatorContext &context, QgsFeedback *feedback ) = 0;
 %Docstring
 Retrieves the filter results for a specified search ``string``. The ``context``
@@ -120,6 +146,9 @@ signal whenever they encounter a matching result.
 Subclasses should periodically check the ``feedback`` object to determine
 whether the query has been canceled. If so, the subclass should return
 from this method as soon as possible.
+
+This will be called from a background thread unless flags() returns the
+QgsLocatorFilter.FlagFast flag.
 %End
 
     virtual void triggerResult( const QgsLocatorResult &result ) = 0;
@@ -190,6 +219,11 @@ custom configuration widget.
 
   signals:
 
+    void finished();
+%Docstring
+Emitted when the filter finishes fetching results.
+%End
+
     void resultFetched( const QgsLocatorResult &result );
 %Docstring
 Should be emitted by filters whenever they encounter a matching result
@@ -198,6 +232,9 @@ during within their fetchResults() implementation.
 
 };
 
+QFlags<QgsLocatorFilter::Flag> operator|(QgsLocatorFilter::Flag f1, QFlags<QgsLocatorFilter::Flag> f2);
+
+
 
 
 

python/plugins/processing/gui/AlgorithmLocatorFilter.py

@@ -40,6 +40,9 @@ class AlgorithmLocatorFilter(QgsLocatorFilter):
     def __init__(self, parent=None):
         super(AlgorithmLocatorFilter, self).__init__(parent)
 
+    def clone(self):
+        return AlgorithmLocatorFilter()
+
     def name(self):
         return 'processing_alg'
 
@@ -52,10 +55,13 @@ def priority(self):
     def prefix(self):
         return 'a'
 
+    def flags(self):
+        return QgsLocatorFilter.FlagFast
+
     def fetchResults(self, string, context, feedback):
+        # 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
 
@@ -81,7 +87,7 @@ def triggerResult(self, result):
                 dlg.setMessage(message)
                 dlg.exec_()
                 return
-            dlg = alg.createCustomParametersWidget()
+            dlg = alg.createCustomParametersWidget(None)
             if not dlg:
                 dlg = AlgorithmDialog(alg)
             canvas = iface.mapCanvas()

src/app/locator/qgsinbuiltlocatorfilters.cpp

@@ -31,19 +31,20 @@ 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::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback * )
 {
   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() );
@@ -68,18 +69,19 @@ 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::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback * )
 {
   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() );
@@ -109,15 +111,20 @@ 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::fetchResults( const QString &string, const QgsLocatorContext &, QgsFeedback * )
+{
+  // 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 );
   }
 }
@@ -131,8 +138,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 );
   }
@@ -154,7 +161,6 @@ 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();
@@ -171,7 +177,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;
@@ -183,11 +194,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;
@@ -211,17 +220,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() )
@@ -236,10 +253,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 );
 

src/app/locator/qgsinbuiltlocatorfilters.h

@@ -19,6 +19,9 @@
 #define QGSINBUILTLOCATORFILTERS_H
 
 #include "qgslocatorfilter.h"
+#include "qgsexpressioncontext.h"
+#include "qgsfeatureiterator.h"
+
 class QAction;
 
 class QgsLayerTreeLocatorFilter : public QgsLocatorFilter
@@ -28,10 +31,12 @@ class QgsLayerTreeLocatorFilter : public QgsLocatorFilter
   public:
 
     QgsLayerTreeLocatorFilter( QObject *parent = nullptr );
+    QgsLayerTreeLocatorFilter *clone() const override;
     QString name() const override { return QStringLiteral( "layertree" ); }
     QString displayName() const override { return tr( "Project Layers" ); }
     Priority priority() const override { return Highest; }
     QString prefix() const override { return QStringLiteral( "l" ); }
+    QgsLocatorFilter::Flags flags() const override { return QgsLocatorFilter::FlagFast; }
 
     void fetchResults( const QString &string, const QgsLocatorContext &context, QgsFeedback *feedback ) override;
     void triggerResult( const QgsLocatorResult &result ) override;
@@ -45,10 +50,12 @@ class QgsLayoutLocatorFilter : public QgsLocatorFilter
   public:
 
     QgsLayoutLocatorFilter( QObject *parent = nullptr );
+    QgsLayoutLocatorFilter *clone() const override;
     QString name() const override { return QStringLiteral( "layouts" ); }
     QString displayName() const override { return tr( "Project Layouts" ); }
     Priority priority() const override { return Highest; }
     QString prefix() const override { return QStringLiteral( "pl" ); }
+    QgsLocatorFilter::Flags flags() const override { return QgsLocatorFilter::FlagFast; }
 
     void fetchResults( const QString &string, const QgsLocatorContext &context, QgsFeedback *feedback ) override;
     void triggerResult( const QgsLocatorResult &result ) override;
@@ -62,10 +69,12 @@ class QgsActionLocatorFilter : public QgsLocatorFilter
   public:
 
     QgsActionLocatorFilter( const QList<QWidget *> &parentObjectsForActions, QObject *parent = nullptr );
+    QgsActionLocatorFilter *clone() const override;
     QString name() const override { return QStringLiteral( "actions" ); }
     QString displayName() const override { return tr( "Actions" ); }
     Priority priority() const override { return Lowest; }
     QString prefix() const override { return QStringLiteral( "." ); }
+    QgsLocatorFilter::Flags flags() const override { return QgsLocatorFilter::FlagFast; }
 
     void fetchResults( const QString &string, const QgsLocatorContext &context, QgsFeedback *feedback ) override;
     void triggerResult( const QgsLocatorResult &result ) override;
@@ -84,13 +93,23 @@ class QgsActiveLayerFeaturesLocatorFilter : public QgsLocatorFilter
   public:
 
     QgsActiveLayerFeaturesLocatorFilter( QObject *parent = nullptr );
+    QgsActiveLayerFeaturesLocatorFilter *clone() const override;
     QString name() const override { return QStringLiteral( "features" ); }
     QString displayName() const override { return tr( "Active Layer Features" ); }
     Priority priority() const override { return Medium; }
     QString prefix() const override { return QStringLiteral( "f" ); }
 
+    void prepare( const QString &string, const QgsLocatorContext &context ) override;
     void fetchResults( const QString &string, const QgsLocatorContext &context, QgsFeedback *feedback ) override;
     void triggerResult( const QgsLocatorResult &result ) override;
+
+  private:
+
+    QgsExpression mDispExpression;
+    QgsExpressionContext mContext;
+    QgsFeatureIterator mIterator;
+    QString mLayerId;
+    QIcon mLayerIcon;
 };