Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(toolbar): new Toolbar to enable reactive state synchronization #3983

Merged
merged 88 commits into from
Mar 27, 2024

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Mar 8, 2024

This PR will address some fundamental issues with the current implementation of the toolbar and toolbar service.

Fixing these

There were so many things wrong with our toolbar, where to synchronize the button states
with the tool state you should have activated the tool via toolbarService.recordInteraction which
was horrible.

This PR will add a new concept of evaluators for each button, which will be used to determine the
state of the button based on the app context.

There are some breaking changes in this PR, but I think they are necessary to make the toolbar more
flexible and easier to use. We will talk about the breaking changes later down

Reactive Toolbar

Addressing

By the introduction of the evaluators, the toolbar will be reactive to the viewports and tooLGroups see below movie

Area.mp4

Flexible Button Sections

You can assign any button to any section you want, in your extensions you can provide a
template that accepts button section and in your modes you should be able to assign buttons
to it.

We have created a new general ToolBox component that you can use as many time as you want
with different section names. We are shipping it for the segmentation mode for now.
You should be able to do the following for instance

CleanShot 2024-03-08 at 14 55 27

by saying

toolbarService.addButtons(toolbarButtons)
toolbarService.createButtonSection("primary", [
  "Pan",
  "Capture",
  "Layout",
  "MPR",
  "Crosshairs",
  "Zoom",
  "MoreTools",
])
toolbarService.createButtonSection("segmentationToolbox", [
  "Zoom",
  "WindowLevel",
])

and you should be able to modify the segmentation toolbox initial setup too via your modes by assigning buttons.

{
    id: 'Shapes',
    uiType: 'ohif.radioGroup',
    props: {
      label: 'Shapes',
      evaluate: {
        name: 'evaluate.cornerstone.segmentation',
        options: { toolNames: ['CircleScissor', 'SphereScissor', 'RectangleScissor'] },
      },
      icon: 'icon-tool-shape',
      commands: _createSetToolActiveCommands('CircleScissor'),
      options: [
        {
          name: 'Shape',
          type: 'radio',
          value: 'CircleScissor',
          id: 'shape-mode',
          values: [
            { value: 'CircleScissor', label: 'Circle' },
            { value: 'SphereScissor', label: 'Sphere' },
            { value: 'RectangleScissor', label: 'Rectangle' },
          ],
          commands:'setToolActiveToolbar'
        },
      ],
    },
  },

above as you see we use commands to set the most updated option value via a command, also you could have more sophisticated option change handling such as

{
      name: 'Radius (mm)',
      id: 'eraser-radius',
      type: 'range',
      min: 0.5,
      max: 99.5,
      step: 0.5,
      value: 25,
      commands: {
        commandName: 'setBrushSize',
        commandOptions: { toolNames: ['CircularEraser', 'SphereEraser'] },
      },
    },

You can now have toolbox (in a modal) with tools and advanced options inside and everything will be synced in terms of state

CleanShot 2024-03-25 at 14 20 28@2x

Read more in Toolbar.md

initial states for toggles

Addressing

Now you can set the toggles initially and start the viewer with them, e.g., start viewer by reference lines on

CleanShot 2024-03-08 at 15 09 21

You only need to active/enable them in the initToolGroups and everything will work

Hotkeys synchronization with toolbar (in progress)

Will address this

Breaking Changes

  1. There concept of activeTool and their getter and setter is removed. The active tool
    should be derived by the toolGroup and the viewport and by manually settings it.

action need to be taken

let unsubscribe
toolbarService.setDefaultTool({
  groupId: "WindowLevel",
  itemId: "WindowLevel",
  interactionType: "tool",
  commands: [
    {
      commandName: "setToolActive",
      commandOptions: {
        toolName: "WindowLevel",
      },
      context: "CORNERSTONE",
    },
  ],
})

const activateTool = () => {
  toolbarService.recordInteraction(toolbarService.getDefaultTool())

  // We don't need to reset the active tool whenever a viewport is getting
  // added to the toolGroup.
  unsubscribe()
}

// Since we only have one viewport for the basic cs3d mode and it has
// only one hanging protocol, we can just use the first viewport
;({ unsubscribe } = toolGroupService.subscribe(
  toolGroupService.EVENTS.VIEWPORT_ADDED,
  activateTool
))

Since tools are setup to be (active, passive, disabled and enabled) by the toolGroup
in initToolGroups you shouldn't worry about this, and after this PR we will
automatically set the active tool based on the toolGroup and the viewport.

The only thing as a Mode writer you should do is to write what buttons you have and
where you want to put them, as simple as

toolbarService.addButtons([...toolbarButtons, ...moreTools])
toolbarService.createButtonSection("primary", [
  "MeasurementTools",
  "Zoom",
  "WindowLevel",
  "Pan",
  "Capture",
  "Layout",
  "MPR",
  "Crosshairs",
  "MoreTools",
])
  1. There is no concept of button type anymore, there is no need to decide on toggle , action or tool.
    Buttons now are just buttons, which are some properties (icon, label, etc.) with commands that will
    run (by toolbarService) after interaction. Evaluators will be used to determine the state of the button
    (color, disabled etc.)

remove the type property from the buttons, and instead use the uiType for the top level ui type def.

old implementation

{
    id: 'Capture',
    type: 'ohif.action',
    props: {
      icon: 'tool-capture',
      label: 'Capture',
      type: 'action',
      commands: [
        {
          commandName: 'showDownloadViewportModal',
          commandOptions: {},
          context: 'CORNERSTONE',
        },
      ],
    },
  },

New implementation

{
  id: 'Capture',
  uiType: 'ohif.radioGroup',
  props: {
    icon: 'tool-capture',
    label: 'Capture',
    commands: [
      {
        commandName: 'showDownloadViewportModal',
        context: 'CORNERSTONE',
      },
    ],
    evaluate: 'evaluate.action',
  },
},

(Notice the uiType)
(we will talk about the evaluate property later down)

In addition, the previous signature of button definition (ToolbarService.createButton), which relied on positional arguments, is removed and replaced by a more flexible object-based definition.

**old implementation **

ToolbarService._createToolButton(
  'ArrowAnnotate',
  'tool-annotate',
  'Annotation',
  [
    {
      commandName: 'setToolActive',
      commandOptions: {
        toolName: 'ArrowAnnotate',
      },
      context: 'CORNERSTONE',
    },
    {
      commandName: 'setToolActive',
      commandOptions: {
        toolName: 'SRArrowAnnotate',
        toolGroupId: 'SRToolGroup',
      },
      context: 'CORNERSTONE',
    },
  ],
  'Arrow Annotate'
),

New implementation

ToolbarService.createButton({
  id: 'ArrowAnnotate',
  icon: 'tool-annotate',
  label: 'Annotation',
  tooltip: 'Arrow Annotate',
  commands: {
      commandName: 'setToolActiveToolbar',
      commandOptions: {
          toolGroupIds: ['default', 'SRToolGroup'],
      },
   },
  evaluate: 'evaluate.cornerstoneTool',
}),

(notice the object based definition in the new implementation)
(we will talk about the evaluate property next)

  1. Add the proper evaluate property to the button definition, which will be used to determine the state of the button
    based on the app context.

    You can check out the modes/longitudinal/src/toolbarButtons.ts for the usage of the evaluate property.

    But in general

    • Use the evaluate.cornerstoneTool if you want the button to be highlighted if it is only on active primary (left mouse)
    • Use evaluate.cornerstoneTool.toggle if the tool is a toggle tool (like reference lines or image overlay)
    • Read more in the toolbar module documentation

Changes & Results

Testing

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] OS:
  • [] Node version:
  • [] Browser:

***Add button condition for toolGroup.id !== 'mpr'***
***Update ListMenu component to handle disabled items***
***Add evaluateToolbarButtonState module
***Description:***
This commit updates the SyncGroupService, segmentationButtons, and MeasurementService files.

In SyncGroupService, the code has been modified to handle cases where the renderingEngine is not found for a given viewportId. It now falls back to the first renderingEngine in the list.

In segmentationButtons, the default values for the brush size, eraser size, and threshold size have been changed from 15 to 25.

In MeasurementService, the unmappedMeasurements variable has been changed from a Set to a Map. Additionally, a new error handling logic has been added to handle cases where mapping fails.

In toolbar.md, a new section has been added to explain how to change the toolbar based on the hanging protocol.

***Note:
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 44.41%. Comparing base (8a335bd) to head (2141064).
Report is 263 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3983      +/-   ##
==========================================
- Coverage   46.23%   44.41%   -1.83%     
==========================================
  Files          78       80       +2     
  Lines        1276     1333      +57     
  Branches      312      327      +15     
==========================================
+ Hits          590      592       +2     
- Misses        548      588      +40     
- Partials      138      153      +15     
Files Coverage Δ
platform/app/src/appInit.js 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79d5c36...2141064. Read the comment docs.

Copy link
Contributor

@wayfarer3130 wayfarer3130 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks wonderful - very much looking forward to having this in.

@sedghi sedghi merged commit 566b25a into master Mar 27, 2024
10 of 11 checks passed
edcheyjr pushed a commit to edcheyjr/Viewers that referenced this pull request Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants