Merge pull request #807 from lassoan/fix-dicom-database-reinsertions

BUG: Fixed failed re-import of DICOM images

Libs/DICOM/Core/Testing/Cpp/ctkDICOMIndexerTest1.cpp

@@ -38,11 +38,15 @@ int ctkDICOMIndexerTest1( int argc, char * argv [] )
 
   // Test ctkDICOMIndexer::addDirectory()
   // just check if it doesn't crash
-  indexer.addDirectory(database, QString());
-  // might work (if there are some DCM images in temp
-  indexer.addDirectory(database, QDir::tempPath());
-  // give an invalid destination name
-  indexer.addDirectory(database, QDir::tempPath(), QDir::tempPath() + "/@#$%^&*{}[]");
+  // Create block to test batch indexing using indexingBatch helper class.
+  {
+    ctkDICOMIndexer::ScopedIndexing indexingBatch(indexer, database);
+    indexer.addDirectory(database, QString());
+    // might work (if there are some DCM images in temp
+    indexer.addDirectory(database, QDir::tempPath());
+    // give an invalid destination name
+    indexer.addDirectory(database, QDir::tempPath(), QDir::tempPath() + "/@#$%^&*{}[]");
+  }
 
   // make sure it doesn't crash
   indexer.refreshDatabase(database, QString());

Libs/DICOM/Core/ctkDICOMDatabase.cpp

@@ -1039,6 +1039,19 @@ QString ctkDICOMDatabase::groupElementToTag(const unsigned short& group, const u
 //
 
 //------------------------------------------------------------------------------
+void ctkDICOMDatabase::prepareInsert()
+{
+  Q_D(ctkDICOMDatabase);
+  // Although resetLastInsertedValues is called when items are deleted through
+  // this database connection, there may be concurrent database modifications
+  // (even in the same application) through other connections.
+  // Therefore, we cannot be sure that the last added patient, study, series,
+  // items are still in the database. We clear cached Last... IDs to make sure
+  // the patient, study, series items are created.
+  d->resetLastInsertedValues();
+}
+
+//------------------------------------------------------------------------------
 void ctkDICOMDatabase::insert( DcmItem *item, bool storeFile, bool generateThumbnail)
 {
   if (!item)

Libs/DICOM/Core/ctkDICOMDatabase.h

@@ -54,8 +54,10 @@ class CTK_DICOM_CORE_EXPORT ctkDICOMDatabase : public QObject
 
   Q_OBJECT
   Q_PROPERTY(bool isOpen READ isOpen)
+  Q_PROPERTY(bool isInMemory READ isInMemory)
   Q_PROPERTY(QString lastError READ lastError)
   Q_PROPERTY(QString databaseFilename READ databaseFilename)
+  Q_PROPERTY(QString databaseDirectory READ databaseDirectory)
   Q_PROPERTY(QStringList tagsToPrecache READ tagsToPrecache WRITE setTagsToPrecache)
 
 public:
@@ -87,10 +89,10 @@ public:
 
   ///
   /// set thumbnail generator object
-  void setThumbnailGenerator(ctkDICOMAbstractThumbnailGenerator* generator);
+  Q_INVOKABLE void setThumbnailGenerator(ctkDICOMAbstractThumbnailGenerator* generator);
   ///
   /// get thumbnail genrator object
-  ctkDICOMAbstractThumbnailGenerator* thumbnailGenerator();
+  Q_INVOKABLE ctkDICOMAbstractThumbnailGenerator* thumbnailGenerator();
 
   ///
   /// open the SQLite database in @param databaseFile . If the file does not
@@ -197,15 +199,27 @@ public:
                             bool createHierarchy = true,
                             const QString& destinationDirectoryName = QString() );
 
+  /// Reset cached item IDs to make sure previous
+  /// inserts do not interfere with upcoming insert operations.
+  /// Typically, it should be call just before a batch of files
+  /// insertion is started.
+  ///
+  /// This has to be called before an insert() call if there is a chance
+  /// that items have been deleted from the database since the
+  /// the last insert() call. If there has been not been any insert() calls since
+  /// connected to the database, then it should be called before the first
+  /// insert().
+  Q_INVOKABLE void prepareInsert();
+
   /// Check if file is already in database and up-to-date
-  bool fileExistsAndUpToDate(const QString& filePath);
+  Q_INVOKABLE bool fileExistsAndUpToDate(const QString& filePath);
 
   /// remove the series from the database, including images and
   /// thumbnails
   Q_INVOKABLE bool removeSeries(const QString& seriesInstanceUID);
   Q_INVOKABLE bool removeStudy(const QString& studyInstanceUID);
   Q_INVOKABLE bool removePatient(const QString& patientID);
-  bool cleanup();
+  Q_INVOKABLE bool cleanup();
 
   ///
   /// \brief access element values for given instance

Libs/DICOM/Core/ctkDICOMIndexer.cpp

@@ -59,7 +59,10 @@ static ctkLogger logger("org.commontk.dicom.DICOMIndexer" );
 // ctkDICOMIndexerPrivate methods
 
 //------------------------------------------------------------------------------
-ctkDICOMIndexerPrivate::ctkDICOMIndexerPrivate(ctkDICOMIndexer& o) : q_ptr(&o), Canceled(false)
+ctkDICOMIndexerPrivate::ctkDICOMIndexerPrivate(ctkDICOMIndexer& o)
+  : q_ptr(&o)
+  , Canceled(false)
+  , StartedIndexing(0)
 {
 }
 
@@ -89,7 +92,7 @@ void ctkDICOMIndexer::addFile(ctkDICOMDatabase& database,
                                    const QString filePath,
                                    const QString& destinationDirectoryName)
 {
-  std::cout << filePath.toStdString();
+  ctkDICOMIndexer::ScopedIndexing indexingBatch(*this, database);
   if (!destinationDirectoryName.isEmpty())
   {
     logger.warn("Ignoring destinationDirectoryName parameter, just taking it as indication we should copy!");
@@ -101,17 +104,18 @@ void ctkDICOMIndexer::addFile(ctkDICOMDatabase& database,
 }
 
 //------------------------------------------------------------------------------
-void ctkDICOMIndexer::addDirectory(ctkDICOMDatabase& ctkDICOMDatabase,
+void ctkDICOMIndexer::addDirectory(ctkDICOMDatabase& database,
                                    const QString& directoryName,
                                    const QString& destinationDirectoryName,
                                    bool includeHidden/*=true*/)
 {
+  ctkDICOMIndexer::ScopedIndexing indexingBatch(*this, database);
   QStringList listOfFiles;
   QDir directory(directoryName);
 
   if(directory.exists("DICOMDIR"))
   {
-    addDicomdir(ctkDICOMDatabase,directoryName,destinationDirectoryName);
+    addDicomdir(database,directoryName,destinationDirectoryName);
   }
   else
   {
@@ -126,16 +130,17 @@ void ctkDICOMIndexer::addDirectory(ctkDICOMDatabase& ctkDICOMDatabase,
       listOfFiles << it.next();
     }
     emit foundFilesToIndex(listOfFiles.count());
-    addListOfFiles(ctkDICOMDatabase,listOfFiles,destinationDirectoryName);
+    addListOfFiles(database,listOfFiles,destinationDirectoryName);
   }
 }
 
 //------------------------------------------------------------------------------
-void ctkDICOMIndexer::addListOfFiles(ctkDICOMDatabase& ctkDICOMDatabase,
+void ctkDICOMIndexer::addListOfFiles(ctkDICOMDatabase& database,
                                      const QStringList& listOfFiles,
                                      const QString& destinationDirectoryName)
 {
   Q_D(ctkDICOMIndexer);
+  ctkDICOMIndexer::ScopedIndexing indexingBatch(*this, database);
   QTime timeProbe;
   timeProbe.start();
   d->Canceled = false;
@@ -151,7 +156,7 @@ void ctkDICOMIndexer::addListOfFiles(ctkDICOMDatabase& ctkDICOMDatabase,
       emit this->progress(percent);
       lastReportedPercent = percent;
       }
-    this->addFile(ctkDICOMDatabase, filePath, destinationDirectoryName);
+    this->addFile(database, filePath, destinationDirectoryName);
     CurrentFileIndex++;
 
     if( d->Canceled )
@@ -164,15 +169,15 @@ void ctkDICOMIndexer::addListOfFiles(ctkDICOMDatabase& ctkDICOMDatabase,
       << QString("DICOM indexer has successfully processed %1 files [%2s]")
          .arg(CurrentFileIndex)
          .arg(QString::number(elapsedTimeInSeconds,'f', 2));
-  emit this->indexingComplete();
 }
 
 //------------------------------------------------------------------------------
-bool ctkDICOMIndexer::addDicomdir(ctkDICOMDatabase& ctkDICOMDatabase,
+bool ctkDICOMIndexer::addDicomdir(ctkDICOMDatabase& database,
                  const QString& directoryName,
                  const QString& destinationDirectoryName
                  )
 {
+  ctkDICOMIndexer::ScopedIndexing indexingBatch(*this, database);
   //Initialize dicomdir with directory path
   QString dcmFilePath = directoryName;
   dcmFilePath.append("/DICOMDIR");
@@ -260,15 +265,15 @@ bool ctkDICOMIndexer::addDicomdir(ctkDICOMDatabase& ctkDICOMDatabase,
            .arg(directoryName)
            .arg(QString::number(elapsedTimeInSeconds,'f', 2));
     emit foundFilesToIndex(listOfInstances.count());
-    addListOfFiles(ctkDICOMDatabase,listOfInstances,destinationDirectoryName);
+    addListOfFiles(database,listOfInstances,destinationDirectoryName);
   }
   return success;
 }
 
 //------------------------------------------------------------------------------
-void ctkDICOMIndexer::refreshDatabase(ctkDICOMDatabase& dicomDatabase, const QString& directoryName)
+void ctkDICOMIndexer::refreshDatabase(ctkDICOMDatabase& database, const QString& directoryName)
 {
-  Q_UNUSED(dicomDatabase);
+  Q_UNUSED(database);
   Q_UNUSED(directoryName);
   /*
    * Probably this should go to the database class as well
@@ -276,7 +281,7 @@ void ctkDICOMIndexer::refreshDatabase(ctkDICOMDatabase& dicomDatabase, const QSt
    * without using SQL directly
 
   /// get all filenames from the database
-  QSqlQuery allFilesQuery(dicomDatabase.database());
+  QSqlQuery allFilesQuery(database.database());
   QStringList databaseFileNames;
   QStringList filesToRemove;
   this->loggedExec(allFilesQuery, "SELECT Filename from Images;");
@@ -318,3 +323,32 @@ void ctkDICOMIndexer::cancel()
   Q_D(ctkDICOMIndexer);
   d->Canceled = true;
 }
+
+//----------------------------------------------------------------------------
+void ctkDICOMIndexer::startIndexing(ctkDICOMDatabase& database)
+{
+  Q_D(ctkDICOMIndexer);
+  if (d->StartedIndexing == 0)
+  {
+    // Indexing has just been started
+    database.prepareInsert();
+  }
+  d->StartedIndexing++;
+}
+
+//----------------------------------------------------------------------------
+void ctkDICOMIndexer::endIndexing()
+{
+  Q_D(ctkDICOMIndexer);
+  d->StartedIndexing--;
+  if (d->StartedIndexing == 0)
+  {
+    // Indexing has just been completed
+    emit this->indexingComplete();
+  }
+  if (d->StartedIndexing < 0)
+  {
+    qWarning() << QString("ctkDICOMIndexer::endIndexing called without matching startIndexing");
+    d->StartedIndexing = 0;
+  }
+}

Libs/DICOM/Core/ctkDICOMIndexer.h

@@ -95,6 +95,49 @@ public:
   ///
   Q_INVOKABLE void waitForImportFinished();
 
+  /// Call this before performing multiple add...() calls in one batch
+  /// to slightly increase indexing performance and to make only a single
+  /// indexingComplete() signal emitted for multiple add...() operations.
+  /// 
+  /// If startIndexing() is called before a batch of insertions, then
+  /// endIndexing() method must be called after the insertions are completed.
+  ///
+  /// It is recommended to use ScopedIndexing helper class to call startIndexing
+  /// and endIndexing automatically.
+  Q_INVOKABLE void startIndexing(ctkDICOMDatabase& database);
+
+  /// Call this method after batch insertion is completed, and only if startIndexing()
+  /// was called before batch insertion was started.
+  Q_INVOKABLE void endIndexing();
+
+  /// Helper class to automatically call startIndexing and endIndexing.
+  /// Its constructor calls startIndexing and its destructor calls endIndexing.
+  ///
+  /// Example:
+  ///   ...
+  ///   {
+  ///     ctkDICOMIndexer::ScopedIndexing indexingBatch(indexer, database); // this calls startIndexing
+  ///     indexer.addDirectory(database, dir1);
+  ///     indexer.addDirectory(database, dir2);
+  ///     indexer.addDirectory(database, dir3);
+  ///   } // endIndexing is called when indexingBatch goes out of scope
+  ///
+  class ScopedIndexing
+  {
+    public:
+    ScopedIndexing(ctkDICOMIndexer& indexer, ctkDICOMDatabase& database)
+    {
+      this->Indexer = &indexer;
+      this->Indexer->startIndexing(database);
+    }
+    ~ScopedIndexing()
+    {
+      this->Indexer->endIndexing();
+    }
+    private:
+    ctkDICOMIndexer* Indexer;
+  };
+
 Q_SIGNALS:
   void foundFilesToIndex(int);
   void indexingFileNumber(int);

Libs/DICOM/Core/ctkDICOMIndexer_p.h

@@ -42,6 +42,15 @@ public:
 public:
   ctkDICOMAbstractThumbnailGenerator* thumbnailGenerator;
   bool                    Canceled;
+
+  // Incremented each time startIndexing is called
+  // and decremented when endIndexing is called.
+  // This makes sure that when a batch of indexing
+  // operations are performed, at any level
+  // (file, folder, or set of folders) then
+  // batch processing initialization and finalization
+  // are performed exactly once.
+  int                     StartedIndexing;
 };
 
 

Libs/DICOM/Widgets/ctkDICOMBrowser.cpp

@@ -705,6 +705,16 @@ void ctkDICOMBrowser::importDirectories(QStringList directories, ctkDICOMBrowser
 {
   Q_D(ctkDICOMBrowser);
   ctkDICOMImportStats stats(d);
+
+  if (!d->DICOMDatabase || !d->DICOMIndexer)
+    {
+    qWarning() << Q_FUNC_INFO << " failed: database or indexer is invalid";
+    return;
+    }
+
+  // Only emit one indexingComplete event, when all imports have been completed
+  ctkDICOMIndexer::ScopedIndexing indexingBatch(*d->DICOMIndexer, *d->DICOMDatabase);
+
   foreach (const QString& directory, directories)
     {
     d->importDirectory(directory, mode);

Libs/DICOM/Widgets/ctkDICOMBrowser.h

@@ -56,11 +56,13 @@ class CTK_DICOM_WIDGETS_EXPORT ctkDICOMBrowser : public QWidget
 {
   Q_OBJECT
   Q_ENUMS(ImportDirectoryMode)
-  Q_PROPERTY(ctkDICOMDatabase* database READ database)
   Q_PROPERTY(QString databaseDirectory READ databaseDirectory WRITE setDatabaseDirectory)
+  Q_PROPERTY(int patientsAddedDuringImport READ patientsAddedDuringImport)
+  Q_PROPERTY(int studiesAddedDuringImport READ studiesAddedDuringImport)
+  Q_PROPERTY(int seriesAddedDuringImport READ seriesAddedDuringImport)
+  Q_PROPERTY(int instancesAddedDuringImport READ instancesAddedDuringImport)
   Q_PROPERTY(QStringList tagsToPrecache READ tagsToPrecache WRITE setTagsToPrecache)
   Q_PROPERTY(bool displayImportSummary READ displayImportSummary WRITE setDisplayImportSummary)
-  Q_PROPERTY(ctkDICOMTableManager* dicomTableManager READ dicomTableManager)
   Q_PROPERTY(ctkDICOMBrowser::ImportDirectoryMode ImportDirectoryMode READ importDirectoryMode WRITE setImportDirectoryMode)
 
 public:
@@ -74,7 +76,7 @@ public:
   QString databaseDirectory() const;
 
   /// Return settings key used to store the directory.
-  static QString databaseDirectorySettingsKey();
+  Q_INVOKABLE static QString databaseDirectorySettingsKey();
 
   /// See ctkDICOMDatabase for description - these accessors
   /// delegate to the corresponding routines of the internal
@@ -86,11 +88,11 @@ public:
   /// Updates schema of loaded database to match the one
   /// coded by the current version of ctkDICOMDatabase.
   /// Also provides a dialog box for progress
-  void updateDatabaseSchemaIfNeeded();
+  Q_INVOKABLE void updateDatabaseSchemaIfNeeded();
 
-  ctkDICOMDatabase* database();
+  Q_INVOKABLE ctkDICOMDatabase* database();
 
-  ctkDICOMTableManager* dicomTableManager();
+  Q_INVOKABLE ctkDICOMTableManager* dicomTableManager();
 
   /// Option to show or not import summary dialog.
   /// Since the summary dialog is modal, we give the option

Libs/DICOM/Widgets/ctkDICOMTableManager.h

@@ -50,12 +50,6 @@ class CTK_DICOM_WIDGETS_EXPORT ctkDICOMTableManager : public QWidget
     */
   Q_PROPERTY(bool dynamicTableLayout READ dynamicTableLayout WRITE setDynamicTableLayout)
 
-  /**
-  * Properties for the different table views (patients, studies, series). 
-  */
-  Q_PROPERTY( ctkDICOMTableView* patientsTable READ patientsTable )
-  Q_PROPERTY( ctkDICOMTableView* studiesTable READ studiesTable )
-  Q_PROPERTY( ctkDICOMTableView* seriesTable READ seriesTable )
 
   Q_ENUMS(DisplayDensity)
   /**
@@ -76,7 +70,7 @@ public:
    * @brief Set the ctkDICOMDatabase
    * @param db the dicom database which should be used
    */
-  void setDICOMDatabase(ctkDICOMDatabase* db);
+  Q_INVOKABLE void setDICOMDatabase(ctkDICOMDatabase* db);
 
   void setTableOrientation(const Qt::Orientation&) const;
   Qt::Orientation tableOrientation();
@@ -85,14 +79,14 @@ public:
    * @brief Get the current selection of the dicomTableViews
    * @return a list with the uids of the selected items
    */
-  QStringList currentPatientsSelection();
-  QStringList currentStudiesSelection();
-  QStringList currentSeriesSelection();
+  Q_INVOKABLE QStringList currentPatientsSelection();
+  Q_INVOKABLE QStringList currentStudiesSelection();
+  Q_INVOKABLE QStringList currentSeriesSelection();
 
   void setDynamicTableLayout(bool);
   bool dynamicTableLayout() const;
 
-  void updateTableViews();
+  Q_INVOKABLE void updateTableViews();
 
   enum DisplayDensity
   {
@@ -104,9 +98,9 @@ public:
   DisplayDensity displayDensity();
   void setDisplayDensity(DisplayDensity density);
 
-  ctkDICOMTableView* patientsTable();
-  ctkDICOMTableView* studiesTable();
-  ctkDICOMTableView* seriesTable();
+  Q_INVOKABLE ctkDICOMTableView* patientsTable();
+  Q_INVOKABLE ctkDICOMTableView* studiesTable();
+  Q_INVOKABLE ctkDICOMTableView* seriesTable();
 
 
 public Q_SLOTS: