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: Ability to configure cornerstone tools via extension configuration #1229

Merged
merged 76 commits into from
Dec 9, 2019

Conversation

igoroctaviano
Copy link
Contributor

@igoroctaviano igoroctaviano commented Nov 26, 2019

Won't fix:

  • Make a more generic context menu with customizable options and actions when calling show.
  • Refactor LabellingFlow to DialogService with DialogContent passed from this extension. (Maybe LabellingFlow doesn't need to an independent provider).
  • Pass the ServicesManager the "factory function" instead of the plain object.(reducing chances of accidental service use in core app)
  • Services should all have a 'common through all services' .create() method instead of a .createUIModalService() method.
  • Remove brushTool from our default tools. Include this by default in the seg specific extension.
  • UIContextMenuService and UILabelingFlowService look similar to Dialog. We should find a smarter way to say these a flavoured version of UIDialogService.
  • Simplify LabellingFlow by absorving LabellingManager logic.
  • Pass in rootConfig or applicationConfig to preInit/getModule.

User Cases:

  • As a developer, I can configure cornerstone tools via extension configuration.
  • As a developer, I can use a supported context menu provider and service which allows me to use or control the context menu dialog through a UI service in my extensions.
  • As a developer, I can use a supported labelling flow provider and service which allows me to control the labelling flow dialog through a UI service in my extensions.
  • As a developer, I can now use function type configuration which allows me to inject dependencies and have access to ServicesManager and CommandsManager.
  • As a developer, I can now have CommandsManager available to my extensions.
  • As a developer, I can now use LabellingFlow component independently of ToolContextMenu through a supported UI service.
  • As a developer, I can now use a new cornerstone command to get nearby tool data.

@igoroctaviano igoroctaviano force-pushed the feat/igor/1222-configure-cornerstone-tools branch from c915f31 to 4e33770 Compare November 26, 2019 21:17
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

Merging #1229 into master will increase coverage by 0.34%.
The diff coverage is 3.93%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1229      +/-   ##
=========================================
+ Coverage     9.5%   9.85%   +0.34%     
=========================================
  Files         266     261       -5     
  Lines        6785    6605     -180     
  Branches     1267    1245      -22     
=========================================
+ Hits          645     651       +6     
+ Misses       4986    4822     -164     
+ Partials     1154    1132      -22
Flag Coverage Δ
#core 13.9% <21.73%> (+0.06%) ⬆️
#viewer 0.4% <0%> (+0.03%) ⬆️
Impacted Files Coverage Δ
platform/core/src/index.js 100% <ø> (ø) ⬆️
platform/viewer/src/connectedComponents/Viewer.js 0% <ø> (ø) ⬆️
...latform/core/src/services/UIDialogService/index.js 13.33% <ø> (ø) ⬆️
...tform/viewer/src/connectedComponents/ViewerMain.js 0% <ø> (ø) ⬆️
...nts/EditDescriptionDialog/EditDescriptionDialog.js 0% <ø> (ø) ⬆️
platform/viewer/src/store/index.js 0% <ø> (ø) ⬆️
platform/core/src/classes/CommandsManager.js 92.59% <ø> (ø) ⬆️
...iewer/src/appExtensions/MeasurementsPanel/index.js 0% <0%> (ø) ⬆️
...viewer/src/appExtensions/MeasurementsPanel/init.js 0% <0%> (ø) ⬆️
platform/viewer/src/App.js 0% <0%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

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

@igoroctaviano
Copy link
Contributor Author

The closest thing to QA we have for user stories that impact developers are unit tests. I think we can forego the normal @mirnasilva approved process until we have a better way to run integration/e2e tests for extension and service changes.

It's interesting how much of this work hopefully has no impact on current functionality, but also improves the experience for others trying to tweak the viewer.

I think the only things we have to make sure we do before this can be merged are:

  • Update any documentation and code around the updated <App /> component props

  • Verify the cornerstone extension config changes work

    • Update the default config for a single commit, and share the Deploy Preview for it (then revert in next commit)?
    • Draft at least example schema for the cornerstone extension in it's readme
  • Make sure we note the breaking change

@igoroctaviano, feel free to push back on, or add to, this list.

https://5dee6d5c04b544000807e9db--ohif.netlify.com/pwa/viewer/1.2.826.0.13854362241694438965858641723883466450351448

The configuration can also be written as a JS Function in case you need to inject dependencies like external services:

```js
window.config = ({ randomService } = {}) => {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't randomService actually be the ServicesManager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad example =S, updated. Thanks.

UIContextMenuService,
UILabellingFlowService,
]);
_initExtensions([...defaultExtensions, ...extensions], tools);
Copy link
Member

Choose a reason for hiding this comment

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

Okay. I see why you did this. We are tightly coupled to cornerstone extension, so it's always baked in. That makes it difficult to provide it's configuration in the extensions array.

Having a specific spot for it in the application config stinks, but it's our only option until we decouple.

My only ask is that we switch from providing something as specific as tools, and just pass in an entire cornerstoneExtensionConfig that has a tools key, that way we won't have a breaking change as we add more values we'd like to set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@dannyrb dannyrb merged commit 55a5806 into master Dec 9, 2019
@dannyrb dannyrb deleted the feat/igor/1222-configure-cornerstone-tools branch June 24, 2020 12:52
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