-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: Add simple multi-monitor support #4178
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
You can then navigate either window from the 'all' studies in the study browser tab. Study navigation currently refreshes the page, which loses the position on that page, plus any markup/annotations you have created. This is a temporary issue while the navigation is updated to use internal navigation to preserve data. |
Viewers Run #4609
Run Properties:
|
Project |
Viewers
|
Branch Review |
feat/multi-monitor-take2
|
Run status |
Failed #4609
|
Run duration | 02m 12s |
Commit |
351c909247: Relaunch without refresh in any windows
|
Committer | Bill Wallace |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
1
|
Flaky |
0
|
Pending |
2
|
Skipped |
2
|
Passing |
41
|
View all changes introduced in this branch ↗︎ |
Tests for review
cypress/integration/customization/HangingProtocol.spec.js • 1 failed test
Test | Artifacts | |
---|---|---|
OHIF HP > Should display 3 up |
Test Replay
Screenshots
Video
|
Means that the study change doesn't have to occur by updating the URL, which clears all the internal data.
} | ||
|
||
public run(screenDelta = 1, commands, options) { | ||
const screenNumber = (this.screenNumber + (screenDelta ?? 1)) % this.numberOfScreens; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using negative delta to run commands from window 2 into window 1 feels a bit weird I think. I could see explicit screen id being more intuitive. Or what's the use case with delta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case is really left/right windows or "other window" for the two window use case.
this.isMultimonitor = false; | ||
} | ||
|
||
public run(screenDelta = 1, commands, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts about a function that would run commands and options in all windows except the origin from which it was called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, future enhancement. This is baby steps to get started.
Context
The ability to use more monitor space for viewing studies is quite useful for being able to compare current/prior or comparing various series.
Changes & Results
Added some basic navigation controls to the study browser (thumbnails list) to navigate the study to the specified one.
Testing
Launch the localhost url, OR an https URL with ?multimonitor=split or ?multimonitor=2
(use 2 only if you have two physical monitors)
Display a study which has several studies for the same MRN
Launch the study in basic test mode or in viewer mode
Right click the study title in the study browser to launch studies in the other window.
There aren't navigation settings for the launch window.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment