diff --git a/extensions/cornerstone-dicom-pmap/src/getSopClassHandlerModule.ts b/extensions/cornerstone-dicom-pmap/src/getSopClassHandlerModule.ts index 3764e96e1d1..558fd5745db 100644 --- a/extensions/cornerstone-dicom-pmap/src/getSopClassHandlerModule.ts +++ b/extensions/cornerstone-dicom-pmap/src/getSopClassHandlerModule.ts @@ -27,7 +27,7 @@ function _getDisplaySetsFromSeries( // The date/time of a display set is the date/time of the instance it shows, // chosen from all the attributes that instance carries. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(instance); const displaySet = { // Parametric map use to have the same modality as its referenced volume but diff --git a/extensions/cornerstone-dicom-rt/src/getSopClassHandlerModule.ts b/extensions/cornerstone-dicom-rt/src/getSopClassHandlerModule.ts index a9612f8faf4..aa81a118746 100644 --- a/extensions/cornerstone-dicom-rt/src/getSopClassHandlerModule.ts +++ b/extensions/cornerstone-dicom-rt/src/getSopClassHandlerModule.ts @@ -39,7 +39,7 @@ function _getDisplaySetsFromSeries( * usually the structure set date/time, and for one saved into an existing * series only the instance level date/time reflects the save. */ - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(instance); const displaySet = { Modality: 'RTSTRUCT', diff --git a/extensions/cornerstone-dicom-seg/src/commandsModule.ts b/extensions/cornerstone-dicom-seg/src/commandsModule.ts index 8b365a075cf..a4cf5ad6444 100644 --- a/extensions/cornerstone-dicom-seg/src/commandsModule.ts +++ b/extensions/cornerstone-dicom-seg/src/commandsModule.ts @@ -1,6 +1,6 @@ import dcmjs from 'dcmjs'; import { classes, Types, utils } from '@ohif/core'; -import { cache, Enums as csEnums, metaData } from '@cornerstonejs/core'; +import { cache, metaData } from '@cornerstonejs/core'; import { segmentation as cornerstoneToolsSegmentation } from '@cornerstonejs/tools'; import { adaptersRT, adaptersSEG } from '@cornerstonejs/adapters'; import { createReportDialogPrompt, useUIStateStore } from '@ohif/extension-default'; @@ -341,9 +341,12 @@ const commandsModule = ({ throw new Error('No segmentation found'); } - const { label, predecessorImageId } = segmentation; + const { label, predecessorImageId, labelIsGenerated } = segmentation; + // Only a name the user chose goes to `itemName`, which the dialog offers + // first; a generated one goes to `defaultSeriesDescription`, offered last. + const chosenLabel = labelIsGenerated ? '' : label || ''; const defaultSeriesDescription = - label || (modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'); + (labelIsGenerated && label) || (modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'); const { value: reportName, @@ -357,6 +360,7 @@ const commandsModule = ({ predecessorImageId, title: modality === 'RTSTRUCT' ? 'Save Contours' : 'Save Segmentation', modality, + itemName: chosenLabel, defaultSeriesDescription, enableDownload: true, }); @@ -383,7 +387,7 @@ const commandsModule = ({ options: { // Resolve store overrides against the data source we are storing into. dataSource: dataSourceName, - SeriesDescription: series ? undefined : reportName || defaultSeriesDescription, + SeriesDescription: series ? undefined : reportName || label || defaultSeriesDescription, SeriesNumber: series ? undefined : seriesNumber, predecessorImageId: series, }, @@ -399,27 +403,7 @@ const commandsModule = ({ const { dataset: naturalizedReport } = generatedData; - // The SEG adapter's `generateSegmentation` assigns the predecessor's - // series data to the derivation it returns rather than to the dataset - // inside it, so the stored instance would keep the series dcmjs made up - // for it - a new series named `Research Derived series` numbered 99, - // with no predecessor sequence - however this dialog was answered. - // Applying it to the dataset puts the instance in the series that was - // chosen, with that series' number and description. It is the same data - // the adapter resolves, so this stays correct once the adapter does. - if (series) { - Object.assign( - naturalizedReport, - metaData.get(csEnums.MetadataModules.PREDECESSOR_SEQUENCE, series) - ); - } - - // A segmentation saved into an existing series inherits that series' - // date and time, and its instance number is derived from the one - // predecessor instance, which is not necessarily the highest in the - // series. Stamp both so this segmentation is identifiable as the most - // recent instance. After the predecessor series data above, which is - // what names the series whose instances are numbered here. + // After the generation, which is what names the series numbered here. utils.updateNewInstanceMetadata(naturalizedReport); // DCMJS assigns a dummy study id during creation, and this can cause problems, so clearing it out diff --git a/extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts b/extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts index a44f2d08dd7..92db66096e1 100644 --- a/extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts +++ b/extensions/cornerstone-dicom-seg/src/getSopClassHandlerModule.ts @@ -261,7 +261,7 @@ function _getDisplaySetsFromSeries( // typically the content or structure set date/time rather than the series // one, and for a SEG saved into an existing series only the instance level // date/time reflects the save. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(instance); const displaySet = { Modality: 'SEG', diff --git a/extensions/cornerstone-dicom-sr/src/getSopClassHandlerModule.ts b/extensions/cornerstone-dicom-sr/src/getSopClassHandlerModule.ts index 898bb94de41..c001291db11 100644 --- a/extensions/cornerstone-dicom-sr/src/getSopClassHandlerModule.ts +++ b/extensions/cornerstone-dicom-sr/src/getSopClassHandlerModule.ts @@ -73,7 +73,7 @@ function addInstances(instances: InstanceMetadata[], _displaySetService: Display // still hold those of the first report saved into the series, so leaving // them would show, and summarize by, a date older than the report shown and // older than the position the series list has just sorted this one into. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(this.instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(this.instance); this.SeriesDate = SeriesDate; this.SeriesTime = SeriesTime; this.isLoaded = false; @@ -120,7 +120,7 @@ function _getDisplaySetsFromSeries( // The date/time of the display set is that of the instance it shows. A // report saved into an existing series keeps the original SeriesDate, so only // the instance level content date/time places it as the newest one. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(instance); const is3DSR = SOPClassUID === sopClassDictionary.Comprehensive3DSR; diff --git a/extensions/cornerstone/src/commandsModule.ts b/extensions/cornerstone/src/commandsModule.ts index 722f3d267a5..344c5fb99df 100644 --- a/extensions/cornerstone/src/commandsModule.ts +++ b/extensions/cornerstone/src/commandsModule.ts @@ -1873,6 +1873,7 @@ function commandsModule({ placeholder: i18n.t('Tools:Enter new label'), defaultValue: label, }).then(label => { + // The update clears `labelIsGenerated` - the user chose this label. segmentationService.addOrUpdateSegmentation({ segmentationId, label }); }); }, diff --git a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts index d5530dc9654..fec04a454ac 100644 --- a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts +++ b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts @@ -1242,6 +1242,8 @@ describe('SegmentationService', () => { info: 'S1: Series Description', }, label: 'Segmentation 2', + // The caller gave no label, so the service invented this one. + labelIsGenerated: true, fallbackLabel: 'S:1 SEG', segments: { '1': { @@ -1263,6 +1265,56 @@ describe('SegmentationService', () => { expect(retrievedSegmentationId).toEqual(expect.any(String)); }); + describe('labelIsGenerated', () => { + // `storeSegmentation` offers the label of a segmentation as the first name + // for a new series, but only when the user chose that name. The service + // marks the label that the service invents, so the save can tell the two + // apart without a comparison of two strings. The mark goes into the + // public input, and `normalizeSegmentationInput` puts the mark on the + // segmentation; the service writes nothing onto the state afterwards. + const displaySet = { + imageIds: ['imageId'], + isDynamicVolume: false, + SeriesNumber: 1, + SeriesDescription: 'Series Description', + Modality: 'SEG', + } as unknown as AppTypes.DisplaySet; + + const createWith = async (options?: Record) => { + jest + .spyOn(imageLoader, 'createAndCacheDerivedLabelmapImages') + .mockReturnValue([{ imageId: 'imageId' }] as csTypes.IImage[]); + jest + .spyOn(cstSegmentation.state, 'getSegmentations') + .mockReturnValue([{ segmentationId: 'segmentationId' }] as cstTypes.Segmentation[]); + const add = jest.spyOn(service, 'addOrUpdateSegmentation').mockReturnValue(undefined); + + await service.createLabelmapForDisplaySet(displaySet, options); + return (add.mock.calls[0][0] as cstTypes.SegmentationPublicInput).config; + }; + + it('marks a label that the service invents', async () => { + const config = await createWith(); + + expect(config.label).toBe('Segmentation 2'); + expect(config.labelIsGenerated).toBe(true); + }); + + it('marks nothing for a label that the caller gives', async () => { + const config = await createWith({ label: 'Liver' }); + + expect(config.label).toBe('Liver'); + expect(config.labelIsGenerated).toBe(false); + }); + + it('marks a label that the caller reports as generated', async () => { + const config = await createWith({ label: 'Segmentation 7', labelIsGenerated: true }); + + expect(config.label).toBe('Segmentation 7'); + expect(config.labelIsGenerated).toBe(true); + }); + }); + it('should create a labelmap for a dynamic volume display set', async () => { const segmentationId = 'segmentationId'; const displaySet = { @@ -1311,6 +1363,8 @@ describe('SegmentationService', () => { info: 'S1: Series Description', }, label: 'Segmentation 2', + // The caller gave the label, so the label is not a generated one. + labelIsGenerated: false, fallbackLabel: 'S:1 SEG', segments: { '1': { diff --git a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts index e0a54fe37e7..5dbc7bf1c16 100644 --- a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts +++ b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts @@ -428,6 +428,8 @@ class SegmentationService extends PubSubService implements ISegmentationServiceI segments?: { [segmentIndex: number]: Partial }; FrameOfReferenceUID?: string; label?: string; + /** The caller invented the label, so the user has not chosen a name. */ + labelIsGenerated?: boolean; } ): Promise { return this._createSegmentationForDisplaySet(displaySet, LABELMAP, options); @@ -440,6 +442,8 @@ class SegmentationService extends PubSubService implements ISegmentationServiceI segments?: { [segmentIndex: number]: Partial }; FrameOfReferenceUID?: string; label?: string; + /** The caller invented the label, so the user has not chosen a name. */ + labelIsGenerated?: boolean; } ): Promise { return this._createSegmentationForDisplaySet(displaySet, CONTOUR, options); @@ -461,6 +465,8 @@ class SegmentationService extends PubSubService implements ISegmentationServiceI segments?: { [segmentIndex: number]: Partial }; FrameOfReferenceUID?: string; label?: string; + /** The caller invented the label, so the user has not chosen a name. */ + labelIsGenerated?: boolean; } ): Promise { // Todo: random does not makes sense, make this better, like @@ -496,6 +502,8 @@ class SegmentationService extends PubSubService implements ISegmentationServiceI }, config: { label, + // Explicit, because `label` below always has a value by this point. + labelIsGenerated: options?.labelIsGenerated ?? !options?.label, fallbackLabel: `S:${displaySet.SeriesNumber} ${displaySet.Modality}`, segments: options?.segments && Object.keys(options.segments).length > 0 diff --git a/extensions/cornerstone/src/utils/createSegmentationForViewport.ts b/extensions/cornerstone/src/utils/createSegmentationForViewport.ts index 24e40e867ab..a0bb977b31e 100644 --- a/extensions/cornerstone/src/utils/createSegmentationForViewport.ts +++ b/extensions/cornerstone/src/utils/createSegmentationForViewport.ts @@ -64,6 +64,7 @@ export async function createSegmentationForViewport( const segmentationCreationOptions = { label, segmentationId, + labelIsGenerated: !options.label, segments: _createDefaultSegments(options.createInitialSegment), }; diff --git a/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js b/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js index 9c3c5901fdc..680d10ea42c 100644 --- a/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js +++ b/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js @@ -82,10 +82,27 @@ export function retrieveStudyMetadata( * Delete the cached study metadata retrieval promise to ensure that the browser will * re-retrieve the study metadata when it is next requested. * + * Callers know only the study, so this matches every + * `:` key rather than looking the bare UID + * up as one. + * * @param {String} StudyInstanceUID The UID of the Study to be removed from cache */ export function deleteStudyMetadataPromise(StudyInstanceUID) { - if (StudyMetaDataPromises.has(StudyInstanceUID)) { - StudyMetaDataPromises.delete(StudyInstanceUID); + if (!StudyInstanceUID) { + return; } + + const suffix = `:${StudyInstanceUID}`; + + for (const promiseId of [...StudyMetaDataPromises.keys()]) { + if (promiseId === StudyInstanceUID || promiseId.endsWith(suffix)) { + StudyMetaDataPromises.delete(promiseId); + } + } +} + +/** Test seam: the cached promises, so a test can assert what invalidation removed. */ +export function _getStudyMetadataPromiseCache() { + return StudyMetaDataPromises; } diff --git a/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.test.js b/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.test.js new file mode 100644 index 00000000000..c88724827c0 --- /dev/null +++ b/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.test.js @@ -0,0 +1,75 @@ +import { + deleteStudyMetadataPromise, + _getStudyMetadataPromiseCache, +} from './retrieveStudyMetadata.js'; + +const STUDY = '1.2.840.113619.2.55.3.1234'; +const OTHER_STUDY = '9.9.9'; + +describe('deleteStudyMetadataPromise', () => { + beforeEach(() => { + _getStudyMetadataPromiseCache().clear(); + }); + + // Promises are cached under `:`, but every + // caller holds only the study UID. Looking the bare UID up as a key matched + // nothing, so storing a derived artifact never invalidated anything and a + // re-retrieve returned the pre-save promise. + it('removes the promise cached under the data source qualified key', () => { + const cache = _getStudyMetadataPromiseCache(); + cache.set(`dicomweb:${STUDY}`, 'stale'); + + deleteStudyMetadataPromise(STUDY); + + expect(cache.has(`dicomweb:${STUDY}`)).toBe(false); + }); + + it('removes the study from every data source that cached it', () => { + const cache = _getStudyMetadataPromiseCache(); + cache.set(`dicomweb:${STUDY}`, 'stale'); + cache.set(`dicomwebproxy:${STUDY}`, 'stale'); + + deleteStudyMetadataPromise(STUDY); + + expect(cache.size).toBe(0); + }); + + it('leaves other studies cached', () => { + const cache = _getStudyMetadataPromiseCache(); + cache.set(`dicomweb:${STUDY}`, 'stale'); + cache.set(`dicomweb:${OTHER_STUDY}`, 'keep'); + + deleteStudyMetadataPromise(STUDY); + + expect(cache.has(`dicomweb:${OTHER_STUDY}`)).toBe(true); + }); + + // A study whose UID is a suffix of another must not be caught by the match. + it('does not remove a study whose UID merely ends with the same digits', () => { + const cache = _getStudyMetadataPromiseCache(); + cache.set(`dicomweb:${STUDY}`, 'stale'); + cache.set(`dicomweb:77${STUDY}`, 'keep'); + + deleteStudyMetadataPromise(STUDY); + + expect(cache.has(`dicomweb:77${STUDY}`)).toBe(true); + }); + + it('still removes an unqualified key, for any caller that cached one', () => { + const cache = _getStudyMetadataPromiseCache(); + cache.set(STUDY, 'stale'); + + deleteStudyMetadataPromise(STUDY); + + expect(cache.has(STUDY)).toBe(false); + }); + + it('does nothing without a study', () => { + const cache = _getStudyMetadataPromiseCache(); + cache.set(`dicomweb:${STUDY}`, 'keep'); + + deleteStudyMetadataPromise(undefined); + + expect(cache.size).toBe(1); + }); +}); diff --git a/extensions/default/src/Panels/createReportDialogPrompt.tsx b/extensions/default/src/Panels/createReportDialogPrompt.tsx index 75f342cbe73..c5b486650ab 100644 --- a/extensions/default/src/Panels/createReportDialogPrompt.tsx +++ b/extensions/default/src/Panels/createReportDialogPrompt.tsx @@ -11,9 +11,11 @@ import PROMPT_RESPONSES from '../utils/_shared/PROMPT_RESPONSES'; * from. That is the series the dialog offers to extend, and it defaults to * extending it instead of creating a new series. Without one, the dialog * only offers to create a new series. - * - `defaultSeriesDescription` is the series description offered when a new - * series is being created, typically the name of the thing being saved such - * as the segmentation name or 'Contours'. + * - `itemName` is the name the user chose, such as the segmentation name. A + * new series offers it first. A generated name belongs in + * `defaultSeriesDescription` instead. + * - `defaultSeriesDescription` is the name for an item with no chosen name, + * such as 'Contours' or 'Measurements'. A new series offers it last. * - `itemType` is the type of item being stored, used as the key that the * series descriptions used before are remembered under. Defaults to the * modality, so that segmentations, contours and reports are remembered @@ -22,14 +24,12 @@ import PROMPT_RESPONSES from '../utils/_shared/PROMPT_RESPONSES'; * descriptions to remember and offer for this type of item, 0 to remember * none of them. * - * The dialog offers exactly two destinations, and says which one is in effect: - * - `New Series` creates a new series, with an editable series number - * (defaulting to one past the existing series of this modality) and series - * description. The description defaults to the one last used for this type - * of item, or to `defaultSeriesDescription` when there isn't one, and both - * are offered as completions of what gets typed. - * - `Extend Existing` stores into the series the data was loaded from, which - * keeps its own series number and description, so neither is editable. + * The dialog offers three destinations - `Save to current`, `Save as new` and + * `Replace existing` - and says which one is in effect. Each destination stores + * all of the current data as one object, and the dialog merges nothing. The + * behaviour doc describes the destinations, the series that the dialog offers, + * and the names for a new series: + * `platform/docs/docs/behaviours/report-dialog-save-destinations.md`. * * The response is: * - `value`, the series description of the object/series being created. When @@ -52,6 +52,7 @@ export default function CreateReportDialogPrompt({ modality = 'SR', minSeriesNumber = 0, predecessorImageId, + itemName = '', defaultSeriesDescription = '', itemType, rememberedDescriptionCount = 5, @@ -90,6 +91,7 @@ export default function CreateReportDialogPrompt({ dataSources: allowMultipleDataSources ? dataSources : undefined, predecessorImageId, minSeriesNumber, + itemName, defaultSeriesDescription, itemType, rememberedDescriptionCount, diff --git a/extensions/default/src/SOPClassHandlers/chartSOPClassHandler.ts b/extensions/default/src/SOPClassHandlers/chartSOPClassHandler.ts index d6db0c628d1..65915705fd8 100644 --- a/extensions/default/src/SOPClassHandlers/chartSOPClassHandler.ts +++ b/extensions/default/src/SOPClassHandlers/chartSOPClassHandler.ts @@ -25,7 +25,7 @@ const makeChartDataDisplaySet = (instance, sopClassUids) => { // The date/time of a display set is the date/time of the instance it shows, // chosen from all the attributes that instance carries. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(instance); return { Modality: CHART_MODALITY, @@ -58,7 +58,7 @@ const makeChartDataDisplaySet = (instance, sopClassUids) => { // The date/time shown and sorted by is that of the instance the display // set shows, so it moves with that instance rather than staying on the // one the chart was first created with. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(this.instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(this.instance); this.SeriesDate = SeriesDate; this.SeriesTime = SeriesTime; diff --git a/extensions/default/src/customizations/reportDialogCustomization.test.ts b/extensions/default/src/customizations/reportDialogCustomization.test.ts index eb6a5b6003e..4225e0e477c 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.test.ts +++ b/extensions/default/src/customizations/reportDialogCustomization.test.ts @@ -114,6 +114,25 @@ const UNRELATED_SERIES = { SeriesDescription: 'Axial', }; +// An uploaded instance carries a local id, which the provider resolves. +const LOCAL_SERIES = { + displaySetInstanceUID: 'ds-local', + Modality: 'SEG', + SeriesInstanceUID: '1.2.6', + SeriesNumber: 3107, + SeriesDescription: 'Kidney', + predecessorImageId: 'dicomfile:3', +}; + +// The viewer never stored this series, so the series has no predecessor image id. +const DOWNLOADED_SERIES = { + displaySetInstanceUID: 'ds-downloaded', + Modality: 'SEG', + SeriesInstanceUID: '1.2.7', + SeriesNumber: 3108, + SeriesDescription: 'Pancreas', +}; + const HISTORY_STORAGE_KEY = 'ohif.seriesDescriptionHistory'; function setDisplaySets(displaySets) { @@ -242,6 +261,15 @@ describe('ReportDialog', () => { expect(seriesNumberField().value).toBe('3101'); }); + it('counts a series that it does not offer as a destination', () => { + // The lists do not offer series 3108, because that series has no + // predecessor image id. A new series must still take a number past 3108. + setDisplaySets([CURRENT_SERIES, DOWNLOADED_SERIES]); + renderDialog(); + + expect(seriesNumberField().value).toBe('3109'); + }); + it('saves an edited series number and description', () => { const { onSave } = renderDialog(); @@ -314,6 +342,45 @@ describe('ReportDialog', () => { expect(screen.getByText('Series 3103')).toBeTruthy(); }); + + it('does not offer a series that no predecessor image id names', () => { + setDisplaySets([CURRENT_SERIES, OTHER_SERIES, LOCAL_SERIES, DOWNLOADED_SERIES]); + renderDialog({ predecessorImageId: CURRENT_SERIES_IMAGE_ID }); + + fireEvent.click(tab('replace')); + + expect(screen.getByText('Spleen')).toBeTruthy(); + // An uploaded instance carries a local id, which the provider resolves. + expect(screen.getByText('Kidney')).toBeTruthy(); + // The list gave the `SeriesInstanceUID` of this display set before, which + // is not an image id, and the provider then threw a `TypeError`. + expect(screen.queryByText('Pancreas')).toBeNull(); + }); + + it('cannot replace when the only other series has no predecessor image id', () => { + setDisplaySets([CURRENT_SERIES, DOWNLOADED_SERIES]); + renderDialog({ predecessorImageId: CURRENT_SERIES_IMAGE_ID }); + + expect(isDisabled(tab('replace'))).toBe(true); + }); + + it('stores into an uploaded series through its local image id', () => { + // A user must be able to save against an uploaded instance more than once. + setDisplaySets([LOCAL_SERIES]); + const { onSave } = renderDialog(); + + fireEvent.click(tab('replace')); + fireEvent.click(screen.getByText('Kidney')); + fireEvent.click(saveButton()); + + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ + reportName: 'Kidney', + series: 'dicomfile:3', + seriesNumber: 3107, + }) + ); + }); }); describe('remembered series descriptions', () => { @@ -326,6 +393,29 @@ describe('ReportDialog', () => { expect(storedHistory()).toEqual({ SEG: ['Right kidney'] }); }); + it('remembers nothing for the name that the caller provides', () => { + // `Segmentation 1` is the generated name of this segmentation. A later + // segmentation carries `Segmentation 2`, and the history would otherwise + // offer `Segmentation 1` as the name of that unrelated segmentation. + const { onSave } = renderDialog(); + + fireEvent.click(saveButton()); + + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ reportName: 'Segmentation 1' }) + ); + expect(storedHistory()).toEqual({}); + }); + + it('remembers nothing when the typed name is the provided one', () => { + renderDialog({ defaultSeriesDescription: 'Contours' }); + + fireEvent.change(descriptionField(), { target: { value: ' contours ' } }); + fireEvent.click(saveButton()); + + expect(storedHistory()).toEqual({}); + }); + it('remembers nothing when an existing series is stored into', () => { renderDialog({ predecessorImageId: CURRENT_SERIES_IMAGE_ID }); @@ -342,8 +432,120 @@ describe('ReportDialog', () => { expect(descriptionField().value).toBe('Right kidney'); fireEvent.click(screen.getByTestId('report-series-description-options')); - // The provided description first, then the ones used before it. - expect(shownDescriptions()).toEqual(['Segmentation 1', 'Right kidney', 'Left kidney']); + // The ones used before, most recent first, then the generic name. + expect(shownDescriptions()).toEqual(['Right kidney', 'Left kidney', 'Segmentation 1']); + }); + + it('offers the item name before every other description', () => { + // The user renamed `Liver` to `Liver + tumor` before the save. + setStoredHistory({ SEG: ['Right kidney', 'Left kidney'] }); + renderDialog({ predecessorImageId: CURRENT_SERIES_IMAGE_ID, itemName: 'Liver + tumor' }); + + fireEvent.click(tab('new')); + + expect(descriptionField().value).toBe('Liver + tumor'); + + fireEvent.click(screen.getByTestId('report-series-description-options')); + expect(shownDescriptions()).toEqual([ + 'Liver + tumor', + 'Liver', + 'Right kidney', + 'Left kidney', + 'Segmentation 1', + ]); + }); + + it('ignores an item name of spaces', () => { + // A blank name must not hide the description of the loaded series. + setStoredHistory({ SEG: ['Right kidney'] }); + renderDialog({ predecessorImageId: CURRENT_SERIES_IMAGE_ID, itemName: ' ' }); + + fireEvent.click(tab('new')); + + expect(descriptionField().value).toBe('Liver'); + }); + + it('offers the description of the loaded series when there is no item name', () => { + // The name of the last save comes before the remembered names. + setStoredHistory({ SEG: ['Right kidney', 'Left kidney'] }); + renderDialog({ predecessorImageId: CURRENT_SERIES_IMAGE_ID }); + + fireEvent.click(tab('new')); + + expect(descriptionField().value).toBe('Liver'); + + fireEvent.click(screen.getByTestId('report-series-description-options')); + expect(shownDescriptions()).toEqual([ + 'Liver', + 'Right kidney', + 'Left kidney', + 'Segmentation 1', + ]); + }); + + it('offers the last used description when the caller provides none', () => { + // A caller such as the findings flow of a fork provides no description. + setStoredHistory({ SEG: ['Right kidney', 'Left kidney'] }); + renderDialog({ defaultSeriesDescription: '' }); + + expect(descriptionField().value).toBe('Right kidney'); + }); + + it('saves when the caller provides a null description', () => { + // A mode context, or a customization, can give an explicit null, and the + // default of the prop replaces `undefined` alone. A save read `.trim()` + // off that null and threw, and the save then stored nothing. + setStoredHistory({ SEG: ['Right kidney'] }); + const { onSave } = renderDialog({ defaultSeriesDescription: null }); + + fireEvent.click(saveButton()); + + expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Right kidney' })); + expect(storedHistory()).toEqual({ SEG: ['Right kidney'] }); + }); + + it('drops the outer spaces of an offered name', () => { + // The dialog shows the trimmed name, so the save stores the trimmed name + // as well, and the history holds the same name as the series. + const { onSave } = renderDialog({ itemName: ' Right kidney ' }); + + expect(descriptionField().value).toBe('Right kidney'); + + fireEvent.click(saveButton()); + + expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Right kidney' })); + expect(storedHistory()).toEqual({ SEG: ['Right kidney'] }); + }); + + it('drops the outer spaces of the name an emptied field falls back to', () => { + const { onSave } = renderDialog({ itemName: ' Right kidney ' }); + + fireEvent.change(descriptionField(), { target: { value: ' ' } }); + fireEvent.click(saveButton()); + + expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Right kidney' })); + }); + + it('drops the second copy of a description that two sources hold', () => { + // The history holds the generic name, so the list holds that name once. + setStoredHistory({ SEG: ['Segmentation 1', 'Left kidney'] }); + renderDialog(); + + expect(descriptionField().value).toBe('Segmentation 1'); + + fireEvent.click(screen.getByTestId('report-series-description-options')); + expect(shownDescriptions()).toEqual(['Segmentation 1', 'Left kidney']); + }); + + it('falls back to the offered description when the field is emptied', () => { + setStoredHistory({ SEG: ['Right kidney'] }); + const { onSave } = renderDialog(); + + fireEvent.change(descriptionField(), { target: { value: ' ' } }); + fireEvent.click(saveButton()); + + // The name the field offered, and not the generic name behind it. + expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Right kidney' })); }); it('moves a reused description back to the front, without duplicating it', () => { @@ -377,14 +579,55 @@ describe('ReportDialog', () => { expect(storedHistory()).toEqual({ SEG: ['Right kidney'] }); }); + it('still starts from the item name for a count of 0', () => { + setStoredHistory({ SEG: ['Right kidney'] }); + renderDialog({ rememberedDescriptionCount: 0, itemName: 'Liver + tumor' }); + + expect(descriptionField().value).toBe('Liver + tumor'); + expect(screen.queryByTestId('report-series-description-options')).toBeNull(); + }); + + it('ignores an item name of spaces for a count of 0', () => { + // A blank name must not empty the field, and must not hide `Liver`. + renderDialog({ + rememberedDescriptionCount: 0, + itemName: ' ', + predecessorImageId: CURRENT_SERIES_IMAGE_ID, + }); + + fireEvent.click(tab('new')); + + expect(descriptionField().value).toBe('Liver'); + expect(screen.queryByTestId('report-series-description-options')).toBeNull(); + }); + + it('offers the default when the current description is blank, for a count of 0', () => { + // A blank loaded description must fall through to the generic name. + setDisplaySets([{ ...CURRENT_SERIES, SeriesDescription: ' ' }]); + const { onSave } = renderDialog({ + rememberedDescriptionCount: 0, + predecessorImageId: CURRENT_SERIES_IMAGE_ID, + }); + + fireEvent.click(tab('new')); + expect(descriptionField().value).toBe('Segmentation 1'); + + fireEvent.change(descriptionField(), { target: { value: ' ' } }); + fireEvent.click(saveButton()); + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ reportName: 'Segmentation 1' }) + ); + }); + it('keeps each type of item separate', () => { setStoredHistory({ SEG: ['Right kidney'] }); renderDialog({ modality: 'RTSTRUCT', defaultSeriesDescription: 'Contours' }); expect(descriptionField().value).toBe('Contours'); + fireEvent.change(descriptionField(), { target: { value: 'Left lung' } }); fireEvent.click(saveButton()); - expect(storedHistory()).toEqual({ SEG: ['Right kidney'], RTSTRUCT: ['Contours'] }); + expect(storedHistory()).toEqual({ SEG: ['Right kidney'], RTSTRUCT: ['Left lung'] }); }); it('narrows the offered descriptions to what is being typed', () => { @@ -426,11 +669,12 @@ describe('ReportDialog', () => { fireEvent.keyDown(descriptionField(), { key: 'ArrowDown' }); fireEvent.keyDown(descriptionField(), { key: 'Enter' }); - expect(descriptionField().value).toBe('Right kidney'); + // The field starts from the first entry, so the arrow key picks the second. + expect(descriptionField().value).toBe('Left kidney'); expect(onSave).not.toHaveBeenCalled(); fireEvent.keyDown(descriptionField(), { key: 'Enter' }); - expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Right kidney' })); + expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Left kidney' })); }); }); diff --git a/extensions/default/src/customizations/reportDialogCustomization.tsx b/extensions/default/src/customizations/reportDialogCustomization.tsx index 077dbf2fe4a..af93a79aa28 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -22,6 +22,11 @@ import { rememberSeriesDescription, } from '../utils/seriesDescriptionHistory'; +/** + * The dialog that stores a segmentation, a contour set or a measurement report. + * See `platform/docs/docs/behaviours/report-dialog-save-destinations.md`. + */ + type DataSource = { value: string; label: string; @@ -47,11 +52,7 @@ type ExistingSeries = { * - `new` a series created for it * - `replace` another loaded series of this modality * - * All three store the same object - all of the current data, as selected from - * the service holding it. The destination only decides which series that object - * belongs to, and so which instance it supersedes. Nothing is merged: storing - * into a series that already has data neither loads that data to add to it, nor - * leaves any of the current data out. + * All three store the same object, and the dialog merges nothing. */ type Destination = 'current' | 'new' | 'replace'; @@ -93,8 +94,14 @@ type ReportDialogProps = { /** Lowest series number to use for a newly created series of this modality. */ minSeriesNumber?: number; /** - * Series description to offer when creating a new series - typically the name - * of the object being saved, such as the segmentation name or 'Contours'. + * The name that the user chose for the item - the segmentation name, for + * example. A new series offers this name first. A generated name is not such + * a name, and belongs in `defaultSeriesDescription`. + */ + itemName?: string; + /** + * The name for an item that has no other name, such as 'Contours' or + * 'Measurements'. A new series offers this name last. */ defaultSeriesDescription?: string; /** @@ -124,6 +131,7 @@ function ReportDialog({ modality = 'SR', predecessorImageId, minSeriesNumber = 3000, + itemName = '', defaultSeriesDescription = '', itemType, rememberedDescriptionCount = 5, @@ -141,18 +149,24 @@ function ReportDialog({ ); const { displaySetService } = servicesManager.services; + /** Every loaded display set of the stored modality. */ + const modalityDisplaySets = useMemo( + () => + Array.from(displaySetService.getDisplaySetCache().values()).filter( + ds => ds.Modality === modality + ), + [displaySetService, modality] + ); + const existingSeries = useMemo((): ExistingSeries[] => { - const displaySetsMap = displaySetService.getDisplaySetCache(); - const displaySets = Array.from(displaySetsMap.values()); const seen = new Set(); - return displaySets - .filter(ds => ds.Modality === modality) + return modalityDisplaySets .map(ds => { const hasSeriesNumber = isFinite(ds.SeriesNumber); const seriesNumberLabel = hasSeriesNumber ? `${ds.SeriesNumber}` : 'Not specified'; return { - value: ds.predecessorImageId || ds.SeriesInstanceUID, + value: ds.predecessorImageId, seriesNumber: hasSeriesNumber ? Number(ds.SeriesNumber) : minSeriesNumber, seriesNumberLabel, description: ds.SeriesDescription || null, @@ -160,15 +174,15 @@ function ReportDialog({ }; }) .filter(series => { - // Two display sets of one series would otherwise both be offered, and - // the select needs unique values. + // The value names the superseded instance, so a display set without one + // cannot be a target. Two display sets can share a series. if (!series.value || seen.has(series.value)) { return false; } seen.add(series.value); return true; }); - }, [displaySetService, modality, minSeriesNumber]); + }, [modalityDisplaySets, minSeriesNumber]); /** * The series the data was loaded from, which `Save to current` writes into. @@ -185,35 +199,56 @@ function ReportDialog({ [existingSeries, currentSeries] ); - /** The series number offered for a new series - one past the existing ones. */ + /** One past every loaded series of the modality, not only the offered ones. */ const defaultNewSeriesNumber = useMemo( - () => 1 + Math.max(minSeriesNumber, ...existingSeries.map(series => series.seriesNumber)), - [existingSeries, minSeriesNumber] + () => + 1 + + modalityDisplaySets.reduce( + (highest, ds) => + isFinite(ds.SeriesNumber) ? Math.max(highest, Number(ds.SeriesNumber)) : highest, + minSeriesNumber + ), + [modalityDisplaySets, minSeriesNumber] ); /** - * The series descriptions to offer for a new series, being the one provided - * for this data followed by the ones last used for this type of item. The - * most recently used one is what gets offered in the field, as it is the most - * likely one to want again. + * The series descriptions to offer for a new series: `itemName`, then the + * description of `currentSeries`, then the ones used before for this type of + * item, then `defaultSeriesDescription`. */ const descriptionOptions = useMemo(() => { - const history = getSeriesDescriptionHistory(itemType || modality, rememberedDescriptionCount); - const options = [defaultSeriesDescription, ...history].filter(option => !!option?.trim()); - - return options.filter( + // A count of 0 turns the history off, and the list then holds one name. + const remembered = + rememberedDescriptionCount > 0 + ? getSeriesDescriptionHistory(itemType || modality, rememberedDescriptionCount) + : []; + + // Trimmed here, so the stored, shown and remembered names all match. + const offered = [itemName, currentSeries?.description, ...remembered, defaultSeriesDescription] + .map(option => option?.trim()) + .filter((option): option is string => !!option); + + const options = offered.filter( (option, index) => - options.findIndex(other => other.toLowerCase() === option.toLowerCase()) === index + offered.findIndex(other => other.toLowerCase() === option.toLowerCase()) === index ); - }, [defaultSeriesDescription, itemType, modality, rememberedDescriptionCount]); + + return rememberedDescriptionCount > 0 ? options : options.slice(0, 1); + }, [ + currentSeries, + itemName, + defaultSeriesDescription, + itemType, + modality, + rememberedDescriptionCount, + ]); + + /** The name a new series starts from, and the fallback for an emptied field. */ + const baseSeriesDescription = descriptionOptions[0] ?? ''; const [destination, setDestination] = useState(currentSeries ? 'current' : 'new'); const [newSeriesNumber, setNewSeriesNumber] = useState(String(defaultNewSeriesNumber)); - const [newSeriesDescription, setNewSeriesDescription] = useState( - // The provided description is the first option, so anything after it is a - // remembered one, and the first of those was the last one used. - () => descriptionOptions[1] ?? defaultSeriesDescription - ); + const [newSeriesDescription, setNewSeriesDescription] = useState(baseSeriesDescription); const [replacedSeriesValue, setReplacedSeriesValue] = useState(null); const [descriptionsOpen, setDescriptionsOpen] = useState(false); // Typing narrows the list to what it can complete; opening the list from its @@ -279,9 +314,14 @@ function ReportDialog({ // the user to have changed there. const storedDescription = targetSeries ? (targetSeries.description ?? '') - : newSeriesDescription.trim() || defaultSeriesDescription; + : newSeriesDescription.trim() || baseSeriesDescription; + + // The history keeps only a name the user chose, so the offered default + // does not consume a slot. + const isProvidedName = + storedDescription.toLowerCase() === defaultSeriesDescription?.trim().toLowerCase(); - if (!targetSeries) { + if (!targetSeries && !isProvidedName) { rememberSeriesDescription( itemType || modality, storedDescription, @@ -305,6 +345,7 @@ function ReportDialog({ targetSeries, seriesNumber, newSeriesDescription, + baseSeriesDescription, defaultSeriesDescription, itemType, modality, diff --git a/extensions/default/src/customizations/thumbnailDetailsCustomization.ts b/extensions/default/src/customizations/thumbnailDetailsCustomization.ts index 1d37d0e3985..0a648dd87e9 100644 --- a/extensions/default/src/customizations/thumbnailDetailsCustomization.ts +++ b/extensions/default/src/customizations/thumbnailDetailsCustomization.ts @@ -1,6 +1,6 @@ import { utils } from '@ohif/core'; -const { getSeriesDateTime } = utils; +const { getLatestInstanceDateTime } = utils; /** * Named sources for the study browser thumbnail detail items, so an item can @@ -22,7 +22,7 @@ export const thumbnailDetailSources = { /** * When the display set was created, which is the date/time the series list is - * sorted by - see `getSeriesDateTime`. Without this on the thumbnail, several + * sorted by - see `getLatestInstanceDateTime`. Without this on the thumbnail, several * reports or segmentations saved on the same day all read as the same date and * their order looks arbitrary. * @@ -30,7 +30,7 @@ export const thumbnailDetailSources = { * a reader can use, and it is not reliably recorded either. */ instanceDateTime: ({ displaySet, instance, formatters }) => { - const { SeriesDate, SeriesTime } = getSeriesDateTime(instance ?? displaySet); + const { SeriesDate, SeriesTime } = getLatestInstanceDateTime(instance ?? displaySet); if (!SeriesDate) { return ''; } diff --git a/extensions/default/src/utils/getCurrentDicomDateTime.ts b/extensions/default/src/utils/getCurrentDicomDateTime.ts deleted file mode 100644 index 2266998eb28..00000000000 --- a/extensions/default/src/utils/getCurrentDicomDateTime.ts +++ /dev/null @@ -1,20 +0,0 @@ -export const getSeriesDateTime = (jsDate: Date = new Date()) => { - const dicomDateTime = getDicomDateTime(jsDate); - return { - SeriesDate: dicomDateTime.date, - SeriesTime: dicomDateTime.time, - }; -}; - -export const getDicomDateTime = (jsDate: Date = new Date()) => { - const month = String(jsDate.getUTCMonth() + 1).padStart(2, '0'); - const day = String(jsDate.getUTCDate()).padStart(2, '0'); - const year = String(jsDate.getUTCFullYear()).padStart(4, '0'); - const date = `${year}${month}${day}`; - const hours = String(jsDate.getUTCHours()).padStart(2, '0'); - const minutes = String(jsDate.getUTCMinutes()).padStart(2, '0'); - const seconds = String(jsDate.getUTCSeconds()).padStart(2, '0'); - const time = `${hours}${minutes}${seconds}`; - - return { date, time }; -}; diff --git a/extensions/dicom-pdf/src/getSopClassHandlerModule.js b/extensions/dicom-pdf/src/getSopClassHandlerModule.js index 4a695fd8879..c1e1ccb4091 100644 --- a/extensions/dicom-pdf/src/getSopClassHandlerModule.js +++ b/extensions/dicom-pdf/src/getSopClassHandlerModule.js @@ -17,7 +17,7 @@ const _getDisplaySetsFromSeries = (instances, servicesManager, extensionManager) const { SeriesNumber, SeriesInstanceUID, StudyInstanceUID, SOPClassUID } = instance; // The date/time of a display set is the date/time of the instance it shows, // chosen from all the attributes that instance carries. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(instance); // The declared type is only a claim. It is resolved against the displayable // type allowlist, and the payload is re-wrapped in a Blob of the canonical // type, so the instance cannot steer how the browser parses the document. diff --git a/extensions/dicom-video/src/getSopClassHandlerModule.js b/extensions/dicom-video/src/getSopClassHandlerModule.js index 6a67019e0d5..df18919d9ef 100644 --- a/extensions/dicom-video/src/getSopClassHandlerModule.js +++ b/extensions/dicom-video/src/getSopClassHandlerModule.js @@ -58,7 +58,7 @@ const _getDisplaySetsFromSeries = (instances, servicesManager, extensionManager) const { SeriesNumber, SeriesInstanceUID, StudyInstanceUID, NumberOfFrames, url } = instance; // The date/time of a display set is the date/time of the instance it // shows, chosen from all the attributes that instance carries. - const { SeriesDate, SeriesTime } = utils.getSeriesDateTime(instance); + const { SeriesDate, SeriesTime } = utils.getLatestInstanceDateTime(instance); const videoUrlParams = { instance, singlepart: 'video', diff --git a/extensions/tmtv/src/commandsModule.ts b/extensions/tmtv/src/commandsModule.ts index 9d457c2f13e..04da9efa2c9 100644 --- a/extensions/tmtv/src/commandsModule.ts +++ b/extensions/tmtv/src/commandsModule.ts @@ -146,6 +146,7 @@ const commandsModule = ({ servicesManager, commandsManager, extensionManager }: const segmentationId = await segmentationService.createLabelmapForDisplaySet(displaySet, { label: `Segmentation ${currentSegmentations.length + 1}`, + labelIsGenerated: true, segments: { 1: { label: `${i18n.t('Segment')} 1`, active: true } }, }); diff --git a/platform/app/public/customizations/studyBrowser/derivedDateTime.jsonc b/platform/app/public/customizations/studyBrowser/derivedDateTime.jsonc index eb409c4ccc6..ce31b3c4910 100644 --- a/platform/app/public/customizations/studyBrowser/derivedDateTime.jsonc +++ b/platform/app/public/customizations/studyBrowser/derivedDateTime.jsonc @@ -6,7 +6,7 @@ // // Derived series are listed newest first, and the date/time they are sorted by // is the creation date/time of the instance each display set shows (see -// `getSeriesDateTime`). The default thumbnail shows only the series number and +// `getLatestInstanceDateTime`). The default thumbnail shows only the series number and // the instance count, so several reports saved on the same day give no sign of // which is which and their order reads as arbitrary. This shows it. // diff --git a/platform/core/src/classes/MetadataProvider.test.ts b/platform/core/src/classes/MetadataProvider.test.ts index 265b38ff9d7..c61e57a9ec1 100644 --- a/platform/core/src/classes/MetadataProvider.test.ts +++ b/platform/core/src/classes/MetadataProvider.test.ts @@ -79,4 +79,32 @@ describe('MetadataProvider', () => { frameNumber: '3', }); }); + + describe('the module a UID belongs to', () => { + const instance = { + SOPInstanceUID: 'sop-general-image', + SOPClassUID: '1.2.840.10008.5.1.4.1.1.88.33', + InstanceNumber: '2', + }; + + it('gives the instance number, and no SOP Class UID, for the General Image module', () => { + // SOPClassUID is not in the General Image module, so this provider must + // not answer it here. A consumer that reads the pair of UIDs off this + // module reads the wrong module. + const generalImage = metadataProvider.getTagFromInstance('generalImageModule', instance); + + expect(generalImage).toMatchObject({ + sopInstanceUID: 'sop-general-image', + instanceNumber: 2, + }); + expect(generalImage.sopClassUID).toBeUndefined(); + }); + + it('gives both UIDs for the SOP Common module', () => { + expect(metadataProvider.getTagFromInstance('sopCommonModule', instance)).toEqual({ + sopClassUID: '1.2.840.10008.5.1.4.1.1.88.33', + sopInstanceUID: 'sop-general-image', + }); + }); + }); }); diff --git a/platform/core/src/classes/MetadataProvider.ts b/platform/core/src/classes/MetadataProvider.ts index c60e2a46fd7..b9d2c2a0607 100644 --- a/platform/core/src/classes/MetadataProvider.ts +++ b/platform/core/src/classes/MetadataProvider.ts @@ -365,14 +365,12 @@ class MetadataProvider { case WADO_IMAGE_LOADER_TAGS.GENERAL_IMAGE_MODULE: metadata = { sopInstanceUID: instance.SOPInstanceUID, - // Used, with the instance number below, to reference an instance as - // the predecessor of a newly created one. - sopClassUID: instance.SOPClassUID, + // No SOPClassUID here - it belongs to the SOP Common module. instanceNumber: toNumber(instance.InstanceNumber), // The instance level date/time is the only one that distinguishes a // newly saved instance from the rest of its series, whose // SeriesDate/SeriesTime it inherits, so it has to be carried through - // to instances derived from this one. See `getSeriesDateTime`. + // to instances derived from this one. See `getLatestInstanceDateTime`. instanceCreationDate: instance.InstanceCreationDate, instanceCreationTime: instance.InstanceCreationTime, contentDate: instance.ContentDate, diff --git a/platform/core/src/types/DisplaySet.ts b/platform/core/src/types/DisplaySet.ts index 64f3986cce8..f33257b3428 100644 --- a/platform/core/src/types/DisplaySet.ts +++ b/platform/core/src/types/DisplaySet.ts @@ -70,16 +70,19 @@ export type DisplaySet = { * series that the instances belong to, and it must not be copied back to the * series metadata or to an instance. * - * The SOP class handler that creates the display set writes both, with - * `getSeriesDateTime` of the instance the display set shows: + * The SOP class handler that creates the display set writes both, and the two + * kinds of handler take the value from different places: * - * - An image display set takes the instance's `SeriesDate`/`SeriesTime`. - * Every instance of a series carries those two identically, so every - * display set of one image series holds the same value, ties on the sort - * key, and the display sets stay together in the series list - ordered - * among themselves by `compareSameSeriesDisplaySet`. + * - An image display set takes the instance's `SeriesDate`/`SeriesTime` + * directly. Every instance of a series carries those two identically, so + * every display set of one image series holds the same value, ties on the + * sort key, and the display sets stay together in the series list - ordered + * among themselves by `compareSameSeriesDisplaySet`. Such a handler must not + * call `getLatestInstanceDateTime`, which also reads + * `AcquisitionDate`/`AcquisitionTime` and so differs per instance. * - A derived display set - SEG, RTSTRUCT, SR, PMAP, PDF, video, chart - - * takes the creation date/time of the instance it shows. A report saved + * takes the creation date/time of the instance it shows, from + * `getLatestInstanceDateTime`. A report saved * today into a series created last week gets today's date, and the series * list places that display set as today's work. The series' own * `SeriesDate`/`SeriesTime`, in the instance metadata and in the archive, diff --git a/platform/core/src/utils/index.ts b/platform/core/src/utils/index.ts index 0aa97725627..915a7c87978 100644 --- a/platform/core/src/utils/index.ts +++ b/platform/core/src/utils/index.ts @@ -42,10 +42,10 @@ import { } from './sortStudy'; import { dateTimeAttributes, - getSeriesDateTime, - getSeriesDateTimeSortKey, + getLatestInstanceDateTime, + getLatestInstanceDateTimeSortKey, getDateTimeSortKey, -} from './seriesDateTime'; +} from './latestInstanceDateTime'; import { getCurrentDicomDateTime, updateNewInstanceMetadata } from './updateNewInstanceMetadata'; import { splitComma, getSplitParam } from './splitComma'; import { createStudyBrowserTabs } from './createStudyBrowserTabs'; @@ -84,8 +84,8 @@ const utils = { seriesSortCriteria, instancesSortCriteria, dateTimeAttributes, - getSeriesDateTime, - getSeriesDateTimeSortKey, + getLatestInstanceDateTime, + getLatestInstanceDateTimeSortKey, getDateTimeSortKey, getCurrentDicomDateTime, updateNewInstanceMetadata, @@ -144,8 +144,8 @@ export { formatDate, formatValue, dateTimeAttributes, - getSeriesDateTime, - getSeriesDateTimeSortKey, + getLatestInstanceDateTime, + getLatestInstanceDateTimeSortKey, getDateTimeSortKey, getCurrentDicomDateTime, updateNewInstanceMetadata, diff --git a/platform/core/src/utils/seriesDateTime.test.js b/platform/core/src/utils/latestInstanceDateTime.test.js similarity index 87% rename from platform/core/src/utils/seriesDateTime.test.js rename to platform/core/src/utils/latestInstanceDateTime.test.js index c7a6cecccd9..a5f78e3b077 100644 --- a/platform/core/src/utils/seriesDateTime.test.js +++ b/platform/core/src/utils/latestInstanceDateTime.test.js @@ -1,25 +1,25 @@ import { getDateTimeSortKey, expandDicomDateTime, - getSeriesDateTime, - getSeriesDateTimeSortKey, + getLatestInstanceDateTime, + getLatestInstanceDateTimeSortKey, parseUTCOffset, -} from './seriesDateTime'; +} from './latestInstanceDateTime'; -describe('getSeriesDateTime', () => { +describe('getLatestInstanceDateTime', () => { test('uses the series date and time when they are the only pair', () => { - expect(getSeriesDateTime({ SeriesDate: '20260817', SeriesTime: '093000' })).toEqual({ + expect(getLatestInstanceDateTime({ SeriesDate: '20260817', SeriesTime: '093000' })).toEqual({ SeriesDate: '20260817', SeriesTime: '093000', }); }); test('reports an empty date and time rather than undefined', () => { - expect(getSeriesDateTime({})).toEqual({ SeriesDate: '', SeriesTime: '' }); + expect(getLatestInstanceDateTime({})).toEqual({ SeriesDate: '', SeriesTime: '' }); }); test('reads the lower camel case spelling of series metadata', () => { - expect(getSeriesDateTime({ seriesDate: '20260817', seriesTime: '093000' })).toEqual({ + expect(getLatestInstanceDateTime({ seriesDate: '20260817', seriesTime: '093000' })).toEqual({ SeriesDate: '20260817', SeriesTime: '093000', }); @@ -27,7 +27,7 @@ describe('getSeriesDateTime', () => { test('takes the latest date of all the attributes', () => { expect( - getSeriesDateTime({ + getLatestInstanceDateTime({ SeriesDate: '20260817', SeriesTime: '090000', ContentDate: '20260819', @@ -41,7 +41,7 @@ describe('getSeriesDateTime', () => { // time say that it has just been added to. test('prefers the instance date over an older series date', () => { expect( - getSeriesDateTime({ + getLatestInstanceDateTime({ SeriesDate: '20260817', SeriesTime: '090000', InstanceCreationDate: '20260819', @@ -52,7 +52,7 @@ describe('getSeriesDateTime', () => { test('takes the latest time of the attributes carrying the winning date', () => { expect( - getSeriesDateTime({ + getLatestInstanceDateTime({ SeriesDate: '20260819', SeriesTime: '090000', ContentDate: '20260819', @@ -68,7 +68,7 @@ describe('getSeriesDateTime', () => { // within its day. test('never takes a time from a date other than the winning one', () => { expect( - getSeriesDateTime({ + getLatestInstanceDateTime({ SeriesDate: '20260818', StructureSetDate: '20260817', StructureSetTime: '090000', @@ -78,7 +78,7 @@ describe('getSeriesDateTime', () => { test('uses the time of the winning date even when it comes from another attribute', () => { expect( - getSeriesDateTime({ + getLatestInstanceDateTime({ SeriesDate: '20260817', StructureSetDate: '20260817', StructureSetTime: '090000', @@ -91,7 +91,7 @@ describe('getSeriesDateTime', () => { // images. The structure set pair is the one that says when the SEG was made. test('prefers a later structure set date over the series date of a SEG', () => { expect( - getSeriesDateTime({ + getLatestInstanceDateTime({ Modality: 'SEG', SeriesDate: '20260817', SeriesTime: '090000', @@ -105,7 +105,7 @@ describe('getSeriesDateTime', () => { // belongs to the day the images were acquired and not to the SEG. test('leaves the time empty when the later SEG date carries none', () => { expect( - getSeriesDateTime({ + getLatestInstanceDateTime({ Modality: 'SEG', SeriesDate: '20260817', SeriesTime: '090000', @@ -115,7 +115,7 @@ describe('getSeriesDateTime', () => { }); test('ignores a time that has no date with it', () => { - expect(getSeriesDateTime({ SeriesTime: '090000', ContentDate: '20260817' })).toEqual({ + expect(getLatestInstanceDateTime({ SeriesTime: '090000', ContentDate: '20260817' })).toEqual({ SeriesDate: '20260817', SeriesTime: '', }); @@ -125,29 +125,29 @@ describe('getSeriesDateTime', () => { // `19-Jan-2026` would read as `192026`, ordering by day of month and making // two different months compare as equal, so it counts as no date at all. test('ignores a date that is not a DICOM DA value', () => { - expect(getSeriesDateTime({ SeriesDate: '19-Jan-2026' })).toEqual({ + expect(getLatestInstanceDateTime({ SeriesDate: '19-Jan-2026' })).toEqual({ SeriesDate: '', SeriesTime: '', }); - expect(getSeriesDateTimeSortKey({ seriesDate: '05-Feb-2026' })).toBe(''); + expect(getLatestInstanceDateTimeSortKey({ seriesDate: '05-Feb-2026' })).toBe(''); }); test('reads the dotted date of the retired DICOM form', () => { - expect(getSeriesDateTime({ SeriesDate: '2026.08.17' })).toEqual({ + expect(getLatestInstanceDateTime({ SeriesDate: '2026.08.17' })).toEqual({ SeriesDate: '2026.08.17', SeriesTime: '', }); }); test('ignores the study date, which every series in the study shares', () => { - expect(getSeriesDateTime({ StudyDate: '20260819', StudyTime: '080000' })).toEqual({ + expect(getLatestInstanceDateTime({ StudyDate: '20260819', StudyTime: '080000' })).toEqual({ SeriesDate: '', SeriesTime: '', }); }); test('splits a combined acquisition date time', () => { - expect(getSeriesDateTime({ AcquisitionDateTime: '20260819143000.000000' })).toEqual({ + expect(getLatestInstanceDateTime({ AcquisitionDateTime: '20260819143000.000000' })).toEqual({ SeriesDate: '20260819', SeriesTime: '143000.000000', }); @@ -163,8 +163,8 @@ describe('getSeriesDateTime', () => { ['a value with no time', '20260819+0000', '20260818200000-0400'], ['a value that crosses the day boundary', '20260819233000-0500', '20260820043000+0000'], ])('gives one sort key to %s in two zones', (_name, west, utc) => { - expect(getSeriesDateTimeSortKey({ AcquisitionDateTime: west })).toBe( - getSeriesDateTimeSortKey({ AcquisitionDateTime: utc }) + expect(getLatestInstanceDateTimeSortKey({ AcquisitionDateTime: west })).toBe( + getLatestInstanceDateTimeSortKey({ AcquisitionDateTime: utc }) ); }); @@ -188,11 +188,11 @@ describe('getSeriesDateTime', () => { ); test('leaves a combined date time that declares no offset exactly as it is', () => { - expect(getSeriesDateTime({ AcquisitionDateTime: '20260819' })).toEqual({ + expect(getLatestInstanceDateTime({ AcquisitionDateTime: '20260819' })).toEqual({ SeriesDate: '20260819', SeriesTime: '', }); - expect(getSeriesDateTime({ AcquisitionDateTime: '202608191030' })).toEqual({ + expect(getLatestInstanceDateTime({ AcquisitionDateTime: '202608191030' })).toEqual({ SeriesDate: '20260819', SeriesTime: '1030', }); @@ -205,7 +205,7 @@ describe('getSeriesDateTime', () => { { ContentDate: '20260818', ContentTime: '235959' }, ]; - expect(getSeriesDateTime(instances)).toEqual({ + expect(getLatestInstanceDateTime(instances)).toEqual({ SeriesDate: '20260819', SeriesTime: '143000', }); @@ -232,7 +232,7 @@ describe('parseUTCOffset', () => { /** * The local offset is supplied to every case here, so the expected value does - * not depend on the zone the test runs in. `getSeriesDateTime` supplies none + * not depend on the zone the test runs in. `getLatestInstanceDateTime` supplies none * and gets the viewer's own offset at that instant instead. */ describe('expandDicomDateTime', () => { @@ -352,8 +352,8 @@ describe('expandDicomDateTime', () => { ); }); -describe('getSeriesDateTimeSortKey', () => { - const sortKey = source => getSeriesDateTimeSortKey(source); +describe('getLatestInstanceDateTimeSortKey', () => { + const sortKey = source => getLatestInstanceDateTimeSortKey(source); test('is empty with no date, which sorts as the oldest', () => { expect(sortKey({})).toBe(''); diff --git a/platform/core/src/utils/seriesDateTime.ts b/platform/core/src/utils/latestInstanceDateTime.ts similarity index 86% rename from platform/core/src/utils/seriesDateTime.ts rename to platform/core/src/utils/latestInstanceDateTime.ts index 9b647c3019f..a865301a770 100644 --- a/platform/core/src/utils/seriesDateTime.ts +++ b/platform/core/src/utils/latestInstanceDateTime.ts @@ -38,7 +38,7 @@ export const dateTimeAttributes: Array<[string, string]> = [ */ export const dateTimeCombinedAttributes: string[] = ['AcquisitionDateTime']; -export type SeriesDateTime = { +export type LatestInstanceDateTime = { /** The chosen date, as found in the source, or `''`. */ SeriesDate: string; /** The time belonging to that same date, as found in the source, or `''`. */ @@ -104,7 +104,7 @@ const pad = (value: number) => `${value}`.padStart(2, '0'); export function expandDicomDateTime( value, localOffsetMinutes?: number -): SeriesDateTime | undefined { +): LatestInstanceDateTime | undefined { const match = dicomDateTime.exec(`${value ?? ''}`.trim()); if (!match) { return undefined; @@ -206,13 +206,29 @@ const timeSortKey = (value): string => { * one of its most recently created instance. * * The values are returned as found, so they are safe to store on a display set - * and to display; use {@link getSeriesDateTimeSortKey} to compare them. The - * one exception is a DT value that declares a UTC offset, which + * and to display; use {@link getLatestInstanceDateTimeSortKey} to compare + * them. The one exception is a DT value that declares a UTC offset, which * {@link expandDicomDateTime} moves to the offset of the viewer first - the * date/time returned is then the local wall clock reading of the same instant, * and a valid DA and TM rather than the offset-bearing DT it came from. + * + * **A handler of a derived display set calls this function. The handler of an + * image display set must not call this function.** A SEG, an RTSTRUCT, an SR, + * a PMAP, a PDF, a video and a chart each need the date/time of creation of the + * object, so a report that the user saves today into a series of last week is + * listed as the work of today. An image instance is different: an image + * instance carries `AcquisitionDate` and `AcquisitionTime`, and those two + * attributes differ between the instances of one series. This function would + * therefore give a different date/time to each display set of one split series. + * The display sets of one series must tie on `dateTimeSortKey`, because only a + * tie sends them to `compareSameSeriesDisplaySet` and to every comparison that + * `addSameSeriesCompare` registers. The handler of an image display set writes + * `instance.SeriesDate` and `instance.SeriesTime` directly, and + * `extensions/default/src/getSopClassHandlerModule.js` does exactly that. A + * change of that handler to this function raises no error, and puts the series + * list in the wrong order. */ -export function getSeriesDateTime(source): SeriesDateTime { +export function getLatestInstanceDateTime(source): LatestInstanceDateTime { const sources = Array.isArray(source) ? source : [source]; let SeriesDate = ''; let SeriesTime = ''; @@ -271,10 +287,10 @@ export function getDateTimeSortKey(date, time): string { } /** - * The {@link getSeriesDateTime} of the given instance, series or display set as - * a {@link getDateTimeSortKey} comparable string. + * The {@link getLatestInstanceDateTime} of the given instance, series or + * display set as a {@link getDateTimeSortKey} comparable string. */ -export function getSeriesDateTimeSortKey(source): string { - const { SeriesDate, SeriesTime } = getSeriesDateTime(source); +export function getLatestInstanceDateTimeSortKey(source): string { + const { SeriesDate, SeriesTime } = getLatestInstanceDateTime(source); return getDateTimeSortKey(SeriesDate, SeriesTime); } diff --git a/platform/core/src/utils/sortStudy.test.js b/platform/core/src/utils/sortStudy.test.js index 41baf5cdbc2..a6522674c01 100644 --- a/platform/core/src/utils/sortStudy.test.js +++ b/platform/core/src/utils/sortStudy.test.js @@ -7,7 +7,7 @@ import { seriesInfoSortingCriteria, sortByInstanceNumber, } from './sortStudy'; -import { getSeriesDateTime } from './seriesDateTime'; +import { getLatestInstanceDateTime } from './latestInstanceDateTime'; addSameSeriesCompare('default', (a, b) => compare(a.default, b.default), 5); const altCompare = 'altCompare'; @@ -202,7 +202,7 @@ describe('compareSeriesDateTime', () => { SeriesInstanceUID, instance, ...seriesDateTime, - ...getSeriesDateTime(instance), + ...getLatestInstanceDateTime(instance), }; }; diff --git a/platform/core/src/utils/sortStudy.ts b/platform/core/src/utils/sortStudy.ts index 071b4cfa732..4a6c27c43c1 100644 --- a/platform/core/src/utils/sortStudy.ts +++ b/platform/core/src/utils/sortStudy.ts @@ -2,7 +2,7 @@ import { vec3 } from 'gl-matrix'; import isLowPriorityModality from './isLowPriorityModality'; import calculateScanAxisNormal from './calculateScanAxisNormal'; import areAllImageOrientationsEqual from './areAllImageOrientationsEqual'; -import { getDateTimeSortKey, getSeriesDateTimeSortKey } from './seriesDateTime'; +import { getDateTimeSortKey, getLatestInstanceDateTimeSortKey } from './latestInstanceDateTime'; export const compare = (a, b) => { if (a == b) return 0; @@ -68,11 +68,11 @@ export const compareSeriesUID = (a, b) => * The date/time a display set is ordered by is the display set's own * `SeriesDate`/`SeriesTime`, which is the *display set* date/time and not * necessarily the date/time of the series the instances belong to - see the - * `SeriesDate` field of the `DisplaySet` type for the whole contract. The SOP - * class handler writes it with {@link getSeriesDateTime} of the instance the - * display set shows, so a report or a segmentation saved into an existing - * series carries the date/time of that save rather than the date/time the - * series was first created. + * `SeriesDate` field of the `DisplaySet` type for the whole contract. A + * handler of a derived display set writes it with + * {@link getLatestInstanceDateTime} of the instance the display set shows, so a + * report or a segmentation saved into an existing series carries the date/time + * of that save rather than the date/time the series was first created. * * The key is read from the display set and never from `displaySet.instance`, * for two reasons. @@ -177,7 +177,7 @@ export const sortByInstanceNumber = (a, b) => { // instance number that fails to say which that is has to be replaced by // something that does. return ( - compare(getSeriesDateTimeSortKey(a), getSeriesDateTimeSortKey(b)) || + compare(getLatestInstanceDateTimeSortKey(a), getLatestInstanceDateTimeSortKey(b)) || compare(a.SOPInstanceUID, b.SOPInstanceUID) ); }; diff --git a/platform/core/src/utils/updateNewInstanceMetadata.test.js b/platform/core/src/utils/updateNewInstanceMetadata.test.js index c29eb6c4433..1df97c66cf4 100644 --- a/platform/core/src/utils/updateNewInstanceMetadata.test.js +++ b/platform/core/src/utils/updateNewInstanceMetadata.test.js @@ -1,5 +1,5 @@ import { getCurrentDicomDateTime, updateNewInstanceMetadata } from './updateNewInstanceMetadata'; -import { getSeriesDateTime } from './seriesDateTime'; +import { getLatestInstanceDateTime } from './latestInstanceDateTime'; describe('getCurrentDicomDateTime', () => { // Built with the local constructor, because a DICOM DA/TM pair with no @@ -90,7 +90,7 @@ describe('updateNewInstanceMetadata', () => { expect(dataset.ContentDate).toBe(dataset.InstanceCreationDate); expect(dataset.ContentTime).toBe(dataset.InstanceCreationTime); - expect(getSeriesDateTime(dataset)).toEqual({ + expect(getLatestInstanceDateTime(dataset)).toEqual({ SeriesDate: dataset.ContentDate, SeriesTime: dataset.ContentTime, }); @@ -121,7 +121,7 @@ describe('updateNewInstanceMetadata', () => { expect(dataset.StructureSetTime).toBe(dataset.InstanceCreationTime); expect(dataset.ContentDate).toBeUndefined(); expect(dataset.ContentTime).toBeUndefined(); - expect(getSeriesDateTime(dataset)).toEqual({ + expect(getLatestInstanceDateTime(dataset)).toEqual({ SeriesDate: dataset.StructureSetDate, SeriesTime: dataset.StructureSetTime, }); @@ -135,7 +135,7 @@ describe('updateNewInstanceMetadata', () => { expect(dataset.PresentationCreationDate).toBe(dataset.InstanceCreationDate); expect(dataset.PresentationCreationTime).toBe(dataset.InstanceCreationTime); expect(dataset.ContentDate).toBeUndefined(); - expect(getSeriesDateTime(dataset)).toEqual({ + expect(getLatestInstanceDateTime(dataset)).toEqual({ SeriesDate: dataset.PresentationCreationDate, SeriesTime: dataset.PresentationCreationTime, }); diff --git a/platform/core/src/utils/updateNewInstanceMetadata.ts b/platform/core/src/utils/updateNewInstanceMetadata.ts index 5f2b759db4e..64ea8e59d0a 100644 --- a/platform/core/src/utils/updateNewInstanceMetadata.ts +++ b/platform/core/src/utils/updateNewInstanceMetadata.ts @@ -1,5 +1,5 @@ import { DicomMetadataStore } from '../services/DicomMetadataStore/DicomMetadataStore'; -import { parseUTCOffset } from './seriesDateTime'; +import { parseUTCOffset } from './latestInstanceDateTime'; /** * The current date and time as DICOM DA and TM values. @@ -42,7 +42,7 @@ export function getCurrentDicomDateTime( * the same story with the Presentation State Identification module, whose * `PresentationCreationDate`/`PresentationCreationTime` are type 1. * - * Both pairs are read back by `getSeriesDateTime`, so an instance stamped + * Both pairs are read back by `getLatestInstanceDateTime`, so an instance stamped * through this map orders exactly as one stamped with a content date/time. */ const modalityDateTimeAttributes: Record = { @@ -69,7 +69,7 @@ const defaultDateTimeAttributes: [string, string] = ['ContentDate', 'ContentTime * `SeriesDate`/`SeriesTime` belong to the original series and must stay as they * are, so only the instance level creation date/time say that the series has * just been added to. Those are what the display set date/time is chosen from - * (see `getSeriesDateTime`), so they have to be set on every save. They are + * (see `getLatestInstanceDateTime`), so they have to be set on every save. They are * stamped in the dataset's own timezone - `TimezoneOffsetFromUTC` when it has * one, the local zone otherwise - because that is the wall clock reading a * viewer displays them as. diff --git a/platform/docs/docs/behaviours/README.md b/platform/docs/docs/behaviours/README.md index 08eea9437af..1197044e445 100644 --- a/platform/docs/docs/behaviours/README.md +++ b/platform/docs/docs/behaviours/README.md @@ -36,6 +36,11 @@ references) so each behaviour doc stays anchored to the code it describes. via `loadMultiframeAsPart10: false` (data source config or the `cornerstone.segmentation.loadMultiframeAsPart10` customization). +- [Report dialog: where a save goes, and what the series is called](./report-dialog-save-destinations.md) + — _implemented_. The three destinations of a save, the `predecessorImageId` + rule that decides which loaded series the dialog offers, the four names that a + new series starts from, and the descriptions that the viewer remembers. + ## Writing a new behaviour doc 1. Add a `kebab-case.md` file in this directory. diff --git a/platform/docs/docs/behaviours/report-dialog-save-destinations.md b/platform/docs/docs/behaviours/report-dialog-save-destinations.md new file mode 100644 index 00000000000..6678e7b1930 --- /dev/null +++ b/platform/docs/docs/behaviours/report-dialog-save-destinations.md @@ -0,0 +1,135 @@ +# Report dialog: where a save goes, and what the series is called + +Status: **Implemented — current behaviour** + +The report dialog stores a segmentation, a contour set, or a measurement report. +The dialog asks the user two things: which series the object goes into, and what +that series is called. + +Implemented across: + +- `extensions/default/src/customizations/reportDialogCustomization.tsx` — the + dialog, and the `ohif.createReportDialog` customization. +- `extensions/default/src/Panels/createReportDialogPrompt.tsx` — the prompt that + shows the dialog, and the input and the output of the prompt. +- `extensions/default/src/utils/seriesDescriptionHistory.ts` — the descriptions + that the viewer remembers. +- `extensions/cornerstone-dicom-seg/src/commandsModule.ts` — `storeSegmentation`, + the SEG caller and the RTSTRUCT caller. +- `extensions/default/src/utils/promptSaveReport.tsx` — the measurement report + caller. +- `libs/@cornerstonejs/packages/adapters/src/utilities/referencedMetadataProvider.ts` + — the `PredecessorSequence` provider, which reads the predecessor instance. + +## One save stores one object + +Each destination stores all of the current data as one object, and the dialog +merges nothing. A save into a series that already holds data does not read that +data, and the save leaves no part of the current data out. The destination +decides which series holds the new object, and therefore which instance the new +object supersedes. The earlier instances stay in the series, and the new +instance becomes the one that the viewer loads by default. + +## The three destinations + +| Destination | Where the object goes | When the dialog offers the destination | +| --- | --- | --- | +| `Save to current` | The series that the viewer loaded the data from | The `predecessorImageId` of the data names a loaded series | +| `Save as new` | A new series, with an editable number and description | Always | +| `Replace existing` | Another loaded series of this modality, which the user selects | The viewer holds at least one other such series | + +`Save to current` is the default choice when the dialog offers it, and +`Save as new` is the default choice otherwise. `Save to current` and +`Replace existing` keep the number and the description of the target series, so +the dialog does not let the user edit either one. + +## Which series the dialog offers + +The dialog offers a loaded series as a destination only when both of these hold: + +1. The series holds the same type of object, which is the modality of the save. +2. The series has a `predecessorImageId` value. + +The `predecessorImageId` value names the immediate prior object that someone +saved into the series. The save supersedes that one instance. The adapter reads +the series and the instance number through the value, from the +`PredecessorSequence` provider. + +The dialog does **not** fall back to the `SeriesInstanceUID` value of the display +set. A UID names a series and not an instance, so a UID names no prior object, +and a UID is not an image id. The provider finds no instance for a UID, and the +save then starts a new series rather than extending the chosen one. + +A local id, such as `dicomfile:3`, is a valid value. The viewer registers an +uploaded instance under a local id, and the provider resolves a local id, so a +user can save against an uploaded instance more than once. + +A display set that the viewer downloaded and never stored has no +`predecessorImageId` value, and the dialog does not offer that display set. The +number of a new series still counts such a series, because the number comes from +every loaded series of the modality and not from the offered ones alone. + +## What the new series is called + +`Save as new` offers four names, and the field starts from the first name that is +not blank: + +1. `itemName`, the name that the user chose for the item. A rename before the + save therefore reaches the field. `storeSegmentation` passes + `segmentation.label`, but only when the user chose that label: the service + invents a name such as `Segmentation 3` for a new segmentation, and marks the + segmentation with `labelIsGenerated`. A label that is still marked this way + goes to `defaultSeriesDescription` instead, so a generated name does not + outrank the remembered descriptions. A rename clears the mark, and the save + compares no two strings, so a user who types the invented name still gets the + name in `itemName`. +2. The description of the series that the viewer loaded the data from, which is + the name of the last save of this data. +3. The descriptions that the user used before for this type of item, most recent + first. See [remembered descriptions](#remembered-descriptions). +4. `defaultSeriesDescription`, the name for an item that has no other name. The + in-tree callers pass `Segmentation`, `Contours` and `Measurements`. + +The pull-down holds the four names in that order. A blank name, and a name of +spaces, drops out of the list, so such a name cannot hide a later name. The +dialog compares the names without case, and the list holds each name once. An +emptied field falls back to the first name of the list. + +A caller with no editable name passes no `itemName`: the measurement report has +no such name, and the report therefore starts from the description of the loaded +series, or from the last used name. + +### Remembered descriptions + +The viewer remembers a description when the user creates a new series with the +description, and a download counts as a save. A save into a series that already +exists remembers nothing, because that series keeps its own description. + +The history holds a name that the user chose. A save that uses +`defaultSeriesDescription` remembers nothing, because the caller supplies that +name at every save, and the dialog offers the name in any case. A generated +segmentation label reaches `defaultSeriesDescription`, so `Segmentation 1` stays +out of the history and the dialog cannot offer `Segmentation 1` as the name of a +later, unrelated segmentation. + +- `itemType` is the key that the viewer remembers the descriptions under, and + `itemType` defaults to the modality. +- `rememberedDescriptionCount` is how many descriptions the viewer remembers, and + the count defaults to 5. +- A count of 0 turns the history off. The list then holds one name, and the + dialog does not render the pull-down. + +The user reaches the list in three ways: the pull-down button shows every name; +typing narrows the list to the names that the typing can complete; **Tab** +completes to the first of those names, and the arrow keys plus **Enter** select +one. + +## The series number of a new series + +The dialog offers one past the highest series number of the loaded series of this +modality, and at least `minSeriesNumber`. The user can edit the number. An empty +number, and a number that is not valid, falls back to the offered number. + +`createReportDialogPrompt` returns `seriesNumber`, and also +`priorSeriesNumber`, which is one less. A caller that computes the number as +`1 + priorSeriesNumber` gets the number that the dialog shows. diff --git a/platform/docs/docs/development/notes-requirements.md b/platform/docs/docs/development/notes-requirements.md index 5e0c5540c46..d84e195a0f8 100644 --- a/platform/docs/docs/development/notes-requirements.md +++ b/platform/docs/docs/development/notes-requirements.md @@ -67,7 +67,7 @@ a value less than `25` here. [24, 'CC', 'B'] ``` -### Display Set Date and Time `getSeriesDateTime` +### Display Set Date and Time `getLatestInstanceDateTime` Derived series - reports, segmentations, structure sets - are listed after the images in reverse date/time order, so the most recently created one is the one @@ -90,7 +90,7 @@ instance level date/time say when the report itself was made. - **Display sets** are ordered by their own `SeriesDate`/`SeriesTime`, which hold the date/time *of the display set* and not always the date/time of the series the instances belong to. The SOP class handler writes both, with - `getSeriesDateTime` of the instance the display set shows. A handler whose + `getLatestInstanceDateTime` of the instance the display set shows. A handler whose `addInstances` advances that instance - the SR and the chart one, which append to their display set rather than making a new one - has to write both again, or the date shown for the display set stays that of the report it replaced and @@ -100,7 +100,7 @@ instance level date/time say when the report itself was made. date/time sort it always was. The sort reads the display set and never `displaySet.instance`, because -`getSeriesDateTime(instance)` differs between the display sets of one split +`getLatestInstanceDateTime(instance)` differs between the display sets of one split series. A key that varies inside a series hides the `addSameSeriesCompare` comparison, which runs only when the key ties, and it makes the comparator inconsistent: with one key inside a series and another between series, a series @@ -125,7 +125,7 @@ Two consequences follow, and both are intended: DICOM records "when this was created" in several different attribute pairs, and which of them are present depends on the modality and on whoever wrote the -object, so `getSeriesDateTime` chooses one pair from all of them: +object, so `getLatestInstanceDateTime` chooses one pair from all of them: `InstanceCreationDate`/`Time`, `ContentDate`/`Time`, `AcquisitionDate`/`Time` (or the combined `AcquisitionDateTime`), `StructureSetDate`/`Time`, `PresentationCreationDate`/`Time` and `SeriesDate`/`Time`. The rules are: diff --git a/platform/docs/docs/migration-guide/3p13-to-3p14/display-set-ordering.md b/platform/docs/docs/migration-guide/3p13-to-3p14/display-set-ordering.md index ac864d1479f..bf5d67c788a 100644 --- a/platform/docs/docs/migration-guide/3p13-to-3p14/display-set-ordering.md +++ b/platform/docs/docs/migration-guide/3p13-to-3p14/display-set-ordering.md @@ -18,7 +18,7 @@ the date/time of a display set is chosen and what it is used for. `compareSeriesDateTime` still compares the `SeriesDate`/`SeriesTime` of the two sides, and `dateTimeSortKey` still reads them from the display set. What changed is the value the SOP class handler puts there. The handler now writes -`getSeriesDateTime` of the instance the display set shows, chosen from every +`getLatestInstanceDateTime` of the instance the display set shows, chosen from every creation attribute that instance carries - `InstanceCreationDate`/`Time`, `ContentDate`/`Time`, `AcquisitionDate`/`Time` (or `AcquisitionDateTime`), `StructureSetDate`/`Time`, `PresentationCreationDate`/`Time` and @@ -68,7 +68,7 @@ the series and register an `addSameSeriesCompare` comparison to order them. ## A DT that declares a UTC offset is read in the viewer's own offset `AcquisitionDateTime` is a DICOM DT, and a DT may end with the `&ZZXX` UTC -offset the rest of it is written in. `getSeriesDateTime` used to take the first +offset the rest of it is written in. `getLatestInstanceDateTime` used to take the first 8 characters as the date and everything after as the time, which read the offset digits as time digits: `20260819+0500` became five in the morning, and the `05` of `202608191030-0500` became the seconds. @@ -147,7 +147,7 @@ them. The creation date/time of the object itself depends on the modality: The RTSTRUCT and PR IODs define no content date/time at all, so a `ContentDate`/`Time` on one of them is an attribute a strict validator or -archive can reject the instance for. `getSeriesDateTime` reads all three pairs, +archive can reject the instance for. `getLatestInstanceDateTime` reads all three pairs, so the ordering is the same whichever pair the modality gets. The date/time are read as wall clock values in the dataset's own timezone - @@ -164,3 +164,35 @@ zone rather than the UTC values dcmjs and the adapters default to. not have before. If you post-process saved instances and relied on the instance number coming from a single predecessor instance, note that it is now derived from the highest instance number in the whole series. + +## The exported name is `getLatestInstanceDateTime` + +`platform/core` exported this function as `getSeriesDateTime` in +3.14.0-beta.25. The name says that the function gives the date and the time of +the series, and the function does not do that. The function gives the latest +date of the attributes that the instance carries, together with the latest time +that carries the same date. The export is `getLatestInstanceDateTime` now, and +the sort key export is `getLatestInstanceDateTimeSortKey`. + +**What changes for you:** change the name at every call. The behaviour of both +functions is exactly the same as before. + +| Before | Now | +| --- | --- | +| `utils.getSeriesDateTime` | `utils.getLatestInstanceDateTime` | +| `utils.getSeriesDateTimeSortKey` | `utils.getLatestInstanceDateTimeSortKey` | +| the type `SeriesDateTime` | the type `LatestInstanceDateTime` | +| the module `platform/core/src/utils/seriesDateTime` | the module `platform/core/src/utils/latestInstanceDateTime` | + +The two fields of the type keep the names `SeriesDate` and `SeriesTime`, +because a handler assigns the two fields to a display set, and the display set +holds the two fields under those names. + +`getSeriesDateTime` gets no alias, because the name was in no stable release of +OHIF. The name was in the 3.14.0-beta.25 line only. + +`extensions/default/src/utils/getCurrentDicomDateTime.ts` also exported a +`getSeriesDateTime`, and that function gave the current date and time. No module +imports that file, and this release deletes the file. Use +`utils.getCurrentDicomDateTime` of `platform/core` for the current date and +time. diff --git a/platform/docs/docs/migration-guide/3p13-to-3p14/report-dialog.md b/platform/docs/docs/migration-guide/3p13-to-3p14/report-dialog.md index 9db9f962e5b..763d9644a52 100644 --- a/platform/docs/docs/migration-guide/3p13-to-3p14/report-dialog.md +++ b/platform/docs/docs/migration-guide/3p13-to-3p14/report-dialog.md @@ -25,7 +25,8 @@ button that commits the save: - **Save as new** creates a separate series, with no predecessor. The series number and the series description are both editable: the number is offered as one past the existing series of this modality (at least `minSeriesNumber`), and - the description as the one last used for this type of item, falling back to + the description as the first of four names - `itemName`, the description the + data was loaded from, the one last used for this type of item, then `defaultSeriesDescription` - see [remembered series descriptions](#remembered-series-descriptions). It is the default choice when the data has not been stored before, and is always @@ -63,9 +64,14 @@ the primary action on the right, rather than the right-aligned cluster ## `createReportDialogPrompt` input -`defaultSeriesDescription` is new, and optional: +`itemName` and `defaultSeriesDescription` are new, and both are optional: ```ts +// `labelIsGenerated` says that the service invented the label, so the user has +// chosen no name for this segmentation. A generated name goes to +// `defaultSeriesDescription`, and a name that the user chose goes to `itemName`. +const { label, labelIsGenerated } = segmentation; + const { value, series, seriesNumber, dataSourceName, action } = await createReportDialogPrompt({ servicesManager, @@ -73,19 +79,40 @@ const { value, series, seriesNumber, dataSourceName, action } = title: 'Save Segmentation', modality: 'SEG', predecessorImageId, - // New: the series description offered when a new series is created - defaultSeriesDescription: segmentation.label, + // New: the name that the user chose for the item, offered first for a new + // series + itemName: labelIsGenerated ? '' : label, + // New: the name for an item that has no other, offered last + defaultSeriesDescription: (labelIsGenerated && label) || 'Segmentation', enableDownload: true, }); ``` -It should be the name of the thing being stored, so that the user is offered a -meaningful series description rather than an empty field. The in-tree callers -pass the segmentation name (falling back to `Contours` for an RTSTRUCT export -and `Segmentation` for a SEG), and `Measurements` for a measurement report. -Previously an unedited description stored the *title of the dialog* as the series -description, so a measurement report saved without typing a name was stored as -`Create Report`. +`itemName` is the name that the user chose for the item being stored. A new +series offers `itemName` first, so a rename that the user makes before the save +reaches the description field. A caller with no such name passes nothing: the +measurement report passes no `itemName`, and `storeSegmentation` passes no +`itemName` while the label is still the name that the service invented. A +generated name belongs in `defaultSeriesDescription`, so that the name does not +outrank the remembered descriptions - see +[the behaviour doc](../../behaviours/report-dialog-save-destinations.md). + +`Segmentation.labelIsGenerated` is new in `@cornerstonejs/tools`, and +`SegmentationPublicInput.config` carries the flag. A creator that invents a +label passes `labelIsGenerated: true` beside the label. A creator that gives no +label at all gets the flag anyway, because such a creator gives no name that the +user chose. An update that carries a `label` and no `labelIsGenerated` clears +the flag, so a rename gives a name that the user chose. The viewer wrote a +private `generatedLabel` attribute onto the segmentation state before, and a +consumer compared the two strings; a consumer reads the flag now. + +`defaultSeriesDescription` is the name for an item that has no other name, and a +new series offers `defaultSeriesDescription` last. The in-tree callers pass +`Segmentation` for a SEG, `Contours` for an RTSTRUCT export, and `Measurements` +for a measurement report. A user therefore gets a meaningful series description +rather than an empty field. Previously an unedited description stored the *title +of the dialog* as the series description, so a measurement report saved without +typing a name was stored as `Create Report`. `itemType` and `rememberedDescriptionCount` are also new and optional - see [remembered series descriptions](#remembered-series-descriptions). @@ -96,6 +123,13 @@ defaults to. A `predecessorImageId` that does not belong to a loaded series of the given modality falls back to creating a new series, rather than claiming to update a series that cannot be described. +The dialog offers a loaded series as a destination only when the series holds the +modality being stored, and when the series has a `predecessorImageId` value. The +dialog does not fall back to the `SeriesInstanceUID` value of the display set, +because the `PredecessorSequence` provider throws on a UID. For the whole rule, +and for the local ids that an uploaded instance carries, see +[the behaviour doc](../../behaviours/report-dialog-save-destinations.md). + ## `createReportDialogPrompt` output `seriesNumber` is new: @@ -177,21 +211,25 @@ Two new optional inputs control this: kinds of item that deserve their own list. - `rememberedDescriptionCount` is how many to remember, and defaults to **5**. **0 disables the feature**: nothing is remembered, nothing is offered, and the - pull down is not shown, leaving a plain description field. + pull-down is not shown, leaving a plain description field. In the dialog, the `Save as new` description field: -- is prefilled with the description last used for this type of item, and with - `defaultSeriesDescription` when there is none yet; -- offers a pull down whose first entry is `defaultSeriesDescription`, followed by - the remembered descriptions, most recent first; +- starts from the first of four names: `itemName`, then the description the data + was loaded from, then the one last used for this type of item, then + `defaultSeriesDescription`. An emptied field falls back to that same name; +- offers a pull-down that holds those names in that order, with the remembered + ones most recent first; - narrows that list to the entries the typing can complete, with **Tab** completing to the first of them, and the arrow keys plus Enter picking one. A description is remembered when it is used to create a new series, including a download. Storing into a series that already exists records nothing, since that -series keeps its own description. Reusing a description moves it back to the -front of the list rather than duplicating it, matched without regard to case. +series keeps its own description. A save that uses `defaultSeriesDescription` +also records nothing, because the caller supplies that name at every save, and +the dialog offers the name in any case; a generated segmentation label therefore +stays out of the history. Reusing a description moves it back to the front of the +list rather than duplicating it, matched without regard to case. Deployments that must not persist anything into local storage should pass `rememberedDescriptionCount: 0`. diff --git a/platform/docs/docs/platform/services/customization-service/sampleCustomizations.tsx b/platform/docs/docs/platform/services/customization-service/sampleCustomizations.tsx index 9486b9b03c5..46c8b97b59e 100644 --- a/platform/docs/docs/platform/services/customization-service/sampleCustomizations.tsx +++ b/platform/docs/docs/platform/services/customization-service/sampleCustomizations.tsx @@ -2262,7 +2262,7 @@ window.config = { numInstances: ({ displaySet }) => (displaySet?.numImageFrames ?? displaySet?.instances?.length) || 1, seriesDate: ({ displaySet, formatters }) => formatters.formatDate(displaySet?.SeriesDate), - instanceDateTime: '(the creation date/time, see getSeriesDateTime)', + instanceDateTime: '(the creation date/time, see getLatestInstanceDateTime)', }, configuration: ` window.config = { diff --git a/tests/Rectangle.spec.ts b/tests/Rectangle.spec.ts index 9f9cc4568d9..5835288dbd5 100644 --- a/tests/Rectangle.spec.ts +++ b/tests/Rectangle.spec.ts @@ -39,9 +39,13 @@ test('should display the rectangle tool', async ({ const expectedArea = 15959; const expectedMax = 295; - const expectedMean = 80.4; + // cornerstone3D 2744 gave the area annotations one rule for the voxels that + // an annotation covers. The rule changed which voxels the statistics read, + // and the mean and the standard deviation moved. The area, the maximum and + // the minimum did not move. + const expectedMean = 79.8; const expectedMin = -77.0; - const expectedStdDev = 38.2; + const expectedStdDev = 38.1; // RectangleROI panel: area (no prefix) + Max (with prefix). // RectangleROI SVG: Area, Mean, Max, Min, Std Dev (5 lines for CT modality).