Apply Review

Co-authored-by: Andras Lasso <[email protected]>

Libs/Core/ctkAbstractJob.cpp

@@ -213,20 +213,15 @@ void ctkAbstractJob::setRunningThreadID(QString runningThreadID)
 }
 
 //----------------------------------------------------------------------------
-QString ctkAbstractJob::loggedText() const
+QString ctkAbstractJob::log() const
 {
-  return this->LoggedText;
+  return this->Log;
 }
 
 //----------------------------------------------------------------------------
-void ctkAbstractJob::addLoggedText(QString loggedText)
+void ctkAbstractJob::addLog(QString log)
 {
-  if (loggedText.isEmpty())
-  {
-    return;
-  }
-
-  this->LoggedText += loggedText;
+  this->Log += log;
 }
 
 //----------------------------------------------------------------------------

Libs/Core/ctkAbstractJob.h

@@ -55,7 +55,7 @@ class CTK_CORE_EXPORT ctkAbstractJob : public QObject
   Q_PROPERTY(QDateTime startDateTime READ startDateTime);
   Q_PROPERTY(QDateTime completionDateTime READ completionDateTime);
   Q_PROPERTY(QString runningThreadID READ runningThreadID WRITE setRunningThreadID);
-  Q_PROPERTY(QString loggedText READ loggedText WRITE addLoggedText);
+  Q_PROPERTY(QString log READ log);
   Q_PROPERTY(bool destroyAfterUse READ destroyAfterUse WRITE setDestroyAfterUse);
 
 public:
@@ -158,8 +158,8 @@ class CTK_CORE_EXPORT ctkAbstractJob : public QObject
 
   ///@{
   /// Logged Text
-  QString loggedText() const;
-  void addLoggedText(QString loggedText);
+  QString log() const;
+  void addLog(QString log);
   ///@}
 
   /// Generate worker for job
@@ -179,10 +179,10 @@ class CTK_CORE_EXPORT ctkAbstractJob : public QObject
   Q_INVOKABLE virtual QVariant toVariant();
 
   /// Free used resources from job after worker is done
-  Q_INVOKABLE virtual void freeUsedResources() = 0;
+  Q_INVOKABLE virtual void releaseResources() = 0;
 
   ///@{
-  /// Destroy job pointer after worker is done
+  /// Destroy job object after worker is done
   /// default: false
   bool destroyAfterUse() const;
   void setDestroyAfterUse(bool destroyAfterUse);
@@ -208,7 +208,7 @@ class CTK_CORE_EXPORT ctkAbstractJob : public QObject
   QDateTime StartDateTime;
   QDateTime CompletionDateTime;
   QString RunningThreadID;
-  QString LoggedText;
+  QString Log;
   bool DestroyAfterUse;
 
 private:
@@ -227,7 +227,7 @@ struct CTK_CORE_EXPORT ctkJobDetail {
     this->StartDateTime = job.startDateTime().toString("HH:mm:ss.zzz ddd dd MMM yyyy");
     this->CompletionDateTime = job.completionDateTime().toString("HH:mm:ss.zzz ddd dd MMM yyyy");
     this->RunningThreadID = job.runningThreadID();
-    this->Logging = job.loggedText();
+    this->Logging = job.log();
   }
   virtual ~ctkJobDetail() = default;
 

Libs/Core/ctkCorePythonQtDecorators.h

@@ -27,8 +27,9 @@
 // CTK includes
 #include <ctkAbstractJob.h> // For ctkJobDetail
 #include <ctkBooleanMapper.h>
-#include <ctkUtils.h>
 #include <ctkErrorLogContext.h>
+#include <ctkLogger.h>
+#include <ctkUtils.h>
 #include <ctkWorkflowStep.h>
 #include <ctkWorkflowTransitions.h>
 
@@ -39,6 +40,8 @@
 // for non-static methods.
 //
 
+static ctkLogger logger("org.commontk.core.ctkCorePythonQtDecorators");
+
 /// \ingroup Core
 class ctkCorePythonQtDecorators : public QObject
 {
@@ -245,64 +248,147 @@ public Q_SLOTS:
 
   void setJobClass(ctkJobDetail* td, const QString& jobClass)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::setJobClass - Invalid ctkJobDetail");
+      return;
+    }
+
     td->JobClass = jobClass;
   }
   QString jobClass(ctkJobDetail* td)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::jobClass - Invalid ctkJobDetail");
+      return "";
+    }
+
     return td->JobClass;
   }
 
   void setJobUID(ctkJobDetail* td, const QString& jobUID)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::setJobUID - Invalid ctkJobDetail");
+      return;
+    }
+
     td->JobUID = jobUID;
   }
   QString JobUID(ctkJobDetail* td)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::JobUID - Invalid ctkJobDetail");
+      return "";
+    }
+
     return td->JobUID;
   }
-    void setCreationDateTime(ctkJobDetail* td, QString creationDateTime)
+
+  void setCreationDateTime(ctkJobDetail* td, QString creationDateTime)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::setCreationDateTime - Invalid ctkJobDetail");
+      return;
+    }
+
     td->CreationDateTime = creationDateTime;
   }
-
   QString creationDateTime(ctkJobDetail* td)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::creationDateTime - Invalid ctkJobDetail");
+      return "";
+    }
+
     return td->CreationDateTime;
   }
 
   void setStartDateTime(ctkJobDetail* td, QString startDateTime)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::setStartDateTime - Invalid ctkJobDetail");
+      return;
+    }
+
     td->StartDateTime = startDateTime;
   }
   QString startDateTime(ctkJobDetail* td)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::startDateTime - Invalid ctkJobDetail");
+      return "";
+    }
+
     return td->StartDateTime;
   }
 
   void setCompletionDateTime(ctkJobDetail* td, QString completionDateTime)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::setCompletionDateTime - Invalid ctkJobDetail");
+      return;
+    }
+
     td->CompletionDateTime = completionDateTime;
   }
   QString completionDateTime(ctkJobDetail* td)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::completionDateTime - Invalid ctkJobDetail");
+      return "";
+    }
+
     return td->CompletionDateTime;
   }
 
   void setRunningThreadID(ctkJobDetail* td, QString runningThreadID)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::setRunningThreadID - Invalid ctkJobDetail");
+      return;
+    }
+
     td->RunningThreadID = runningThreadID;
   }
   QString runningThreadID(ctkJobDetail* td)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::runningThreadID - Invalid ctkJobDetail");
+      return "";
+    }
+
     return td->RunningThreadID;
   }
 
   void setLogging(ctkJobDetail* td, QString logging)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::setLogging - Invalid ctkJobDetail");
+      return;
+    }
     td->Logging = logging;
   }
   QString logging(ctkJobDetail* td)
   {
+    if (td == nullptr)
+    {
+      logger.error("ctkJobDetail::logging - Invalid ctkJobDetail");
+      return "";
+    }
+
     return td->Logging;
   }
 };

Libs/Core/ctkJobScheduler.cpp

@@ -65,7 +65,7 @@ void ctkJobSchedulerPrivate::init()
 }
 
 //------------------------------------------------------------------------------
-void ctkJobSchedulerPrivate::onQueueJobsInThreadPool()
+void ctkJobSchedulerPrivate::queueJobsInThreadPool()
 {
   Q_Q(ctkJobScheduler);
 
@@ -74,6 +74,9 @@ void ctkJobSchedulerPrivate::onQueueJobsInThreadPool()
     return;
   }
 
+  // No need to queue jobs with a signal/slot mechanism, since the mutex makes
+  // sure that concurrent threads append/clean/delete the jobs map.
+
   {
     // The QMutexLocker is enclosed within brackets to restrict its scope and
     // prevent conflicts with other QMutexLockers within the scheduler's methods.
@@ -190,7 +193,7 @@ bool ctkJobSchedulerPrivate::insertJob(QSharedPointer<ctkAbstractJob> job)
   }
 
   emit q->jobInitialized(job->toVariant());
-  this->onQueueJobsInThreadPool();
+  this->queueJobsInThreadPool();
   return true;
 }
 
@@ -211,10 +214,10 @@ bool ctkJobSchedulerPrivate::cleanJob(const QString &jobUID)
       return false;
     }
 
-    job->freeUsedResources();
+    job->releaseResources();
   }
 
-  this->onQueueJobsInThreadPool();
+  this->queueJobsInThreadPool();
   return true;
 }
 
@@ -223,7 +226,7 @@ void ctkJobSchedulerPrivate::cleanJobs(const QStringList &jobUIDs)
 {
   Q_Q(ctkJobScheduler);
 
-  QList<QVariant> datas;
+  QList<QVariant> dataObjects;
   {
     // The QMutexLocker is enclosed within brackets to restrict its scope and
     // prevent conflicts with other QMutexLockers within the scheduler's methods.
@@ -237,12 +240,12 @@ void ctkJobSchedulerPrivate::cleanJobs(const QStringList &jobUIDs)
         continue;
       }
 
-      datas.append(job->toVariant());
-      job->freeUsedResources();
+      dataObjects.append(job->toVariant());
+      job->releaseResources();
     }
   }
 
-  emit q->jobUserStopped(datas);
+  emit q->jobUserStopped(dataObjects);
 }
 
 //------------------------------------------------------------------------------
@@ -274,14 +277,14 @@ bool ctkJobSchedulerPrivate::removeJob(const QString& jobUID)
     this->JobsQueue.remove(jobUID);
   }
 
-  this->onQueueJobsInThreadPool();
+  this->queueJobsInThreadPool();
   return true;
 }
 
 //------------------------------------------------------------------------------
 void ctkJobSchedulerPrivate::removeJobs(const QStringList &jobUIDs)
 {
-  QList<QVariant> datas;
+  QList<QVariant> dataObjects;
   {
     // The QMutexLocker is enclosed within brackets to restrict its scope and
     // prevent conflicts with other QMutexLockers within the scheduler's methods.
@@ -295,7 +298,7 @@ void ctkJobSchedulerPrivate::removeJobs(const QStringList &jobUIDs)
         continue;
       }
 
-      datas.append(job->toVariant());
+      dataObjects.append(job->toVariant());
 
       QMap<QString, QMetaObject::Connection> connections = this->JobsConnections.value(jobUID);
       QObject::disconnect(connections.value("started"));
@@ -720,7 +723,7 @@ bool ctkJobScheduler::retryJob(const QString &jobUID)
 
   job->setStatus(ctkAbstractJob::JobStatus::Initialized);
   emit this->jobInitialized(job->toVariant());
-  d->onQueueJobsInThreadPool();
+  d->queueJobsInThreadPool();
   return true;
 }
 

Libs/Core/ctkJobScheduler_p.h

@@ -44,9 +44,6 @@ class CTK_CORE_EXPORT ctkJobSchedulerPrivate : public QObject
 protected:
   ctkJobScheduler* const q_ptr;
 
-public Q_SLOTS:
-  virtual void onQueueJobsInThreadPool();
-
 public:
   ctkJobSchedulerPrivate(ctkJobScheduler& object);
   virtual ~ctkJobSchedulerPrivate();
@@ -59,9 +56,10 @@ public Q_SLOTS:
   virtual void cleanJobs(const QStringList& jobUIDs);
   virtual bool removeJob(const QString& jobUID);
   virtual void removeJobs(const QStringList& jobUIDs);
-  int getSameTypeJobsInThreadPoolQueueOrRunning(QSharedPointer<ctkAbstractJob> job);
-  QString generateUniqueJobUID();
-  void clearBactchedJobsLists();
+  virtual int getSameTypeJobsInThreadPoolQueueOrRunning(QSharedPointer<ctkAbstractJob> job);
+  virtual QString generateUniqueJobUID();
+  virtual void queueJobsInThreadPool();
+  virtual void clearBactchedJobsLists();
 
   QMutex QueueMutex;