fix: Initial sort not consistent (#5224)

* fix: Initial sort not consistent

* Add better sort criteria

* Remove sorting changes

* Start replacing screen shots

* Update bun lock for version

* Leave default sort for test mode

* fix: Add consistent sorting for same series splits

* PR comments

* docs: add notes for series/display set sort

* Updated example docs

* Update sortVector to be compareSameStudy

* Add unit test

* PR comments

extensions/default/src/getSopClassHandlerModule.js

@@ -67,7 +67,7 @@ function getDisplaySetInfo(instances) {
   };
 }
 
-const makeDisplaySet = instances => {
+const makeDisplaySet = (instances, index) => {
   // Need to sort the instances in order to get a consistent instance/thumbnail
   sortStudyInstances(instances);
   const instance = instances[0];
@@ -190,7 +190,7 @@ function getDisplaySetsFromSeries(instances) {
   // into their own specific display sets. Place the rest of each
   // series into another display set.
   const stackableInstances = [];
-  instances.forEach(instance => {
+  instances.forEach((instance, instanceIndex) => {
     // All imaging modalities must have a valid value for sopClassUid (x00080016) or rows (x00280010)
     if (!isImage(instance.SOPClassUID) && !instance.Rows) {
       return;
@@ -198,7 +198,7 @@ function getDisplaySetsFromSeries(instances) {
 
     let displaySet;
     if (isMultiFrame(instance)) {
-      displaySet = makeDisplaySet([instance]);
+      displaySet = makeDisplaySet([instance], instanceIndex);
       displaySet.setAttributes({
         sopClassUids,
         numImageFrames: instance.NumberOfFrames,
@@ -207,7 +207,7 @@ function getDisplaySetsFromSeries(instances) {
       });
       displaySets.push(displaySet);
     } else if (isSingleImageModality(instance.Modality)) {
-      displaySet = makeDisplaySet([instance]);
+      displaySet = makeDisplaySet([instance], instanceIndex);
       displaySet.setAttributes({
         sopClassUids,
         instanceNumber: instance.InstanceNumber,
@@ -220,7 +220,7 @@ function getDisplaySetsFromSeries(instances) {
   });
 
   if (stackableInstances.length) {
-    const displaySet = makeDisplaySet(stackableInstances);
+    const displaySet = makeDisplaySet(stackableInstances, displaySets.length);
     displaySet.setAttribute('studyInstanceUid', instances[0].StudyInstanceUID);
     displaySet.setAttributes({
       sopClassUids,

platform/app/src/routes/Mode/defaultRouteInit.ts

@@ -1,8 +1,8 @@
-import getStudies from './studiesList';
 import { DicomMetadataStore, log, utils, Enums } from '@ohif/core';
+import getStudies from './studiesList';
 import isSeriesFilterUsed from '../../utils/isSeriesFilterUsed';
 
-const { getSplitParam } = utils;
+const { seriesSortCriteria, getSplitParam } = utils;
 
 /**
  * Initialize the route.
@@ -14,7 +14,12 @@ const { getSplitParam } = utils;
  * @returns array of subscriptions to cancel
  */
 export async function defaultRouteInit(
-  { servicesManager, studyInstanceUIDs, dataSource, filters, appConfig }: withAppTypes,
+  {
+    servicesManager,
+    studyInstanceUIDs,
+    dataSource,
+    filters,
+  }: withAppTypes & { studyInstanceUIDs?: string[] },
   hangingProtocolId,
   stageIndex
 ) {
@@ -27,13 +32,17 @@ export async function defaultRouteInit(
    */
   function applyHangingProtocol() {
     const displaySets = displaySetService.getActiveDisplaySets();
+    // The display sets are not necessarily in load order, even though the
+    // series got started in load order, so re-sort them before hanging
+    const sortCriteria = seriesSortCriteria.default;
 
     if (!displaySets || !displaySets.length) {
       return;
     }
+    const sortedDisplaySets = [...displaySets].sort(sortCriteria);
 
     // Gets the studies list to use
-    const studies = getStudies(studyInstanceUIDs, displaySets);
+    const studies = getStudies(studyInstanceUIDs, sortedDisplaySets);
 
     // study being displayed, and is thus the "active" study.
     const activeStudy = studies[0];

platform/core/src/services/DisplaySetService/DisplaySetService.ts

@@ -464,12 +464,17 @@ export default class DisplaySetService extends PubSubService {
   /**
    *
    * @param sortFn function to sort the display sets
-   * @param direction direction to sort the display sets
+   * @param direction direction to sort the display sets.  Ascending means
+   *    increasing in value, which will typically put the lowest series numbers
+   *    first, with low priority display sets last with newest first.
+   *    The meaning of this flag may change to leave the image/non-image display
+   *    set sorting alone and only affect sorting within groups, or have additional
+   *    values for specific changes to the sort.
    * @returns void
    */
   public sortDisplaySets(
     sortFn: (a: DisplaySet, b: DisplaySet) => number,
-    direction: string,
+    direction: 'ascending' | 'descending' = 'ascending',
     suppressEvent = false
   ): void {
     this.activeDisplaySets.sort(sortFn);

platform/core/src/types/DisplaySet.ts

@@ -78,6 +78,12 @@ export type DisplaySet = {
   isLoaded?: boolean;
   isHydrated?: boolean;
   isRehydratable?: boolean;
+
+  /**
+   * The name of the comparison function (for sort) to use when comparing display
+   * sets that are coming from same series instanceUID.
+   */
+  compareSameSeries?: string;
 };
 
 export type DisplaySetSeriesMetadataInvalidatedEvent = {

platform/core/src/utils/createStudyBrowserTabs.ts

@@ -1,13 +1,14 @@
 import { useSystem } from '../contextProviders/SystemProvider';
+import { seriesSortCriteria } from './sortStudy';
 
 /**
  * Tab properties that drive which tab group is used for thumbnail display.
  */
 export type TabProp = {
-  name: string,
-  label: string,
-  studies: any[],
-}
+  name: string;
+  label: string;
+  studies: any[];
+};
 
 /**
  * Collection of tab properties with studies presorted depending on tab mod.
@@ -36,7 +37,7 @@ export function createStudyBrowserTabs(
   recentTimeframeMS = 31536000000
 ): TabsProps {
   const { servicesManager } = useSystem();
-  const { displaySetService } = servicesManager.services;
+  const { displaySetService, customizationService } = servicesManager.services;
 
   const shouldSortBySeriesUID = process.env.TEST_ENV === 'true';
   const primaryStudies = [];
@@ -48,17 +49,16 @@ export function createStudyBrowserTabs(
     );
 
     // sort them by seriesInstanceUID
-    let sortedDisplaySets;
-    if (shouldSortBySeriesUID) {
-      sortedDisplaySets = displaySetsForStudy.sort((a, b) => {
-        const displaySetA = displaySetService.getDisplaySetByUID(a.displaySetInstanceUID);
-        const displaySetB = displaySetService.getDisplaySetByUID(b.displaySetInstanceUID);
+    const sortCriteria = shouldSortBySeriesUID
+      ? seriesSortCriteria.compareSeriesUID
+      : (customizationService.getCustomization('sortingCriteria') as (a, b) => number);
+    const sortedDisplaySets = displaySetsForStudy.sort((a, b) => {
+      const displaySetA = displaySetService.getDisplaySetByUID(a.displaySetInstanceUID);
+      const displaySetB = displaySetService.getDisplaySetByUID(b.displaySetInstanceUID);
+      return sortCriteria(displaySetA, displaySetB);
+    });
 
-        return displaySetA.SeriesInstanceUID.localeCompare(displaySetB.SeriesInstanceUID);
-      });
-    } else {
-      sortedDisplaySets = displaySetsForStudy;
-    }
+    // return displaySetA.SeriesInstanceUID.localeCompare(displaySetB.SeriesInstanceUID);
 
     const tabStudy = Object.assign({}, study, {
       displaySets: sortedDisplaySets,

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

@@ -0,0 +1,44 @@
+import { compareSeriesUID, addSameSeriesCompare, compare } from './sortStudy';
+
+addSameSeriesCompare('default', (a,b) => compare(a.default,b.default), 5);
+const altCompare = 'altCompare'
+addSameSeriesCompare(altCompare, (a,b) => compare(a.altCompare,b.altCompare), 3);
+
+const ds1 = {
+  SeriesInstanceUID: '1',
+  default: 'ds1',
+}
+
+const ds2 = {
+  ...ds1,
+  default: 'ds2',
+}
+
+const ds3 = {
+  ...ds2,
+  altCompare: 3,
+  compareSameSeries: altCompare,
+}
+
+const ds4 = {
+  ...ds1,
+  altCompare:4,
+  compareSameSeries: altCompare,
+}
+
+const ds5 = {
+  ...ds1,
+  SeriesInstanceUID: '3',
+}
+
+describe('sortStudy', () => {
+  test('compareSameSeries',() => {
+    const initial = [ds5, ds4,ds3,ds2,ds1];
+    initial.sort(compareSeriesUID);
+    expect(initial[0]).toBe(ds1);
+    expect(initial[1]).toBe(ds2);
+    expect(initial[2]).toBe(ds3);
+    expect(initial[3]).toBe(ds4);
+    expect(initial[4]).toBe(ds5);
+  })
+})

platform/core/src/utils/sortStudy.ts

@@ -3,19 +3,77 @@ import isLowPriorityModality from './isLowPriorityModality';
 import calculateScanAxisNormal from './calculateScanAxisNormal';
 import areAllImageOrientationsEqual from './areAllImageOrientationsEqual';
 
-const compareSeriesDateTime = (a, b) => {
-  const seriesDateA = Date.parse(`${a.seriesDate ?? a.SeriesDate} ${a.seriesTime ?? a.SeriesTime}`);
-  const seriesDateB = Date.parse(`${b.seriesDate ?? b.SeriesDate} ${b.seriesTime ?? b.SeriesTime}`);
-  return seriesDateA - seriesDateB;
+export const compare = (a, b) => {
+  if (a == b) return 0;
+  if (!a && b) return -1;
+  if (!b && a) return 1;
+  if (a < b) return -1;
+  return 1;
 };
 
-const defaultSeriesSort = (a, b) => {
+type CompareSameSeries = {
+  priority: number;
+  compare: (a, b) => number;
+};
+
+const mapCompareSameSeries = new Map<string, CompareSameSeries>();
+
+/**
+ * Adds a comparison for same series display sets.
+ * Supply null for compareF to delete the function.
+ */
+export function addSameSeriesCompare(name: string, compareF: (a, b) => number, priority: number) {
+  if (!compareF) {
+    mapCompareSameSeries.delete(name);
+  } else {
+    mapCompareSameSeries.set(name, { compare: compareF, priority });
+  }
+}
+
+/**
+ * When the "series" sort is used on display sets, it is possible to get the
+ * same series twice.  This method compares two display sets from the same series
+ *
+ * If both display sets have the same compareSameSeries name, then the
+ * function registered for that name will be used.
+ *
+ * If they differ, then the priority between the two functions will be used.
+ *
+ * Otherwise, the instance compare will be used on the default instance.
+ *
+ * This provides a configurable well defined sorting order.
+ */
+export const compareSameSeriesDisplaySet = (a, b) => {
+  const { compareSameSeries: compareAName = 'default' } = a;
+  const { compareSameSeries: compareBName = 'default' } = b;
+  const compareA = mapCompareSameSeries.get(compareAName);
+  const compareB = mapCompareSameSeries.get(compareBName);
+  if (compareA && compareB) {
+    const compareValue =
+      compareA === compareB
+        ? compareA.compare(a, b)
+        : compare(compareA.priority, compareB.priority);
+    if (!compareValue) {
+      return compareValue;
+    }
+  }
+  return sortByInstanceNumber(a.instance, b.instance);
+};
+
+export const compareSeriesUID = (a, b) =>
+  compare(a.SeriesInstanceUID, b.SeriesInstanceUID) || compareSameSeriesDisplaySet(a, b);
+
+export const compareSeriesDateTime = (a, b) => {
+  // Natural order of string is good enough here
+  const seriesDateA = `${a.seriesDate ?? a.SeriesDate} ${a.seriesTime ?? a.SeriesTime}`;
+  const seriesDateB = `${b.seriesDate ?? b.SeriesDate} ${b.seriesTime ?? b.SeriesTime}`;
+  return compare(seriesDateA, seriesDateB) || compareSeriesUID(a, b);
+};
+
+export const defaultSeriesSort = (a, b) => {
   const seriesNumberA = a.SeriesNumber ?? a.seriesNumber;
   const seriesNumberB = b.SeriesNumber ?? b.seriesNumber;
-  if (seriesNumberA === seriesNumberB) {
-    return compareSeriesDateTime(a, b);
-  }
-  return seriesNumberA - seriesNumberB;
+  return compare(seriesNumberA, seriesNumberB) || compareSeriesDateTime(a, b);
 };
 
 /**
@@ -24,14 +82,14 @@ const defaultSeriesSort = (a, b) => {
  * @param {Object} firstSeries
  * @param {Object} secondSeries
  */
-function seriesInfoSortingCriteria(firstSeries, secondSeries) {
+export function seriesInfoSortingCriteria(firstSeries, secondSeries) {
   const aLowPriority = isLowPriorityModality(firstSeries.Modality ?? firstSeries.modality);
   const bLowPriority = isLowPriorityModality(secondSeries.Modality ?? secondSeries.modality);
 
   if (aLowPriority) {
     // Use the reverse sort order for low priority modalities so that the
     // most recent one comes up first as usually that is the one of interest.
-    return bLowPriority ? defaultSeriesSort(secondSeries, firstSeries) : 1;
+    return bLowPriority ? compareSeriesDateTime(secondSeries, firstSeries) : 1;
   } else if (bLowPriority) {
     return -1;
   }
@@ -39,44 +97,50 @@ function seriesInfoSortingCriteria(firstSeries, secondSeries) {
   return defaultSeriesSort(firstSeries, secondSeries);
 }
 
-const seriesSortCriteria = {
+export const seriesSortCriteria = {
   default: seriesInfoSortingCriteria,
   seriesInfoSortingCriteria,
+  compareSameSeries: compareSameSeriesDisplaySet,
+  compareSeriesDateTime,
+  compareSeriesUID,
 };
 
-const sortByInstanceNumber = (a, b) => {
-  // Sort by InstanceNumber (0020,0013)
+/**
+ * Compares two instances first by instance number, and then by
+ * sop and frame numbers.
+ * Handles undefined values for use with display set comparison.
+ */
+export const sortByInstanceNumber = (a, b) => {
+  if (!a || !b) {
+    return (!a && !b && 0) || (!a && -1) || 1;
+  }
   const aInstance = parseInt(a.InstanceNumber) || 0;
   const bInstance = parseInt(b.InstanceNumber) || 0;
   if (aInstance !== bInstance) {
     return (parseInt(a.InstanceNumber) || 0) - (parseInt(b.InstanceNumber) || 0);
   }
-  // Fallback rule to enable consistent sorting
-  if (a.SOPInstanceUID === b.SOPInstanceUID) {
-    return 0;
-  }
-  return a.SOPInstanceUID < b.SOPInstanceUID ? -1 : 1;
+  return compare(a.SOPInstanceUID, b.SOPInstanceUID) || compare(a.frameNumber, b.frameNumber);
 };
 
-const instancesSortCriteria = {
+export const instancesSortCriteria = {
   default: sortByInstanceNumber,
   sortByInstanceNumber,
 };
 
-const sortingCriteria = {
+export const sortingCriteria = {
   seriesSortCriteria,
   instancesSortCriteria,
 };
 
 /**
- * Sorts given series (given param is modified)
+ * Sorts given series or display sets
  * The default criteria is based on series number in ascending order.
  *
- * @param {Array} series List of series
- * @param {function} seriesSortingCriteria method for sorting
- * @returns {Array} sorted series object
+ * @param series -  List of series (modified in place)
+ * @param seriesSortingCriteria - method for sorting
+ * @returns sorted series object
  */
-const sortStudySeries = (
+export const sortStudySeries = (
   series,
   seriesSortingCriteria = seriesSortCriteria.default,
   sortFunction = null
@@ -96,7 +160,7 @@ const sortStudySeries = (
  * @param {function} instancesSortingCriteria method for sorting
  * @returns {Array} sorted instancesList object
  */
-const sortStudyInstances = (
+export const sortStudyInstances = (
   instancesList,
   instancesSortingCriteria = instancesSortCriteria.default
 ) => {
@@ -113,7 +177,7 @@ const sortStudyInstances = (
  * @param {function} [instancesSortingCriteria = instancesSortCriteria.default] method for sorting instances
  * @returns {Object} sorted study object
  */
-export default function sortStudy(
+export function sortStudy(
   study,
   deepSort = true,
   seriesSortingCriteria = seriesSortCriteria.default,
@@ -134,7 +198,7 @@ export default function sortStudy(
   return study;
 }
 
-function isValidForPositionSort(images): boolean {
+export function isValidForPositionSort(images): boolean {
   if (images.length <= 1) {
     return false; // No need to sort if there's only one image
   }
@@ -161,7 +225,7 @@ function isValidForPositionSort(images): boolean {
  *
  * @returns images - reference to images after sorting
  */
-const sortImagesByPatientPosition = images => {
+export const sortImagesByPatientPosition = images => {
   const referenceImagePositionPatient = images[0].ImagePositionPatient;
   const imageOrientationPatient = images[0].ImageOrientationPatient;
 
@@ -188,13 +252,4 @@ const sortImagesByPatientPosition = images => {
   return images;
 };
 
-export {
-  sortStudy,
-  sortStudySeries,
-  sortStudyInstances,
-  sortingCriteria,
-  seriesSortCriteria,
-  instancesSortCriteria,
-  isValidForPositionSort,
-  sortImagesByPatientPosition,
-};
+export default sortStudy;

platform/docs/docs/development/notes-requirements.md

@@ -0,0 +1,68 @@
+
+---
+sidebar_position: 14
+sidebar_label: Notes and Requirements
+title: Notes and Requirements for general OHIF behaviour
+summary: Specifies some of the expected behavior of OHIF generally
+---
+
+# Notes and Requirements
+
+This document just lists general notes and requirements for how OHIF behaves.
+The plan is to break this document down into a new sub-category once there
+are sufficient notes/requirements.
+
+
+## Series and Display Set Sorting `sortStudy.ts`
+
+Often a user will want to see a sorted list of series, or more generally
+a sorted list of display sets.  Series are the original data and can be split
+up into several display sets, although they are the same general sort of concept
+
+For example, an MR series might contain both T1 and T2 echos, and the T2 echo
+should occur after the T1.  Or, a single series might contain 4 mammography views:
+`LCC`, `RCC`, `LMLO`, `RMLO` with all the `CC` views shown first, and within
+that all the left views first for a given sub-type of CC view.
+
+To allow controlling that, the `sortStudy` can register sort functions
+that user used when two display sets come from the same series.  Between
+those display sets, the registration also registers a default ordering
+for that compare function.  Thus, the registration might look like:
+
+```javascript
+   addSameSeriesCompare('mammographyCompare', mammographyCompare, 5)
+   addSameSeriesCompare('mrT1T2Compare', mrT1T2Compare, 7);
+```
+
+Then, the display set for mr and mammography need to set the field `compareSameSeries`
+to the value `mammographyCompare`.
+
+```javascript
+   makeDisplaySet
+     ...
+     displaySet = {
+      ...,
+      compareSameSeries: 'mammographyCompare',
+```
+
+### Specifying Sort Order from Series Split
+
+The series split rules (`getSopClassHandlerModule`) can specify a custom order
+of display sets for the same series by adding a `sortVector`
+to the display set created.  Display sets which match on series instance uid
+are then compared using the sort vector.  The first element is the general sort
+order for this type of value among all other sort types, and must be numeric.
+The remainder of the values in the vector should be consistent for all
+sort vectors whose first value is the same value.
+
+For example, the mammography sort vector might have a primary value of '25',
+and then use the next three values for `view type`, `sub type` and `side`.
+It might also be true that "both" side views sort before everything and would be assigned
+a value less than `25` here.
+
+```
+   // LCC view
+   [25, 'CC', 'L', 'XO']
+   // BCC view
+   [24, 'CC', 'B']
+```