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): enable extensions to change toolbar button sections #4367

Conversation

pedrokohler
Copy link
Collaborator

Context

Currently, it's not possible to modify the primary toolbar button section with an extension. There are a few scenarios in which one could want to do that. For example:

  1. It's possible you want to add a button to all modes at once by using your extension.
  2. You might not have a custom mode in your project, and the only thing you want to do is add a button to your primary toolbar button section

The reason it's not possible to do that today is the following:

The mode's toolbar operations override the extensions', because the onModeEnter method of the mode runs after the extensions' one:

image

Since OHIF's pre-existing modes, such as longitudinal, tmtv, etc, create button sections from scratch, instead of adding buttons to a button section, it's not possible to add anything to the primary button section from the extension, since it gets overwritten later on.

Changes & Results

With these changes, one can now add buttons to every mode by using the extensions onModeEnter to alter the button section. See an example:

Before (extension has no bearing in the toolbar):
image

After (extension can add buttons to the toolbar):
image

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:

Copy link

netlify bot commented Sep 11, 2024

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit acb3e66
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/66e330ba4d43f5000751e995
😎 Deploy Preview https://deploy-preview-4367--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 11, 2024

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit acb3e66
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/66e330ba3c3af900075d34fa
😎 Deploy Preview https://deploy-preview-4367--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pedrokohler pedrokohler changed the title feat: enable extensions to change toolbar sections feat: enable extensions to change toolbar button sections Sep 11, 2024
Copy link
Collaborator

@IbrahimCSAE IbrahimCSAE left a comment

Choose a reason for hiding this comment

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

This PR makes sense to me, unless i'm missing something. It's a nice fix. I also tried locally and for example, if in your extensions 'onModeEnter' you define something like:

   toolbarService.createButtonSection('primary', ['CornerstoneButton']);
    toolbarService.addButtons([
      {
        id: 'CornerstoneButton',
        uiType: 'ohif.radioGroup',
        props: {
          icon: 'tool-zoom',
          label: 'Zoom',
          commands: [
            {
              commandName: 'setToolActiveToolbar',
              commandOptions: {
                toolGroupIds: ['default', 'mpr', 'SRToolGroup', 'volume3d'],
              },
            },
          ],
          evaluate: 'evaluate.cornerstoneTool',
        },
      },
    ]);

It seems that later on when mode.OnModeEnter() is called, that also calls the toolbarServices.createButtonSection(), which does set the buttons but also overrides the existing section at the same time

  createButtonSection(key, buttons) {
    this.state.buttonSections[key] = buttons;
    this._broadcastEvent(this.EVENTS.TOOL_BAR_MODIFIED, { ...this.state });
  }

I thought we could try using a different buttonSectionId for our section like cornerstoneSection, but the top toolbar uses primary by default so it won't appear up there but will be in the state,

however, do you think this is a cleaner fix? I think this way no one needs to modify their modes or add anything extra.

  createButtonSection(key, buttons) {
    if (this.state.buttonSections[key]) {
      this.state.buttonSections[key].push(...buttons);
    } else {
      this.state.buttonSections[key] = buttons;
    }
    this._broadcastEvent(this.EVENTS.TOOL_BAR_MODIFIED, { ...this.state });
  }

This appends to the existing section if it exists, so if a mode finds that primary exists, it will add on top of it, and it should work fine since the toolbar is always reset when you enter a brand new mode, so the appending shouldn't cause any collisions or state management issues.

@sedghi

@IbrahimCSAE IbrahimCSAE changed the title feat: enable extensions to change toolbar button sections feat(toolbar): enable extensions to change toolbar button sections Sep 12, 2024
@pedrokohler
Copy link
Collaborator Author

pedrokohler commented Sep 12, 2024

however, do you think this is a cleaner fix? I think this way no one needs to modify their modes or add anything extra.

that's a good idea @IbrahimCSAE, the only thing I'd do differently is that I'd create a new method with a different name, since now we're not creating a button section anymore, we're sort of "upserting". The current name could be misleading and changing it's behavior right now could affect other people's customizations.

@IbrahimCSAE IbrahimCSAE merged commit 1bfce0a into OHIF:master Sep 12, 2024
9 of 10 checks passed
pedrokohler added a commit to ImagingDataCommons/ViewersV3 that referenced this pull request Sep 13, 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.

3 participants