feat(Thumbnail): Display set messages support & displaying of series inconsistencies in the thumbnail #3499

rodrigobasilio2022 · 2023-06-27T20:11:24Z

When the user has studies with series supposed to be reconstructable but they arent, at first its hard to identify them and know why a particular series cant be viewed in MPR mode. This PR proposes a warning button in thumbnail list that highlights problems in series and help users know geometry issues in the series. It is related to issue IDC #3360

Below I added some studies from the IDC database that has series with different problems

Study with a series that change position during scan
http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.7009.9004.229337801792248347333415175865
Study with a series with irregular pixel spacing
http://localhost:3000/viewer?StudyInstanceUIDs=1.2.840.113654.2.55.69925400202012637024302712643359784845
Studies has at least a series that can be viewed in MPR mode, despite the problems
http://localhost:3000/viewer?StudyInstanceUIDs=1.2.840.113654.2.55.268978779666003033721491501200337886584 (S. 3)
http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.3098.5025.250976478345829511981090188621 (S. 5)
Study has a serie with different orientations that wrongly can be reconstructed in current OHIF version. My fixes prevent that.
http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.99.1071.55696427943949105811475330286060 (S. 601)
Studies with series with MIP images and which the first image appears to be an localizer for the series in another orientation
http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.7009.2406.131628405171680429117301556825
http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.99.1071.16051849653578423672035750287473
Study with series composed by two volumes of data
http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.99.1071.16051849653578423672035750287473 (S. 2)

netlify · 2023-06-27T20:11:29Z

✅ Deploy Preview for ohif-dev ready!

Name	Link
🔨 Latest commit
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-dev/deploys/64d651db54f36875d1ce5716
😎 Deploy Preview	https://deploy-preview-3499--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-06-27T20:11:30Z

✅ Deploy Preview for ohif-platform-docs ready!

Name	Link
🔨 Latest commit	`39ef0e8`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-platform-docs/deploys/64d63f63e30f9d0008331064
😎 Deploy Preview	https://deploy-preview-3499--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-06-27T20:15:31Z

Codecov Report

Merging #3499 (39ef0e8) into master (8adf663) will not change coverage.
Report is 8 commits behind head on master.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #3499   +/-   ##
=======================================
  Coverage   42.81%   42.81%           
=======================================
  Files          80       80           
  Lines        1448     1448           
  Branches      340      340           
=======================================
  Hits          620      620           
  Misses        663      663           
  Partials      165      165

Files Changed	Coverage Δ
...form/core/src/utils/isDisplaySetReconstructable.js	`4.76% <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 9ae3d9f...39ef0e8. Read the comment docs.

igoroctaviano

Thanks for putting this together 🎖️
I left some comments/questions, let's discuss them later today.

extensions/default/src/validateInstances.js

...ions/measurement-tracking/src/panels/PanelStudyBrowserTracking/PanelStudyBrowserTracking.tsx

jbocce

See my suggestions and comments.

...ions/measurement-tracking/src/panels/PanelStudyBrowserTracking/PanelStudyBrowserTracking.tsx

platform/app/public/config/default.js

platform/core/src/utils/formatPN.js

platform/ui/src/components/ThumbnailNoImage/ThumbnailNoImage.tsx

platform/ui/src/components/Thumbnail/Thumbnail.tsx

igoroctaviano

Reviewed. Left some comments.
👍

extensions/default/src/validateInstances.ts

platform/core/src/services/DisplaySetService/DisplaySetMessage.ts

platform/ui/src/components/DisplaySetMessageListTooltip/DisplaySetMessageListTooltip.tsx

extensions/default/src/getSopClassHandlerModule.js

extensions/default/src/validateInstances.ts

extensions/default/src/Panels/PanelStudyBrowser.tsx

platform/ui/src/components/DisplaySetMessageListTooltip/DisplaySetMessageListTooltip.tsx

platform/i18n/src/locales/en-US/Messages.json

platform/core/src/services/DisplaySetService/DisplaySetMessage.ts

extensions/default/src/getSopClassHandlerModule.js

platform/core/src/services/DisplaySetService/DisplaySetMessage.ts

extensions/default/src/validateInstances.ts

jbocce

Looks good. Thanks so much.

igoroctaviano · 2023-08-09T13:22:46Z

@sedghi @rodrigobasilio2022 It looks good to me except for one hiccup I found during my tests:

http://localhost:3000/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.7009.2406.131628405171680429117301556825

If you try to see the last series (thumbnail) tooltip, it's cut in half, and if you try to scroll down to see you cant:

@sedghi feel free to merge it although I'm still not happy with this flickering thing. I think we should refactor the whole left panel to avoid getting stuck with these UI limitations but this work should be done in a separate issue.

sedghi

I feel like not merging now honestly.
First it was ok let's merge it although it should have appeared on the right side, now is it has weird behaviour when it is the last item. We don't know when the left panel is reworked maybe in 6 months, and I don't want to ship buggy feature.

igoroctaviano · 2023-08-09T13:41:18Z

I feel like not merging now honestly. First it was ok let's merge it although it should have appeared on the right side, now is it has weird behaviour when it is the last item. We don't know when the left panel is reworked maybe in 6 months, and I don't want to ship buggy feature.

@sedghi I'll try to find a fix for this specific bug myself later today 👍 @rodrigobasilio2022 feel free to do the same.

…ewers into feat/WarningButton

igoroctaviano · 2023-08-10T14:26:30Z

@sedghi I pushed a new tooltip that uses React portals to avoid these overflow issues. Can you please take a look?

jbocce · 2023-08-10T16:41:59Z

platform/ui/src/components/Tooltip/PortalTooltip.tsx

+/**
+ * A portal based tooltip.
+ */
+export default class ToolTip extends React.Component {


I sort of expected this to be named PortalTooltip to match the name of the file. I think I prefer it being named PortalTooltip but maybe I am being picky. Also note that the other/existing tool tip component is named Tooltip (i.e. small t for tip). We should be consistent one way or the other.

Sure, i'll update it.

fedorov · 2023-08-10T16:45:42Z

Why introducing this completely foreign to most users concept "Display set"? That thumbnail corresponds to a DICOM series, why not call it "DICOM series"?

jbocce · 2023-08-10T16:48:32Z

platform/ui/src/components/Tooltip/PortalTooltip.tsx

+const portalNodes = {};
+
+/**
+ * A portal based tooltip.


React portals sound cool - until now I did not know about them. Thanks for thinking of this. However I am bit surprised that we need to use the older style class type components. I did a quick search of the code base and noticed that this would be the first/only component that uses the old style - but I might be wrong. Are we actually forced to use this? I did see this https://react.dev/reference/react-dom/createPortal#rendering-to-a-different-part-of-the-dom, does it help so that we can use a function component instead?

React portals sound cool - until now I did not know about them. Thanks for thinking of this. However I am bit surprised that we need to use the older style class type components. I did a quick search of the code base and noticed that this would be the first/only component that uses the old style - but I might be wrong. Are we actually forced to use this? I did see this https://react.dev/reference/react-dom/createPortal#rendering-to-a-different-part-of-the-dom, does it help so that we can use a function component instead?

I repurposed this component. It's not exposed and not worth going through the trouble of migrating it to function component. I added it just to workaround this specific issue, we should refactor the left panel so we can go back and use the original tooltip instead. Also, this is my last effort to address this specific issue without touching the left panel.

igoroctaviano · 2023-08-10T16:56:13Z

Why introducing this completely foreign to most users concept "Display set"? That thumbnail corresponds to a DICOM series, why not call it "DICOM series"?

@fedorov The concept of a display set has persisted since the first version of OHIF, this tooltip is tied to the display set messages which is part of the display set. I think this was done like that for consistency and also for extensibility once that messages could extend beyond just series.

igoroctaviano

LGTM

sedghi

@igoroctaviano Can you please add comment on top of each PortalToolTip and PortalTooltipCard on where the code is being copied over? thanks

igoroctaviano · 2023-08-11T14:02:18Z

@igoroctaviano Can you please add comment on top of each PortalToolTip and PortalTooltipCard on where the code is being copied over? thanks

Done, thanks.

jbocce

See my comments.

sedghi

Looks great thanks for this.
Also thanks @igoroctaviano for adding the portal

rodrigobasilio2022 changed the title ~~Feat/warning button~~ WIP Feat/warning button Jun 28, 2023

rodrigobasilio2022 force-pushed the feat/WarningButton branch from 43ccd25 to a4fbfff Compare June 28, 2023 10:18

igoroctaviano requested changes Jun 28, 2023

View reviewed changes

jbocce added the Community PR label Jun 28, 2023

jbocce reviewed Jun 28, 2023

View reviewed changes

igoroctaviano assigned rodrigobasilio2022 Jun 28, 2023

rodrigobasilio2022 force-pushed the feat/WarningButton branch from f36b9f6 to 8344c61 Compare June 30, 2023 10:16

igoroctaviano requested changes Jun 30, 2023

View reviewed changes

extensions/default/src/validateInstances.ts Outdated Show resolved Hide resolved

extensions/default/src/validateInstances.ts Outdated Show resolved Hide resolved

jbocce reviewed Jul 4, 2023

View reviewed changes

platform/core/src/services/DisplaySetService/DisplaySetMessage.ts Outdated Show resolved Hide resolved

jbocce reviewed Jul 4, 2023

View reviewed changes

platform/core/src/services/DisplaySetService/DisplaySetMessage.ts Outdated Show resolved Hide resolved

jbocce reviewed Jul 4, 2023

View reviewed changes

platform/ui/src/components/DisplaySetMessageListTooltip/DisplaySetMessageListTooltip.tsx Outdated Show resolved Hide resolved

jbocce reviewed Jul 4, 2023

View reviewed changes

extensions/default/src/getSopClassHandlerModule.js Outdated Show resolved Hide resolved

jbocce reviewed Jul 4, 2023

View reviewed changes

extensions/default/src/getSopClassHandlerModule.js Outdated Show resolved Hide resolved

jbocce reviewed Jul 4, 2023

View reviewed changes

extensions/default/src/validateInstances.ts Outdated Show resolved Hide resolved