Slicer
ENH: Add a transform plugin in subject hierarchy tree view
Transform related functions were originally not included into a specific plugin. Thus some functionalities related to the management of plugins were not available. Fix https://github.com/Slicer/Slicer/issues/6621
Hi all, I was a bit confused about the formatting that slicer is using. Can you help me on that please ? @jcfr @lassoan
Before I do a thorough review, can you please
- Summarize the change in a bit more detail in the commit message. It facilitates future maintenance to have meaningful and informative commit messages that contain more than just the few dozen characters allowed in the first line
- Remove the style changes. It makes the review much harder (lots of unrelated changes), and much of it seems subjective. Style changes typically arrive in large commits with automatic formatting. Let's focus on the functional part here not the style. If you worry about the style, then I suggest doing it in a separate PR.
Thank you!
Remove the style changes.
I agree, I wanted to review the changes, but all the whitespace changes made it much more difficult to see the actual changes. If you want to make style changes then you can put them in a separate commit.
Thanks for the contribution :pray:
While the following GitHub feature may help ignore the white space changes, it would be helpful to avoid introducing newlines and alike.
It will be idea to explain the new API introduces, and how those are used.
In any case the comment of @riep saying "I was a bit confused about the formatting that slicer is using" is completely valid, as I also am not sure about the exact guidelines. Some of his changes seem to go from one acceptable style to another, so I'd not integrate those. Unless there is a clear guideline and the change makes the code conform with that.
Dear all, Modifications done regarding formatting. @cpinter I agree some changes are not essential. I just like to apply a formatting tool and do not think about it while coding. There are no additional functional added in this PR. It is only code moving related to transform actions from qMRMLSubjectHierarchyTreeView to a new dedicated plugin. The behavior MUST be unchanged.
Thanks for the update @riep. I'm refactoring the code to be a bit more consistent with other plugins and will push the proposed changes to this branch soon.
I've finished with the refactoring to make this new plugin more conform to other plugins. It was not trivial, as this plugin was a bit special in that it generates menu items quite dynamically (based what transform nodes are available in the scene).
@riep please test if this works well for you and if you have any comments or suggestions.
Load a volume > apply a transform to the loaded volume > check interaction to display the handler > right click on the volume and select "None" as transform after these steps the handler is still visible which is unexpected then right click on the volume and select a transform > check interaction to display the handler after these step a second handler is visible for the volume
This is the intended behavior. "Interaction" checkbox includes a convenience feature that it creates a parent transform if the node does not have a parent transform already.
The "Interaction" word may not be ideal (it might suggest that it is a property of the selected node) and overall transform interactions can be complex. Let us know if you have any specific suggestion how to make the behavior more intuitive.
Load a volume > apply a transform to the loaded volume > check interaction to display the handler > right click on the volume and select "None" as transform after these steps the handler is still visible which is unexpected then right click on the volume and select a transform > check interaction to display the handler after these step a second handler is visible for the volume
This is the intended behavior. "Interaction" checkbox includes a convenience feature that it creates a parent transform if the node does not have a parent transform already.
The "Interaction" word may not be ideal (it might suggest that it is a property of the selected node) and overall transform interactions can be complex. Let us know if you have any specific suggestion how to make the behavior more intuitive.
Load a volume > apply a transform to the loaded volume > check interaction to display the handler > right click on the volume and select "None" as transform after these steps the handler is still visible which is unexpected then right click on the volume and select a transform > check interaction to display the handler after these step a second handler is visible for the volume
This is the intended behavior. "Interaction" checkbox includes a convenience feature that it creates a parent transform if the node does not have a parent transform already.
The "Interaction" word may not be ideal (it might suggest that it is a property of the selected node) and overall transform interactions can be complex. Let us know if you have any specific suggestion how to make the behavior more intuitive.
Yes you are right, I understood the feature. One issue, we now have to possibility to show the interaction/ interaction option when right clicking on the "visible" and "color" columns which is not expected. Is it ? In addition, "interaction options" is not available in transform column ...
I can have a look at this
We are trying to find a balance between making available features at convenient places, but not to have too complex GUI with too many options exposed.
Let us know if you have any specific suggestions on how to tweak the GUI.
Current behavior: "Interaction" check box can be checked by right clicking on "visibility", "color" and "transform" columns When checking "interaction" by right clicking on "visibility" and "color" , "interaction options" appears BUT NOT for "transform" column
What I would expect: "Interaction" check box AND "interaction options" are only visible by right clicking on "transform" column
In fact there is some "overlay" between qSlicerSubjectHierarchyTransformPlugin and qSlicerSubjectHierarchyTransformsPlugin (note the s that differs between the two names)
Current behavior: "Interaction" check box can be checked by right clicking on "visibility", "color" and "transform" columns When checking "interaction" by right clicking on "visibility" and "color" , "interaction options" appears BUT NOT for "transform" column
What I would expect: "Interaction" check box AND "interaction options" are only visible by right clicking on "transform" column
WELL, I think I understand the idea behind this choice. I wont explain what I understood :) ... but I am fine with this. Thanks for you help Andras !
Good catch, we should not have two transform plugins! I'll merge the two.
If it is not too difficult to add interaction options to the transform column then I'll add it there, too, for consistency.
Thanks for the note, I've removed the unnecessary empty lines in https://github.com/Slicer/Slicer/pull/8022.