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.

Sign up for GitHub

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

Jump to bottom

feat(measurements): Provide for the Load (SR) measurements button to optionally clear existing measurements prior to loading the SR. #4586

Merged
merged 11 commits into from
Dec 18, 2024
Merged

feat(measurements): Provide for the Load (SR) measurements button to optionally clear existing measurements prior to loading the SR. #4586

merged 11 commits into from
Dec 18, 2024

Conversation

jbocce
Copy link
Collaborator

@jbocce jbocce commented Dec 10, 2024 •
edited
Loading

Context

Prior to these changes, the load of SR measurements was confined to the OHIFCornerstoneSRMeasurementViewport. This meant that actions such as clearing the measurements prior to load were not easy to customize. Now the command(s) to run for the load are defined in a command and associated with a toolbar button.

Changes & Results

Refactored the load measurements code in OHIFCornerstoneSRMeasurementViewport.tsx to a new loadMeasurements command. The SR measurements viewport status bar now extracts the Load button to use from the 'loadMeasurements' toolbar. The Load button added to the toolbar defines the commands to be run. Typically this includes the (new) loadMeasurements command and optionally the clearMeasurements command.

Moved the load measurements toolbar buttons and section out of the modes and into the cornerstone-dicom-sr extension onModeEnter hook.

Refactored the HTML for the Load button out of OHIFCornerstoneSRMeasurementViewport.tsx into its own component named ViewportActionButton. The load measurements toolbar button uses this new component.
RT struct and SEG viewport statuses now use ViewportActionButton as well.

Testing

Test A

The load of measurements, SEG and RT struct should work as before.

Test B

  1. To test the customization, add a new mode with no measurement tracking.
  2. Edit the LoadMeasurements button in the mode to not clear the measurements prior to load. Alternatively, create a custom list of commands to execute instead.
  3. The customization should work as whatever was intended in step 2 (e.g. the measurements should not clear, etc.).

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: Windows 11 Pro
  • Node version: 18.16.1
  • Browser: Chrome 131.0.6778.110

@jbocce
feat(measurements): Provide for the Load (SR) measurements button to …
5e082b6 
…optionally clear existing measurements prior to loading the SR.

Refactored the load measurements code in OHIFCornerstoneSRMeasurementViewport.tsx to a new loadMeasurements command.
The SR measurements viewport status bar now extracts the Load button from the 'loadMeasurements' toolbar.
The Load button added to the toolbar defines the commands to be run. Typically this includes the loadMeasurements command
and optionally the clearMeasurements command.
@jbocce jbocce requested a review from wayfarer3130 December 10, 2024 21:22
@netlify Netlify
Copy link

netlify bot commented Dec 10, 2024 •
edited
Loading

✅ Deploy Preview for ohif-dev canceled.

Name Link
🔨 Latest commit 8dda15d
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/6762e0b1e6ad590008e93af0

@netlify Netlify
Copy link

netlify bot commented Dec 10, 2024 •
edited
Loading

✅ Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 8dda15d
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/6762e0b1f1076c0009aa651f
😎 Deploy Preview https://deploy-preview-4586--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.

jbocce
jbocce commented Dec 10, 2024
View reviewed changes
extensions/cornerstone-dicom-sr/src/components/OHIFCornerstoneSRMeasurementViewport.tsx
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is some reformatting that was just done automagically. Hopefully it is all cool.

@jbocce
Incorporated draft PR feedback.
acd2922 
Append the various command options for loadMeasurements to the arguments to onInteraction.
Moved the toolbar buttons and section out of the modes and into the cornerstone-dicom-sr extension onModeEnter hook.
RT struct viewport status now uses ViewportActionButton.
@jbocce
Merge remote-tracking branch 'origin/master' into feat/allow-existing…
1bdff6e 
…-measurements-to-be-added-to-loaded-SR-measurements
@jbocce
Added a clearMeasurements flag to the loadMeasurements command to opt…
863d9fd 
…ionally clear measurements for the non-tracked measurements' load.
@jbocce
Draft PR feedback. Added some comments in the extension where the def…
c5e8103 
…ault load measurements button and toolbar are defined.
@jbocce jbocce marked this pull request as ready for review December 13, 2024 14:28
@jbocce jbocce requested a review from sedghi December 13, 2024 14:29
sedghi
sedghi reviewed Dec 13, 2024
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

great addition, see my comments please

@jbocce
Both the measurement-tracking and the cornerstone-dicom-sr extensions…
bf7ce42 
… add

a loadSRMeasurements button, but each defines different commands to run.
The button added by the measurement-tracking is prioritized.
The command for loading tracked measurements is defined in TrackedMeasurementsContext
because it must be bound to the function that sends the measurement tracking event.
Changes in ToolbarService include optionally replacing existing buttons in addButtons
and avoiding duplicate buttons in a toolbar section.
sedghi
sedghi reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

looks much better, thanks minor comment

@jbocce
Copy link
Collaborator Author

jbocce commented Dec 17, 2024

looks much better, thanks minor comment

forget about it 😉

sedghi
sedghi approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

Great addition

@sedghi sedghi merged commit 4d3d5e7 into OHIF:master Dec 18, 2024
17 checks passed
@jbocce jbocce deleted the feat/allow-existing-measurements-to-be-added-to-loaded-SR-measurements branch December 18, 2024 17:33
jbocce added a commit to jbocce/Viewers that referenced this pull request Dec 18, 2024
@jbocce
fix(SR): Bring back the onModeEnter for the cornerstone-dicom-sr exte…
ec4ef06 
…nsion that was accidentally removed by PR OHIF#4586
@jbocce jbocce mentioned this pull request Dec 18, 2024
Merged
6 tasks
sedghi pushed a commit that referenced this pull request Dec 18, 2024
@jbocce
fix(SR): Bring back the onModeEnter for the cornerstone-dicom-sr exte…
2df8e1d 
…nsion that was accidentally removed by PR #4586 (#4616)
@jbocce jbocce mentioned this pull request Dec 19, 2024
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wayfarer3130 wayfarer3130 wayfarer3130 left review comments

@sedghi sedghi sedghi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jbocce @sedghi @wayfarer3130