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

Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>

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);
+
       // For whatever reason, with Cypress tests, we have to activate the
       // viewport we are double clicking first.
       cy.get('[data-cy="viewport-pane"]')
         .eq(i)
-        .trigger('mousedown', 'center', { force: true })
-        .trigger('mouseup', 'center', { force: true });
+        .trigger('mousedown', 'center', {
+          force: true,
+        })
+        .trigger('mouseup', 'center', {
+          force: true,
+        });
 
       // Wait for the viewport to be 'active'.
       // TODO Is there a better way to do this?
@@ -41,12 +47,16 @@ describe('OHIF Double Click', () => {
         .should('be.eq', 1);
 
       cy.get('[data-cy="viewport-pane"]')
-        .eq(0)
-        .trigger('dblclick', 'center');
+        .trigger('mousedown', 'center', {
+          force: true,
+        })
+        .trigger('mouseup', 'center', {
+          force: true,
+        });
 
       cy.get('[data-cy="viewport-pane"]')
-        .its('length')
-        .should('be.eq', numExpectedViewports);
+        .eq(0)
+        .trigger('dblclick', 'center');
     }
   });
 });

platform/app/cypress/integration/volume/MPR.spec.js

@@ -66,9 +66,10 @@ describe('OHIF MPR', () => {
       .then(cornerstone => {
         const viewports = cornerstone.getRenderingEngines()[0].getViewports();
 
-        const imageData1 = viewports[0].getImageData();
-        const imageData2 = viewports[1].getImageData();
-        const imageData3 = viewports[2].getImageData();
+        // The stack viewport still exists after the changes to viewportId and inde
+        const imageData1 = viewports[1].getImageData();
+        const imageData2 = viewports[2].getImageData();
+        const imageData3 = viewports[3].getImageData();
 
         // for some reason map doesn't work here
         cy.wrap(imageData1).should('not.be', undefined);

platform/app/src/components/ViewportGrid.tsx

@@ -10,7 +10,7 @@ function ViewerViewportGrid(props) {
   const { servicesManager, viewportComponents, dataSource } = props;
   const [viewportGrid, viewportGridService] = useViewportGrid();
 
-  const { layout, activeViewportIndex, viewports } = viewportGrid;
+  const { layout, activeViewportId, viewports } = viewportGrid;
   const { numCols, numRows } = layout;
 
   // TODO -> Need some way of selecting which displaySets hit the viewports.
@@ -51,10 +51,11 @@ function ViewerViewportGrid(props) {
      * specify the viewport match details, which specifies the size and
      * setup of the various viewports.
      */
-    const findOrCreateViewport = viewportIndex => {
-      const details = viewportMatchDetails.get(viewportIndex);
+    const findOrCreateViewport = pos => {
+      const viewportId = Array.from(viewportMatchDetails.keys())[pos];
+      const details = viewportMatchDetails.get(viewportId);
       if (!details) {
-        console.log('No match details for viewport', viewportIndex);
+        console.log('No match details for viewport', viewportId);
         return;
       }
 
@@ -99,11 +100,11 @@ function ViewerViewportGrid(props) {
   };
 
   const _getUpdatedViewports = useCallback(
-    (viewportIndex, displaySetInstanceUID) => {
+    (viewportId, displaySetInstanceUID) => {
       let updatedViewports = [];
       try {
         updatedViewports = hangingProtocolService.getViewportsRequireUpdate(
-          viewportIndex,
+          viewportId,
           displaySetInstanceUID
         );
       } catch (error) {
@@ -144,7 +145,7 @@ function ViewerViewportGrid(props) {
   useEffect(() => {
     const { unsubscribe } = measurementService.subscribe(
       MeasurementService.EVENTS.JUMP_TO_MEASUREMENT_LAYOUT,
-      ({ viewportIndex, measurement, isConsumed }) => {
+      ({ viewportId, measurement, isConsumed }) => {
         if (isConsumed) {
           return;
         }
@@ -155,7 +156,7 @@ function ViewerViewportGrid(props) {
           measurement;
 
         const updatedViewports = _getUpdatedViewports(
-          viewportIndex,
+          viewportId,
           referencedDisplaySetInstanceUID
         );
         // Arbitrarily assign the viewport to element 0
@@ -197,7 +198,7 @@ function ViewerViewportGrid(props) {
   }, [viewports]);
 
   /**
-  const onDoubleClick = viewportIndex => {
+  const onDoubleClick = viewportId => {
     // TODO -> Disabled for now.
     // onNewImage on a cornerstone viewport is firing setDisplaySetsForViewport.
     // Which it really really shouldn't. We need a larger fix for jump to
@@ -206,7 +207,7 @@ function ViewerViewportGrid(props) {
       viewportGridService.set({
         numCols: cachedLayout.numCols,
         numRows: cachedLayout.numRows,
-        activeViewportIndex: cachedLayout.activeViewportIndex,
+        activeViewportId: cachedLayout.activeViewportId,
         viewports: cachedLayout.viewports,
         cachedLayout: null,
       });
@@ -223,10 +224,10 @@ function ViewerViewportGrid(props) {
     viewportGridService.set({
       numCols: 1,
       numRows: 1,
-      activeViewportIndex: 0,
+      activeViewportId: 0,
       viewports: [
         {
-          displaySetInstanceUID: viewports[viewportIndex].displaySetInstanceUID,
+          displaySetInstanceUID: viewports[viewportId].displaySetInstanceUID,
           imageIndex: undefined,
         },
       ],
@@ -234,15 +235,15 @@ function ViewerViewportGrid(props) {
         numCols,
         numRows,
         viewports: cachedViewports,
-        activeViewportIndex: viewportIndex,
+        activeViewportId: viewportId,
       },
     });
   };
   */
 
-  const onDropHandler = (viewportIndex, { displaySetInstanceUID }) => {
+  const onDropHandler = (viewportId, { displaySetInstanceUID }) => {
     const updatedViewports = _getUpdatedViewports(
-      viewportIndex,
+      viewportId,
       displaySetInstanceUID
     );
     viewportGridService.setDisplaySetsForViewports(updatedViewports);
@@ -253,13 +254,7 @@ function ViewerViewportGrid(props) {
 
     const numViewportPanes = viewportGridService.getNumViewportPanes();
     for (let i = 0; i < numViewportPanes; i++) {
-      const viewportIndex = i;
-      const isActive = activeViewportIndex === viewportIndex;
-      const paneMetadata = viewports[i] || {};
-      const viewportId = paneMetadata.viewportId || `viewport-${i}`;
-      if (!paneMetadata.viewportId) {
-        paneMetadata.viewportId = viewportId;
-      }
+      const paneMetadata = Array.from(viewports.values())[i] || {};
       const {
         displaySetInstanceUIDs,
         viewportOptions,
@@ -271,9 +266,13 @@ function ViewerViewportGrid(props) {
         viewportLabel,
       } = paneMetadata;
 
+      const viewportId = viewportOptions.viewportId
+      const isActive = activeViewportId === viewportId;
+
+
       const displaySetInstanceUIDsToUse = displaySetInstanceUIDs || [];
 
-      // This is causing the viewport components re-render when the activeViewportIndex changes
+      // This is causing the viewport components re-render when the activeViewportId changes
       const displaySets = displaySetInstanceUIDsToUse.map(
         displaySetInstanceUID => {
           return (
@@ -307,17 +306,23 @@ function ViewerViewportGrid(props) {
           event.stopPropagation();
         }
 
-        viewportGridService.setActiveViewportIndex(viewportIndex);
+        viewportGridService.setActiveViewportId(viewportId);
       };
 
-      // TEMP -> Double click disabled for now
-      // onDoubleClick={() => onDoubleClick(viewportIndex)}
-
       viewportPanes[i] = (
         <ViewportPane
+          // Note: It is highly important that the key is the viewportId here,
+          // since it is used to determine if the component should be re-rendered
+          // by React, and also in the hanging protocol and stage changes if the
+          // same viewportId is used, React, by default, will only move (not re-render)
+          // those components. For instance, if we have a 2x3 layout, and we move
+          // from 2x3 to 1x1 (second viewport), if the key is the viewportIndex,
+          // React will RE-RENDER the resulting viewport as the key will be different.
+          // however, if the key is the viewportId, React will only move the component
+          // and not re-render it.
           key={viewportId}
           acceptDropsFor="displayset"
-          onDrop={onDropHandler.bind(null, viewportIndex)}
+          onDrop={onDropHandler.bind(null, viewportId)}
           onInteraction={onInteractionHandler}
           customStyle={{
             position: 'absolute',
@@ -336,8 +341,8 @@ function ViewerViewportGrid(props) {
           >
             <ViewportComponent
               displaySets={displaySets}
-              viewportIndex={viewportIndex}
-              viewportLabel={viewports.length > 1 ? viewportLabel : ''}
+              viewportLabel={viewports.size > 1 ? viewportLabel : ''}
+              viewportId={viewportId}
               dataSource={dataSource}
               viewportOptions={viewportOptions}
               displaySetOptions={displaySetOptions}
@@ -349,7 +354,7 @@ function ViewerViewportGrid(props) {
     }
 
     return viewportPanes;
-  }, [viewports, activeViewportIndex, viewportComponents, dataSource]);
+  }, [viewports, activeViewportId, viewportComponents, dataSource]);
 
   /**
    * Loading indicator until numCols and numRows are gotten from the HangingProtocolService