ENH: Redesign of the dicom model that fixes bugs and speed issues

Libs/DICOM/Core/Testing/ctkDICOMModelTest1.cxx

@@ -9,6 +9,7 @@
 // ctkDICOMCore includes
 #include "ctkDICOM.h"
 #include "ctkDICOMModel.h"
+#include "ctkModelTester.h"
 
 // STD includes
 #include <iostream>
@@ -22,15 +23,24 @@ int ctkDICOMModelTest1( int argc, char * argv [] )
 {
   QApplication app(argc, argv);
   
-  if (argc <= 1)
+  if (argc <= 2)
     {
     std::cerr << "Warning, no sql file given. Test stops" << std::endl;
     return EXIT_FAILURE;
     }
   
   ctkDICOM myCTK;
-  myCTK.openDatabase( argv[1] );
-  myCTK.initializeDatabase(argv[2]);
+  if (!myCTK.openDatabase( argv[1] ))
+    {
+    std::cerr << "Error when opening the data base file: " << argv[1] 
+              << " error: " << myCTK.GetLastError().toStdString();
+    return EXIT_FAILURE;
+    }
+  if (!myCTK.initializeDatabase(argv[2]))
+    {
+    std::cerr << "Error when initializing the data base: " << argv[2] 
+              << " error: " << myCTK.GetLastError().toStdString();
+    }
     /*
   QSqlQuery toto("SELECT PatientsName as 'Name tt' FROM Patients ORDER BY \"Name tt\" ASC", myCTK.database());
   qDebug() << "toto: " << myCTK.GetLastError() ;
@@ -45,7 +55,16 @@ int ctkDICOMModelTest1( int argc, char * argv [] )
   qDebug() << "tutu: " << tutu.seek(0) << myCTK.GetLastError();
   */
 
+  ctkModelTester tester(0); 
+  tester.setNestedInserts(true);
+  tester.setThrowOnError(false);
   ctkDICOMModel model(0);
+  tester.setModel(&model);
+
+  model.setDatabase(myCTK.database());
+  
+  model.setDatabase(QSqlDatabase());
+ 
   model.setDatabase(myCTK.database());
 
   QTreeView viewer(0);
@@ -56,6 +75,6 @@ int ctkDICOMModelTest1( int argc, char * argv [] )
   qDebug() << model.rowCount() << model.columnCount();
   qDebug() << model.index(0,0);
   viewer.show();
-  //return app.exec();
+  return app.exec();
   return EXIT_SUCCESS;
 }

Libs/DICOM/Core/ctkDICOMModel.cxx

@@ -20,7 +20,7 @@ class ctkDICOMModelPrivate:public qCTKPrivate<ctkDICOMModel>
 {
 public:
   ctkDICOMModelPrivate();
-  ~ctkDICOMModelPrivate();
+  virtual ~ctkDICOMModelPrivate();
   void init();
 
   enum IndexType{
@@ -32,8 +32,13 @@ public:
   };
  
   void fetch(const QModelIndex& index, int limit);
-  Node* createNode(int row, int column, const QModelIndex& parent)const;
+  Node* createNode(int row, const QModelIndex& parent)const;
   Node* nodeFromIndex(const QModelIndex& index)const;
+  //QModelIndexList indexListFromNode(const Node* node)const;
+  //QModelIndexList modelIndexList(Node* node = 0)const;
+  //int childrenCount(Node* node = 0)const;
+  // move it in the Node struct
+  QVariant value(Node* parent, int row, int field)const;
   QVariant value(const QModelIndex& index, int row, int field)const;
   QString  generateQuery(const QString& fields, const QString& table, const QString& conditions = QString())const;
   void updateQueries(Node* node)const;
@@ -45,6 +50,7 @@ public:
 };
 
 //------------------------------------------------------------------------------
+// 1 node per row
 struct Node
 {
   ~Node()
@@ -59,11 +65,10 @@ struct Node
   Node*     Parent;
   QVector<Node*> Children;
   int       Row;
-  int       Column;
   QSqlQuery Query;
   QString   UID;
   int       RowCount;
-  bool      End;
+  bool      AtEnd;
   bool      Fetching;
 };
 
@@ -94,14 +99,83 @@ Node* ctkDICOMModelPrivate::nodeFromIndex(const QModelIndex& index)const
   return index.isValid() ? reinterpret_cast<Node*>(index.internalPointer()) : this->RootNode;
 }
 
+/*
+//------------------------------------------------------------------------------
+QModelIndexList ctkDICOMModelPrivate::indexListFromNode(const Node* node)const
+{
+  QCTK_P(const ctkDICOMModel);
+  Q_ASSERT(node);
+  QModelIndexList indexList;
+  
+  Node* parentNode = node->Parent;
+  if (parentNode == 0)
+    {
+    return indexList;
+    }
+  int field = parentNode->Query.record().indexOf("UID");
+  int row = -1;
+  for (row = 0; row < parentNode->RowCount; ++row)
+    {
+    QString uid = this->value(parentNode, row, field).toString();
+    if (uid == node->UID)
+      {
+      break;
+      }
+    }
+  if (row >= parentNode->RowCount)
+    {
+    return indexList;
+    }
+  for (int column = 0 ; column < this->Headers.size(); ++column)
+    {
+    indexList.append(p->createIndex(row, column, parentNode));
+    }
+  return indexList;
+}
+
+//------------------------------------------------------------------------------
+QModelIndexList ctkDICOMModelPrivate::modelIndexList(Node* node)const
+{
+  QModelIndexList list;
+  if (node == 0)
+    {
+    node = this->RootNode;
+    }
+  foreach(Node* child, node->Children)
+    {
+    list.append(this->indexListFromNode(child));
+    }
+  foreach(Node* child, node->Children)
+    {
+    list.append(this->modelIndexList(child));
+    }
+  return list;
+}
+
+//------------------------------------------------------------------------------
+int ctkDICOMModelPrivate::childrenCount(Node* node)const
+{
+  int count = 0;
+  if (node == 0)
+    {
+    node = this->RootNode;
+    }
+  count += node->Children.size();
+  foreach(Node* child, node->Children)
+    {
+    count += this->childrenCount(child);
+    }
+  return count;
+}
+*/
 //------------------------------------------------------------------------------
-Node* ctkDICOMModelPrivate::createNode(int row, int column, const QModelIndex& parent)const
+Node* ctkDICOMModelPrivate::createNode(int row, const QModelIndex& parent)const
 {
   QCTK_P(const ctkDICOMModel);
   
   Node* node = new Node;
   Node* nodeParent = 0;
-  if (row == -1 || column == -1)
+  if (row == -1)
     {// root node
     node->Type = ctkDICOMModelPrivate::RootType;
     node->Parent = 0;
@@ -110,11 +184,10 @@ Node* ctkDICOMModelPrivate::createNode(int row, int column, const QModelIndex& p
     {
     nodeParent = this->nodeFromIndex(parent); 
     nodeParent->Children.push_back(node);
-    node->Parent = (nodeParent == this->RootNode ? 0: nodeParent);
+    node->Parent = nodeParent;
     node->Type = ctkDICOMModelPrivate::IndexType(nodeParent->Type + 1);
     }
   node->Row = row;
-  node->Column = column;
   if (node->Type != ctkDICOMModelPrivate::RootType)
     {
     int field = nodeParent->Query.record().indexOf("UID");
@@ -122,7 +195,7 @@ Node* ctkDICOMModelPrivate::createNode(int row, int column, const QModelIndex& p
     }
   
   node->RowCount = 0;
-  node->End = false;
+  node->AtEnd = false;
   node->Fetching = false;
 
   this->updateQueries(node);
@@ -138,13 +211,23 @@ QVariant ctkDICOMModelPrivate::value(const QModelIndex& parent, int row, int col
     {      
     const_cast<ctkDICOMModelPrivate *>(this)->fetch(parent, row + 256);
     }
+  return this->value(node, row, column);
+}
 
-  if (!node->Query.seek(row)) 
+//------------------------------------------------------------------------------
+QVariant ctkDICOMModelPrivate::value(Node* parent, int row, int column) const
+{
+  Q_ASSERT(row < parent->RowCount);
+
+  if (!parent->Query.seek(row)) 
     {
-    qDebug() << node->Query.lastError();
+    qDebug() << parent->Query.lastError();
+    Q_ASSERT(parent->Query.seek(row));
     return QVariant();
     }
-  return node->Query.value(column);
+  QVariant res = parent->Query.value(column);
+  Q_ASSERT(res.isValid());
+  return res;
 }
 
 //------------------------------------------------------------------------------
@@ -165,6 +248,7 @@ QString ctkDICOMModelPrivate::generateQuery(const QString& fields, const QString
 //------------------------------------------------------------------------------
 void ctkDICOMModelPrivate::updateQueries(Node* node)const
 {
+  QCTK_P(const ctkDICOMModel);
   // are you kidding me, it should be virtualized here :-)
   QString query;
   switch(node->Type)
@@ -203,7 +287,7 @@ void ctkDICOMModelPrivate::fetch(const QModelIndex& index, int limit)
 {
   QCTK_P(ctkDICOMModel);
   Node* node = this->nodeFromIndex(index);
-  if (node->End || limit <= node->RowCount || node->Fetching/*|| bottom.column() == -1*/)
+  if (node->AtEnd || limit <= node->RowCount || node->Fetching/*|| bottom.column() == -1*/)
     {
     return;
     }
@@ -232,7 +316,7 @@ void ctkDICOMModelPrivate::fetch(const QModelIndex& index, int limit)
       // empty or invalid query
       newRowCount = 0;
       }
-    node->End = true; // this is the end.
+    node->AtEnd = true; // this is the end.
     }
   if (newRowCount > 0 && newRowCount > node->RowCount) 
     {
@@ -265,7 +349,7 @@ bool ctkDICOMModel::canFetchMore ( const QModelIndex & parent ) const
 {
   QCTK_D(const ctkDICOMModel);
   Node* node = d->nodeFromIndex(parent);
-  return !node->End;
+  return !node->AtEnd;
 }
 
 //------------------------------------------------------------------------------
@@ -273,7 +357,7 @@ int ctkDICOMModel::columnCount ( const QModelIndex & _parent ) const
 {
   QCTK_D(const ctkDICOMModel);
   Q_UNUSED(_parent);
-  return d->Headers.size();
+  return d->RootNode != 0 ? d->Headers.size() : 0;
 }
 
 //------------------------------------------------------------------------------
@@ -285,9 +369,8 @@ QVariant ctkDICOMModel::data ( const QModelIndex & index, int role ) const
     return QVariant();
     }
   QModelIndex indexParent = this->parent(index);
-  Node* node = d->nodeFromIndex(indexParent);
-  Q_ASSERT(node->Row == indexParent.row());
-  if (index.row() >= node->RowCount)
+  Node* parentNode = d->nodeFromIndex(indexParent);
+  if (index.row() >= parentNode->RowCount)
     {      
     const_cast<ctkDICOMModelPrivate *>(d)->fetch(index, index.row());
     }
@@ -298,10 +381,10 @@ QVariant ctkDICOMModel::data ( const QModelIndex & index, int role ) const
     return QVariant();
     }
     */
-  int field = node->Query.record().indexOf(d->Headers[index.column()]);
+  int field = parentNode->Query.record().indexOf(d->Headers[index.column()]);
   if (field < 0)
     {
-    return QVariant();
+    return QString();
     }
   return d->value(indexParent, index.row(), field);
   //return node->Query.value(field);
@@ -325,8 +408,21 @@ Qt::ItemFlags ctkDICOMModel::flags ( const QModelIndex & index ) const
 bool ctkDICOMModel::hasChildren ( const QModelIndex & parent ) const
 {
   QCTK_D(const ctkDICOMModel);
+  if (parent.column() > 0)
+    {
+    return false;
+    }
   Node* node = d->nodeFromIndex(parent);
-  return node->RowCount > 0 || (!node->End && node->Query.seek(0));
+  if (!node)
+    {
+    return false;
+    }
+  if (node->RowCount == 0 && !node->AtEnd)
+    {
+    //const_cast<qCTKDCMTKModelPrivate*>(d)->fetch(parent, 1);
+    return node->Query.seek(0);
+    }
+  return node->RowCount > 0;
 }
 
 //------------------------------------------------------------------------------
@@ -350,12 +446,17 @@ QVariant ctkDICOMModel::headerData(int section, Qt::Orientation orientation, int
 QModelIndex ctkDICOMModel::index ( int row, int column, const QModelIndex & parent ) const
 {
   QCTK_D(const ctkDICOMModel);
+  if (d->RootNode == 0 || parent.column() > 0)
+    {
+    return QModelIndex();
+    }
   Node* parentNode = d->nodeFromIndex(parent);
+  int field = parentNode->Query.record().indexOf("UID");
+  QString uid = d->value(parent, row, field).toString();
   Node* node = 0;
   foreach(Node* tmpNode, parentNode->Children)
     {
-    if (tmpNode->Row == row && 
-        tmpNode->Column == column)
+    if (tmpNode->UID == uid)
       {
       node = tmpNode;
       break;
@@ -363,7 +464,7 @@ QModelIndex ctkDICOMModel::index ( int row, int column, const QModelIndex & pare
     }
   if (node == 0)
     {
-    node = d->createNode(row, column, parent);
+    node = d->createNode(row, parent);
     }
   return this->createIndex(row, column, node);
 }
@@ -373,21 +474,47 @@ QModelIndex ctkDICOMModel::parent ( const QModelIndex & index ) const
 {
   QCTK_D(const ctkDICOMModel);
   Node* node = d->nodeFromIndex(index);
-  if (node == 0 || node->Parent == 0)
+  Q_ASSERT(node);
+  Node* parentNode = node->Parent;
+  if (parentNode == 0)
+    {// node is root
+    return QModelIndex();
+    }
+  return parentNode == d->RootNode ? QModelIndex() : this->createIndex(parentNode->Row, 0, parentNode);
+  /* need to recalculate the parent row
+  Node* greatParentNode = parentNode->Parent;
+  if (greatParentNode == 0)
     {
     return QModelIndex();
     }
-  return this->createIndex(node->Parent->Row, node->Parent->Column, node->Parent);
+  int field = greatParentNode->Query.record().indexOf("UID");
+  int row = -1;
+  for (row = 0; row < greatParentNode->RowCount; ++row)
+    {
+    QString uid = d->value(greatParentNode, row, field).toString();
+    if (uid == parentNode->UID)
+      {
+      break;
+      }
+    }
+  Q_ASSERT(row < greatParentNode->RowCount);
+  return this->createIndex(row, 0, parentNode);
+  */
 }
 
 //------------------------------------------------------------------------------
 int ctkDICOMModel::rowCount ( const QModelIndex & parent ) const
 {
   QCTK_D(const ctkDICOMModel);
+  if (d->RootNode == 0 || parent.column() > 0)
+    {
+    return 0;
+    }
   Node* node = d->nodeFromIndex(parent);
-  if (node->RowCount == 0 && node->End)
+  Q_ASSERT(node);
+  if (node->RowCount == 0 && !node->AtEnd)
     {
-    const_cast<ctkDICOMModelPrivate*>(d)->fetch(parent, 256);
+    //const_cast<ctkDICOMModelPrivate*>(d)->fetch(parent, 256);
     }
   return node->RowCount;
 }
@@ -405,26 +532,24 @@ void ctkDICOMModel::setDatabase(const QSqlDatabase &db)
 
   if (d->DataBase.tables().empty())
     {
-    Q_ASSERT(d->DataBase.isOpen());
+    //Q_ASSERT(d->DataBase.isOpen());
+    this->endResetModel();
     return;
     }
     
-  d->RootNode = d->createNode(-1, -1, QModelIndex());
+  d->RootNode = d->createNode(-1, QModelIndex());
   
   this->endResetModel();
 
+  // TODO, use hasQuerySize everywhere, not only in setDataBase()
   bool hasQuerySize = d->RootNode->Query.driver()->hasFeature(QSqlDriver::QuerySize);
   if (hasQuerySize && d->RootNode->Query.size() > 0) 
     {
     int newRowCount= d->RootNode->Query.size();
     beginInsertRows(QModelIndex(), 0, qMax(0, newRowCount - 1));
     d->RootNode->RowCount = newRowCount;
-    d->RootNode->End = true;
+    d->RootNode->AtEnd = true;
     endInsertRows();
-    } 
-  else
-    {
-    d->RootNode->RowCount = 0;
     }
   d->fetch(QModelIndex(), 256);
 }
@@ -433,12 +558,30 @@ void ctkDICOMModel::setDatabase(const QSqlDatabase &db)
 void ctkDICOMModel::sort(int column, Qt::SortOrder order)
 {
   QCTK_D(ctkDICOMModel);
+  /* The following would work if there is no fetch involved.
+     ORDER BY doesn't just apply on the fetched item. By sorting
+     new items can show up in the model, and we need to be more
+     careful
   emit layoutAboutToBeChanged();
+  QModelIndexList oldIndexList = d->modelIndexList();
   d->Sort = QString("\"%1\" %2")
     .arg(d->Headers[column])
     .arg(order == Qt::AscendingOrder ? "ASC" : "DESC");
   d->updateQueries(d->RootNode);
+  QModelIndexList newIndexList = d->modelIndexList();
+  Q_ASSERT(oldIndexList.count() == newIndexList.count());
+  this->changePersistentIndexList(oldIndexList, newIndexList);
   emit layoutChanged();
+  */
+  this->beginResetModel();
+  delete d->RootNode;
+  d->RootNode = 0;
+  d->Sort = QString("\"%1\" %2")
+    .arg(d->Headers[column])
+    .arg(order == Qt::AscendingOrder ? "ASC" : "DESC");
+  d->RootNode = d->createNode(-1, QModelIndex());
+  
+  this->endResetModel();
 }
 
 //------------------------------------------------------------------------------

Libs/DICOM/Core/ctkDICOMModel.h

@@ -25,12 +25,15 @@ public:
   virtual QVariant data ( const QModelIndex & index, int role = Qt::DisplayRole ) const;
   virtual void fetchMore ( const QModelIndex & parent );
   virtual Qt::ItemFlags flags ( const QModelIndex & index ) const;
+  // can return true even if rowCount returns 0, you should use canFetchMore/fetchMore to populate
+  // the children.
   virtual bool hasChildren ( const QModelIndex & parent = QModelIndex() ) const;
   virtual QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole)const;
   virtual QModelIndex index ( int row, int column, const QModelIndex & parent = QModelIndex() ) const;
   virtual QModelIndex parent ( const QModelIndex & index ) const;
   virtual int rowCount ( const QModelIndex & parent = QModelIndex() ) const;
   virtual bool setHeaderData ( int section, Qt::Orientation orientation, const QVariant & value, int role = Qt::EditRole );
+  // Sorting resets the model because fetched/unfetched items could disappear/appear respectively.
   virtual void sort(int column, Qt::SortOrder order = Qt::AscendingOrder);
 
 private: