qMRMLSubjectHierarchyTreeView::setCurrentItem proposal

Development
1

Dear Slicer developers,

I would like to get some feedback on a potential fix in qMRMLSubjectHierarchyTreeView: ENH: qMRMLSubjectHierarchyTreeView: call setCurrentIndex rather than … · Slicer/Slicer@1fdee41 · GitHub

In my custom 3d slicer app I load a case which:

  • contains only 2 volumes in the data tree
  • sets the current item of a qMRMLSubjectHierarchyCombobox to the last volume (this combobox is used in my custom module)

Scenario 1:

  • Load the case.
  • Remove the last volume from the data tree.
  • Result: the combobox displays None.

Scenario 2:

  • Load the case.
  • Click on the combobox and click on the last volume (which is already selected)
  • Remove the last volume from the data tree.
  • Result: the combobox displays the first volume.

Analysis

When the last volume is removed from the data tree, this method is called:

QAbstractItemView::rowsAboutToBeRemoved

This method checks the current index:

if the current index is valid then the next index is selected (previous volume => scenario 2)

if the current index is NOT valid then no index is selected (None => scenario 1)

In scenario 1 there is no current index because qMRMLSubjectHierarchyCombobox simply calls qMRMLSubjectHierarchyTreeView which calls only selectionModel()->select which does not set the current index.

As you can see in this commit , this->selectionModel()->setCurrentIndex selects the item AND set the current index.

Do you see any case where an item must be selected without being the current index ?

Do you see any other issue ?

Best regards

2

Both behaviors are very useful. The developer should be able to choose between them using the noneEnabled flag the same way as in the node combobox.

3

noneEnabled is already set to True in my qMRMLSubjectHierarchyCombobox.

Unfortunately I do not find any way to create a reproducer in 3D Slicer.

According to me, removing a row of the subject hierarchy tree should always lead to the same behavior in a qMRMLSubjectHierarchyCombobox, it should not change if the user already clicked on an item or not.

Please let me know if this commit could be interesting or if I have to implement a fix only in my custom app.

Regards

4

Yes, I agree. If this is not the case then it is a bug that should be fixed.

This widget is intended to mirror the behavior of qMRMLNodeComboBox, making it suitable as a drop-in replacement.

5

If this is the case, then I think it is a bug, and your change is well justified. I can’t think of a case when we want to set selection only in the view without setting the same selection in the model.

Thanks for the investigation!

1 Like
6

Here is the pull request: BUG: Manage correctly row deletion in qMRMLSubjectHierarchyCombobox by achataigner · Pull Request #8692 · Slicer/Slicer · GitHub

7

(I cannot approve the PR unless you request me specifically as reviewer, so need to wait until someone with higher rights approves it)

1 Like
8

Ok I just added a comment in the PR to know if you can review it