BUG: Fixed failed re-import of DICOM images

IDs of last inserted patient, study, series items were cached in the ctkDICOMDatabase to avoid re-inserting when adding each file instance. However, if this patient, study, or series are removed from the database, then their IDs have to be cleared to force adding them back to the database when adding a file instance. Clearing of IDs when items are deleted has been implemented in ctkDICOMDatabase. The problems is that an application may use multiple database connections (ctkDICOMDatabase instances) to connect to the same database, and delete items corresponding to the cached IDs.

Fixed the problem by deleting cached item IDs once, just before a batch of instances are inserted to the database.

To define what is the beginning and end of a "batch", startIndexing and endIndexing method was added to ctkDICOMIndexer. They are called automatically when any of the ctkDICOMIndexer::add...() methods is used. Start/end indexing methods are exposed on the public API as well so that applications can do multiple ctkDICOMIndexer::add...() calls in one batch.

Also fixed/completed Python wrapping for ctkDICOMTableManager, ctkDICOMBrowser, and ctkDICOMDatabase.

Fixes https://issues.slicer.org/view.php?id=3806

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: