feature(Mode): Ability to overwrite mode configuration and small improvements #3580

igoroctaviano · 2023-08-07T21:33:51Z

Context

Allowing the override of mode's configuration through viewer config and small improvements.

Changes & Results

[Feature] Ability to override module configuration
[Improvement] Hardening of patientName util
[Improvement] Hardening default layout header's return click handler (validating dataSource before appending query params)
[Improvement] Only render display name button in the worklist if defined in the mode configuration
[Improvement] Pass appConfig to onModeExit and onModeEnter hooks

What are the effects of this change?

Before vs After
Screenshots / GIFs / Videos
-->

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:

netlify · 2023-08-07T21:33:57Z

✅ Deploy Preview for ohif-dev ready!

Name	Link
🔨 Latest commit
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-dev/deploys/650ba069752dff797ebc04b9
😎 Deploy Preview	https://deploy-preview-3580--ohif-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

netlify · 2023-08-07T21:33:57Z

✅ Deploy Preview for ohif-platform-docs ready!

Name	Link
🔨 Latest commit	`1ca97e6`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-platform-docs/deploys/64d246147d71db0008edfe88
😎 Deploy Preview	https://deploy-preview-3580--ohif-platform-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

codecov · 2023-08-07T21:34:07Z

Codecov Report

Merging #3580 (1ca97e6) into master (72207e6) will decrease coverage by 0.06%.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3580      +/-   ##
==========================================
- Coverage   42.93%   42.87%   -0.06%     
==========================================
  Files          80       80              
  Lines        1444     1446       +2     
  Branches      338      339       +1     
==========================================
  Hits          620      620              
- Misses        661      662       +1     
- Partials      163      164       +1

Files Changed	Coverage Δ
platform/core/src/utils/formatPN.js	`0.00% <0.00%> (ø)`

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 58d3849...1ca97e6. Read the comment docs.

cypress · 2023-08-07T21:46:14Z

Passing run #3417 ↗︎

0	36	0	0	0

Details:

Add note to doc
Project: Viewers	Commit: `1ca97e61e0`
Status: Passed	Duration: 03:08 💡
Started: Aug 8, 2023 1:48 PM	Ended: Aug 8, 2023 1:51 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

sedghi · 2023-08-08T12:00:14Z

platform/docs/docs/configuration/configurationFiles.md

+  - Example config:
+  ```js
+    modesConfiguration: {
+      ['@ohif/mode-longitudinal']: {


why it is an array here?

It would be great if you can add an example of changing the mode path too

The array is just to define a property with special characters like the mode id (just a map).
I'll add more examples 👍

I don't understand why you can't say [{ '@ohif/mode-longitudinal': {...}, { '@ohif/mode-segmentation':{...}]

and you need to say ['@ohif/mode-longitudinal']

I don't understand why you can't say [{ '@ohif/mode-longitudinal': {...}, { '@ohif/mode-segmentation':{...}]

and you need to say ['@ohif/mode-longitudinal']

I can use array but I think its a lot easier to use a map instead? so later I can just const config = modesConfiguration[modeId] instead of iterating the array looking for the config.

Furthermore, I am trying to test this PR and when I go to save the configuration js file, the ['@ohif/mode-longitudinal']: automatically changes to '@ohif/mode-longitudinal':.

It's not required to use the brackets, this is just an explicit way of doing it. I'll update the docs.

Is it worth mentioning in the docs that although the modeConfiguration is passed to the mode factory it is up to the particular mode itself if and how it applies the configuration? That is, for example, basic-dev-mode and basic-test-mode do not currently use this modeConfiguration. Just a thought.

Agree, updated.

sedghi

see my comments

igoroctaviano · 2023-08-08T12:38:50Z

see my comments

Updated, is this what you meant?

jbocce · 2023-08-08T12:49:44Z

extensions/default/src/ViewerLayout/index.tsx

    // Todo: Handle parameters in a better way.
    const query = new URLSearchParams(window.location.search);
    const configUrl = query.get('configUrl');

+    const dataSourceName = pathname.substring(dataSourceIdx + 1);
+    const existingDataSource = extensionManager.getDataSources(dataSourceName);


Are we anticipating a data source to be in the URL that is NOT an actual, configured data source? If so, under what circumstances would this occur? I am a bit confused as to why we need this check.

I might have a mode that does not use /${mode.routeName}/${dataSourceName}. This means that my mode can rely on the defaultDataSource from the viewer config to define its dataSource. This way I don't need to add the datasources query param and change the defaultDataSource of the worklist coming from a random mode URL e.g. /myModeURL/otherParamWhichIsNotNecessarilyADataSourceName (no dataSource being used)

Ok cool. Thanks.

jbocce

Looks good. Approved.

igoroctaviano added 7 commits August 7, 2023 16:15

Hardening of format pn util

4d75668

Add support for mode configuration and new hooks

04d2f37

Remove unused hook

aa6a824

Only render worklist button if displayName is provided

521ac1e

Only append valid datasources

516a51a

Rollback unused changes

d0b3e04

Add documentation

962422c

Allow config overwrite of microscopy and tmtv mode

64636d2

igoroctaviano requested a review from sedghi August 7, 2023 21:38

igoroctaviano requested a review from jbocce August 8, 2023 00:03

sedghi reviewed Aug 8, 2023

View reviewed changes

Improve example

bd2802d

jbocce reviewed Aug 8, 2023

View reviewed changes

igoroctaviano added 2 commits August 8, 2023 10:11

Update doc object def code style

7c27bc5

Add note to doc

1ca97e6

jbocce approved these changes Aug 8, 2023

View reviewed changes

sedghi merged commit 0493a0d into master Aug 8, 2023
12 of 13 checks passed

rodrigobasilio2022 mentioned this pull request Aug 9, 2023

fix exception in OHIF patient name format ImagingDataCommons/Viewers#225

Closed

sedghi mentioned this pull request Sep 26, 2023

release candidate 3p7 RC1 #3680

Closed

This was referenced Oct 10, 2023

candidate 3p7 rc 2 #3712

Closed

feat(v3p7): segmentation mode and cloud server configuration #3717

Closed

feat(v3p7): segmentation mode and cloud server configuration #3718

Closed

feat(v3p7): segmentation mode and cloud server configuration #3720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature(Mode): Ability to overwrite mode configuration and small improvements #3580

feature(Mode): Ability to overwrite mode configuration and small improvements #3580

igoroctaviano commented Aug 7, 2023 •

edited

netlify bot commented Aug 7, 2023 •

edited

netlify bot commented Aug 7, 2023 •

edited

codecov bot commented Aug 7, 2023 •

edited

cypress bot commented Aug 7, 2023 •

edited

sedghi Aug 8, 2023

sedghi Aug 8, 2023

igoroctaviano Aug 8, 2023

sedghi Aug 8, 2023

igoroctaviano Aug 8, 2023

jbocce Aug 8, 2023

igoroctaviano Aug 8, 2023

jbocce Aug 8, 2023

igoroctaviano Aug 8, 2023

sedghi left a comment

igoroctaviano commented Aug 8, 2023

jbocce Aug 8, 2023

igoroctaviano Aug 8, 2023

jbocce Aug 8, 2023

jbocce left a comment

feature(Mode): Ability to overwrite mode configuration and small improvements #3580

feature(Mode): Ability to overwrite mode configuration and small improvements #3580

Conversation

igoroctaviano commented Aug 7, 2023 • edited

Context

Changes & Results

Testing

Checklist

PR

Code

Public Documentation Updates

Tested Environment

netlify bot commented Aug 7, 2023 • edited

✅ Deploy Preview for ohif-dev ready!

netlify bot commented Aug 7, 2023 • edited

✅ Deploy Preview for ohif-platform-docs ready!

codecov bot commented Aug 7, 2023 • edited

Codecov Report

cypress bot commented Aug 7, 2023 • edited

Passing run #3417 ↗︎

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedghi left a comment

Choose a reason for hiding this comment

igoroctaviano commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbocce left a comment

Choose a reason for hiding this comment

igoroctaviano commented Aug 7, 2023 •

edited

netlify bot commented Aug 7, 2023 •

edited

netlify bot commented Aug 7, 2023 •

edited

codecov bot commented Aug 7, 2023 •

edited

cypress bot commented Aug 7, 2023 •

edited