feat(grid): remove viewportIndex and only rely on viewportId #3591

sedghi · 2023-08-09T21:02:12Z

Context

During the development of OHIF, the reliance on viewport index has proven to be problematic. This issue has seen in several ways:

Inconsistent Viewport Indexing with Layout Changes: A change from a 2x2 to a 3x3 grid alters the viewport index, causing unnecessary re-rendering, even though the display sets remain the same. This has resulted in a lack of optimization in grid rendering during stage and layout changes.
Viewport Movement Problems: for moving a viewport (similar to moving a person's window in Zoom, which we have in plan to support in future), the change in viewport index forces React to re-render the whole grid (as right now we have the key as viewportIndex). While the element itself hasn't changed, the viewport index change causes a problematic full re-render.

Changes Made

Removed Viewport Index: All occurrences of the viewport index have been replaced with viewportId. This prevents the above issues related to the inconsistency of the viewport index.
Changed Viewport Grid State: The viewport grid state has been modified from an array to a map, using viewport IDs as keys. This provides a more stable mapping of viewports.

Migration Guide

You should not rely on viewportIndex and activeViewportIndex anymore.

Instead try

const { activeViewportId, viewports } = viewportGridService.getState() OR useViewportGrid()

Viewports also has been changed from an array to a Map

viewports: new Map(
    Object.entries({
      default: {
        viewportId: 'default',
        displaySetInstanceUIDs: [],
        viewportOptions: {
          viewportId: 'default',
        },
        displaySetSelectors: [],
        displaySetOptions: [{}],
        x: 0, // left
        y: 0, // top
        width: 100,
        height: 100,
        viewportLabel: null,
      },
    })
  ),

So you need to viewports.get(viewportId instead of viewports[viewportIndex]

Testing

The implemented changes have been tested, and the following features are working as expected:

Rendering a default viewport
Rendering a TMTV
Rendering segmentation and RT struct
Enabling double-click on a thumbnail
Changing layout, going MPR, adding measurements
Toggle One Up works
Viewport Labels (ABCD) works
Presentation States is preserved
Change from 2x2 to 3x3 works

Cheers

…portIndex

netlify · 2023-08-09T21:02:40Z

✅ Deploy Preview for ohif-platform-docs ready!

Name	Link
🔨 Latest commit	`50d0d1e`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-platform-docs/deploys/64efa069daf4fd00087a860e
😎 Deploy Preview	https://deploy-preview-3591--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.

netlify · 2023-08-09T21:02:47Z

✅ Deploy Preview for ohif-dev ready!

Name	Link
🔨 Latest commit	`50d0d1e`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-dev/deploys/64efa0699eb45100078023c7
😎 Deploy Preview	https://deploy-preview-3591--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.

…portIndex

wayfarer3130 · 2023-08-15T12:28:46Z

extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts

-      throw new Error('Should have viewport ID afterwards');
-    }
-
+  public enableViewport(viewportId: string, elementRef: HTMLDivElement): void {


Why doesn't this have the viewport options any longer - are those always already provided?

we were only using viewportOptions to grab viewportId, so just using viewportId now

extensions/cornerstone/src/utils/stackSync/toggleStackImageSync.ts

…portIndex

sedghi · 2023-08-23T18:27:40Z

@jbocce Thanks for thorough review, applied/fixed based on your comment

extensions/tmtv/src/getHangingProtocolModule.js

extensions/default/src/commandsModule.ts

jbocce · 2023-08-24T13:27:27Z

Great work fixing the issues I found! Thanks so much.

I think the following comments should be discussed and/or addressed...

sedghi · 2023-08-24T16:52:22Z

@jbocce applied all three comments

sedghi · 2023-08-24T20:09:38Z

@jbocce I think I got it working for the double click too

extensions/default/src/commandsModule.ts

platform/core/src/utils/subscribeToNextViewportGridChange.ts

jbocce · 2023-08-25T11:36:14Z

@jbocce I think I got it working for the double click too

Yes. Thank you for that. See my comments.

jbocce

Approved.

wayfarer3130

There are some smallish changes, and a couple of questions, but I think this is important enough that we should proceed on it.
If you decide to include any of the fixes, that is great, but otherwise I wouldn't worry about them - you decide which comments are still worth applying.

wayfarer3130 · 2023-08-22T15:06:28Z

extensions/dicom-microscopy/src/getCommandsModule.ts

-      const nextViewportIndex = (activeViewportIndex + 1) % viewports.length;
-      viewportGridService.setActiveViewportIndex(nextViewportIndex);
+      const { activeViewportId, viewports } = viewportGridService.getState();
+      const nextViewportIndex = (activeViewportId + 1) % viewports.size;


What about adding get viewport ids function that returns a list, and takes an optional sorting parameter. Then you still setActiveViewportId, but you provide it an actual viewport - and you can decide based on the mode or extension how things get sorted. That is, implement in the service itself, based on the getState call:
getViewportIds(sortFunction) {
return [...getState().viewports].map(viewport => viewport.viewportId).sort(sortFunction)
That says nothing about indices, just gets the ids of interest. You could add a filter function to it too.

wayfarer3130 · 2023-08-23T12:46:08Z

platform/ui/src/contextProviders/ViewportGridProvider.tsx

 import viewportLabels from '../utils/viewportLabels';

-const DEFAULT_STATE = {
-  activeViewportIndex: 0,
+interface Viewport {


Can you add export types on the interfaces?

platform/ui/src/contextProviders/ViewportGridProvider.tsx

platform/ui/src/components/Viewport/Viewport.tsx

wayfarer3130 · 2023-08-23T12:56:55Z

platform/core/src/services/ViewportGridService/ViewportGridService.ts

-    if (setActiveViewportIndexImplementation) {
-      this.serviceImplementation._setActiveViewportIndex =
-        setActiveViewportIndexImplementation;
+    if (setActiveViewportIdImplementation) {


You might consider:
this.serviceImplementation._setActiveViewport = setActiveViewportIdImplementation || this.serviceImplementation._setActiveViewport;

extensions/cornerstone/src/utils/stackSync/toggleStackImageSync.ts

wayfarer3130 · 2023-08-30T18:07:55Z

platform/app/cypress/integration/customization/OHIFDoubleClick.spec.js

@@ -16,12 +16,18 @@ describe('OHIF Double Click', () => {
      .should('be.eq', numExpectedViewports);

    for (let i = 0; i < numExpectedViewports; i += 1) {
+      cy.wait(2000);


If you merge the CINE fix in CS3D, then this wait gets removed and you just do a waitDicomImage() above line 18.
I'd really like to see it do waitDicomImage instead of cy.wait as it is much more reliable, and is fast on fast systems, and slow on slower connections.

platform/core/src/services/HangingProtocolService/HPMatcher.js

wayfarer3130 · 2023-08-30T18:12:30Z

platform/core/src/services/HangingProtocolService/HangingProtocolService.ts

-            }
+          displaySet,
+          seriesMatchingRules,
+          // Todo: why we have images here since the matching type does not have it


I don't think we need the actual instance any longer.

wayfarer3130 · 2023-08-30T18:15:44Z

platform/ui/src/contextProviders/ViewportGridProvider.tsx

-            ...(updatedViewport.viewportOptions ||
-              previousViewport.viewportOptions),
-          };
+          const viewportOptions = merge(


Does this need a real merge? It feels like it might cause mixtures that aren't intended, where one version has a value for key A, and the next one has a value for keyB, but doesn't specifically null out keyA, then keyA still applies.
Not saying it doesn't, just asking.

I think it is fine

…portIndex

Co-authored-by: Bill Wallace <[email protected]>

sedghi and others added 13 commits July 27, 2023 14:26

invert sync bug

66198d8

fix the custom load strategy

35c2f95

fix disable element by id

51d06d8

remove unnecessary code from viewportGrid

0da10b1

cleanup

c3c72bc

Added an example hanging protocol to the test extension

d3d6f90

Fix the positioning names on the viewports so they are re-used

53458f5

Additional HP for working with viewports

77018bc

Merge branch 'master' of github.com:OHIF/Viewers into fix/remove-view…

9a9a1ec

…portIndex

render something initially

b08a519

working tmtv

27d715c

fix drag and drop

3b4267f

fix navigation

6ef26cf

sedghi mentioned this pull request Aug 9, 2023

feat(viewportGrid): simplifying ViewportGrid and enhancing performance by aligning with hanging protocol viewportIds #3566

Closed

sedghi marked this pull request as draft August 9, 2023 21:03

sedghi changed the title ~~fix/remove viewportIndex~~ feat(grid): remove viewportIndex and only rely on viewportId Aug 9, 2023

sedghi added 6 commits August 14, 2023 13:03

Merge branch 'master' of github.com:OHIF/Viewers into fix/remove-view…

14baf5e

…portIndex

fix viewportlabel

b38dc77

fix mpr

8e67863

fix 2x2 to 2x3 layout change

4918ce8

fix double click

f45a400

fix active ViewportId when switching layout

1aef2dc

sedghi marked this pull request as ready for review August 14, 2023 19:39

sedghi requested review from wayfarer3130 and jbocce August 14, 2023 19:39

wayfarer3130 reviewed Aug 15, 2023

View reviewed changes

extensions/cornerstone/src/utils/stackSync/toggleStackImageSync.ts Outdated Show resolved Hide resolved

wayfarer3130 reviewed Aug 15, 2023

View reviewed changes

extensions/cornerstone/src/utils/stackSync/toggleStackImageSync.ts Outdated Show resolved Hide resolved

sedghi added 2 commits August 23, 2023 14:27

apply review comments

eaeb7fe

Merge branch 'master' of github.com:OHIF/Viewers into fix/remove-view…

8cbb36f

…portIndex

sedghi requested review from jbocce and wayfarer3130 August 23, 2023 18:27

jbocce reviewed Aug 24, 2023

View reviewed changes

extensions/tmtv/src/getHangingProtocolModule.js Outdated Show resolved Hide resolved

jbocce reviewed Aug 24, 2023

View reviewed changes

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

apply review comments

b25c9af

apply review comments

dc3c9e6

jbocce reviewed Aug 25, 2023

View reviewed changes

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

jbocce reviewed Aug 25, 2023

View reviewed changes

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

jbocce reviewed Aug 25, 2023

View reviewed changes

platform/core/src/utils/subscribeToNextViewportGridChange.ts Show resolved Hide resolved

jbocce reviewed Aug 25, 2023

View reviewed changes

platform/core/src/utils/subscribeToNextViewportGridChange.ts Show resolved Hide resolved

apply review comments

0c3b037

sedghi requested a review from jbocce August 25, 2023 13:15

jbocce approved these changes Aug 25, 2023

View reviewed changes

wayfarer3130 approved these changes Aug 30, 2023

View reviewed changes

sedghi added 2 commits August 30, 2023 15:45

Merge branch 'master' of github.com:OHIF/Viewers into fix/remove-view…

494e2fd

…portIndex

apply review comments

50d0d1e

sedghi merged commit 4c6ff87 into master Aug 30, 2023
10 of 11 checks passed

sedghi mentioned this pull request Sep 26, 2023

release candidate 3p7 RC1 #3680

Closed

Sofien-Sellami pushed a commit to Sofien-Sellami/Viewers that referenced this pull request Oct 5, 2023

feat(grid): remove viewportIndex and only rely on viewportId (OHIF#3591)

9970df3

Co-authored-by: Bill Wallace <[email protected]>

This was referenced Oct 10, 2023

candidate 3p7 rc 2 #3712

Closed

feat(v3p7): segmentation mode and cloud server configuration #3717

Closed

feat(v3p7): segmentation mode and cloud server configuration #3718

Closed

feat(v3p7): segmentation mode and cloud server configuration #3720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(grid): remove viewportIndex and only rely on viewportId #3591

feat(grid): remove viewportIndex and only rely on viewportId #3591

sedghi commented Aug 9, 2023 •

edited

Loading

netlify bot commented Aug 9, 2023 •

edited

Loading

netlify bot commented Aug 9, 2023 •

edited

Loading

wayfarer3130 Aug 15, 2023

sedghi Aug 17, 2023

sedghi commented Aug 23, 2023

jbocce commented Aug 24, 2023

sedghi commented Aug 24, 2023

sedghi commented Aug 24, 2023 •

edited

Loading

jbocce commented Aug 25, 2023

jbocce left a comment

wayfarer3130 left a comment

wayfarer3130 Aug 22, 2023

wayfarer3130 Aug 23, 2023

wayfarer3130 Aug 23, 2023

wayfarer3130 Aug 30, 2023

wayfarer3130 Aug 30, 2023

wayfarer3130 Aug 30, 2023

sedghi Aug 30, 2023

feat(grid): remove viewportIndex and only rely on viewportId #3591

feat(grid): remove viewportIndex and only rely on viewportId #3591

Conversation

sedghi commented Aug 9, 2023 • edited Loading

Context

Changes Made

Migration Guide

Testing

netlify bot commented Aug 9, 2023 • edited Loading

✅ Deploy Preview for ohif-platform-docs ready!

netlify bot commented Aug 9, 2023 • edited Loading

✅ Deploy Preview for ohif-dev ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedghi commented Aug 23, 2023

jbocce commented Aug 24, 2023

sedghi commented Aug 24, 2023

sedghi commented Aug 24, 2023 • edited Loading

jbocce commented Aug 25, 2023

jbocce left a comment

Choose a reason for hiding this comment

wayfarer3130 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sedghi commented Aug 9, 2023 •

edited

Loading

netlify bot commented Aug 9, 2023 •

edited

Loading

netlify bot commented Aug 9, 2023 •

edited

Loading

sedghi commented Aug 24, 2023 •

edited

Loading