Merge branch '292-non-threaded-inserts'

* 292-non-threaded-inserts:
  Update test to reflect current behavior of database/cache
  Remove threading code from dicom database and indexer
  Only use sql transactions for tag cache
  Update the progress dialog as files are indexed
  Remove threading to avoid crashes (see #292)
  Improve sql safety and performance

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

@@ -136,9 +136,9 @@ int ctkDICOMDatabaseTest2( int argc, char * argv [] )
   // Test the tag cache
   //
 
-  if (database.tagCacheExists())
+  if (!database.tagCacheExists())
     {
-    std::cerr << "ctkDICOMDatabase: tag cache should not exist in fresh database" << std::endl;
+    std::cerr << "ctkDICOMDatabase: tag cache should be configured when database opens" << std::endl;
     return EXIT_FAILURE;
     }
 

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

@@ -54,8 +54,5 @@ int ctkDICOMIndexerTest1( int argc, char * argv [] )
   // make sure it doesn't crash
   indexer.refreshDatabase(database, QDir::tempPath());
 
-  // ensure all concurrent inserts are complete
-  indexer.waitForImportFinished();
-
   return EXIT_SUCCESS;
 }

Libs/DICOM/Core/ctkDICOMDatabase.cpp

@@ -23,11 +23,9 @@
 // Qt includes
 #include <QDate>
 #include <QDebug>
-#include <QDirIterator>
 #include <QFile>
 #include <QFileInfo>
 #include <QFileSystemWatcher>
-#include <QMutexLocker>
 #include <QSet>
 #include <QSqlError>
 #include <QSqlQuery>
@@ -86,6 +84,12 @@ public:
   bool loggedExec(QSqlQuery& query, const QString& queryString);
   bool LoggedExecVerbose;
 
+  ///
+  /// \brief group several inserts into a single transaction
+  ///
+  void beginTransaction();
+  void endTransaction();
+
   // dataset must be set always
   // filePath has to be set if this is an import of an actual file
   void insert ( const ctkDICOMDataset& ctkDataset, const QString& filePath, bool storeFile = true, bool generateThumbnail = true);
@@ -125,9 +129,6 @@ public:
   /// resets the variables to new inserts won't be fooled by leftover values
   void resetLastInsertedValues();
 
-  /// parallel inserts are not allowed (yet)
-  QMutex insertMutex;
-
   /// tagCache table has been checked to exist
   bool TagCacheVerified;
   /// tag cache has independent database to avoid locking issue
@@ -230,6 +231,26 @@ bool ctkDICOMDatabasePrivate::loggedExec(QSqlQuery& query, const QString& queryS
 }
 
 //------------------------------------------------------------------------------
+void ctkDICOMDatabasePrivate::beginTransaction()
+{
+  Q_Q(ctkDICOMDatabase);
+
+  QSqlQuery transaction( this->Database );
+  transaction.prepare( "BEGIN TRANSACTION" );
+  transaction.exec();
+}
+
+//------------------------------------------------------------------------------
+void ctkDICOMDatabasePrivate::endTransaction()
+{
+  Q_Q(ctkDICOMDatabase);
+
+  QSqlQuery transaction( this->Database );
+  transaction.prepare( "END TRANSACTION" );
+  transaction.exec();
+}
+
+//------------------------------------------------------------------------------
 void ctkDICOMDatabasePrivate::createBackupFileList()
 {
   QSqlQuery query(this->Database);
@@ -275,9 +296,11 @@ void ctkDICOMDatabase::openDatabase(const QString databaseFile, const QString& c
     }
 
   //Disable synchronous writing to make modifications faster
+  {
   QSqlQuery pragmaSyncQuery(d->Database);
   pragmaSyncQuery.exec("PRAGMA synchronous = OFF");
   pragmaSyncQuery.finish();
+  }
 
   // set up the tag cache for use later
   QFileInfo fileInfo(d->DatabaseFileName);
@@ -985,7 +1008,7 @@ void ctkDICOMDatabasePrivate::insertSeries(const ctkDICOMDataset& ctkDataset, QS
   checkSeriesExistsQuery.prepare ( "SELECT * FROM Series WHERE SeriesInstanceUID = ?" );
   checkSeriesExistsQuery.bindValue ( 0, seriesInstanceUID );
   logger.warn ( "Statement: " + checkSeriesExistsQuery.lastQuery() );
-  loggedExec(checkSeriesExistsQuery);
+  checkSeriesExistsQuery.exec();
   if(!checkSeriesExistsQuery.next())
     {
       qDebug() << "Need to insert new series: " << seriesInstanceUID;
@@ -1060,9 +1083,7 @@ void ctkDICOMDatabasePrivate::precacheTags( const QString sopInstanceUID )
   QString fileName = q->fileForInstance(sopInstanceUID);
   dataset.InitializeFromFile(fileName);
 
-  QSqlQuery transaction( this->TagCacheDatabase );
-  transaction.prepare( "BEGIN TRANSACTION" );
-  this->loggedExec(transaction);
+  this->beginTransaction();
 
   foreach (const QString &tag, this->TagsToPrecache)
     {
@@ -1073,8 +1094,7 @@ void ctkDICOMDatabasePrivate::precacheTags( const QString sopInstanceUID )
     q->cacheTag(sopInstanceUID, tag, value);
     }
 
-  transaction.prepare( "END TRANSACTION" );
-  this->loggedExec(transaction);
+  this->endTransaction();
 }
 
 //------------------------------------------------------------------------------
@@ -1084,26 +1104,27 @@ void ctkDICOMDatabasePrivate::insert( const ctkDICOMDataset& ctkDataset, const Q
 
   // this is the method that all other insert signatures end up calling
   // after they have pre-parsed their arguments
-
-  QMutexLocker lock(&insertMutex);
-
+ 
   // Check to see if the file has already been loaded
   // TODO:
   // It could make sense to actually remove the dataset and re-add it. This needs the remove
   // method we still have to write.
   //
-
+  //
+  
   QString sopInstanceUID ( ctkDataset.GetElementAsString(DCM_SOPInstanceUID) );
 
   QSqlQuery fileExists ( Database );
   fileExists.prepare("SELECT InsertTimestamp,Filename FROM Images WHERE SOPInstanceUID == :sopInstanceUID");
   fileExists.bindValue(":sopInstanceUID",sopInstanceUID);
+  {
   bool success = fileExists.exec();
   if (!success)
     {
       logger.error("SQLITE ERROR: " + fileExists.lastError().driverText());
       return;
     }
+  }
 
   QString databaseFilename(fileExists.value(1).toString());
   QDateTime fileLastModified(QFileInfo(databaseFilename).lastModified());

Libs/DICOM/Core/ctkDICOMIndexer.cpp

@@ -31,7 +31,6 @@
 #include <QFileInfo>
 #include <QDebug>
 #include <QPixmap>
-#include <QtConcurrentRun>
 
 // ctkDICOM includes
 #include "ctkLogger.h"
@@ -51,25 +50,6 @@
 #include <dcmtk/dcmimgle/dcmimage.h>  /* for class DicomImage */
 #include <dcmtk/dcmimage/diregist.h>  /* include support for color images */
 
-class AddFileFunctor
-{
-public:
-     AddFileFunctor(ctkDICOMIndexer* indexer, ctkDICOMDatabase& database,
-                    const QString& destinationDirectoryName = "")
-       : Indexer(indexer), Database(database), DestinationDirectoryName(destinationDirectoryName) { }
-
-     bool operator()(const QString &filePath)
-     {
-         Indexer->addFile(Database,filePath,DestinationDirectoryName);
-         return false; // make sure it is removed;
-     }
-
-     ctkDICOMIndexer* Indexer;
-     ctkDICOMDatabase& Database;
-     QString DestinationDirectoryName;
-
- };
-
 
 //------------------------------------------------------------------------------
 static ctkLogger logger("org.commontk.dicom.DICOMIndexer" );
@@ -80,33 +60,14 @@ static ctkLogger logger("org.commontk.dicom.DICOMIndexer" );
 // ctkDICOMIndexerPrivate methods
 
 //------------------------------------------------------------------------------
-ctkDICOMIndexerPrivate::ctkDICOMIndexerPrivate(ctkDICOMIndexer& o) : q_ptr(&o), Canceled(false), CurrentPercentageProgress(-1)
+ctkDICOMIndexerPrivate::ctkDICOMIndexerPrivate(ctkDICOMIndexer& o) : q_ptr(&o), Canceled(false)
 {
   Q_Q(ctkDICOMIndexer);
-  connect(&DirectoryImportWatcher,SIGNAL(progressValueChanged(int)),this,SLOT(OnProgress(int)));
-  connect(&DirectoryImportWatcher,SIGNAL(finished()),q,SIGNAL(indexingComplete()));
-  connect(&DirectoryImportWatcher,SIGNAL(canceled()),q,SIGNAL(indexingComplete()));
 }
 
 //------------------------------------------------------------------------------
 ctkDICOMIndexerPrivate::~ctkDICOMIndexerPrivate()
 {
-  DirectoryImportWatcher.cancel();
-  DirectoryImportWatcher.waitForFinished();
-
-}
-
-void ctkDICOMIndexerPrivate::OnProgress(int)
-{
-  Q_Q(ctkDICOMIndexer);
-
-  int newPercentageProgress = ( 100 * DirectoryImportFuture.progressValue() ) / DirectoryImportFuture.progressMaximum();
-  if (newPercentageProgress != CurrentPercentageProgress)
-    {
-      CurrentPercentageProgress = newPercentageProgress;
-      emit q->progress(newPercentageProgress);
-    }
-
 }
 
 //------------------------------------------------------------------------------
@@ -142,7 +103,7 @@ void ctkDICOMIndexer::addFile(ctkDICOMDatabase& database,
 }
 
 //------------------------------------------------------------------------------
-void ctkDICOMIndexer::addDirectory(ctkDICOMDatabase& ctkDICOMDatabase, 
+void ctkDICOMIndexer::addDirectory(ctkDICOMDatabase& ctkDICOMDatabase,
                                    const QString& directoryName,
                                    const QString& destinationDirectoryName)
 {
@@ -173,17 +134,21 @@ void ctkDICOMIndexer::addListOfFiles(ctkDICOMDatabase& ctkDICOMDatabase,
                                      const QString& destinationDirectoryName)
 {
   Q_D(ctkDICOMIndexer);
-  if(!listOfFiles.isEmpty())
+  d->Canceled = false;
+  int CurrentFileIndex = 0;
+  foreach(QString filePath, listOfFiles)
   {
-    if(d->DirectoryImportWatcher.isRunning())
-    {
-      d->DirectoryImportWatcher.cancel();
-      d->DirectoryImportWatcher.waitForFinished();
-    }
-    d->FilesToIndex.append(listOfFiles);
-    d->DirectoryImportFuture = QtConcurrent::filter(d->FilesToIndex,AddFileFunctor(this,ctkDICOMDatabase,destinationDirectoryName));
-    d->DirectoryImportWatcher.setFuture(d->DirectoryImportFuture);
+    int percent = ( 100 * CurrentFileIndex ) / listOfFiles.size();
+    emit this->progress(percent);
+    this->addFile(ctkDICOMDatabase, filePath, destinationDirectoryName);
+    CurrentFileIndex++;
+
+    if( d->Canceled )
+      {
+      break;
+      }
   }
+  emit this->indexingComplete();
 }
 
 //------------------------------------------------------------------------------
@@ -287,24 +252,17 @@ void ctkDICOMIndexer::refreshDatabase(ctkDICOMDatabase& dicomDatabase, const QSt
     {
     filesytemFiles.insert(dirIt.next());
     }
-  
+
   // TODO: it looks like this function was never finished...
-  // 
+  //
   // I guess the next step is to remove all filesToRemove from the database
   // and also to add filesystemFiles into the database tables
-  */ 
+  */
   }
 
 //----------------------------------------------------------------------------
-void ctkDICOMIndexer::waitForImportFinished()
-{
-  Q_D(ctkDICOMIndexer);
-  d->DirectoryImportWatcher.waitForFinished();
-}
-
-//----------------------------------------------------------------------------
 void ctkDICOMIndexer::cancel()
 {
   Q_D(ctkDICOMIndexer);
-  d->DirectoryImportWatcher.cancel();
+  d->Canceled = true;
 }

Libs/DICOM/Core/ctkDICOMIndexer.h

@@ -82,12 +82,6 @@ public:
 
   Q_INVOKABLE void refreshDatabase(ctkDICOMDatabase& database, const QString& directoryName);
 
-  ///
-  /// \brief ensures that the QFuture threads have all finished indexing
-  /// before returning control.
-  ///
-  Q_INVOKABLE void waitForImportFinished();
-
 Q_SIGNALS:
   void foundFilesToIndex(int);
   void indexingFileNumber(int);

Libs/DICOM/Core/ctkDICOMIndexer_p.h

@@ -39,17 +39,9 @@ public:
   ctkDICOMIndexerPrivate(ctkDICOMIndexer&);
   ~ctkDICOMIndexerPrivate();
 
-public Q_SLOTS:
-
-  void OnProgress(int progress);
 public:
-
   ctkDICOMAbstractThumbnailGenerator* thumbnailGenerator;
   bool                    Canceled;
-  QStringList FilesToIndex;
-  QFutureWatcher<void> DirectoryImportWatcher;
-  QFuture<void> DirectoryImportFuture;
-  int CurrentPercentageProgress;
 };
 
 

Libs/DICOM/Widgets/Testing/Cpp/ctkDICOMAppWidgetTest1.cpp

@@ -23,6 +23,7 @@
 #include <QDebug>
 #include <QDir>
 #include <QTimer>
+#include <QString>
 
 // ctkDICOMCore includes
 #include "ctkDICOMAppWidget.h"
@@ -40,7 +41,8 @@ int ctkDICOMAppWidgetTest1( int argc, char * argv [] )
   
   ctkDICOMAppWidget appWidget;
   appWidget.setDatabaseDirectory(QDir::currentPath());
-  appWidget.onAddToDatabase();
+  QString testString = QString("Test String");
+  appWidget.onFileIndexed(testString);
   appWidget.openImportDialog();
   appWidget.openExportDialog();
   appWidget.openQueryDialog();

Libs/DICOM/Widgets/ctkDICOMAppWidget.cpp

@@ -25,6 +25,7 @@
 
 // Qt includes
 #include <QAction>
+#include <QCoreApplication>
 #include <QCheckBox>
 #include <QDebug>
 #include <QMetaType>
@@ -135,12 +136,7 @@ void ctkDICOMAppWidgetPrivate::showUpdateSchemaDialog()
     // by creating our own
     QLabel* progressLabel = new QLabel(q->tr("Initialization..."));
     UpdateSchemaProgress->setLabel(progressLabel);
-#ifdef Q_WS_MAC
-    // BUG: avoid deadlock of dialogs on mac
-    UpdateSchemaProgress->setWindowModality(Qt::NonModal);
-#else
     UpdateSchemaProgress->setWindowModality(Qt::ApplicationModal);
-#endif
     UpdateSchemaProgress->setMinimumDuration(0);
     UpdateSchemaProgress->setValue(0);
 
@@ -182,12 +178,7 @@ void ctkDICOMAppWidgetPrivate::showIndexerDialog()
     // by creating our own
     QLabel* progressLabel = new QLabel(q->tr("Initialization..."));
     IndexerProgress->setLabel(progressLabel);
-#ifdef Q_WS_MAC
-    // BUG: avoid deadlock of dialogs on mac
-    IndexerProgress->setWindowModality(Qt::NonModal);
-#else
     IndexerProgress->setWindowModality(Qt::ApplicationModal);
-#endif
     IndexerProgress->setMinimumDuration(0);
     IndexerProgress->setValue(0);
 
@@ -198,6 +189,8 @@ void ctkDICOMAppWidgetPrivate::showIndexerDialog()
             IndexerProgress, SLOT(setValue(int)));
     q->connect(DICOMIndexer.data(), SIGNAL(indexingFilePath(QString)),
             progressLabel, SLOT(setText(QString)));
+    q->connect(DICOMIndexer.data(), SIGNAL(indexingFilePath(QString)),
+            q, SLOT(onFileIndexed(QString)));
 
     // close the dialog
     q->connect(DICOMIndexer.data(), SIGNAL(indexingComplete()),
@@ -422,11 +415,13 @@ void ctkDICOMAppWidget::setSearchWidgetPopUpMode(bool flag){
 }
 
 //----------------------------------------------------------------------------
-void ctkDICOMAppWidget::onAddToDatabase()
+void ctkDICOMAppWidget::onFileIndexed(const QString& filePath)
 {
-  //Q_D(ctkDICOMAppWidget);
-
-  //d->
+  // Update the progress dialog when the file name changes
+  // - also allows for cancel button
+  QCoreApplication::instance()->processEvents();
+  qDebug() << "Indexing \n\n\n\n" << filePath <<"\n\n\n";
+  
 }
 
 //----------------------------------------------------------------------------

Libs/DICOM/Widgets/ctkDICOMAppWidget.h

@@ -66,7 +66,7 @@ public:
 
 public Q_SLOTS:
   void setDatabaseDirectory(const QString& directory);
-  void onAddToDatabase();
+  void onFileIndexed(const QString& filePath);
 
   void openImportDialog();
   void openExportDialog();