BUG: SHTree setCurrentItems do not deselect items before selecting th…

…em again CTS-2376
Slicer · Apr 3, 2024 · fb0ab78 · fb0ab78
1 parent 1ca0ceb
commit fb0ab78
Show file tree

Hide file tree

Showing 2 changed files with 97 additions and 17 deletions.
diff --git a/Modules/Loadable/SubjectHierarchy/Testing/Python/SubjectHierarchyGenericSelfTest.py b/Modules/Loadable/SubjectHierarchy/Testing/Python/SubjectHierarchyGenericSelfTest.py
@@ -106,6 +106,7 @@ def test_SubjectHierarchyGenericSelfTest_FullTest1(self):
         self.section_AddNodeToSubjectHierarchy()
         self.section_CLI()
         self.section_CreateSecondBranch()
+        self.section_SetCurrentItems()
         self.section_ReparentNodeInSubjectHierarchy()
         self.section_LoadScene()
         self.section_TestCircularParenthood()
@@ -334,6 +335,72 @@ def section_CreateSecondBranch(self):
         self.assertEqual(shNode.GetItemParent(self.study2ItemID), self.patient2ItemID)
         self.assertEqual(shNode.GetItemParent(self.folderItemID), self.study2ItemID)
 
+    # ------------------------------------------------------------------------------
+    def section_SetCurrentItems(self):
+        self.delayDisplay("Set current items", self.delayMs)
+
+        # Get subject hierarchy scene model
+        dataWidget = slicer.modules.data.widgetRepresentation()
+        self.assertIsNotNone(dataWidget)
+        shTreeView = slicer.util.findChild(dataWidget, name="SubjectHierarchyTreeView")
+        self.assertIsNotNone(shTreeView)
+
+        # Ensure signal is received when selecting one item
+        currentItemChangedArguments = []
+        shTreeView.currentItemChanged.connect(lambda x: currentItemChangedArguments.append(x))
+        shTreeView.setCurrentItem(self.patientItemID)
+        self.assertEqual(currentItemChangedArguments, [self.patientItemID])
+
+        # Check that current items matched the selected item
+        self.assertEqual(shTreeView.currentItem(), self.patientItemID)
+
+        # Clear selection and select the same item but using a list
+        shTreeView.clearSelection()
+        currentItemChangedArguments.clear()
+        itemsToSelect = vtk.vtkIdList()
+        itemsToSelect.InsertNextId(self.patientItemID)
+        shTreeView.setCurrentItems(itemsToSelect)
+        self.assertEqual(currentItemChangedArguments, [self.patientItemID])
+
+        # Check that current items matched the selected item
+        selectedItems = vtk.vtkIdList()
+        shTreeView.currentItems(selectedItems)
+        self.assertEqual(itemsToSelect.GetNumberOfIds(), selectedItems.GetNumberOfIds())
+        for i in range(itemsToSelect.GetNumberOfIds()):
+          self.assertEqual(itemsToSelect.GetId(i), selectedItems.GetId(i))
+
+        # Select another item
+        currentItemChangedArguments.clear()
+        itemsToSelect.Reset()
+        itemsToSelect.InsertNextId(self.patient2ItemID)
+        shTreeView.setCurrentItems(itemsToSelect)
+        shNode = slicer.mrmlScene.GetSubjectHierarchyNode()
+        # SceneItemID is received in currentItemChanged connected slot because previous item is first deselected.
+        self.assertEqual(currentItemChangedArguments, [shNode.GetSceneItemID(), self.patient2ItemID])
+
+        # Check that current items matched the selected item
+        selectedItems.Reset()
+        shTreeView.currentItems(selectedItems)
+        self.assertEqual(itemsToSelect.GetNumberOfIds(), selectedItems.GetNumberOfIds())
+        for i in range(itemsToSelect.GetNumberOfIds()):
+          self.assertEqual(itemsToSelect.GetId(i), selectedItems.GetId(i))
+
+        # Select the already selected item
+        currentItemChangedArguments.clear()
+        itemsToSelect.Reset()
+        itemsToSelect.InsertNextId(self.patient2ItemID)
+        shTreeView.setCurrentItems(itemsToSelect)
+        self.assertEqual(currentItemChangedArguments, [])
+
+        # Check that current items matched the selected item
+        selectedItems.Reset()
+        shTreeView.currentItems(selectedItems)
+        self.assertEqual(itemsToSelect.GetNumberOfIds(), selectedItems.GetNumberOfIds())
+        for i in range(itemsToSelect.GetNumberOfIds()):
+          self.assertEqual(itemsToSelect.GetId(i), selectedItems.GetId(i))
+
+        shTreeView.currentItemChanged.disconnect()
+
     # ------------------------------------------------------------------------------
     def section_ReparentNodeInSubjectHierarchy(self):
         self.delayDisplay("Reparent node in subject hierarchy", self.delayMs)

diff --git a/Modules/Loadable/SubjectHierarchy/Widgets/qMRMLSubjectHierarchyTreeView.cxx b/Modules/Loadable/SubjectHierarchy/Widgets/qMRMLSubjectHierarchyTreeView.cxx
@@ -816,12 +816,34 @@ void qMRMLSubjectHierarchyTreeView::setCurrentItems(QList<vtkIdType> items)
     return;
   }
 
-  this->clearSelection();
-
-  foreach (long itemID, items)
+  // Get requested selection
+  QSet<QModelIndex> requestedSelectedItems;
+  foreach (vtkIdType itemID, items)
   {
-    QModelIndex itemIndex = d->SortFilterModel->indexFromSubjectHierarchyItem(vtkIdType(itemID));
+    QModelIndex itemIndex = d->SortFilterModel->indexFromSubjectHierarchyItem(itemID);
     if (itemIndex.isValid())
+    {
+      requestedSelectedItems.insert(itemIndex);
+    }
+  }
+
+  // Get previous selection
+  const QModelIndexList previouslySelectedItemsList = this->selectionModel()->selectedRows();
+  QSet<QModelIndex> previouslySelectedItems(previouslySelectedItemsList.begin(), previouslySelectedItemsList.end());
+
+  // Deselect items that were previously selected but not requested anymore
+  foreach(QModelIndex itemIndex, previouslySelectedItems)
+  {
+    if (itemIndex.isValid() && !requestedSelectedItems.contains(itemIndex))
+    {
+      this->selectionModel()->select(itemIndex, QItemSelectionModel::Deselect | QItemSelectionModel::Rows);
+    }
+  }
+
+  // Select items that are requested but have not been previously selected
+  foreach(QModelIndex itemIndex, requestedSelectedItems)
+  {
+    if (!previouslySelectedItems.contains(itemIndex))
     {
       this->selectionModel()->select(itemIndex, QItemSelectionModel::Select | QItemSelectionModel::Rows);
     }
@@ -832,28 +854,19 @@ void qMRMLSubjectHierarchyTreeView::setCurrentItems(QList<vtkIdType> items)
 void qMRMLSubjectHierarchyTreeView::setCurrentItems(vtkIdList* items)
 {
   Q_D(qMRMLSubjectHierarchyTreeView);
-  if (!d->SortFilterModel)
-  {
-    qCritical() << Q_FUNC_INFO << ": Invalid data model";
-    return;
-  }
-
   if (!items)
   {
     qCritical() << Q_FUNC_INFO << ": Invalid item list";
     return;
   }
 
-  this->clearSelection();
-
+  QList<vtkIdType> itemsToSelect;
   for (int index=0; index<items->GetNumberOfIds(); ++index)
   {
-    QModelIndex itemIndex = d->SortFilterModel->indexFromSubjectHierarchyItem(items->GetId(index));
-    if (itemIndex.isValid())
-    {
-      this->selectionModel()->select(itemIndex, QItemSelectionModel::Select | QItemSelectionModel::Rows);
-    }
+    itemsToSelect.append(items->GetId(index));
   }
+
+  this->setCurrentItems(itemsToSelect);
 }
 
 //------------------------------------------------------------------------------