PR review comments

extensions/cornerstone-dicom-sr/src/commandsModule.ts

@@ -11,7 +11,6 @@ const { MeasurementReport } = adaptersSR.Cornerstone3D;
 const { log } = OHIF;
 
 /**
- *
  * @param measurementData An array of measurements from the measurements service
  * that you wish to serialize.
  * @param additionalFindingTypes toolTypes that should be stored with labels as Findings
@@ -46,52 +45,24 @@ const commandsModule = (props: withAppTypes) => {
   const { customizationService, measurementService, viewportGridService, uiDialogService } =
     servicesManager.services;
   const actions = {
-    jumpToMeasurement: ({ uid }) => {
-      measurementService.jumpToMeasurement(viewportGridService.getActiveViewportId(), uid);
-    },
-
-    removeMeasurement: ({ uid }) => {
-      measurementService.remove(uid);
-    },
-
-    renameMeasurement: ({ uid }) => {
-      const labelConfig = customizationService.get('measurementLabels');
-      const measurement = measurementService.getMeasurement(uid);
-      showLabelAnnotationPopup(measurement, uiDialogService, labelConfig).then(val => {
-        measurementService.update(
-          uid,
-          {
-            ...val,
-          },
-          true
-        );
-      });
-    },
-
     changeColorMeasurement: ({ uid }) => {
-      const { color } = measurementService.getMeasurement(uid);
-      const rgbaColor = {
-        r: color[0],
-        g: color[1],
-        b: color[2],
-        a: color[3] / 255.0,
-      };
-      colorPickerDialog(uiDialogService, rgbaColor, (newRgbaColor, actionId) => {
-        if (actionId === 'cancel') {
-          return;
-        }
-
-        const color = [newRgbaColor.r, newRgbaColor.g, newRgbaColor.b, newRgbaColor.a * 255.0];
-        // segmentationService.setSegmentColor(viewportId, segmentationId, segmentIndex, color);
-      });
-    },
-
-    toggleLockMeasurement: ({ uid }) => {
-      measurementService.toggleLockMeasurement(uid);
-    },
-
-    toggleVisibilityMeasurement: ({ uid }) => {
-      measurementService.toggleVisibilityMeasurement(uid);
+      // When this gets supported, it probably belongs in cornerstone, not sr
+      throw new Error('Unsupported operation: changeColorMeasurement');
+      // const { color } = measurementService.getMeasurement(uid);
+      // const rgbaColor = {
+      //   r: color[0],
+      //   g: color[1],
+      //   b: color[2],
+      //   a: color[3] / 255.0,
+      // };
+      // colorPickerDialog(uiDialogService, rgbaColor, (newRgbaColor, actionId) => {
+      //   if (actionId === 'cancel') {
+      //     return;
+      //   }
+
+      //   const color = [newRgbaColor.r, newRgbaColor.g, newRgbaColor.b, newRgbaColor.a * 255.0];
+      // segmentationService.setSegmentColor(viewportId, segmentationId, segmentIndex, color);
+      // });
     },
 
     /**
@@ -111,13 +82,6 @@ const commandsModule = (props: withAppTypes) => {
       window.location.assign(objectUrl);
     },
 
-    /**
-     * Clear the measurements
-     */
-    clearMeasurements: ({ measurementFilter }) => {
-      measurementService.clearMeasurements(measurementFilter);
-    },
-
     /**
      * Download the CSV report for the measurements.
      */
@@ -196,27 +160,9 @@ const commandsModule = (props: withAppTypes) => {
     storeMeasurements: {
       commandFn: actions.storeMeasurements,
     },
-    clearMeasurements: {
-      commandFn: actions.clearMeasurements,
-    },
     downloadCSVMeasurementsReport: {
       commandFn: actions.downloadCSVMeasurementsReport,
     },
-    jumpToMeasurement: {
-      commandFn: actions.jumpToMeasurement,
-    },
-    removeMeasurement: {
-      commandFn: actions.removeMeasurement,
-    },
-    renameMeasurement: {
-      commandFn: actions.renameMeasurement,
-    },
-    toggleLockMeasurement: {
-      commandFn: actions.toggleLockMeasurement,
-    },
-    toggleVisibilityMeasurement: {
-      commandFn: actions.toggleVisibilityMeasurement,
-    },
   };
 
   return {

extensions/cornerstone/src/commandsModule.ts

@@ -266,6 +266,42 @@ function commandsModule({
       measurementService.update(updatedMeasurement.uid, updatedMeasurement, true);
     },
 
+    jumpToMeasurement: ({ uid }) => {
+      measurementService.jumpToMeasurement(viewportGridService.getActiveViewportId(), uid);
+    },
+
+    removeMeasurement: ({ uid }) => {
+      measurementService.remove(uid);
+    },
+
+    renameMeasurement: ({ uid }) => {
+      const labelConfig = customizationService.get('measurementLabels');
+      const measurement = measurementService.getMeasurement(uid);
+      showLabelAnnotationPopup(measurement, uiDialogService, labelConfig).then(val => {
+        measurementService.update(
+          uid,
+          {
+            ...val,
+          },
+          true
+        );
+      });
+    },
+
+    toggleLockMeasurement: ({ uid }) => {
+      measurementService.toggleLockMeasurement(uid);
+    },
+
+    toggleVisibilityMeasurement: ({ uid }) => {
+      measurementService.toggleVisibilityMeasurement(uid);
+    },
+    /**
+     * Clear the measurements
+     */
+    clearMeasurements: ({ measurementFilter }) => {
+      measurementService.clearMeasurements(measurementFilter);
+    },
+
     // Retrieve value commands
     getActiveViewportEnabledElement: _getActiveViewportEnabledElement,
 
@@ -1252,6 +1288,24 @@ function commandsModule({
     updateMeasurement: {
       commandFn: actions.updateMeasurement,
     },
+    clearMeasurements: {
+      commandFn: actions.clearMeasurements,
+    },
+    jumpToMeasurement: {
+      commandFn: actions.jumpToMeasurement,
+    },
+    removeMeasurement: {
+      commandFn: actions.removeMeasurement,
+    },
+    renameMeasurement: {
+      commandFn: actions.renameMeasurement,
+    },
+    toggleLockMeasurement: {
+      commandFn: actions.toggleLockMeasurement,
+    },
+    toggleVisibilityMeasurement: {
+      commandFn: actions.toggleVisibilityMeasurement,
+    },
     setViewportWindowLevel: {
       commandFn: actions.setViewportWindowLevel,
     },

extensions/cornerstone/src/panels/PanelMeasurement.tsx

@@ -122,11 +122,6 @@ export default function PanelMeasurementTable({
             title="Additional Findings"
             {...onArgs}
           >
-            <MeasurementTable.Header>
-              <>
-                <div>Hello World</div>
-              </>
-            </MeasurementTable.Header>
             <MeasurementTable.Body />
           </MeasurementTable>
         )}
@@ -137,11 +132,6 @@ export default function PanelMeasurementTable({
             title="Untracked Findings"
             {...onArgs}
           >
-            <MeasurementTable.Header>
-              <>
-                <span>Untracked</span>
-              </>
-            </MeasurementTable.Header>
             <MeasurementTable.Body />
           </MeasurementTable>
         )}

extensions/default/src/commandsModule.ts

@@ -47,6 +47,39 @@ const commandsModule = ({
   const contextMenuController = new ContextMenuController(servicesManager, commandsManager);
 
   const actions = {
+    /**
+     * Runs commands specified in the options.
+     * This allows running multiple commands when only one is specified.  It
+     * also allows an async instantiation of a command so that the return is
+     * immediate without waiting for the command to complete.
+     *
+     * @param options.options - shared options for the child command
+     * @param options.commands - a list of commands to run.  This is an array, and can
+     *      be either a full command, or just a name.
+     * @param options.async - set to true to run async
+     * @param options.parallel - set to false, WITH async true to run non-parallel
+     */
+    run: options => {
+      const { options: sharedOptions, commands = [], parallel = true, async = false } = options;
+      const childOptions = {
+        ...options,
+        options: undefined,
+        commands: undefined,
+        parallel: undefined,
+        async: undefined,
+        ...sharedOptions,
+      };
+      if (async) {
+        if (parallel) {
+          return Promise.all(
+            commands.map(async command => commandsManager.run(command, childOptions))
+          );
+        }
+        return (async () => await commandsManager.run(commands, childOptions))();
+      }
+      return commandsManager.run(commands, childOptions);
+    },
+
     /**
      * Show the context menu.
      * @param options.menuId defines the menu name to lookup, from customizationService
@@ -554,9 +587,8 @@ const commandsModule = ({
   };
 
   const definitions = {
-    showContextMenu: {
-      commandFn: actions.showContextMenu,
-    },
+    run: actions.run,
+    showContextMenu: actions.showContextMenu,
     closeContextMenu: {
       commandFn: actions.closeContextMenu,
     },

platform/core/src/extensions/ExtensionManager.ts

@@ -596,7 +596,10 @@ export default class ExtensionManager extends PubSubService {
     }
 
     Object.keys(definitions).forEach(commandName => {
-      const commandDefinition = definitions[commandName];
+      let commandDefinition = definitions[commandName];
+      if (typeof commandDefinition === 'function') {
+        commandDefinition = { commandFn: commandDefinition };
+      }
       const commandHasContextThatDoesNotExist =
         commandDefinition.context && !this._commandsManager.getContext(commandDefinition.context);
 

platform/core/src/utils/measurementFilters.ts

@@ -1,3 +1,7 @@
+/**
+ * Returns a filter function which filters for measurements belonging to both
+ * the study and series.
+ */
 export function filterTracked(trackedStudy: string, trackedSeries) {
   return measurement => {
     const result =
@@ -7,44 +11,92 @@ export function filterTracked(trackedStudy: string, trackedSeries) {
   };
 }
 
+/** A filter that always returns true */
 export function filterAny(_measurement) {
   return true;
 }
 
+/** A filter that excludes everything */
 export function filterNone(_measurement) {
   return false;
 }
 
-export function filterOthers(...filters) {
-  return item => !filters.find(filter => filter(item));
-}
-
+/**
+ *  Filters the measurements which are found in any of the filters in the provided
+ * object.  This can be used to query for any matching of a set.
+ * This passes the this argument to the child function(s)
+ */
 export function filterOr(measurementFilters) {
-  return item => {
+  return function (item) {
     for (const filter of Object.values(measurementFilters)) {
       if (typeof filter !== 'function') {
         continue;
       }
-      if (filter(item)) {
+      if (filter.call(this, item)) {
         return true;
       }
     }
     return false;
   };
 }
 
+/**
+ * Filters for additional findings, that is, measurements with
+ * a value of type point, and having a referenced image
+ */
 export function filterAdditionalFinding(measurementService) {
   const { POINT } = measurementService.VALUE_TYPES;
   return dm => dm.type === POINT && dm.referencedImageId;
 }
 
+/**
+ * Returns a filter that applies the second filter unless the first filter would
+ * include the given measurement.
+ * That is, (!filterUnless) && filterThen
+ */
 export function filterUnless(filterUnless, filterThen) {
   return item => (filterUnless(item) ? false : filterThen(item));
 }
 
 const isString = s => typeof s === 'string' || s instanceof String;
 
-export function filterNot(filter) {
+/**
+ * Returns true if all the filters return true.
+ * Any filter can be a string name of a filter on the "this" object
+ * called on the final filter call.
+ */
+export function filterAnd(...filters) {
+  return function (item) {
+    for (const filter of filters) {
+      if (isString(filter)) {
+        if (!this[filter](item)) {
+          return false;
+        }
+      } else if (!filter.call(this, item)) {
+        return false;
+      }
+    }
+    return true;
+  };
+}
+
+/**
+ * Returns a filter that returns true if none of the filters supplied return true.
+ * Any filter supplied can be a name, in which case hte filter will be retrieved
+ * from "this" object on the call.
+ *
+ * For example, for filterNot("otherFilterName"), if that is called on
+ * `{ otherFilterName: filterNone }`
+ * then otherFilterName will be called, returning false in this case and
+ * filterNot will return true.
+ *
+ *
+ */
+export function filterNot(...filters) {
+  if (filters.length !== 1) {
+    return filterAnd.apply(null, filters.map(filterNot));
+  }
+  const [filter] = filters;
   if (isString(filter)) {
     return function (item) {
       return !this[filter](item);