fix(viewports): The display of linked viewports during drag and drop has a race (#3286)

* fix: The display of linked viewports during drag and drop has a race

* PR review comments

* fix: Segmentation display two up

* Removing console logs

* Fix the blank viewport can have stuff added to it

* Fix the null name on HP module

* Fix the navigate to initial image

* Fix the nth interleave loader

* Fix the unit tests

* PR comments - docs mostly

* fix: Exception thrown on change displayset after double click

platform/core/src/extensions/ExtensionManager.test.js

@@ -206,40 +206,40 @@ describe('ExtensionManager.ts', () => {
       const extension = {
         id: 'hello-world',
         getViewportModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getSopClassHandlerModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getPanelModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getToolbarModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getCommandsModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getLayoutTemplateModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getDataSourcesModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getHangingProtocolModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getContextModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getUtilityModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getCustomizationModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
         getStateSyncModule: () => {
-          return [{}];
+          return [{ name: 'test' }];
         },
       };
 

platform/core/src/extensions/ExtensionManager.ts

@@ -283,6 +283,11 @@ export default class ExtensionManager {
             // Default for most extension points,
             // Just adds each entry ready for consumption by mode.
             extensionModule.forEach(element => {
+              if (!element.name) {
+                throw new Error(
+                  `Extension ID ${extensionId} module ${moduleType} element has no name`
+                );
+              }
               const id = `${extensionId}.${moduleType}.${element.name}`;
               element.id = id;
               this.modulesMap[id] = element;
@@ -366,10 +371,10 @@ export default class ExtensionManager {
 
   _initHangingProtocolsModule = (extensionModule, extensionId) => {
     const { hangingProtocolService } = this._servicesManager.services;
-    extensionModule.forEach(({ id, protocol }) => {
+    extensionModule.forEach(({ name, protocol }) => {
       if (protocol) {
         // Only auto-register if protocol specified, otherwise let mode register
-        hangingProtocolService.addProtocol(id, protocol);
+        hangingProtocolService.addProtocol(name, protocol);
       }
     });
   };

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

@@ -26,7 +26,6 @@ class ViewportGridService extends PubSubService {
   public setServiceImplementation({
     getState: getStateImplementation,
     setActiveViewportIndex: setActiveViewportIndexImplementation,
-    setDisplaySetsForViewport: setDisplaySetsForViewportImplementation,
     setDisplaySetsForViewports: setDisplaySetsForViewportsImplementation,
     setLayout: setLayoutImplementation,
     reset: resetImplementation,
@@ -40,9 +39,6 @@ class ViewportGridService extends PubSubService {
     if (setActiveViewportIndexImplementation) {
       this.serviceImplementation._setActiveViewportIndex = setActiveViewportIndexImplementation;
     }
-    if (setDisplaySetsForViewportImplementation) {
-      this.serviceImplementation._setDisplaySetsForViewport = setDisplaySetsForViewportImplementation;
-    }
     if (setDisplaySetsForViewportsImplementation) {
       this.serviceImplementation._setDisplaySetsForViewports = setDisplaySetsForViewportsImplementation;
     }
@@ -77,22 +73,13 @@ class ViewportGridService extends PubSubService {
     return this.serviceImplementation._getState();
   }
 
-  public setDisplaySetsForViewport({
-    viewportIndex,
-    displaySetInstanceUIDs,
-    viewportOptions,
-    displaySetOptions,
-  }) {
-    this.serviceImplementation._setDisplaySetsForViewport({
-      viewportIndex,
-      displaySetInstanceUIDs,
-      viewportOptions,
-      displaySetOptions,
-    });
+  public setDisplaySetsForViewport(props) {
+    // Just update a single viewport, but use the multi-viewport update for it.
+    this.serviceImplementation._setDisplaySetsForViewports([props]);
   }
 
-  public setDisplaySetsForViewports(viewports) {
-    this.serviceImplementation._setDisplaySetsForViewports(viewports);
+  public setDisplaySetsForViewports(props) {
+    this.serviceImplementation._setDisplaySetsForViewports(props);
   }
 
   /**

platform/ui/src/contextProviders/ViewportGridProvider.tsx

@@ -34,54 +34,85 @@ const DEFAULT_STATE = {
 
 export const ViewportGridContext = createContext(DEFAULT_STATE);
 
+/** A viewport is reuseable if it is the same size as the old
+ * one and has the same display sets, in the same position.
+ * It SHOULD be possible to re-use them at different positions, but
+ * this causes problems with segmentation.
+ */
+const isReuseableViewport = (oldViewport, newViewport) => {
+  const sameDiplaySets = isEqual(
+    oldViewport.displaySetInstanceUIDs,
+    newViewport.displaySetInstanceUIDs
+  );
+  return (
+    oldViewport.viewportIndex === newViewport.viewportIndex &&
+    sameDiplaySets &&
+    oldViewport.height === newViewport.height &&
+    oldViewport.width === newViewport.width
+  );
+};
+
+// Holds a global viewport counter - used to assign new id's to viewports
+// Starts at a value above zero so that any of the old viewport id's are
+// immediately obvious and if we get any index generated viewports, they are
+// definitely distinct from these ones.
+let viewportCounter = 5000;
+
 /**
- * Find a viewport to re-use, and then set the viewportId
+ * Find a viewportId to re-use if possible, preserving the existing
+ * viewport information, OR create a new one if the viewport given isn't
+ * compatible with what was there before.
  *
- * @param idSet
+ * @param viewportIdSet
  * @param viewport
  * @param stateViewports
  * @returns
  */
-const reuseViewport = (idSet, viewport, stateViewports) => {
-  const oldIds = {};
+const reuseViewportId = (viewportIdSet: Set, viewport, stateViewports) => {
   for (const oldViewport of stateViewports) {
     const { viewportId: oldId } = oldViewport;
-    oldIds[oldId] = true;
-    if (!oldId || idSet[oldId]) continue;
-    if (
-      !isEqual(
-        oldViewport.displaySetInstanceUIDs,
-        viewport.displaySetInstanceUIDs
-      )
-    ) {
+    if (!oldId) {
+      // This occurs on startup, so skip re-using it
       continue;
     }
-    idSet[oldId] = true;
-    // TODO re-use viewports once the flickering/wrong size redraw is fixed
-    // return {
-    //   ...oldViewport,
-    //   ...viewport,
-    //   viewportOptions: {
-    //     ...oldViewport.viewportOptions,
+    if (viewportIdSet.has(oldId)) {
+      // oldId is already used - we can't reuse it
+      continue;
+    }
+    if (isReuseableViewport(oldViewport, viewport)) {
+      viewportIdSet.add(oldId);
+      // This means the old and the new viewport are compatible, and
+      // since we have gotten here, the viewport ID isn't used, so we
+      // are good to reuse it.
+      // This will remember the old viewport options, assuming they are unchanging.
+      return {
+        ...oldViewport,
+        ...viewport,
+        id: oldId,
+        viewportId: oldId,
+        viewportOptions: {
+          // Update any viewport options from new
+          ...viewport.viewportOptions,
+          viewportId: oldId,
+        },
+      };
+    }
+  }
 
-    //     viewportId: oldViewport.viewportId,
-    //   },
-    // };
-  }
-  // Find a viewport instance number different from earlier viewports having
-  // the same presentationIds as this one would - will be less than 10k
-  // viewports hopefully :-)
-  for (let i = 0; i < 10000; i++) {
-    const viewportId = 'viewport-' + i;
-    if (idSet[viewportId] || oldIds[viewportId]) continue;
-    idSet[viewportId] = true;
-    return {
-      ...viewport,
-      viewportId,
-      viewportOptions: { ...viewport.viewportOptions, viewportId },
-    };
-  }
-  throw new Error('No ID found');
+  // There wasn't an old id found to be reused, so create a new one
+  // Find a viewport instance number different from earlier viewports
+  const viewportId = 'viewport-' + viewportCounter;
+  viewportIdSet.add(viewportId);
+  // Loop over viewport counters in case of a really long lived display
+  viewportCounter = (viewportCounter + 1) % 100000;
+  // viewportOptions is already a copy, so can just update direct
+  viewport.viewportOptions.viewportId = viewportId;
+
+  return {
+    ...viewport,
+    id: viewportId,
+    viewportId,
+  };
 };
 
 export function ViewportGridProvider({ children, service }) {
@@ -90,53 +121,72 @@ export function ViewportGridProvider({ children, service }) {
       case 'SET_ACTIVE_VIEWPORT_INDEX': {
         return { ...state, ...{ activeViewportIndex: action.payload } };
       }
-      case 'SET_DISPLAYSET_FOR_VIEWPORT': {
-        const payload = action.payload;
-        const { viewportIndex, displaySetInstanceUIDs } = payload;
-
-        // Note: there should be no inheritance happening at this level,
-        // we can't assume the new displaySet can inherit the previous
-        // displaySet's or viewportOptions at all. For instance, dragging
-        // and dropping a SEG/RT displaySet without any viewportOptions
-        // or displaySetOptions should not inherit the previous displaySet's
-        // which might have been a PDF Viewport. The viewport itself
-        // will deal with inheritance if required. Here is just a simple
-        // provider.
-        const viewport = state.viewports[viewportIndex] || {};
-        const viewportOptions = { ...payload.viewportOptions };
-
-        const displaySetOptions = payload.displaySetOptions || [];
-        if (displaySetOptions.length === 0) {
-          // Only copy index 0, as that is all that is currently supported by this
-          // method call.
-          displaySetOptions.push({ ...viewport.displaySetOptions?.[0] });
-        }
 
+      /**
+       * Sets the display sets for multiple viewports.
+       * This is a replacement for the older set display set for viewport (single)
+       * because the old one had race conditions wherein the viewports could
+       * render partially in various ways causing exceptions.
+       */
+      case 'SET_DISPLAYSETS_FOR_VIEWPORTS': {
+        const { payload } = action;
         const viewports = state.viewports.slice();
 
-        let newView = {
-          ...viewport,
-          displaySetInstanceUIDs,
-          viewportOptions,
-          displaySetOptions,
-          viewportLabel: viewportLabels[viewportIndex],
-        };
-        viewportOptions.presentationIds = getPresentationIds(
-          newView,
-          viewports
-        );
+        // Have the initial id set contain all viewports not updated here
+        const viewportIdSet = new Set();
+        viewports.forEach((viewport, index) => {
+          if (!viewport.viewportId) return;
+          const isUpdated = payload.find(
+            newViewport => newViewport.viewportIndex === index
+          );
+          if (isUpdated) {
+            return;
+          }
+          viewportIdSet.add(viewport.viewportId);
+        });
 
-        // Make sure we assign a viewport id
-        newView = reuseViewport({}, newView, state.viewports);
-        console.log(
-          'Creating new viewport',
-          viewportIndex,
-          newView.viewportOptions.viewportId,
-          displaySetInstanceUIDs,
-          displaySetOptions
-        );
+        for (const updatedViewport of payload) {
+          // Use the newly provide viewportOptions and display set options
+          // when provided, and otherwise fall back to the previous ones.
+          // That allows for easy updates of just the display set.
+          const { viewportIndex, displaySetInstanceUIDs } = updatedViewport;
+          const previousViewport = viewports[viewportIndex] || {};
+          const viewportOptions = {
+            ...(updatedViewport.viewportOptions ||
+              previousViewport.viewportOptions),
+          };
 
-        viewports[viewportIndex] = newView;
+          const displaySetOptions = updatedViewport.displaySetOptions || [];
+          if (!displaySetOptions.length) {
+            // Copy all the display set options, assuming a full set of displa
+            // set UID's is provided.
+            displaySetOptions.push(...previousViewport.displaySetOptions);
+            if (!displaySetOptions.length) {
+              displaySetOptions.push({});
+            }
+          }
+
+          let newViewport = {
+            ...previousViewport,
+            displaySetInstanceUIDs,
+            viewportOptions,
+            displaySetOptions,
+            viewportLabel: viewportLabels[viewportIndex],
+          };
+          viewportOptions.presentationIds = getPresentationIds(
+            newViewport,
+            viewports
+          );
+
+          newViewport = reuseViewportId(
+            viewportIdSet,
+            newViewport,
+            state.viewports
+          );
+          newViewport.viewportIndex = previousViewport.viewportIndex;
+
+          viewports[viewportIndex] = newViewport;
+        }
 
         return { ...state, viewports };
       }
@@ -203,13 +253,13 @@ export function ViewportGridProvider({ children, service }) {
 
         activeViewportIndexToSet = activeViewportIndexToSet ?? 0;
 
-        const viewportIdSet = {};
+        const viewportIdSet = new Set();
         for (
           let viewportIndex = 0;
           viewportIndex < viewports.length;
           viewportIndex++
         ) {
-          const viewport = reuseViewport(
+          const viewport = reuseViewportId(
             viewportIdSet,
             viewports[viewportIndex],
             state.viewports
@@ -268,36 +318,15 @@ export function ViewportGridProvider({ children, service }) {
     [dispatch]
   );
 
-  const setDisplaySetsForViewport = useCallback(
-    ({
-      viewportIndex,
-      displaySetInstanceUIDs,
-      viewportOptions,
-      displaySetSelectors,
-      displaySetOptions,
-    }) =>
+  const setDisplaySetsForViewports = useCallback(
+    viewports =>
       dispatch({
-        type: 'SET_DISPLAYSET_FOR_VIEWPORT',
-        payload: {
-          viewportIndex,
-          displaySetInstanceUIDs,
-          viewportOptions,
-          displaySetSelectors,
-          displaySetOptions,
-        },
+        type: 'SET_DISPLAYSETS_FOR_VIEWPORTS',
+        payload: viewports,
       }),
     [dispatch]
   );
 
-  const setDisplaySetsForViewports = useCallback(
-    viewports => {
-      viewports.forEach(data => {
-        setDisplaySetsForViewport(data);
-      });
-    },
-    [setDisplaySetsForViewport]
-  );
-
   const setLayout = useCallback(
     ({
       layoutType,
@@ -355,7 +384,6 @@ export function ViewportGridProvider({ children, service }) {
       service.setServiceImplementation({
         getState,
         setActiveViewportIndex,
-        setDisplaySetsForViewport,
         setDisplaySetsForViewports,
         setLayout,
         reset,
@@ -368,7 +396,6 @@ export function ViewportGridProvider({ children, service }) {
     getState,
     service,
     setActiveViewportIndex,
-    setDisplaySetsForViewport,
     setDisplaySetsForViewports,
     setLayout,
     reset,
@@ -379,7 +406,6 @@ export function ViewportGridProvider({ children, service }) {
   const api = {
     getState,
     setActiveViewportIndex: index => service.setActiveViewportIndex(index), // run it through the service itself since we want to publish events
-    setDisplaySetsForViewport,
     setDisplaySetsForViewports,
     setLayout: layout => service.setLayout(layout), // run it through the service itself since we want to publish events
     reset,

platform/viewer/public/config/multiple.js

@@ -42,6 +42,31 @@ window.config = {
         singlepart: 'bulkdata,video,pdf',
       },
     },
+    {
+      friendlyName: 'dcmjs DICOMWeb Server',
+      namespace: '@ohif/extension-default.dataSourcesModule.dicomweb',
+      sourceName: 'ohif',
+      configuration: {
+        name: 'aws',
+        // old server
+        // wadoUriRoot: 'https://server.dcmjs.org/dcm4chee-arc/aets/DCM4CHEE/wado',
+        // qidoRoot: 'https://server.dcmjs.org/dcm4chee-arc/aets/DCM4CHEE/rs',
+        // wadoRoot: 'https://server.dcmjs.org/dcm4chee-arc/aets/DCM4CHEE/rs',
+        // new server
+        wadoUriRoot: 'https://domvja9iplmyu.cloudfront.net/dicomweb',
+        qidoRoot: 'https://domvja9iplmyu.cloudfront.net/dicomweb',
+        wadoRoot: 'https://domvja9iplmyu.cloudfront.net/dicomweb',
+        qidoSupportsIncludeField: false,
+        supportsReject: false,
+        imageRendering: 'wadors',
+        thumbnailRendering: 'wadors',
+        enableStudyLazyLoad: true,
+        supportsFuzzyMatching: false,
+        supportsWildcard: true,
+        staticWado: true,
+        singlepart: 'bulkdata,video,pdf',
+      },
+    },
     {
       friendlyName: 'AWS S3 OHIF',
       namespace: '@ohif/extension-default.dataSourcesModule.dicomweb',