BUG: Suppress unnecessary warning for hidden subject hierarchy items

[jamesobutler]

Nodes defined to be hidden from editors were being unnecessarily warned by the "Unable to find subject hierarchy item by data node" log message because it is expected for them to not be visible and available in the subject hierarchy as they are hidden.

Code that previously produced the unnecessary warning:

slicer.util.selectModule("Markups")
line_node = slicer.vtkMRMLMarkupsLineNode()
line_node.SetHideFromEditors(True)
slicer.mrmlScene.AddNode(line_node)

main	This PR

[lassoan]

There may be many reasons why a subject hierarchy item is not created for a node. We should not treat it as a special case when an item is not created because HideFromEditors=False.

Also, HideFromEditors is just a hint that GUI widgets should not show that node (and therefore a corresponding SH item) but it should still be possible to create an item for it.

So, I would just remove the warning message.

[jamesobutler]

I've pushed an additional commit that removes the qCritical warning based on the reasoning that @lassoan described.

[jcfr]

but it should still be possible to create an item for it.

So, I would just remove the warning message.

I think @lassoan meant we should also remove the early return ... this means of the relevant SH node are explicitly created. It should be possible to establish the association.

[lassoan]

I think @lassoan meant we should also remove the early return ... this means of the relevant SH node are explicitly created. It should be possible to establish the association.

Yes.

The qMRMLNodeComboBox sets the selection to None if a non-existent or non-visible node is set in it. You can try by running this code snippet:

w = slicer.qMRMLNodeComboBox()
w.setMRMLScene(slicer.mrmlScene)
w.show()
w.setCurrentNodeID("vtkMRMLColorTableNodeFilePlasma.txt")

To make the behavior consistent, calling setCurrentNode with a non-existent or invisible item should set the selection to None in the SH tree view, too. So, instead of an early return it should update the selection.

[jamesobutler]

If I had a current selection in the subject hierarchy tree view and I added a node to the scene that I predefined as hidden from editors, wouldn’t I want to keep my original subject hierarchy selection rather than selecting None? I would assume the hide from editors indication is saying to not get involved in that widget and therefore it shouldn’t change to a None state from an existing selection.

[lassoan]

If I had a current selection in the subject hierarchy tree view and I added a node to the scene that I predefined as hidden from editors, wouldn’t I want to keep my original subject hierarchy selection rather than selecting None? I would assume the hide from editors indication is saying to not get involved in that widget and therefore it shouldn’t change to a None state from an existing selection.

Fully agree, adding a node should not impact selection (regardless of an SH item was created for that node or not).

The reason we see the warning is that the Markups module automatically selects the currently active markup node in the GUI.

Slicer/Modules/Loadable/Markups/qSlicerMarkupsModuleWidget.cxx

Line 910 in ee8d54e

d->activeMarkupTreeView->setCurrentNode(d->MarkupsNode);

To be consistent with the rest of the Markups module and the Markups toolbar and to allow placing points for hidden markup nodes (e.g., temporary markups used for interacting in views), we should probably remove the selection from the SH tree view and not display any warning when a a hidden markup node is selected as active placement node:

[lassoan]

So, we should change the behavior to:

• not show a warning
• remove the selection

[jamesobutler]

slicer.util.selectModule("Markups")
line_node = slicer.vtkMRMLMarkupsLineNode()
line_node.SetHideFromEditors(True)
slicer.mrmlScene.AddNode(line_node)

Ok so in the case of an angle node having been selected prior to the hidden line node added in the code above, the Markups module subject hierarchy tree view should still maintain the selection of the angle node with the collapsible sections below still enabled based on the angle node and the Markups Toolbar node combobox still reflecting that angle node is the selection?

[jamesobutler]

I have added a commit showing the case of making sure all the selections are clear when adding a hidden markup node to the scene, though seems a little odd to me that the previous selection of the non-hidden node is lost.

main	This PR

[lassoan]

though seems a little odd to me that the previous selection of the non-hidden node is lost.

I agree, this is unusual behavior compared to other modules. It is a consequence of the unusual behavior that the markup node selection in Markups module shows the active place node. None of the other modules do this (there are "active" nodes for some other classes, but they are only used to propagate selection in viewers, and only on request). I believe this special behavior is useful because it makes easy to see which markup is where points will be added to when the user clicks in a view. We might be able to remove this unusual behavior if we implement focus display in views. Then it may be more clear what markup is being edited and the Markups module's node selection no longer has to show this.

To avoid the side effect of losing selection in Markups module when a hidden markup node is added: we could probably find a way to prevent making a newly added markup node automatically the active place node. It is not a must to change this in Slicer core though, because if a module adds a hidden markup node, then that module can also save the previously selected active place node and restore it after it added the hidden markup node.

[jamesobutler]

@lassoan I have squashed down the various commits here to get to the simplified removal of the warning and the early return. This should address the log spam I was observing while creating some hidden nodes. For the future I'll consider whether or not changing the active place node state upon adding a hidden node, but for now this change is acceptable for my work.

[jcfr]

For the future I'll consider whether or not changing the active place node state upon adding a hidden node

@jamesobutler Thanks for your patience with the whole review process. To capture this requirement, when you have a chance, would you be able to create a feature request ?

[lassoan]

This looks good to me. I've tested it and it behaved as expected - unselecting the item in the subject hierarchy tree in Markups module if the active place node was not visible in the tree.

It would be nice to mention the behavior change in the commit comment (selection is removed in Markups module if the active markup node is not visible in the tree).

[jcfr]

It would be nice to mention the behavior change in the commit comment (selection is removed in Markups module if the active markup node is not visible in the tree).

@lassoan To account for this, I ended up selecting Squash & Merge specifying the following message:

BUG: Suppress unnecessary warning for hidden subject hierarchy items

Nodes marked as hidden from editors were triggering an unnecessary critical warning:
`Unable to find subject hierarchy item by data node`.

This was misleading, since it is expected that hidden nodes do not have a subject
hierarchy item. The warning has been removed to avoid confusion.

Additionally, behavior has been updated so that if the active markup node
is not visible in the subject hierarchy, its selection is cleared in the
**Markups** module. This prevents inconsistencies between the active place
node and the subject hierarchy tree view.

Example of previously logged unnecessary warning:

```python
slicer.util.selectModule("Markups")
line_node = slicer.vtkMRMLMarkupsLineNode()
line_node.SetHideFromEditors(True)
slicer.mrmlScene.AddNode(line_node)
```

Co-authored-by: Andras Lasso <lasso@queensu.ca>