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

[WIP] feat: Multi monitor launch page #3872

Closed
wants to merge 11 commits into from
Closed

Conversation

wayfarer3130
Copy link
Contributor

Context

Users should be able to launch OHIF in a multi-monitor mode. This change adds a worklist on the initial launch page, and automatically launches additional windows when a study is viewed. This may need to be moved to a custom mode/extension design, but right now is just a WIP.

Changes & Results

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:

Copy link

netlify bot commented Dec 30, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit cccb0bf
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/66547e63c02f8e00075bd0a0
😎 Deploy Preview https://deploy-preview-3872--ohif-dev.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.

Copy link

netlify bot commented Dec 30, 2023

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit cccb0bf
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/66547e63404d690008995138
😎 Deploy Preview https://deploy-preview-3872--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.

return newValue;
}

return mergeWith(oldValue || {}, newValue, mergeCustomizer.bind(null, mergeType));

Check warning

Code scanning / CodeQL

Prototype-polluting assignment Medium

This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
.
Copy link

codecov bot commented Dec 30, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8a335bd) 46.23% compared to head (f6e144a) 44.44%.
Report is 230 commits behind head on master.

❗ Current head f6e144a differs from pull request most recent head e7f8352. Consider uploading reports for the commit e7f8352 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3872      +/-   ##
==========================================
- Coverage   46.23%   44.44%   -1.80%     
==========================================
  Files          78       80       +2     
  Lines        1276     1332      +56     
  Branches      312      327      +15     
==========================================
+ Hits          590      592       +2     
- Misses        548      587      +39     
- Partials      138      153      +15     
Files Coverage Δ
platform/app/src/index.js 0.00% <ø> (ø)
platform/app/src/routes/WorkList/filtersMeta.js 0.00% <ø> (ø)
platform/core/src/log.js 14.28% <ø> (-85.72%) ⬇️
...ervices/DicomMetadataStore/createSeriesMetadata.js 0.00% <ø> (ø)
...services/DicomMetadataStore/createStudyMetadata.js 0.00% <ø> (ø)
platform/core/src/utils/addAccessors.js 9.09% <ø> (ø)
platform/core/src/utils/downloadCSVReport.js 0.00% <ø> (ø)
platform/core/src/utils/formatDate.js 0.00% <ø> (ø)
platform/core/src/utils/index.js 100.00% <ø> (ø)
...form/core/src/utils/isDisplaySetReconstructable.js 5.00% <ø> (+0.18%) ⬆️
... and 1 more

... and 1 file with indirect coverage changes


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 1678593...e7f8352. Read the comment docs.

@@ -37,7 +37,7 @@
"@ohif/ui": "3.8.0-beta.36",
"classnames": "^2.3.2",
"dcmjs": "^0.29.12",
"lodash.debounce": "^4.17.21",
"lodash.debounce": "^4.0.8",
Copy link
Member

Choose a reason for hiding this comment

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

Why we are going back? it doesn't have any effect actually because of ^

@@ -0,0 +1,31 @@
import { ServicesManager, CommandsManager, ExtensionManager } from '@ohif/core';

import applyMultiMonitor from './MultiMonitor';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of applyMultiMonitor import from a folder called MultiMonitor which only has a index.tsx

}
}

export default MultiMonitor;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need tsx for this file, ts would do the job

@@ -0,0 +1,28 @@
function MultiMonitor() {
const { href } = window.location;
console.log('multiMonitor', href);
Copy link
Member

Choose a reason for hiding this comment

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

remove log please

Comment on lines +6 to +25
if (screenDetailsPromise) {
screenDetailsPromise.then(screenDetails => {
const { screens } = screenDetails;
const baseURL = href.replace('/multimonitor', '/basic-test');
console.log('Hello multi-monitor', ...screens);
const newScreen = screens.find(screen => screen.isPrimary === true);
const width = Math.floor(newScreen.availWidth / 2) - 2;
const height = newScreen.availHeight;
const window1 = window.open(
`${baseURL}&hangingProtocolId=@ohif/hpMultiMonitor2&screen=0-0.5`,
'ohifScreen1',
`screenX=${width + 1},top=0,width=${width},height=${height}`
);
console.log('Opened', window1);
window.window1 = window1;
window.moveTo(0, 0);
});
} else {
console.log('Not multi monitor', screenDetailsPromise);
}
Copy link
Member

Choose a reason for hiding this comment

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

I prefer

if (!screenDetailsPromise) {
  console.log('Not multi monitor', screenDetailsPromise);
  return 
}

screenDetailsPromise.then(screenDetails => {
  const { screens } = screenDetails;
  const baseURL = href.replace('/multimonitor', '/basic-test');
  console.log('Hello multi-monitor', ...screens);
  const newScreen = screens.find(screen => screen.isPrimary === true);
  const width = Math.floor(newScreen.availWidth / 2) - 2;
  const height = newScreen.availHeight;
  const window1 = window.open(
    `${baseURL}&hangingProtocolId=@ohif/hpMultiMonitor2&screen=0-0.5`,
    'ohifScreen1',
    `screenX=${width + 1},top=0,width=${width},height=${height}`
  );
  console.log('Opened', window1);
  window.window1 = window1;
  window.moveTo(0, 0);
});

Less indentation, more readable IMO

@@ -0,0 +1,28 @@
function MultiMonitor() {
Copy link
Member

Choose a reason for hiding this comment

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

documentation please

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.

Thanks!
I tried it, it opens a second window on my other monitor only half (right half) and shows blank at the moment.

Copy link

cypress bot commented Jan 25, 2024

Passing run #3726 ↗︎

0 44 0 0 Flakiness 0

Details:

Basic changes to layout to add studyData information.
Project: Viewers Commit: f6e144ac03
Status: Passed Duration: 06:24 💡
Started: Jan 25, 2024 1:55 PM Ended: Jan 25, 2024 2:01 PM

Review all test suite changes for PR #3872 ↗︎

@wayfarer3130
Copy link
Contributor Author

Redoing this in a cleaner fashion.

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