feat: Add simple multi-monitor support #4178
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 failed. Why did it fail? →
|
|
In the starting phase, launch a study using multimonitor=split |
|
This results in a second page being displayed with the specified study: |
|
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
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
feat/multi-monitor-take2
|
| Run status |
|
| Run duration | 02m 29s |
| Commit |
|
| Committer | sedghi |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
15
|
Flaky
|
0
|
Pending
|
2
|
Skipped
|
29
|
Passing
|
0
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
study-list/OHIFStudyList.spec.js • 1 failed test
| Test | Artifacts | |
|---|---|---|
| OHIF Study List > Desktop resolution > Displays several studies initially |
Test Replay
Screenshots
Video
|
|
measurement-tracking/OHIFCornerstoneToolbar.spec.js • 1 failed test
| Test | Artifacts | |
|---|---|---|
| OHIF Cornerstone Toolbar > checks if all primary buttons are being displayed |
Test Replay
Screenshots
Video
|
|
volume/MPR.spec.js • 1 failed test
| Test | Artifacts | |
|---|---|---|
| OHIF MPR > should not go MPR for non reconstructible displaySets |
Test Replay
Screenshots
Video
|
|
measurement-tracking/OHIFMeasurementPanel.spec.js • 1 failed test
| Test | Artifacts | |
|---|---|---|
| OHIF Measurement Panel > checks if Measurements right panel can be hidden/displayed |
Test Replay
Screenshots
Video
|
|
customization/HangingProtocol.spec.js • 1 failed test
| Test | Artifacts | |
|---|---|---|
| OHIF HP > Should display 3 up |
Test Replay
Screenshots
Video
|
|
The first 5 failed specs are shown, see all 15 specs in Cypress Cloud.
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.
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.
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.
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.
Yes, future enhancement. This is baby steps to get started.
| @@ -218,6 +218,7 @@ module.exports = (env, argv, { SRC_DIR, ENTRY }) => { | |||
| config.optimization.minimizer = [ | |||
| new TerserJSPlugin({ | |||
| parallel: true, | |||
| exclude: /app-config.js/, | |||
There was a problem hiding this comment.
I guess this is only in debug mode? Sometimes the app config is huge and it is desirable to minify it
| @@ -999,6 +1020,7 @@ export default class HangingProtocolService extends PubSubService { | |||
|
|
|||
| try { | |||
| if (!this.protocol || this.protocol.id !== protocol.id) { | |||
| console.log('***** Resetting protocol', this.protocol, this.activeStudy); | |||
| @@ -0,0 +1,18 @@ | |||
| /** A default viewport options */ | |||
| export const viewportOptions = { | |||
There was a problem hiding this comment.
I think we should move this and other selectors to a utils folder to keep it organized and clarify that it is not a hanging protocol, similar to other files in this path.
There was a problem hiding this comment.
Maybe we also call it defaultViewportOptions?
| } | ||
| } | ||
|
|
||
| public setDisplaySets(displaySets) { |
There was a problem hiding this comment.
I don't understand why we need this, since .run already has displaySets to work with.
public run({ studies, displaySets, activeStudy, activeStudyUID }, protocolId, options = {}) {It also seems like we're mixing things up by adding hpservice.setDisplaySets. How about letting run handle it?
| public hasStudyUID(studyUID: string): boolean { | ||
| return this.studies.some(it => it.StudyInstanceUID === studyUID); | ||
| } | ||
|
|
||
| public addStudy(study) { | ||
| if (!this.hasStudyUID(study.StudyInstanceUID)) { | ||
| this.studies.push(study); | ||
| } | ||
| } |
There was a problem hiding this comment.
There are too many instances of study versus studyUID in this file. I'm unsure why we have both, and this is contributing to the confusion. Can we simplify it to work with study, as it seems sufficient for our needs?
export interface StudyMetadata extends Record<string, unknown> {
readonly StudyInstanceUID?: string;
StudyDescription?: string;
}so I propose hasStudy(study) of hasStudyUID
| @@ -393,16 +411,19 @@ export default class HangingProtocolService extends PubSubService { | |||
| * the studies to display in viewports. | |||
| * @param protocol is a specific protocol to apply. | |||
| */ | |||
| public run({ studies, displaySets, activeStudy }, protocolId, options = {}) { | |||
| public run({ studies, displaySets, activeStudy, activeStudyUID }, protocolId, options = {}) { | |||
There was a problem hiding this comment.
No need to provide activeStudyUID since we already have activeStudy, which includes activeStudyUID.
| @@ -220,6 +226,47 @@ export class CommandsManager { | |||
|
|
|||
| return result; | |||
| } | |||
|
|
|||
| /** Like run, but await each command before continuing */ | |||
| public async runAsync( | |||
There was a problem hiding this comment.
i wanted this many times before, thanks
How about some refactoring
private async executeCommands(
commands: ComplexCommand[],
options: Record<string, unknown>,
asyncMode: boolean
) {
let result: unknown;
for (const command of commands) {
const { commandName, commandOptions, context } = command;
if (commandName) {
const commandOptionsMerged = { ...options, ...commandOptions };
if (asyncMode) {
result = await this.runCommand(commandName, commandOptionsMerged, context);
} else {
result = this.runCommand(commandName, commandOptionsMerged, context);
}
} else if (typeof command === 'function') {
result = asyncMode ? await command() : command();
} else {
console.warn('No command name supplied in', command);
}
}
return result;
}
public run(
toRun: Command | Commands | Command[] | string | undefined,
options: Record<string, unknown> = {}
): unknown {
if (!toRun) return;
const commands = CommandsManager.convertCommands(toRun);
if (commands.length === 0) {
console.log("Command isn't runnable", toRun);
return;
}
return this.executeCommands(commands, options, false);
}
public async runAsync(
toRun: Command | Commands | Command[] | string | undefined,
options: Record<string, unknown> = {}
): Promise<unknown> {
if (!toRun) return;
const commands = CommandsManager.convertCommands(toRun);
if (commands.length === 0) {
console.log("Command isn't runnable", toRun);
return;
}
return this.executeCommands(commands, options, true);
}
| * It is not included in the viewer mode by default. | ||
| */ | ||
| export const hpMN8: Types.HangingProtocol.Protocol = { | ||
| ...hpMN, |
There was a problem hiding this comment.
I mean refactor shared logic, and use it here, this was confusing on the first read, but i understand now, but refactoring the hpMN to have like base make more sense
| /** | ||
| * This hanging protocol can be activated on the primary mode by directly | ||
| * referencing it in a URL or by directly including it within a mode, e.g.: | ||
| * `&hangingProtocolId=@ohif/mnGrid` added to the viewer URL |
| ], | ||
| }, | ||
|
|
||
| ...hpMN.stages, |
| export const hpMNMonitor2: Types.HangingProtocol.Protocol = { | ||
| ...hpMN, | ||
| id: '@ohif/mnGridMonitor2', | ||
| description: 'A basic two monitor view', |
There was a problem hiding this comment.
I guess the description is "second monitor hp in a two monitor config"
| @@ -141,6 +197,7 @@ const commandsModule = ({ | |||
| reset = false, | |||
| }: HangingProtocolParams): boolean => { | |||
| try { | |||
| console.log('******** Set hanging protocol', activeStudyUID); | |||
| @@ -13,7 +13,7 @@ function requestDisplaySetCreationForStudy( | |||
| return; | |||
| } | |||
|
|
|||
| dataSource.retrieve.series.metadata({ StudyInstanceUID, madeInClient }); | |||
| return dataSource.retrieve.series.metadata({ StudyInstanceUID, madeInClient }); | |||
There was a problem hiding this comment.
There seems to be two files named requestDisplaySetCreationForStudy , can we re-use one
…atus only when a target annotation ID is present
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