fix(jump): Jump to measurement wasn't jumping when presentation already existed (#3318)

* fix: Jump to measurement after presentation store

* Fix initial load of DICOM SR

* Fix measurement highlight issues

* Clear measurements changed to array

* Fix merge issues

* PR comments

* Removed unused formatting.  Change to single event

* Fix the unit test

* PR review comments

* refactor(viewport): simplify imports, align destructuring

Simplify import statements by removing unused imports, organize existing imports, and align destructured imports consistently across the component.

---------

platform/core/src/services/MeasurementService/MeasurementService.ts

@@ -67,7 +67,10 @@ const EVENTS = {
   RAW_MEASUREMENT_ADDED: 'event::raw_measurement_added',
   MEASUREMENT_REMOVED: 'event::measurement_removed',
   MEASUREMENTS_CLEARED: 'event::measurements_cleared',
-  JUMP_TO_MEASUREMENT: 'event:jump_to_measurement',
+  // Give the viewport a chance to jump to the measurement
+  JUMP_TO_MEASUREMENT_VIEWPORT: 'event:jump_to_measurement_viewport',
+  // Give the layout a chance to jump to the measurement
+  JUMP_TO_MEASUREMENT_LAYOUT: 'event:jump_to_measurement_layout',
 };
 
 const VALUE_TYPES = {
@@ -104,15 +107,16 @@ class MeasurementService extends PubSubService {
     },
   };
 
+  public static readonly EVENTS = EVENTS;
   public static VALUE_TYPES = VALUE_TYPES;
   public readonly VALUE_TYPES = VALUE_TYPES;
 
+  private measurements = new Map();
+
   constructor() {
     super(EVENTS);
     this.sources = {};
     this.mappings = {};
-    this.measurements = {};
-    this._jumpToMeasurementCache = {};
   }
 
   /**
@@ -120,7 +124,7 @@ class MeasurementService extends PubSubService {
    * This method should be used to add custom tool schema to the measurement service.
    * @param {Array} schema schema for validation
    */
-  addMeasurementSchemaKeys(schema) {
+  public addMeasurementSchemaKeys(schema): void {
     if (!Array.isArray(schema)) {
       schema = [schema];
     }
@@ -160,11 +164,7 @@ class MeasurementService extends PubSubService {
    * @return {Measurement[]} Array of measurements
    */
   getMeasurements() {
-    const measurements = this._arrayOfObjects(this.measurements);
-    return (
-      measurements &&
-      measurements.map(m => this.measurements[Object.keys(m)[0]])
-    );
+    return [...this.measurements.values()];
   }
 
   /**
@@ -173,18 +173,14 @@ class MeasurementService extends PubSubService {
    * @param {string} uid measurement uid
    * @return {Measurement} Measurement instance
    */
-  getMeasurement(measurementUID) {
-    let measurement = null;
-    const measurements = this.measurements[measurementUID];
-
-    if (measurements && Object.keys(measurements).length > 0) {
-      measurement = this.measurements[measurementUID];
-    }
-
-    return measurement;
+  public getMeasurement(measurementUID: string) {
+    return this.measurements.get(measurementUID);
   }
 
-  setMeasurementSelected(measurementUID, selected) {
+  public setMeasurementSelected(
+    measurementUID: string,
+    selected: boolean
+  ): void {
     const measurement = this.getMeasurement(measurementUID);
     if (!measurement) {
       return;
@@ -373,8 +369,8 @@ class MeasurementService extends PubSubService {
     }
   }
 
-  update(measurementUID, measurement, notYetUpdatedAtSource = false) {
-    if (!this.measurements[measurementUID]) {
+  update(measurementUID: string, measurement, notYetUpdatedAtSource = false) {
+    if (!this.measurements.has(measurementUID)) {
       return;
     }
 
@@ -388,7 +384,7 @@ class MeasurementService extends PubSubService {
       updatedMeasurement
     );
 
-    this.measurements[measurementUID] = updatedMeasurement;
+    this.measurements.set(measurementUID, updatedMeasurement);
 
     this._broadcastEvent(this.EVENTS.MEASUREMENT_UPDATED, {
       source: measurement.source,
@@ -470,15 +466,15 @@ class MeasurementService extends PubSubService {
       uid: internalUID,
     };
 
-    if (this.measurements[internalUID]) {
-      this.measurements[internalUID] = newMeasurement;
+    if (this.measurements.get(internalUID)) {
+      this.measurements.set(internalUID, newMeasurement);
       this._broadcastEvent(this.EVENTS.MEASUREMENT_UPDATED, {
         source,
         measurement: newMeasurement,
       });
     } else {
       log.info('Measurement added', newMeasurement);
-      this.measurements[internalUID] = newMeasurement;
+      this.measurements.set(internalUID, newMeasurement);
       this._broadcastEvent(this.EVENTS.RAW_MEASUREMENT_ADDED, {
         source,
         measurement: newMeasurement,
@@ -557,7 +553,7 @@ class MeasurementService extends PubSubService {
       );
     }
 
-    const oldMeasurement = this.measurements[internalUID];
+    const oldMeasurement = this.measurements.get(internalUID);
 
     const newMeasurement = {
       ...oldMeasurement,
@@ -569,7 +565,7 @@ class MeasurementService extends PubSubService {
     if (oldMeasurement) {
       // TODO: Ultimately, each annotation should have a selected flag right from the soure.
       // For now, it is just added in OHIF here and in setMeasurementSelected.
-      this.measurements[internalUID] = newMeasurement;
+      this.measurements.set(internalUID, newMeasurement);
       if (isUpdate) {
         this._broadcastEvent(this.EVENTS.MEASUREMENT_UPDATED, {
           source,
@@ -585,7 +581,7 @@ class MeasurementService extends PubSubService {
       }
     } else {
       log.info('Measurement started.', newMeasurement);
-      this.measurements[internalUID] = newMeasurement;
+      this.measurements.set(internalUID, newMeasurement);
     }
 
     return newMeasurement.uid;
@@ -598,12 +594,12 @@ class MeasurementService extends PubSubService {
    * @param {MeasurementSource} source The measurement source instance
    */
   remove(measurementUID, source, eventDetails) {
-    if (!measurementUID || !this.measurements[measurementUID]) {
+    if (!measurementUID || !this.measurements.has(measurementUID)) {
       log.warn(`No uid provided, or unable to find measurement by uid.`);
       return;
     }
 
-    delete this.measurements[measurementUID];
+    this.measurements.delete(measurementUID);
     this._broadcastEvent(this.EVENTS.MEASUREMENT_REMOVED, {
       source,
       measurement: measurementUID,
@@ -613,9 +609,8 @@ class MeasurementService extends PubSubService {
 
   clearMeasurements() {
     // Make a copy of the measurements
-    const measurements = { ...this.measurements };
-    this.measurements = {};
-    this._jumpToMeasurementCache = {};
+    const measurements = [...this.measurements.values()];
+    this.measurements.clear();
     this._broadcastEvent(this.EVENTS.MEASUREMENTS_CLEARED, { measurements });
   }
 
@@ -628,27 +623,39 @@ class MeasurementService extends PubSubService {
     this.clearMeasurements();
   }
 
-  jumpToMeasurement(viewportIndex, measurementUID) {
-    const measurement = this.measurements[measurementUID];
+  /**
+   * This method calls the subscriptions for JUMP_TO_MEASUREMENT_VIEWPORT
+   * and JUMP_TO_MEASUREMENT_LAYOUT.  There are two events which are
+   * fired because there are two different items which might want to handle
+   * the event.  First, there might already be a viewport which can handle
+   * the event.  If so, then the layout doesn't need to necessarily change.
+   * This is communicated by the isConsumed value on the event itself.
+   * Otherwise, the layout itself may need to be navigated to in order
+   * to provide a viewport which can show the given measurement.
+   *
+   * When a viewport decides to apply the event, it should call the consume()
+   * method on the event, so that other listeners know they do not need to
+   * navigate.  This does NOT affect whether the layout event is fired, and
+   * merely causes it to fire the event with the isConsumed set to true.
+   */
+
+  public jumpToMeasurement(
+    viewportIndex: number,
+    measurementUID: string
+  ): void {
+    const measurement = this.measurements.get(measurementUID);
 
     if (!measurement) {
       log.warn(`No measurement uid, or unable to find by uid.`);
       return;
     }
-    this._addJumpToMeasurement(viewportIndex, measurementUID);
-
-    this._broadcastEvent(this.EVENTS.JUMP_TO_MEASUREMENT, {
+    const consumableEvent = this.createConsumableEvent({
       viewportIndex,
       measurement,
     });
-  }
 
-  getJumpToMeasurement(viewportIndex) {
-    return this._jumpToMeasurementCache[viewportIndex];
-  }
-
-  removeJumpToMeasurement(viewportIndex) {
-    delete this._jumpToMeasurementCache[viewportIndex];
+    this._broadcastEvent(EVENTS.JUMP_TO_MEASUREMENT_VIEWPORT, consumableEvent);
+    this._broadcastEvent(EVENTS.JUMP_TO_MEASUREMENT_LAYOUT, consumableEvent);
   }
 
   _getSourceUID(name, version) {
@@ -663,10 +670,6 @@ class MeasurementService extends PubSubService {
     return sourceUID;
   }
 
-  _addJumpToMeasurement(viewportIndex, measurementUID) {
-    this._jumpToMeasurementCache[viewportIndex] = measurementUID;
-  }
-
   _getMappingByMeasurementSource(measurement, annotationType) {
     if (this._isValidSource(measurement.source)) {
       return this.mappings[measurement.source.uid].find(

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

@@ -1,12 +1,13 @@
 import { PubSubService } from '../_shared/pubSubServiceInterface';
-
-const EVENTS = {
-  ACTIVE_VIEWPORT_INDEX_CHANGED: 'event::activeviewportindexchanged',
-  LAYOUT_CHANGED: 'event::layoutChanged',
-  GRID_STATE_CHANGED: 'event::gridStateChanged',
-};
+import { getPresentationIds, PresentationIds } from './getPresentationIds';
 
 class ViewportGridService extends PubSubService {
+  public static readonly EVENTS = {
+    ACTIVE_VIEWPORT_INDEX_CHANGED: 'event::activeviewportindexchanged',
+    LAYOUT_CHANGED: 'event::layoutChanged',
+    GRID_STATE_CHANGED: 'event::gridStateChanged',
+  };
+
   public static REGISTRATION = {
     name: 'viewportGridService',
     altName: 'ViewportGridService',
@@ -14,12 +15,13 @@ class ViewportGridService extends PubSubService {
       return new ViewportGridService();
     },
   };
-  public static EVENTS = EVENTS;
+
+  public static getPresentationIds = getPresentationIds;
 
   serviceImplementation = {};
 
   constructor() {
-    super(EVENTS);
+    super(ViewportGridService.EVENTS);
     this.serviceImplementation = {};
   }
 
@@ -37,10 +39,12 @@ class ViewportGridService extends PubSubService {
       this.serviceImplementation._getState = getStateImplementation;
     }
     if (setActiveViewportIndexImplementation) {
-      this.serviceImplementation._setActiveViewportIndex = setActiveViewportIndexImplementation;
+      this.serviceImplementation._setActiveViewportIndex =
+        setActiveViewportIndexImplementation;
     }
     if (setDisplaySetsForViewportsImplementation) {
-      this.serviceImplementation._setDisplaySetsForViewports = setDisplaySetsForViewportsImplementation;
+      this.serviceImplementation._setDisplaySetsForViewports =
+        setDisplaySetsForViewportsImplementation;
     }
     if (setLayoutImplementation) {
       this.serviceImplementation._setLayout = setLayoutImplementation;
@@ -55,7 +59,8 @@ class ViewportGridService extends PubSubService {
       this.serviceImplementation._set = setImplementation;
     }
     if (getNumViewportPanesImplementation) {
-      this.serviceImplementation._getNumViewportPanes = getNumViewportPanesImplementation;
+      this.serviceImplementation._getNumViewportPanes =
+        getNumViewportPanesImplementation;
     }
   }
 
@@ -75,11 +80,29 @@ class ViewportGridService extends PubSubService {
 
   public setDisplaySetsForViewport(props) {
     // Just update a single viewport, but use the multi-viewport update for it.
-    this.serviceImplementation._setDisplaySetsForViewports([props]);
+    this.setDisplaySetsForViewports([props]);
   }
 
   public setDisplaySetsForViewports(props) {
     this.serviceImplementation._setDisplaySetsForViewports(props);
+    const state = this.getState();
+    const viewports = [];
+
+    for (const viewport of props) {
+      const updatedViewport = state.viewports[viewport.viewportIndex];
+      if (updatedViewport) {
+        viewports.push(updatedViewport);
+      } else {
+        console.warn(
+          "ViewportGridService::Didn't find updated viewport",
+          viewport
+        );
+      }
+    }
+    this._broadcastEvent(ViewportGridService.EVENTS.GRID_STATE_CHANGED, {
+      state,
+      viewports,
+    });
   }
 
   /**
@@ -150,3 +173,5 @@ class ViewportGridService extends PubSubService {
 }
 
 export default ViewportGridService;
+
+export type { PresentationIds };

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

@@ -118,3 +118,4 @@ const getPresentationIds = (viewport, viewports): PresentationIds => {
 };
 
 export default getPresentationIds;
+export { getPresentationIds };

platform/core/src/services/_shared/pubSubServiceInterface.js

@@ -103,4 +103,20 @@ export class PubSubService {
     this.unsubscriptions.forEach(unsub => unsub());
     this.unsubscriptions = [];
   }
+
+  /**
+   * Creates an event that records whether or not someone
+   * has consumed it.  Call eventData.consume() to consume the event.
+   * Check eventData.isConsumed to see if it is consumed or not.
+   * @param props - to include in the event
+   */
+  protected createConsumableEvent(props) {
+    return {
+      ...props,
+      isConsumed: false,
+      consume: function Consume() {
+        this.isConsumed = true;
+      },
+    }
+  }
 }

platform/core/src/utils/absoluteUrl.test.js

@@ -24,27 +24,22 @@ describe('absoluteUrl', () => {
   });
 
   test('should return the original path when there path in the window.origin after the domain and port', () => {
-    global.window = Object.create(window);
+    delete global.window.location;
     const url = 'http://dummy.com';
-    Object.defineProperty(window, 'location', {
-      value: {
-        origin: url,
-      },
-      writable: true,
-    });
+    global.window.location = {
+      origin: url,
+    };
     const absoluteUrlOutput = absoluteUrl('path_1/path_2/path_3');
     expect(absoluteUrlOutput).toEqual('/path_1/path_2/path_3');
   });
 
   test('should be able to return the absolute path even when the path contains duplicates', () => {
-    global.window = Object.create(window);
+    global.window ||= Object.create(window);
     const url = 'http://dummy.com';
-    Object.defineProperty(window, 'location', {
-      value: {
-        origin: url,
-      },
-      writable: true,
-    });
+    delete global.window.location;
+    global.window.location = {
+      origin: url,
+    };
     const absoluteUrlOutput = absoluteUrl('path_1/path_1/path_1');
     expect(absoluteUrlOutput).toEqual('/path_1/path_1/path_1');
   });

platform/docs/docs/platform/services/data/MeasurementService.md

@@ -29,7 +29,8 @@ There are seven events that get publish in `MeasurementService`:
 | RAW_MEASUREMENT_ADDED | Fires when a raw measurement is added (e.g., dicom-sr) |
 | MEASUREMENT_REMOVED   | Fires when a measurement is removed                    |
 | MEASUREMENTS_CLEARED  | Fires when all measurements are deleted                |
-| JUMP_TO_MEASUREMENT   | Fires when a measurement is requested to be jump to    |
+| JUMP_TO_MEASUREMENT_VIEWPORT   | Fires when a measurement is requested to be jumped to, applying to individual viewports. |
+| JUMP_TO_MEASUREMENT_LAYOUT   | Fires when a measurement is requested to be jumped to, applying to the overall layout. |
 
 ## API
 

platform/ui/src/contextProviders/ViewportGridProvider.tsx

@@ -7,8 +7,8 @@ import React, {
 } from 'react';
 import PropTypes from 'prop-types';
 import isEqual from 'lodash.isequal';
+import { ViewportGridService } from '@ohif/core';
 import viewportLabels from '../utils/viewportLabels';
-import getPresentationIds from './getPresentationIds';
 
 const DEFAULT_STATE = {
   activeViewportIndex: 0,
@@ -173,7 +173,7 @@ export function ViewportGridProvider({ children, service }) {
             displaySetOptions,
             viewportLabel: viewportLabels[viewportIndex],
           };
-          viewportOptions.presentationIds = getPresentationIds(
+          viewportOptions.presentationIds = ViewportGridService.getPresentationIds(
             newViewport,
             viewports
           );
@@ -265,7 +265,7 @@ export function ViewportGridProvider({ children, service }) {
             state.viewports
           );
           if (!viewport.viewportOptions.presentationIds) {
-            viewport.viewportOptions.presentationIds = getPresentationIds(
+            viewport.viewportOptions.presentationIds = ViewportGridService.getPresentationIds(
               viewport,
               viewports
             );
@@ -403,12 +403,16 @@ export function ViewportGridProvider({ children, service }) {
     getNumViewportPanes,
   ]);
 
+  // run many of the calls through the service itself since we want to publish events
   const api = {
     getState,
-    setActiveViewportIndex: index => service.setActiveViewportIndex(index), // run it through the service itself since we want to publish events
-    setDisplaySetsForViewports,
-    setLayout: layout => service.setLayout(layout), // run it through the service itself since we want to publish events
-    reset,
+    setActiveViewportIndex: index => service.setActiveViewportIndex(index),
+    setDisplaySetsForViewport: props =>
+      service.setDisplaySetsForViewports([props]),
+    setDisplaySetsForViewports: props =>
+      service.setDisplaySetsForViewports(props),
+    setLayout: layout => service.setLayout(layout),
+    reset: () => service.reset(),
     set: gridLayoutState => service.setState(gridLayoutState), // run it through the service itself since we want to publish events
     getNumViewportPanes,
   };
@@ -422,9 +426,7 @@ export function ViewportGridProvider({ children, service }) {
 
 ViewportGridProvider.propTypes = {
   children: PropTypes.any,
-  service: PropTypes.shape({
-    setServiceImplementation: PropTypes.func,
-  }).isRequired,
+  service: PropTypes.instanceOf(ViewportGridService).isRequired,
 };
 
 export const useViewportGrid = () => useContext(ViewportGridContext);

platform/ui/src/types/index.ts

@@ -18,4 +18,4 @@ const StringNumber = PropTypes.oneOfType([PropTypes.string, PropTypes.number]);
  */
 const StringArray = PropTypes.oneOfType([PropTypes.string, PropTypes.array]);
 
-export { StringNumber, StringArray, ThumbnailType, PresentationIds };
+export { StringNumber, StringArray, ThumbnailType };

platform/viewer/src/components/ViewportGrid.tsx

@@ -1,6 +1,6 @@
 import React, { useEffect, useCallback } from 'react';
 import PropTypes from 'prop-types';
-import { ServicesManager, Types } from '@ohif/core';
+import { ServicesManager, Types, MeasurementService } from '@ohif/core';
 import { ViewportGrid, ViewportPane, useViewportGrid } from '@ohif/ui';
 import { utils } from '@ohif/core';
 import EmptyViewport from './EmptyViewport';
@@ -23,13 +23,6 @@ const ORIENTATION_MAP = {
   },
 };
 
-const compareViewportOptions = (opts1, opts2) => {
-  if ((opts1.viewportType || 'stack') != opts2.viewportType) {
-    return false;
-  }
-  return true;
-};
-
 function ViewerViewportGrid(props) {
   const { servicesManager, viewportComponents, dataSource } = props;
   const [viewportGrid, viewportGridService] = useViewportGrid();
@@ -160,77 +153,48 @@ function ViewerViewportGrid(props) {
 
   useEffect(() => {
     const { unsubscribe } = measurementService.subscribe(
-      measurementService.EVENTS.JUMP_TO_MEASUREMENT,
-      ({ viewportIndex, measurement }) => {
-        const {
-          displaySetInstanceUID: referencedDisplaySetInstanceUID,
-          metadata: { viewPlaneNormal },
-        } = measurement;
-
-        // if we already have the displaySet in one of the viewports
-        // Todo: handle fusion display sets?
-        for (const viewport of viewports) {
-          const isMatch = viewport.displaySetInstanceUIDs.includes(
-            referencedDisplaySetInstanceUID
-          );
-          if (isMatch) {
-            return;
-          }
-        }
-
-        const displaySet = displaySetService.getDisplaySetByUID(
-          referencedDisplaySetInstanceUID
-        );
-
-        let imageIndex;
-        // jump straight to the initial image index if we can
-        if (displaySet.images && measurement.SOPInstanceUID) {
-          imageIndex = displaySet.images.findIndex(
-            image => image.SOPInstanceUID === measurement.SOPInstanceUID
-          );
-        }
+      MeasurementService.EVENTS.JUMP_TO_MEASUREMENT_LAYOUT,
+      ({ viewportIndex, measurement, isConsumed }) => {
+        if (isConsumed) return;
+        // This occurs when no viewport has elected to consume the event
+        // so we need to change layouts into a layout which can consume
+        // the event.
+        const { displaySetInstanceUID: referencedDisplaySetInstanceUID } =
+          measurement;
 
         const updatedViewports = _getUpdatedViewports(
           viewportIndex,
           referencedDisplaySetInstanceUID
         );
+        // Arbitrarily assign the viewport to element 0
+        const viewport = updatedViewports?.[0];
 
-        if (!updatedViewports || !updatedViewports.length) {
+        if (!viewport) {
+          console.warn(
+            'ViewportGrid::Unable to navigate to viewport containing',
+            referencedDisplaySetInstanceUID
+          );
           return;
         }
 
-        updatedViewports.forEach(vp => {
-          vp.viewportOptions ||= {};
-          const { orientation, viewportType } = vp.viewportOptions;
-          let initialImageOptions;
+        viewport.viewportOptions ||= {};
+        viewport.viewportOptions.orientation = 'acquisition';
 
-          // For initial imageIndex to hang be careful for the volume viewport
-          if (viewportType === 'stack' || !viewportType) {
-            initialImageOptions = {
-              index: imageIndex,
-            };
-          } else if (viewportType === 'volume') {
-            // For the volume viewports, be careful to not jump in the viewports
-            // that are not in the same orientation.
-            // Todo: this doesn't work for viewports that have custom orientation
-            // vectors specified
-            if (
-              orientation &&
-              viewPlaneNormal &&
-              isEqualWithin(
-                ORIENTATION_MAP[orientation]?.viewPlaneNormal,
-                viewPlaneNormal
-              )
-            ) {
-              initialImageOptions = {
-                index: imageIndex,
+        const displaySet = displaySetService.getDisplaySetByUID(
+          referencedDisplaySetInstanceUID
+        );
+        // jump straight to the initial image index if we can
+        if (displaySet.images && measurement.SOPInstanceUID) {
+          for (let index = 0; index < displaySet.images.length; index++) {
+            const image = displaySet.images[index];
+            if (image.SOPInstanceUID === measurement.SOPInstanceUID) {
+              viewport.viewportOptions.initialImageOptions = {
+                index,
               };
+              break;
             }
           }
-
-          vp.viewportOptions.initialImageOptions = initialImageOptions;
-        });
-
+        }
         viewportGridService.setDisplaySetsForViewports(updatedViewports);
       }
     );