Rework validity check API to allow future background threaded use

qgis · Jan 2, 2019 · ddd522b · ddd522b
1 parent ec55304
commit ddd522b
Show file tree

Hide file tree

Showing 14 changed files with 181 additions and 45 deletions.
diff --git a/python/core/auto_generated/validity/qgsabstractvaliditycheck.sip.in b/python/core/auto_generated/validity/qgsabstractvaliditycheck.sip.in
@@ -54,7 +54,7 @@ Checks can be used for many different use cases, e.g. validating a layout's cont
 an export to occur, or validating the contents of a Processing model (and warning if required plugin based
 providers are not available or if compulsory algorithm parameters have not been populated).
 
-Subclasses must indicate the type of check the represent via the checkType() method. When checks are performed,
+Subclasses must indicate the type of check they represent via the checkType() method. When checks are performed,
 all the registered checks with a matching check type are performed sequentially. The check type also
 dictates the subclass of the QgsValidityCheckContext which is given to the subclass' runCheck method.
 
@@ -77,26 +77,50 @@ Checks must be registered in the application's :py:class:`QgsValidityCheckRegist
       TypeUserCheck,
     };
 
+    virtual QgsAbstractValidityCheck *create() const = 0 /Factory/;
+%Docstring
+Creates a new instance of the check and returns it.
+%End
+
     virtual QString id() const = 0;
 %Docstring
 Returns the unique ID of the check.
+
+This is a non-translated, non-user visible string identifying the check.
 %End
 
     virtual int checkType() const = 0;
 %Docstring
 Returns the type of the check.
 %End
 
-    virtual QString name() const = 0;
+    virtual bool prepareCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback );
 %Docstring
-Returns the name of the check.
+Prepares the check for execution, and returns true if the check can be run.
+
+This method is always called from the main thread, and subclasses can implement
+it to do preparatory steps which are not thread safe (e.g. obtaining feature
+sources from vector layers). It is followed by a call to runCheck(), which
+may be performed in a background thread.
+
+Individual calls to prepareCheck()/runCheck() are run on a new instance of the
+check (see create()), so subclasses can safely store state from the prepareCheck() method
+ready for the subsequent runCheck() method.
+
+The ``context`` argument gives the wider in which the check is being run.
 %End
 
-    virtual QList< QgsValidityCheckResult > runCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback ) const = 0;
+    virtual QList< QgsValidityCheckResult > runCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback ) = 0;
 %Docstring
 Runs the check and returns a list of results. If the check is "passed" and no warnings or errors are generated,
 then an empty list should be returned.
 
+This method may be called in a background thread, so subclasses should take care to ensure that
+only thread-safe methods are used. It is always preceeded by a call to prepareCheck().
+
+If a check needs to perform non-thread-safe tests, these should be implemented within prepareCheck()
+and stored in the subclass instance to be returned by this method.
+
 The ``context`` argument gives the wider in which the check is being run.
 %End
 

diff --git a/python/core/auto_generated/validity/qgsvaliditycheckcontext.sip.in b/python/core/auto_generated/validity/qgsvaliditycheckcontext.sip.in
@@ -32,6 +32,17 @@ context, containing a reference to the QgsLayout to be checked.
 %End
   public:
 
+    enum ContextType
+    {
+      TypeLayoutContext,
+      TypeUserContext,
+    };
+
+    virtual int type() const = 0;
+%Docstring
+Returns the context type.
+%End
+
     virtual ~QgsValidityCheckContext();
 
 };
@@ -57,6 +68,8 @@ indicate they are of the QgsAbstractValidityCheck.TypeLayoutCheck type.
 Constructor for QgsLayoutValidityCheckContext for the specified ``layout``.
 %End
 
+    virtual int type() const;
+
     QgsLayout *layout;
 
 };

diff --git a/python/core/auto_generated/validity/qgsvaliditycheckregistry.sip.in b/python/core/auto_generated/validity/qgsvaliditycheckregistry.sip.in
@@ -29,12 +29,12 @@ QgsValidityCheckRegistry is not usually directly created, but rather accessed th
     ~QgsValidityCheckRegistry();
 
 
-    QList<QgsAbstractValidityCheck *> checks() const;
+    QList<const QgsAbstractValidityCheck *> checks() const;
 %Docstring
 Returns the list of available checks.
 %End
 
-    QList<QgsAbstractValidityCheck *> checks( int type ) const;
+    QList<const QgsAbstractValidityCheck *> checks( int type ) const;
 %Docstring
 Returns the list of all available checks of the matching ``type``.
 %End
@@ -62,6 +62,9 @@ The ``context`` argument gives the wider in which the check is being run.
 
 The ``feedback`` argument is used to give progress reports and to support
 cancelation of long-running checks.
+
+This is a blocking call, which will run all matching checks in the main
+thread and only return when they have all completed.
 %End
 
   private:

diff --git a/python/gui/auto_generated/qgsvaliditycheckresultswidget.sip.in b/python/gui/auto_generated/qgsvaliditycheckresultswidget.sip.in
@@ -93,6 +93,8 @@ on the dialog and proceed with the operation. The function will return false.
 
 Returns true if no warnings were encountered (and no dialog was shown to users), or if only
 warnings were shown and the user clicked OK after being shown these warnings.
+
+This method is a blocking method, and runs all checks in the main thread.
 %End
 
 };

diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt
@@ -434,7 +434,6 @@ SET (QGIS_APP_MOC_HDRS
   layout/qgslayoutscalebarwidget.h
   layout/qgslayoutshapewidget.h
   layout/qgslayouttablebackgroundcolorsdialog.h
-  layout/qgslayoutvaliditychecks.h
   layout/qgsreportfieldgroupsectionwidget.h
   layout/qgsreportlayoutsectionwidget.h
   layout/qgsreportorganizerwidget.h

diff --git a/src/app/layout/qgslayoutvaliditychecks.cpp b/src/app/layout/qgslayoutvaliditychecks.cpp
@@ -19,12 +19,23 @@
 #include "qgslayoutitemmap.h"
 #include "qgslayout.h"
 
-QList<QgsValidityCheckResult> QgsLayoutMapCrsValidityCheck::runCheck( const QgsValidityCheckContext *context, QgsFeedback * ) const
+QgsLayoutMapCrsValidityCheck *QgsLayoutMapCrsValidityCheck::create() const
 {
-  QList<QgsValidityCheckResult> results;
-  const QgsLayoutValidityCheckContext *layoutContext = dynamic_cast< const QgsLayoutValidityCheckContext * >( context );
+  return new QgsLayoutMapCrsValidityCheck();
+}
+
+QString QgsLayoutMapCrsValidityCheck::id() const { return QStringLiteral( "map_crs_check" ); }
+
+int QgsLayoutMapCrsValidityCheck::checkType() const { return QgsAbstractValidityCheck::TypeLayoutCheck; }
+
+bool QgsLayoutMapCrsValidityCheck::prepareCheck( const QgsValidityCheckContext *context, QgsFeedback * )
+{
+  if ( context->type() != QgsValidityCheckContext::TypeLayoutContext )
+    return false;
+
+  const QgsLayoutValidityCheckContext *layoutContext = static_cast< const QgsLayoutValidityCheckContext * >( context );
   if ( !layoutContext )
-    return results;
+    return false;
 
   QList< QgsLayoutItemMap * > mapItems;
   layoutContext->layout->layoutItems( mapItems );
@@ -36,9 +47,14 @@ QList<QgsValidityCheckResult> QgsLayoutMapCrsValidityCheck::runCheck( const QgsV
       res.type = QgsValidityCheckResult::Warning;
       res.title = tr( "Map projection is misleading" );
       res.detailedDescription = tr( "The projection for the map item %1 is set to <i>Web Mercator (EPSG:3857)</i> which misrepresents areas and shapes. Consider using an appropriate local projection instead." ).arg( map->displayName() );
-      results.append( res );
+      mResults.append( res );
     }
   }
 
-  return results;
+  return true;
+}
+
+QList<QgsValidityCheckResult> QgsLayoutMapCrsValidityCheck::runCheck( const QgsValidityCheckContext *, QgsFeedback * )
+{
+  return mResults;
 }
diff --git a/src/app/layout/qgslayoutvaliditychecks.h b/src/app/layout/qgslayoutvaliditychecks.h
@@ -22,12 +22,12 @@ class QgsLayoutMapCrsValidityCheck : public QgsAbstractValidityCheck
 
   public:
 
-    QString id() const override { return "map_crs_check"; }
-    int checkType() const override { return QgsAbstractValidityCheck::TypeLayoutCheck; }
-    QString name() const override { return "Map CRS Check"; }
-    QList< QgsValidityCheckResult > runCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback ) const override;
-
-
-
-
+    QgsLayoutMapCrsValidityCheck *create() const override;
+    QString id() const override;
+    int checkType() const override;
+    bool prepareCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback ) override;
+    QList< QgsValidityCheckResult > runCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback ) override;
+
+  private:
+    QList<QgsValidityCheckResult> mResults;
 };
diff --git a/src/core/CMakeLists.txt b/src/core/CMakeLists.txt
@@ -774,8 +774,6 @@ SET(QGIS_CORE_MOC_HDRS
   layertree/qgslayertreenode.h
   layertree/qgslayertreeregistrybridge.h
 
-  validity/qgsabstractvaliditycheck.h
-
   qgsuserprofilemanager.h
 )
 
@@ -1193,6 +1191,7 @@ SET(QGIS_CORE_HDRS
   metadata/qgslayermetadata.h
   metadata/qgslayermetadatavalidator.h
 
+  validity/qgsabstractvaliditycheck.h
   validity/qgsvaliditycheckcontext.h
   validity/qgsvaliditycheckregistry.h
 )

diff --git a/src/core/validity/qgsabstractvaliditycheck.h b/src/core/validity/qgsabstractvaliditycheck.h
@@ -81,7 +81,7 @@ class CORE_EXPORT QgsValidityCheckResult
  * an export to occur, or validating the contents of a Processing model (and warning if required plugin based
  * providers are not available or if compulsory algorithm parameters have not been populated).
  *
- * Subclasses must indicate the type of check the represent via the checkType() method. When checks are performed,
+ * Subclasses must indicate the type of check they represent via the checkType() method. When checks are performed,
  * all the registered checks with a matching check type are performed sequentially. The check type also
  * dictates the subclass of the QgsValidityCheckContext which is given to the subclass' runCheck method.
  *
@@ -94,7 +94,6 @@ class CORE_EXPORT QgsValidityCheckResult
  */
 class CORE_EXPORT QgsAbstractValidityCheck : public QObject
 {
-    Q_OBJECT
 
   public:
 
@@ -105,8 +104,15 @@ class CORE_EXPORT QgsAbstractValidityCheck : public QObject
       TypeUserCheck = 10000, //!< Starting point for custom user types
     };
 
+    /**
+     * Creates a new instance of the check and returns it.
+     */
+    virtual QgsAbstractValidityCheck *create() const = 0 SIP_FACTORY;
+
     /**
      * Returns the unique ID of the check.
+     *
+     * This is a non-translated, non-user visible string identifying the check.
      */
     virtual QString id() const = 0;
 
@@ -116,17 +122,39 @@ class CORE_EXPORT QgsAbstractValidityCheck : public QObject
     virtual int checkType() const = 0;
 
     /**
-     * Returns the name of the check.
+     * Prepares the check for execution, and returns true if the check can be run.
+     *
+     * This method is always called from the main thread, and subclasses can implement
+     * it to do preparatory steps which are not thread safe (e.g. obtaining feature
+     * sources from vector layers). It is followed by a call to runCheck(), which
+     * may be performed in a background thread.
+     *
+     * Individual calls to prepareCheck()/runCheck() are run on a new instance of the
+     * check (see create()), so subclasses can safely store state from the prepareCheck() method
+     * ready for the subsequent runCheck() method.
+     *
+     * The \a context argument gives the wider in which the check is being run.
      */
-    virtual QString name() const = 0;
+    virtual bool prepareCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback )
+    {
+      Q_UNUSED( context );
+      Q_UNUSED( feedback );
+      return true;
+    }
 
     /**
      * Runs the check and returns a list of results. If the check is "passed" and no warnings or errors are generated,
      * then an empty list should be returned.
      *
+     * This method may be called in a background thread, so subclasses should take care to ensure that
+     * only thread-safe methods are used. It is always preceeded by a call to prepareCheck().
+     *
+     * If a check needs to perform non-thread-safe tests, these should be implemented within prepareCheck()
+     * and stored in the subclass instance to be returned by this method.
+     *
      * The \a context argument gives the wider in which the check is being run.
      */
-    virtual QList< QgsValidityCheckResult > runCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback ) const = 0;
+    virtual QList< QgsValidityCheckResult > runCheck( const QgsValidityCheckContext *context, QgsFeedback *feedback ) = 0;
 
 };
 

diff --git a/src/core/validity/qgsvaliditycheckcontext.h b/src/core/validity/qgsvaliditycheckcontext.h
@@ -45,7 +45,18 @@ class CORE_EXPORT QgsValidityCheckContext
 #endif
 
   public:
-    // initially nothing in the base class!
+
+    //! Available check context types
+    enum ContextType
+    {
+      TypeLayoutContext = 1, //!< Layout context, see QgsLayoutValidityCheckContext
+      TypeUserContext = 10001, //!< Starting point for user contexts
+    };
+
+    /**
+     * Returns the context type.
+     */
+    virtual int type() const = 0;
 
     virtual ~QgsValidityCheckContext() = default;
 
@@ -72,6 +83,8 @@ class CORE_EXPORT QgsLayoutValidityCheckContext : public QgsValidityCheckContext
       : layout( layout )
     {}
 
+    int type() const override { return TypeLayoutContext; }
+
     /**
      * Pointer to the layout which the check is being run against.
      */

diff --git a/src/core/validity/qgsvaliditycheckregistry.cpp b/src/core/validity/qgsvaliditycheckregistry.cpp
@@ -25,9 +25,9 @@ QgsValidityCheckRegistry::~QgsValidityCheckRegistry()
   qDeleteAll( mChecks );
 }
 
-QList<QgsAbstractValidityCheck *> QgsValidityCheckRegistry::checks() const
+QList<const QgsAbstractValidityCheck *> QgsValidityCheckRegistry::checks() const
 {
-  QList<QgsAbstractValidityCheck *> results;
+  QList<const QgsAbstractValidityCheck *> results;
   for ( const QPointer< QgsAbstractValidityCheck > &check : mChecks )
   {
     if ( check )
@@ -36,9 +36,9 @@ QList<QgsAbstractValidityCheck *> QgsValidityCheckRegistry::checks() const
   return results;
 }
 
-QList<QgsAbstractValidityCheck *> QgsValidityCheckRegistry::checks( int type ) const
+QList<const QgsAbstractValidityCheck *> QgsValidityCheckRegistry::checks( int type ) const
 {
-  QList< QgsAbstractValidityCheck * > results;
+  QList< const QgsAbstractValidityCheck * > results;
   for ( const QPointer< QgsAbstractValidityCheck > &check : mChecks )
   {
     if ( check && check->checkType() == type )
@@ -62,10 +62,22 @@ void QgsValidityCheckRegistry::removeCheck( QgsAbstractValidityCheck *check )
 QList<QgsValidityCheckResult> QgsValidityCheckRegistry::runChecks( int type, const QgsValidityCheckContext *context, QgsFeedback *feedback ) const
 {
   QList<QgsValidityCheckResult> result;
-  const QList<QgsAbstractValidityCheck *> toCheck = checks( type );
+  const std::vector<std::unique_ptr<QgsAbstractValidityCheck> > toCheck = createChecks( type );
   int i = 0;
-  for ( QgsAbstractValidityCheck *check : toCheck )
+  for ( const std::unique_ptr< QgsAbstractValidityCheck > &check : toCheck )
   {
+    if ( feedback && feedback->isCanceled() )
+      break;
+
+    if ( !check->prepareCheck( context, feedback ) )
+    {
+      if ( feedback )
+      {
+        feedback->setProgress( static_cast< double >( i ) / toCheck.size() * 100 );
+      }
+      continue;
+    }
+
     const QList< QgsValidityCheckResult > checkResults = check->runCheck( context, feedback );
     for ( QgsValidityCheckResult checkResult : checkResults )
     {
@@ -75,11 +87,20 @@ QList<QgsValidityCheckResult> QgsValidityCheckRegistry::runChecks( int type, con
     i++;
     if ( feedback )
     {
-      if ( feedback->isCanceled() )
-        break;
-
-      feedback->setProgress( static_cast< double >( i ) / toCheck.count() * 100 );
+      feedback->setProgress( static_cast< double >( i ) / toCheck.size() * 100 );
     }
   }
   return result;
 }
+
+std::vector<std::unique_ptr<QgsAbstractValidityCheck> > QgsValidityCheckRegistry::createChecks( int type ) const
+{
+  const QList< const QgsAbstractValidityCheck *> toCheck = checks( type );
+  std::vector<std::unique_ptr<QgsAbstractValidityCheck> > results;
+  results.reserve( toCheck.size() );
+  for ( const QgsAbstractValidityCheck *check : toCheck )
+  {
+    results.emplace_back( std::unique_ptr< QgsAbstractValidityCheck >( check->create() ) );
+  }
+  return results;
+}