BUG: Fix deletion of last time point in a sequence

When "save changes" was enabled for a sequence then it was not possible to delete the last time point.

Co-authored-by: Jean-Christophe Fillion-Robin <jchris.fillionr@kitware.com>

Modules/Loadable/Sequences/Logic/vtkSlicerSequencesLogic.cxx

@@ -392,8 +392,10 @@ void vtkSlicerSequencesLogic::UpdateProxyNodesFromSequences(vtkMRMLSequenceBrows
         {
         // There are no data nodes in the sequence.
         // Insert a new node node in the sequence based on missingItemMode.
+        // Except if selectedItemNumber < 0, because that means that there is no timepoint in the sequence
+        // and we should not add one (because then we could never delete the last item in a sequence when recording is enabled).
         sourceDataNode = browserNode->GetProxyNode(synchronizedSequenceNode);
-        if (sourceDataNode)
+        if (sourceDataNode && selectedItemNumber >= 0)
           {
           if (missingItemMode == vtkMRMLSequenceBrowserNode::MissingItemCreateFromPrevious)
             {
@@ -742,6 +744,8 @@ void vtkSlicerSequencesLogic::ProcessMRMLNodesEvents(vtkObject *caller, unsigned
       !this->GetMRMLScene()->IsRestoring())
       {
       // One of the proxy nodes changed, update the sequence as needed
+      // If we wanted to change behavior of "save changes" to create a new time point when the proxy node changes
+      // then we could add a flag to UpdateSequencesFromProxyNodes that would force creation of a new time point.
       this->UpdateSequencesFromProxyNodes(browserNode, vtkMRMLNode::SafeDownCast((vtkObject*)callData));
       }
     }

Modules/Loadable/Sequences/Testing/Cxx/vtkMRMLSequenceBrowserNodeTest1.cxx

@@ -163,11 +163,61 @@ int TestSelectNextItem()
   return EXIT_SUCCESS;
 }
 
+int TestRemoveItem()
+{
+  vtkNew<vtkMRMLScene> scene;
+
+  // Register vtkMRMLSequenceBrowserNode
+  vtkNew<vtkSlicerSequencesLogic> sequencesLogic;
+  sequencesLogic->SetMRMLScene(scene.GetPointer());
+
+  vtkMRMLSequenceNode* sequenceNode = vtkMRMLSequenceNode::SafeDownCast(scene->AddNewNodeByClass("vtkMRMLSequenceNode"));
+  vtkMRMLSequenceBrowserNode* browserNode = vtkMRMLSequenceBrowserNode::SafeDownCast(scene->AddNewNodeByClass("vtkMRMLSequenceBrowserNode"));
+  CHECK_NOT_NULL(browserNode);
+  browserNode->SetAndObserveMasterSequenceNodeID(sequenceNode->GetID());
+
+  // Test with recording mode enabled
+  vtkNew<vtkMRMLTransformNode> transform;
+
+  browserNode->SetSaveChanges(sequenceNode, true);
+  sequenceNode->SetDataNodeAtValue(transform, "1");
+  sequenceNode->SetDataNodeAtValue(transform, "2");
+
+  // Check if a proxy node is created
+  sequencesLogic->UpdateAllProxyNodes();
+  vtkMRMLNode* proxyNode = browserNode->GetProxyNode(sequenceNode);
+  CHECK_NOT_NULL(proxyNode);
+
+  // Remove a data node from the sequence
+  sequenceNode->RemoveDataNodeAtValue("2");
+  CHECK_INT(browserNode->GetSelectedItemNumber(), 0);
+  CHECK_INT(sequenceNode->GetNumberOfDataNodes(), 1);
+
+  // Check if modifying a proxy does not create a new timepoint
+  proxyNode->Modified();
+  CHECK_INT(sequenceNode->GetNumberOfDataNodes(), 1);
+
+  // Remove the last data node from the sequence
+  sequenceNode->RemoveDataNodeAtValue("1");
+  CHECK_INT(browserNode->GetSelectedItemNumber(), -1);
+  CHECK_INT(sequenceNode->GetNumberOfDataNodes(), 0);
+
+  // Check that modifying a proxy node does not create a new timepoint
+  // (enabling "save changes" does not create a new timepoint, it just updates the existing timepoint;
+  // "recording" creates a new timepoint each time there is a modification, if the browser is in recording mode).
+  proxyNode->Modified();
+  CHECK_INT(browserNode->GetSelectedItemNumber(), -1);
+  CHECK_INT(sequenceNode->GetNumberOfDataNodes(), 0);
+
+  return EXIT_SUCCESS;
+}
+
 }  // end anonymous namespace
 
 int vtkMRMLSequenceBrowserNodeTest1(int vtkNotUsed(argc), char* vtkNotUsed(argv)[])
 {
   CHECK_EXIT_SUCCESS(TestIndexFormatting());
   CHECK_EXIT_SUCCESS(TestSelectNextItem());
+  CHECK_EXIT_SUCCESS(TestRemoveItem());
   return EXIT_SUCCESS;
 }