ENH: Allow for custom presets prefix and tooltips by johan-andruejol · Pull Request #1052 · Slicer/SlicerGitSVNArchive

johan-andruejol · 2018-12-06T16:40:31Z

Although Slicer allowed to create custom presets for the Slice views, it
did not allow the user to customize the tooltip nor the prefix that would
show in the view.
It is now possible to customize each of the slice views preset to have
a custom prefix and/or tooltip.

Although Slicer allowed to create custom presets for the Slice views, it did not allow the user to customize the tooltip nor the prefix that would show in the view. It is now possible to customize each of the slice views preset to have a custom prefix and/or tooltip.

jcfr

Well done 👍 I will make use of this right away in an other project 😄

Few nitpicks, and should be good to go.

@lassoan

jcfr · 2018-12-06T16:46:10Z

Libs/MRML/Core/vtkMRMLSliceNode.cxx

-  vtkErrorMacro("GetSliceOrientationPreset: invalid orientation preset name: " << name);
-  return NULL;
+  OrientationPresetType* preset = this->GetOrientationPreset(name);
+  return preset ? preset->Orientation : nullptr;


nullptr -> NULL, since we will most likely create 4.10.1 ... let's continue support c++98

jcfr · 2018-12-06T16:47:36Z

Libs/MRML/Core/vtkMRMLSliceNode.cxx

+    vtkErrorMacro("GetOrientationPreset: The orientation preset "
+      "'" << name << "' does NOT exist.");
+    }
+  return nullptr;


jcfr · 2018-12-06T16:50:56Z

Libs/MRML/Core/vtkMRMLSliceNode.cxx

 }

+//----------------------------------------------------------------------------
+OrientationPresetType* vtkMRMLSliceNode::GetOrientationPreset(const std::string &name, bool error)


bool error -> bool displayError

Instead, what do you think of displaying the error on the user side ?

This is a protected method. It only has this option so you can query the preset existence in HasSliceOrientationPreset without an error.

jcfr · 2018-12-06T16:52:54Z

Libs/MRML/Core/vtkMRMLSliceNode.cxx


 //----------------------------------------------------------------------------
-bool vtkMRMLSliceNode::HasSliceOrientationPreset(const std::string &name)
+bool vtkMRMLSliceNode::RenameSliceOrientationPresetPrefix(const std::string &name, const std::string &prefix)


What about RenameSliceOrientationPresetPrefix -> SetSliceOrientationPresetPrefix ?

jcfr · 2018-12-06T16:53:09Z

Libs/MRML/Core/vtkMRMLSliceNode.cxx

-  std::vector< OrientationPresetType >::iterator it;
-  for (it = this->OrientationMatrices.begin(); it != this->OrientationMatrices.end(); ++it)
+//----------------------------------------------------------------------------
+bool vtkMRMLSliceNode::RenameSliceOrientationPresetTooltip(const std::string &name, const std::string &tip)


RenameSliceOrientationPresetTooltip -> SetSliceOrientationPresetTooltip

jcfr · 2018-12-06T16:55:57Z

Libs/MRML/Core/vtkMRMLSliceNode.h

 class vtkMatrix4x4;

+/// \brief Structure to store preset orientation information.
+struct OrientationPresetType


struct VTK_MRML_EXPORT OrientationPresetType

Instead, I suggest to move the struct into the vtkMRMLSliceNode class, that way it will be available from python.

jcfr · 2018-12-06T16:57:14Z

Libs/MRML/Core/vtkMRMLSliceNode.h

+  /// \sa AddSliceOrientationPreset(
+  ///    const std::string& name, vtkMatrix3x3* sliceToRAS,
+  ///    const std::string& prefix, const std::string& tooltip)
  /// \sa UpdateMatrices()


/// \sa GetSliceOrientationPresetPrefix() /// \sa GetSliceOrientationPresetTooltip()

jcfr · 2018-12-06T16:58:21Z

Libs/MRML/Core/vtkMRMLSliceNode.h


+  /// \brief Return the preset prefix corresponding to a preset \a name.
+  ///
+  /// Returns an empty string if the preset with the given \a name doesn't exist.


\sa SetSliceOrientationPresetPrefix() \sa SetSliceOrientationPresetTooltip()

jcfr · 2018-12-06T16:59:17Z

Libs/MRML/Core/vtkMRMLSliceNode.h

  /// \sa RemoveSliceOrientationPreset(const std::string& name)
-  bool AddSliceOrientationPreset(const std::string& name, vtkMatrix3x3* orientationMatrix);
+  /// \sa RenameSliceOrientationPresetPrefix(const std::string& name, const std::string& prefix)
+  /// \sa RenameSliceOrientationPresetTooltip(const std::string& name, const std::string& tip)


Since there are no ambiguity on the signature, I suggest to reference the function without arguments ... less chance for it become out of the sync

\sa SetSliceOrientationPresetPrefix() \sa SetSliceOrientationPresetTooltip()

pieper · 2018-12-06T18:47:40Z

Can you add a note about why (in what use case) you want to use these options?

johan-andruejol · 2018-12-06T19:08:54Z

@pieper:
Sure !
When you add a custom preset, the tooltip always shows "Oblique" as if you were in a "Reformat" view, which is weird for users.
As an example:

Before this, the only way to modify this would have been to find the qMRMLSliderWidget for each qMRMLSliceControllerWidget and manually change each tooltip and each prefix. This would have to be repeated whenever you change layout as well.

pieper · 2018-12-06T19:10:57Z

Ah - 👍 thanks!

lassoan

Thank you, it will be useful to remove hardcoded offset label and slider tooltip. However, there are some concepts to be cleaned up.

Slice node should not store strings that the application must blindly display in very specific places, as we don't know how exactly the application wants to present slice views. Instead, it should store properties that the application can interpret and decide how to display.

For example, instead of storing preformatted prefix ("S: ") and tooltip ("I <-----> S"), we should just store axis labels for positive and negative directions "S" and "I". Then the application may decide to use axis labels to generate prefix and tooltip strings.

Since axis labels are already defined in the base class (vtkMRMLAbstractViewNode, Get/SetAxisLabel), it is not necessary to define labels again. We can determine the axis index from the slice normal direction (3rd column of the orientation matrix). If the normal is aligned exactly with an axis then you can use the corresponding axis labels. If it is not aligned exactly then we can use multiple directions (non-zero components of the slice normal vector; for example: LA <-----> RP; or LAI <-----> RPS) or just not use a label at all.

While we can always compute axis labels, it may be useful to specify custom axis labels for custom orientations. For example, an organ-oriented preset we might want to use "medial<----->lateral" labels instead of some automatically generated labels.

To summarize: I would recommend to replace "prefix" and "tooltip" members by "LabelAxisPositive" and "LabelAxisNegative". Generate prefix and tooltip strings in the GUI widget class. We can decide if we want to compute axis index from direction matrix to look up axis label or we always rely on "LabelAxisPositive" and "LabelAxisNegative" strings.

lassoan · 2018-12-07T02:13:39Z

Libs/MRML/Core/vtkMRMLSliceNode.h

+/// \brief Structure to store preset orientation information.
+struct OrientationPresetType
+{
+  std::string Name;


Please add documentation for each member. FOr example, I don't know what "Prefix" is used for, how it is different from "Name" and why it is called prefix (prefix to what?).

lassoan · 2018-12-07T02:16:15Z

Libs/MRML/Core/vtkMRMLSliceNode.h

+  std::string Name;
+  vtkSmartPointer<vtkMatrix3x3> Orientation;
+  std::string Prefix;
+  std::string Tooltip;


"tooltip" describes too specifically how this information must be used for. Instead, the variable should be called "description" or something similar and the application could decide how to present it to the user (for example, the application can show it as a tooltip).

@lassoan What about changing Tooltip into Description ?

My general comment above essentially supersedes this comment. We should not store preformatted display strings here. Instead, we should just store axis labels (which by the way are optional because we already define axis labels for the 3 main axes in the abstract view node).

For example just store "S" and "I" axis labels and let the GUI widget generate "S: " prefix and "I <-----> S" tooltip.

Agreed. It makes sense to update qMRMLViewControllerBar to generate reasonable default for Prefix, Tooltip, ...

That said, to support custom value per presets:

for a custom preset named Foo, add user setting entry (e.g ViewControllerBar/OrientationPresets/Foo/Tooltip, ViewControllerBar/OrientationPresets/Foo/Prefix)

the qMRMLViewControllerBar would be updated to check if settings are set and associated values

Finally, if in a custom application more advanced customization are still needed, making sure the "widgets" of the viewer controller are associated with an objectName allow getting their reference and update them. (for example to hide the pin button, set prefix to empty string, ...)

to support custom value per presets

Allowing setting custom positive/negative axis labels should be enough customization. Do you have any use case where this would not give good results?

Finally, if in a custom application more advanced customization are still needed, making sure the "widgets" of the viewer controller are associated with an objectName allow getting their reference and update them.

I agree. This would allow fine-grained customization without overcomplicating the API. For common customizations, such as hiding the pin button and adding custom buttons next to the slider, we should also add explicit method calls.

lassoan · 2018-12-07T02:21:48Z

Libs/MRML/Core/vtkMRMLSliceNode.cxx

 }

+//----------------------------------------------------------------------------
+OrientationPresetType* vtkMRMLSliceNode::GetOrientationPreset(const std::string &name, bool error)


I would not complicate the API with with this "bool error" flag. Since the caller can detect if a preset is not found (by getting NULL as result), it is not essential to log an error..

jcfr requested changes Dec 6, 2018

View reviewed changes

lassoan reviewed Dec 7, 2018

View reviewed changes

Conversation

johan-andruejol commented Dec 6, 2018

Uh oh!

jcfr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pieper commented Dec 6, 2018

Uh oh!

johan-andruejol commented Dec 6, 2018

Uh oh!

pieper commented Dec 6, 2018

Uh oh!

lassoan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants