QgsTask is no longer a QRunnable

Instead use a private wrapper class to make QgsTask compatible with QThreadPool
qgis · Dec 5, 2016 · 297b572 · 297b572
1 parent 29f310c
commit 297b572
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 33 deletions.
diff --git a/python/core/__init__.py b/python/core/__init__.py
@@ -208,7 +208,7 @@ def __init__(self, description, flags, function, on_finished, *args, **kwargs):
         self.returned_values = None
         self.exception = None
 
-    def _run(self):
+    def run(self):
         try:
             self.returned_values = self.function(self, *self.args, **self.kwargs)
         except Exception as ex:

diff --git a/python/core/qgstaskmanager.sip b/python/core/qgstaskmanager.sip
@@ -14,7 +14,7 @@
  *
  * \note Added in version 3.0
  */
-class QgsTask : QObject, QRunnable
+class QgsTask : QObject
 {
 
 %TypeHeaderCode
@@ -92,7 +92,7 @@ class QgsTask : QObject, QRunnable
      * then this method should not be called directly, instead it is left to the
      * task manager to start the task when appropriate.
      */
-    void run();
+    void start();
 
     /**
      * Notifies the task that it should terminate. Calling this is not guaranteed
@@ -211,7 +211,7 @@ class QgsTask : QObject, QRunnable
      * @see completed()
      * @see terminated()
      */
-    virtual TaskResult _run() = 0;
+    virtual TaskResult run() = 0;
 
     /**
      * If the task is managed by a QgsTaskManager, this will be called after the

diff --git a/src/core/qgstaskmanager.cpp b/src/core/qgstaskmanager.cpp
@@ -34,23 +34,19 @@ QgsTask::QgsTask( const QString &name, const Flags& flags )
     , mTotalProgress( 0.0 )
     , mShouldTerminate( false )
     , mStartCount( 0 )
-{
-  setAutoDelete( false );
-}
+{}
 
 QgsTask::~QgsTask()
 {
   Q_ASSERT_X( mStatus != Running, "delete", QString( "status was %1" ).arg( mStatus ).toLatin1() );
 
-  QThreadPool::globalInstance()->cancel( this );
-
   Q_FOREACH ( const SubTask& subTask, mSubTasks )
   {
     delete subTask.task;
   }
 }
 
-void QgsTask::run()
+void QgsTask::start()
 {
   mStartCount++;
   Q_ASSERT( mStartCount == 1 );
@@ -66,7 +62,7 @@ void QgsTask::run()
   // force initial emission of progressChanged, but respect if task has had initial progress manually set
   setProgress( mProgress );
 
-  TaskResult result = _run();
+  TaskResult result = run();
   switch ( result )
   {
     case ResultSuccess:
@@ -94,7 +90,6 @@ void QgsTask::cancel()
 
 #if QT_VERSION >= 0x050500
   //can't cancel queued tasks with qt < 5.5
-  QThreadPool::globalInstance()->cancel( this );
   if ( mStatus == Queued || mStatus == OnHold )
   {
     // immediately terminate unstarted jobs
@@ -296,6 +291,35 @@ void QgsTask::terminated()
 }
 
 
+///@cond PRIVATE
+
+class QgsTaskRunnableWrapper : public QRunnable
+{
+  public:
+
+    QgsTaskRunnableWrapper( QgsTask* task )
+        : QRunnable()
+        , mTask( task )
+    {
+      setAutoDelete( true );
+    }
+
+    void run() override
+    {
+      Q_ASSERT( mTask );
+      mTask->start();
+    }
+
+  private:
+
+    QgsTask* mTask;
+
+};
+
+///@endcond
+
+
+
 //
 // QgsTaskManager
 //
@@ -594,6 +618,11 @@ void QgsTaskManager::taskStatusChanged( int status )
   if ( id < 0 )
     return;
 
+#if QT_VERSION >= 0x050500
+  QgsTaskRunnableWrapper* runnable = mTasks.value( id ).runnable;
+  QThreadPool::globalInstance()->cancel( runnable );
+#endif
+
   if ( status == QgsTask::Terminated || status == QgsTask::Complete )
   {
     QgsTask::TaskResult result = status == QgsTask::Complete ? QgsTask::ResultSuccess
@@ -665,6 +694,8 @@ bool QgsTaskManager::cleanupAndDeleteTask( QgsTask *task )
   if ( id < 0 )
     return false;
 
+  QgsTaskRunnableWrapper* runnable = mTasks.value( id ).runnable;
+
   task->disconnect( this );
 
   mDependenciesMutex->lockForWrite();
@@ -707,7 +738,7 @@ bool QgsTaskManager::cleanupAndDeleteTask( QgsTask *task )
   else
   {
 #if QT_VERSION >= 0x050500
-    QThreadPool::globalInstance()->cancel( task );
+    QThreadPool::globalInstance()->cancel( runnable );
 #endif
     if ( isParent )
     {
@@ -741,7 +772,7 @@ void QgsTaskManager::processQueue()
     QgsTask* task = it.value().task;
     if ( task && task->mStatus == QgsTask::Queued && dependenciesSatisified( it.key() ) && it.value().added.testAndSetRelaxed( 0, 1 ) )
     {
-      QThreadPool::globalInstance()->start( task, it.value().priority );
+      QThreadPool::globalInstance()->start( it.value().runnable, it.value().priority );
     }
 
     if ( task && ( task->mStatus != QgsTask::Complete && task->mStatus != QgsTask::Terminated ) )
@@ -792,3 +823,10 @@ void QgsTaskManager::cancelDependentTasks( long taskId )
     }
   }
 }
+
+QgsTaskManager::TaskInfo::TaskInfo( QgsTask* task, int priority )
+    : task( task )
+    , added( 0 )
+    , priority( priority )
+    , runnable( new QgsTaskRunnableWrapper( task ) )
+{}
diff --git a/src/core/qgstaskmanager.h b/src/core/qgstaskmanager.h
@@ -24,6 +24,7 @@
 #include <QReadWriteLock>
 
 class QgsTask;
+class QgsTaskRunnableWrapper;
 
 //! List of QgsTask objects
 typedef QList< QgsTask* > QgsTaskList;
@@ -35,7 +36,7 @@ typedef QList< QgsTask* > QgsTaskList;
  * or added to a QgsTaskManager for automatic management.
  *
  * Derived classes should implement the process they want to execute in the background
- * within the _run() method. This method will be called when the
+ * within the run() method. This method will be called when the
  * task commences (ie via calling run() ).
  *
  * Long running tasks should periodically check the isCancelled() flag to detect if the task
@@ -44,7 +45,7 @@ typedef QList< QgsTask* > QgsTaskList;
  *
  * \note Added in version 3.0
  */
-class CORE_EXPORT QgsTask : public QObject, public QRunnable
+class CORE_EXPORT QgsTask : public QObject
 {
     Q_OBJECT
 
@@ -122,7 +123,7 @@ class CORE_EXPORT QgsTask : public QObject, public QRunnable
      * then this method should not be called directly, instead it is left to the
      * task manager to start the task when appropriate.
      */
-    void run() override;
+    void start();
 
     /**
      * Notifies the task that it should terminate. Calling this is not guaranteed
@@ -242,7 +243,7 @@ class CORE_EXPORT QgsTask : public QObject, public QRunnable
      * @see completed()
      * @see terminated()
      */
-    virtual TaskResult _run() = 0;
+    virtual TaskResult run() = 0;
 
     /**
      * If the task is managed by a QgsTaskManager, this will be called after the
@@ -334,7 +335,6 @@ class CORE_EXPORT QgsTask : public QObject, public QRunnable
 
 Q_DECLARE_OPERATORS_FOR_FLAGS( QgsTask::Flags )
 
-
 /** \ingroup core
  * \class QgsTaskManager
  * \brief Task manager for managing a set of long-running QgsTask tasks. This class can be created directly,
@@ -496,14 +496,11 @@ class CORE_EXPORT QgsTaskManager : public QObject
 
     struct TaskInfo
     {
-      TaskInfo( QgsTask* task = nullptr, int priority = 0 )
-          : task( task )
-          , added( 0 )
-          , priority( priority )
-      {}
+      TaskInfo( QgsTask* task = nullptr, int priority = 0 );
       QgsTask* task;
       QAtomicInt added;
       int priority;
+      QgsTaskRunnableWrapper* runnable;
     };
 
     mutable QReadWriteLock* mTaskMutex;

diff --git a/tests/src/core/testqgstaskmanager.cpp b/tests/src/core/testqgstaskmanager.cpp
@@ -40,7 +40,7 @@ class TestTask : public QgsTask
 
   protected:
 
-    TaskResult _run() override
+    TaskResult run() override
     {
       runCalled = true;
       return ResultPending;
@@ -62,7 +62,7 @@ class TestTerminationTask : public TestTask
 
   protected:
 
-    TaskResult _run() override
+    TaskResult run() override
     {
       while ( !isCancelled() )
         {}
@@ -85,7 +85,7 @@ class CancelableTask : public QgsTask
 
   protected:
 
-    TaskResult _run() override
+    TaskResult run() override
     {
       while ( !isCancelled() )
         {}
@@ -99,7 +99,7 @@ class SuccessTask : public QgsTask
 
   protected:
 
-    TaskResult _run() override
+    TaskResult run() override
     {
       return ResultSuccess;
     }
@@ -111,7 +111,7 @@ class FailTask : public QgsTask
 
   protected:
 
-    TaskResult _run() override
+    TaskResult run() override
     {
       return ResultFail;
     }
@@ -134,7 +134,7 @@ class FinishTask : public QgsTask
 
   protected:
 
-    TaskResult _run() override
+    TaskResult run() override
     {
       return desiredResult;
     }
@@ -218,7 +218,7 @@ void TestQgsTaskManager::task()
   QSignalSpy startedSpy( task.data(), &QgsTask::begun );
   QSignalSpy statusSpy( task.data(), &QgsTask::statusChanged );
 
-  task->run();
+  task->start();
   QCOMPARE( task->status(), QgsTask::Running );
   QVERIFY( task->isActive() );
   QVERIFY( task->runCalled );
@@ -270,7 +270,7 @@ void TestQgsTaskManager::taskResult()
   QCOMPARE( task->status(), QgsTask::Queued );
   QSignalSpy statusSpy( task.data(), &QgsTask::statusChanged );
 
-  task->run();
+  task->start();
   QCOMPARE( statusSpy.count(), 2 );
   QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy.at( 0 ).at( 0 ).toInt() ), QgsTask::Running );
   QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy.at( 1 ).at( 0 ).toInt() ), QgsTask::Complete );
@@ -280,7 +280,7 @@ void TestQgsTaskManager::taskResult()
   QCOMPARE( task->status(), QgsTask::Queued );
   QSignalSpy statusSpy2( task.data(), &QgsTask::statusChanged );
 
-  task->run();
+  task->start();
   QCOMPARE( statusSpy2.count(), 2 );
   QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy2.at( 0 ).at( 0 ).toInt() ), QgsTask::Running );
   QCOMPARE( static_cast< QgsTask::TaskStatus >( statusSpy2.at( 1 ).at( 0 ).toInt() ), QgsTask::Terminated );