From 4aa195651d06d57f7aa96ba09c0c1f0924ee09b6 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Tue, 8 Sep 2026 15:00:18 -0400 Subject: [PATCH 01/10] fix(ReportDialog): do not offer a series that no image id names A follow-up to OHIF#6221, which OHIF merged as `ab918119f8`. This commit changes the upstream files only, so the commit can go to OHIF as it is. The list of destinations used `ds.predecessorImageId || ds.SeriesInstanceUID`. The fallback gives a value where a save cannot use the value. A `predecessorImageId` value is the image id of one instance, and the save supersedes that one instance. A `SeriesInstanceUID` value is not an image id. The `PredecessorSequence` provider of cornerstone3D reads the general image module of the instance through the value, the provider finds no instance for a UID, and the provider then raises an exception on `1 + Number(generalImage.instanceNumber)`. The Download button of the dialog reaches this case: the store path of that button registers no image id for the stored instance, so the display set of the downloaded object has no `predecessorImageId` value, and the list still offered that display set. The list now offers a series only when that series has a `predecessorImageId` value. A local id, such as `dicomfile:3`, stays in the list: the viewer registers an uploaded instance under a local id, the provider resolves that id, and a user must be able to save more than once against an uploaded instance. The number that the dialog offers for a new series now counts every loaded series of the modality, and not the series that the dialog offers. A series that the dialog drops must still hold its number, or a later save takes a number that is already in use. `createReportDialogPrompt` also gains a correct description of the dialog. The comment said "exactly two destinations", `New Series` and `Extend Existing`. The dialog has three destinations: `Save to current`, `Save as new` and `Replace existing`. Reported by TFRadicalImaging on RadicalImaging/UCalgary#469, findings 3 and 5. Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit 94f2c7e076ddd66ffe9e7c2eb46ba6aa4a1224e5) --- .../src/Panels/createReportDialogPrompt.tsx | 26 ++++++- .../reportDialogCustomization.test.ts | 73 +++++++++++++++++++ .../reportDialogCustomization.tsx | 55 +++++++++++--- .../3p13-to-3p14/report-dialog.md | 12 +++ 4 files changed, 151 insertions(+), 15 deletions(-) diff --git a/extensions/default/src/Panels/createReportDialogPrompt.tsx b/extensions/default/src/Panels/createReportDialogPrompt.tsx index 75f342cbe73..ecd21a371a4 100644 --- a/extensions/default/src/Panels/createReportDialogPrompt.tsx +++ b/extensions/default/src/Panels/createReportDialogPrompt.tsx @@ -22,14 +22,32 @@ 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 + * The dialog offers three destinations, and says which one is in effect. Each + * destination stores all of the current data as one object; the destination + * decides only which series that object belongs to, and so which instance the + * object supersedes. The dialog merges nothing: + * - `Save as new` 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. + * - `Save to current` stores into the series the data was loaded from, which + * keeps its own series number and description, so neither is editable. The + * dialog offers this destination only when `predecessorImageId` names a + * loaded series. + * - `Replace existing` stores into another loaded series of this modality, + * which the user chooses from a list. That series also keeps its own series + * number and description. The dialog offers this destination only when + * there is another such series. + * + * A series appears as a destination only when that series has a + * `predecessorImageId` value. The value is the image id of one instance, the + * save supersedes that one instance, and the save reads the series and the + * instance number through the image id. A local id, such as `dicomfile:3`, is + * a valid value: the viewer registers an uploaded instance under a local id, so + * a user can save more than once against an uploaded instance. A display set + * that the viewer downloaded and never stored has no value, and the dialog does + * not offer that display set. * * The response is: * - `value`, the series description of the object/series being created. When diff --git a/extensions/default/src/customizations/reportDialogCustomization.test.ts b/extensions/default/src/customizations/reportDialogCustomization.test.ts index eb6a5b6003e..1f31fe7fa57 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.test.ts +++ b/extensions/default/src/customizations/reportDialogCustomization.test.ts @@ -114,6 +114,26 @@ const UNRELATED_SERIES = { SeriesDescription: 'Axial', }; +// A series that the user uploaded, which the viewer registers under a local id. +// The provider resolves a local id, so this series is a destination. +const LOCAL_SERIES = { + displaySetInstanceUID: 'ds-local', + Modality: 'SEG', + SeriesInstanceUID: '1.2.6', + SeriesNumber: 3107, + SeriesDescription: 'Kidney', + predecessorImageId: 'dicomfile:3', +}; + +// A series that the viewer downloaded and never stored, so no image id names it. +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 +262,16 @@ describe('ReportDialog', () => { expect(seriesNumberField().value).toBe('3101'); }); + it('counts a series that it does not offer as a destination', () => { + // 3108 belongs to a series that no predecessor image id names, so the + // lists above do not offer that series. A new series must still get a + // number past 3108, or a later save takes a number that is already in use. + setDisplaySets([CURRENT_SERIES, DOWNLOADED_SERIES]); + renderDialog(); + + expect(seriesNumberField().value).toBe('3109'); + }); + it('saves an edited series number and description', () => { const { onSave } = renderDialog(); @@ -314,6 +344,49 @@ 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, and the provider resolves that + // id, so the user can save against the uploaded instance more than once. + expect(screen.getByText('Kidney')).toBeTruthy(); + // A downloaded object has no predecessor image id. The list gave the + // SeriesInstanceUID of that display set before, which is not an image id, + // and the adapter then raised an exception while it made the object. + 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', () => { + // The viewer registers an uploaded instance under a local id, and the + // provider resolves that id. A user must be able to save more than once + // against an uploaded instance, so a local id is a destination. + 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', () => { diff --git a/extensions/default/src/customizations/reportDialogCustomization.tsx b/extensions/default/src/customizations/reportDialogCustomization.tsx index 077dbf2fe4a..bc48aec2fde 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -141,18 +141,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 +166,29 @@ function ReportDialog({ }; }) .filter(series => { - // Two display sets of one series would otherwise both be offered, and - // the select needs unique values. + // A `predecessorImageId` value is the image id of one instance, and the + // save supersedes that one instance. The `PredecessorSequence` provider + // reads the general image module of the instance through the value. + // + // A `SeriesInstanceUID` value is not an image id, so this list does not + // fall back to one. The provider finds no instance for a UID, and the + // provider then raises an exception on `1 + Number(undefined)` while the + // adapter makes the object. A display set that the viewer downloaded and + // never stored has no value, and this list drops that display set. + // + // A local id, such as `dicomfile:3`, stays in this list. The viewer + // registers an uploaded instance under a local id, and the provider + // resolves that id, so a user can save more than once against an + // uploaded instance. + // + // A dropped series still counts towards the number of a new series below. 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,10 +205,23 @@ function ReportDialog({ [existingSeries, currentSeries] ); - /** The series number offered for a new series - one past the existing ones. */ + /** + * The series number offered for a new series - one past the existing ones. + * + * This number counts every loaded series of the modality, and not the series + * that the lists above offer. A series without a `predecessorImageId` value + * is not offered, and a count of the offered series alone therefore gave the + * next new series a number that a loaded series already holds. + */ 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] ); /** 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..64279697e8b 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 @@ -96,6 +96,18 @@ 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. +A `predecessorImageId` value is the image id of one instance, and not the id of a +series. The dialog offers a loaded series as a destination only when that series +has a `predecessorImageId` value. The dialog does not fall back to the +`SeriesInstanceUID` value of the display set, because a `SeriesInstanceUID` value +is not an image id: the `PredecessorSequence` provider finds no instance for a +UID, and the provider then raises an exception while the adapter makes the +object. A local id, such as `dicomfile:3`, is a valid value, and the dialog +offers a series that carries one: the viewer registers an uploaded instance under +a local id, so a user can save more than once against an uploaded instance. A +series that the dialog does not offer still counts towards the number that the +dialog offers for a new series. + ## `createReportDialogPrompt` output `seriesNumber` is new: From 48f3a6d4fb30f33672d290fda56e0c3f6da7f5a0 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 9 Sep 2026 11:17:49 -0400 Subject: [PATCH 02/10] fix(ReportDialog): start a new series from the name it was loaded from The description field of `Save as new` started from `descriptionOptions[1]`. That index assumed two things: the provided name always takes the first place in the list, and the name last used therefore takes the second place. Two cases broke the assumption, and both offered the *second* most recent name: - a caller that provides no name, so the empty value never takes a place; - a provided name that the user already saved something as, which the deduplication removes from the history. The second case is the common one. A user who accepts the offered name on one save makes the name the most recent entry of the history, and the next save then offers the name from two saves ago. A new series now starts from the first of three names: 1. the description the data was loaded from; 2. the description last used for this type of item, most recent first; 3. `defaultSeriesDescription`, the name that the caller provides. The list of completions holds the same three names, in the same order, so the field takes the first entry and no index arithmetic decides the value. An emptied field falls back to that same name, and not to the provided name behind it. A `rememberedDescriptionCount` of 0 still offers one name and shows no pull down, as the migration guide states. This commit also states the rule for a destination series correctly, in the dialog, in the doc comment of `createReportDialogPrompt` and in the migration guide. The rule is applicability, and not the presence of an image id: this save applies to a series when the series holds the same type of object, and when the series has a `predecessorImageId` value that names the immediate prior object of that type that someone saved into the series. A `SeriesInstanceUID` value meets neither condition. Four new tests hold the three names, the two cases above and the fallback of an emptied field. The suite has 31 tests, and every test passes. --- .../src/Panels/createReportDialogPrompt.tsx | 13 ++-- .../reportDialogCustomization.test.ts | 63 +++++++++++++++- .../reportDialogCustomization.tsx | 75 +++++++++++++------ .../3p13-to-3p14/report-dialog.md | 31 +++++--- 4 files changed, 138 insertions(+), 44 deletions(-) diff --git a/extensions/default/src/Panels/createReportDialogPrompt.tsx b/extensions/default/src/Panels/createReportDialogPrompt.tsx index ecd21a371a4..cd2306a3093 100644 --- a/extensions/default/src/Panels/createReportDialogPrompt.tsx +++ b/extensions/default/src/Panels/createReportDialogPrompt.tsx @@ -28,9 +28,10 @@ import PROMPT_RESPONSES from '../utils/_shared/PROMPT_RESPONSES'; * object supersedes. The dialog merges nothing: * - `Save as new` 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. + * description. The description starts from the first of three names: the + * description the data was loaded from, then the one last used for this type + * of item, then `defaultSeriesDescription`. All of them are offered as + * completions of what gets typed. * - `Save to current` stores into the series the data was loaded from, which * keeps its own series number and description, so neither is editable. The * dialog offers this destination only when `predecessorImageId` names a @@ -40,8 +41,10 @@ import PROMPT_RESPONSES from '../utils/_shared/PROMPT_RESPONSES'; * number and description. The dialog offers this destination only when * there is another such series. * - * A series appears as a destination only when that series has a - * `predecessorImageId` value. The value is the image id of one instance, the + * A series appears as a destination only when this save applies to that series. + * Two things must hold: the series holds the same type of object, which is the + * `modality`; and the series has a `predecessorImageId` value, which names the + * immediate prior object of that type that someone saved into the series. The * save supersedes that one instance, and the save reads the series and the * instance number through the image id. A local id, such as `dicomfile:3`, is * a valid value: the viewer registers an uploaded instance under a local id, so diff --git a/extensions/default/src/customizations/reportDialogCustomization.test.ts b/extensions/default/src/customizations/reportDialogCustomization.test.ts index 1f31fe7fa57..4146b4069ef 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.test.ts +++ b/extensions/default/src/customizations/reportDialogCustomization.test.ts @@ -415,8 +415,62 @@ 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 provided one. + expect(shownDescriptions()).toEqual(['Right kidney', 'Left kidney', 'Segmentation 1']); + }); + + it('offers the description the data was loaded from before any other', () => { + // A save of this data into a new series usually keeps the name that the + // data already has, so that name comes before the remembered ones. + 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', () => { + // The findings flow of a fork provides no description. The field started + // from `descriptionOptions[1]` before, which was the name used before the + // last one, because the empty provided name never took the first place. + setStoredHistory({ SEG: ['Right kidney', 'Left kidney'] }); + renderDialog({ defaultSeriesDescription: '' }); + + expect(descriptionField().value).toBe('Right kidney'); + }); + + it('offers the last used description when the provided one is the same', () => { + // The deduplication drops the remembered copy of the provided name, which + // moved every later name up one place and offered the second most recent. + 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 provided one behind it. + expect(onSave).toHaveBeenCalledWith( + expect.objectContaining({ reportName: 'Right kidney' }) + ); }); it('moves a reused description back to the front, without duplicating it', () => { @@ -499,11 +553,12 @@ describe('ReportDialog', () => { fireEvent.keyDown(descriptionField(), { key: 'ArrowDown' }); fireEvent.keyDown(descriptionField(), { key: 'Enter' }); - expect(descriptionField().value).toBe('Right kidney'); + // The second entry of the list, the field having started from the first. + 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 bc48aec2fde..4fa250705bb 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -95,6 +95,9 @@ type ReportDialogProps = { /** * Series description to offer when creating a new series - typically the name * of the object being saved, such as the segmentation name or 'Contours'. + * This is the last of the three names a new series starts from: the name the + * data was loaded from comes first, then the name last used for this type of + * item. */ defaultSeriesDescription?: string; /** @@ -166,20 +169,25 @@ function ReportDialog({ }; }) .filter(series => { - // A `predecessorImageId` value is the image id of one instance, and the - // save supersedes that one instance. The `PredecessorSequence` provider - // reads the general image module of the instance through the value. + // This save applies to a series when two things hold: the series holds + // the same type of object, which the modality filter above decides; and + // the series has a `predecessorImageId` value, which names the immediate + // prior object of that type that someone saved into the series. The + // save supersedes that one instance, and the `PredecessorSequence` + // provider reads the general image module of the instance through the + // value. // - // A `SeriesInstanceUID` value is not an image id, so this list does not - // fall back to one. The provider finds no instance for a UID, and the + // A `SeriesInstanceUID` value meets neither condition, so this list does + // not fall back to one. A UID names a series and not an instance, so it + // names no prior object, and the provider finds no instance for it. The // provider then raises an exception on `1 + Number(undefined)` while the // adapter makes the object. A display set that the viewer downloaded and // never stored has no value, and this list drops that display set. // - // A local id, such as `dicomfile:3`, stays in this list. The viewer - // registers an uploaded instance under a local id, and the provider - // resolves that id, so a user can save more than once against an - // uploaded instance. + // A local id, such as `dicomfile:3`, meets both conditions and stays in + // this list. The viewer registers an uploaded instance under a local id, + // and the provider resolves that id, so a user can save more than once + // against an uploaded instance. // // A dropped series still counts towards the number of a new series below. if (!series.value || seen.has(series.value)) { @@ -225,28 +233,49 @@ function ReportDialog({ ); /** - * 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, most likely to be wanted + * first. The first of them is the one the field starts with: + * + * 1. the description the data was loaded from, since a save of that same + * data into a new series usually keeps its name; + * 2. the descriptions last used for this type of item, most recent first; + * 3. `defaultSeriesDescription`, the name the caller provided for this data. + * + * The field started from `descriptionOptions[1]` before, which assumed that + * the provided name was always the first option and that the last used one + * therefore came second. Two cases broke that assumption, and both offered + * the *second* most recent name: a caller that provides no name at all, and a + * provided name that the user has already saved something as, which the + * deduplication below removes from the history. */ const descriptionOptions = useMemo(() => { - const history = getSeriesDescriptionHistory(itemType || modality, rememberedDescriptionCount); - const options = [defaultSeriesDescription, ...history].filter(option => !!option?.trim()); + // A `rememberedDescriptionCount` of 0 turns the list off, so the field gets + // a name to start from and there is nothing to pick from. + const offered = !(rememberedDescriptionCount > 0) + ? [currentSeries?.description || defaultSeriesDescription] + : [ + currentSeries?.description, + ...getSeriesDescriptionHistory(itemType || modality, rememberedDescriptionCount), + defaultSeriesDescription, + ]; + + const options = offered.filter((option): option is string => !!option?.trim()); return options.filter( (option, index) => options.findIndex(other => other.toLowerCase() === option.toLowerCase()) === index ); - }, [defaultSeriesDescription, itemType, modality, rememberedDescriptionCount]); + }, [currentSeries, defaultSeriesDescription, itemType, modality, rememberedDescriptionCount]); + + /** + * The name a new series starts with, and what an emptied field falls back to, + * being the first of the options above. + */ + 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 @@ -312,7 +341,7 @@ function ReportDialog({ // the user to have changed there. const storedDescription = targetSeries ? (targetSeries.description ?? '') - : newSeriesDescription.trim() || defaultSeriesDescription; + : newSeriesDescription.trim() || baseSeriesDescription; if (!targetSeries) { rememberSeriesDescription( @@ -338,7 +367,7 @@ function ReportDialog({ targetSeries, seriesNumber, newSeriesDescription, - defaultSeriesDescription, + baseSeriesDescription, itemType, modality, rememberedDescriptionCount, 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 64279697e8b..4573a183377 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 three names - 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 @@ -96,13 +97,18 @@ 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. -A `predecessorImageId` value is the image id of one instance, and not the id of a -series. The dialog offers a loaded series as a destination only when that series -has a `predecessorImageId` value. The dialog does not fall back to the -`SeriesInstanceUID` value of the display set, because a `SeriesInstanceUID` value -is not an image id: the `PredecessorSequence` provider finds no instance for a -UID, and the provider then raises an exception while the adapter makes the -object. A local id, such as `dicomfile:3`, is a valid value, and the dialog +The dialog offers a loaded series as a destination only when the save applies to +that series. Two things must hold: the series holds the same type of object, +which is the modality being stored; and the series has a `predecessorImageId` +value, which names the immediate prior object of that type that someone saved +into the series. + +The dialog does not fall back to the `SeriesInstanceUID` value of the display +set, because that value meets neither condition. A `SeriesInstanceUID` value +names a series and not an instance, so it names no prior object, and it is not an +image id: the `PredecessorSequence` provider finds no instance for a UID, and the +provider then raises an exception while the adapter makes the object. A local +id, such as `dicomfile:3`, meets both conditions, and the dialog offers a series that carries one: the viewer registers an uploaded instance under a local id, so a user can save more than once against an uploaded instance. A series that the dialog does not offer still counts towards the number that the @@ -193,10 +199,11 @@ Two new optional inputs control this: 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 three names: 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. From 97083dd6e92492df15efe69b021c6e4a7079b089 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 9 Sep 2026 11:50:24 -0400 Subject: [PATCH 03/10] fix(DicomWebDataSource): make the invalidation of a study promise match `deleteStudyMetadataPromise` has never removed anything. `retrieveStudyMetadata` caches each promise under `` `${dicomWebConfig.name}:${StudyInstanceUID}` ``, and `deleteStudyMetadataPromise` looks the bare `StudyInstanceUID` value up as a key. The key never matches. Every caller holds a study UID and nothing else: a data source exports the function directly and unbound, and the callers that matter are the store of a derived artifact in `extensions/default` and the save of the microscopy extension. A re-retrieve after a store therefore returned the promise that resolved *before* the store, so the viewer did not see the instance that the store had just written. The function now matches on the study, across whichever data sources cached that study. The function also keeps a match on an unqualified key, for a caller that cached one, and the function returns early without a study UID. `_getStudyMetadataPromiseCache` is a test seam. The seam lets a test assert what the invalidation removed, and the export changes no behaviour. Six new tests cover the qualified key, several data sources, an untouched other study, a study whose UID ends with the same digits as another, an unqualified key, and a call with no study. --- .../retrieveStudyMetadata.js | 25 ++++++- .../retrieveStudyMetadata.test.js | 75 +++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.test.js diff --git a/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js b/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js index 9c3c5901fdc..f6a2abc58b6 100644 --- a/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js +++ b/extensions/default/src/DicomWebDataSource/retrieveStudyMetadata.js @@ -82,10 +82,31 @@ 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. * + * Promises are cached under `:` (see above), + * but every caller knows only the study — a data source exports this function + * directly, unbound, and the callers that matter (storing a derived artifact, and + * the microscopy save) hold a study UID and nothing else. Looking the bare UID up + * as a key therefore never matched, and this function had never removed anything: + * any re-retrieve after a store returned the promise resolved *before* it. Match + * on the study instead, across whichever data sources have cached it. + * * @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); + }); +}); From 625493c136dca5888ee816b0ec004de25cfbccdd Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 9 Sep 2026 11:51:20 -0400 Subject: [PATCH 04/10] fix(MetadataProvider): give the SOP Class UID in the general image module `generalImageModule` gave `sopInstanceUID`, `instanceNumber` and the three lossy-compression values, and the module gave no `sopClassUID` value. `@cornerstonejs/metadata` lists `SOPClassUID` in the tags of this same module, in `packages/metadata/src/utilities/modules/generalImage.ts`. A consumer that reads `generalImageModule` therefore expects the value, and this provider was the one provider of the module that withheld the value. The absence has one visible result today. The `PredecessorSequence` module of `referencedMetadataProvider` in `@cornerstonejs/adapters` builds the back pointer of a new revision: ReferencedSOPClassUID: generalImage.sopClassUID, ReferencedSOPInstanceUID: generalImage.sopInstanceUID, `ReferencedSOPClassUID` is a Type 1 element of that sequence. This provider answered `undefined`, and dcmjs drops an undefined key when dcmjs denaturalizes a dataset, so every `PredecessorDocumentsSequence` that the viewer has written carries a `ReferencedSOPInstanceUID` value alone. A save into an existing series therefore wrote a link that is not conformant, and no reader can resolve the SOP class of the instance that the save supersedes. Two new tests: the module gives the SOP Class UID of an instance, and the module gives no value for an instance that carries none. --- .../core/src/classes/MetadataProvider.test.ts | 28 +++++++++++++++++++ platform/core/src/classes/MetadataProvider.ts | 9 ++++++ 2 files changed, 37 insertions(+) diff --git a/platform/core/src/classes/MetadataProvider.test.ts b/platform/core/src/classes/MetadataProvider.test.ts index 265b38ff9d7..3a477a82613 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('generalImageModule', () => { + const instance = { + SOPInstanceUID: 'sop-general-image', + SOPClassUID: '1.2.840.10008.5.1.4.1.1.88.33', + InstanceNumber: '2', + }; + + it('gives the SOP Class UID of the instance', () => { + // `@cornerstonejs/metadata` lists SOPClassUID in this module, and the + // adapters read `generalImage.sopClassUID` to build a + // ReferencedSOPClassUID. That Type 1 element was absent from every + // predecessor back pointer while this provider answered `undefined`. + expect(metadataProvider.getTagFromInstance('generalImageModule', instance)).toMatchObject({ + sopInstanceUID: 'sop-general-image', + sopClassUID: '1.2.840.10008.5.1.4.1.1.88.33', + instanceNumber: 2, + }); + }); + + it('gives no SOP Class UID for an instance that carries none', () => { + const { SOPClassUID: _omitted, ...withoutSopClass } = instance; + + expect( + metadataProvider.getTagFromInstance('generalImageModule', withoutSopClass).sopClassUID + ).toBeUndefined(); + }); + }); }); diff --git a/platform/core/src/classes/MetadataProvider.ts b/platform/core/src/classes/MetadataProvider.ts index 40f7b805c7d..92ee5a88e9e 100644 --- a/platform/core/src/classes/MetadataProvider.ts +++ b/platform/core/src/classes/MetadataProvider.ts @@ -365,6 +365,15 @@ class MetadataProvider { case WADO_IMAGE_LOADER_TAGS.GENERAL_IMAGE_MODULE: metadata = { sopInstanceUID: instance.SOPInstanceUID, + // `@cornerstonejs/metadata` lists SOPClassUID in this module, and a + // consumer that reads the module through either provider expects the + // value. The adapters build a ReferencedSOPClassUID from it: the + // PredecessorSequence module of `referencedMetadataProvider` reads + // `generalImage.sopClassUID` for the back pointer of a new revision, + // and that Type 1 element was absent from every such back pointer, + // because this provider answered `undefined` and dcmjs drops an + // undefined key when dcmjs denaturalizes a dataset. + sopClassUID: instance.SOPClassUID, instanceNumber: toNumber(instance.InstanceNumber), lossyImageCompression: instance.LossyImageCompression, lossyImageCompressionRatio: instance.LossyImageCompressionRatio, From 3e0848c5ff4a5f90cbcc5f3dedd3f51b0722defb Mon Sep 17 00:00:00 2001 From: TFRadicalImaging <115191433+TFRadicalImaging@users.noreply.github.com> Date: Wed, 9 Sep 2026 14:02:30 -0300 Subject: [PATCH 05/10] fix(ReportDialog): offer the default when the loaded description is blank With `rememberedDescriptionCount` at 0 the dialog offers exactly one name, and it took `currentSeries?.description || defaultSeriesDescription`. A blank series description is truthy, so it won that expression, and the filter below then dropped it for being blank. No name survived: the field opened empty, and an emptied field saved an empty `reportName` rather than falling back to the name the caller provided. The name is now chosen on whether it survives that filter, so a blank loaded description falls through to `defaultSeriesDescription`. A `rememberedDescriptionCount` of 0 offering one name is what the migration guide states. One new test covers it, and the migration guide uses the hyphenated `pull-down` for the control. --- .../reportDialogCustomization.test.ts | 24 ++++++++++++++++--- .../reportDialogCustomization.tsx | 7 ++++-- .../3p13-to-3p14/report-dialog.md | 4 ++-- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/extensions/default/src/customizations/reportDialogCustomization.test.ts b/extensions/default/src/customizations/reportDialogCustomization.test.ts index 4146b4069ef..e7d03ea8f6d 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.test.ts +++ b/extensions/default/src/customizations/reportDialogCustomization.test.ts @@ -468,9 +468,7 @@ describe('ReportDialog', () => { fireEvent.click(saveButton()); // The name the field offered, and not the provided one behind it. - expect(onSave).toHaveBeenCalledWith( - expect.objectContaining({ reportName: 'Right kidney' }) - ); + expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Right kidney' })); }); it('moves a reused description back to the front, without duplicating it', () => { @@ -504,6 +502,26 @@ describe('ReportDialog', () => { expect(storedHistory()).toEqual({ SEG: ['Right kidney'] }); }); + it('offers the default when the current description is blank, for a count of 0', () => { + // A blank series description is truthy, so it was taken as the one name + // the count of 0 offers, and then dropped for being blank: the field + // opened empty and an emptied field saved an empty 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' }); diff --git a/extensions/default/src/customizations/reportDialogCustomization.tsx b/extensions/default/src/customizations/reportDialogCustomization.tsx index 4fa250705bb..8c15a2e04a9 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -250,9 +250,12 @@ function ReportDialog({ */ const descriptionOptions = useMemo(() => { // A `rememberedDescriptionCount` of 0 turns the list off, so the field gets - // a name to start from and there is nothing to pick from. + // a name to start from and there is nothing to pick from. The name has to + // be one that survives the filter below: a blank series description is + // truthy, so taking it here and dropping it there left no name at all, and + // an emptied field then saved an empty one. const offered = !(rememberedDescriptionCount > 0) - ? [currentSeries?.description || defaultSeriesDescription] + ? [currentSeries?.description?.trim() ? currentSeries.description : defaultSeriesDescription] : [ currentSeries?.description, ...getSeriesDescriptionHistory(itemType || modality, rememberedDescriptionCount), 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 4573a183377..dd2177dd3a4 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 @@ -195,14 +195,14 @@ 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: - starts from the first of three names: 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 +- 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. From de28ad8dda0f4649ae3809faaded4ebcd71dbb32 Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 9 Sep 2026 13:50:46 -0400 Subject: [PATCH 06/10] fix(ReportDialog): offer the editable item name first for a new series The dialog took one name for two jobs. `defaultSeriesDescription` held the segmentation label when there was one, and the generic `Segmentation` or `Contours` otherwise, so the dialog could not tell an editable name from a fallback. A rename before a save was therefore lost: the field offered the description of the loaded series, which holds the name of the last save, and a save without an edit stored the old name. `itemName` is a new optional input for the editable name, and `defaultSeriesDescription` now means the name for an item that has no other name. A new series offers four names, and the field starts from the first one that is not blank: `itemName`, the description of the loaded series, the descriptions used before for this type of item, then `defaultSeriesDescription`. `storeSegmentation` passes the label as `itemName`. The measurement report passes no `itemName`, and therefore keeps the remembered names at the front of the field. Every candidate now goes through one filter that drops a blank name, and the `rememberedDescriptionCount` of 0 path takes the first name that survives. That removes the `||` chain, in which a name of spaces was truthy and hid every later name. The behaviour of the dialog moves into a behaviour doc: platform/docs/docs/behaviours/report-dialog-save-destinations.md. The doc holds the destinations, the `predecessorImageId` rule for the series that the dialog offers, the four names, and the remembered descriptions. The code comments point at the doc instead of repeating it, and the comment about the exception now names the error that the `PredecessorSequence` provider throws. Four new tests cover `itemName` and the two blank-name cases, for 35 in the file. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/commandsModule.ts | 8 +- .../src/Panels/createReportDialogPrompt.tsx | 44 ++---- .../reportDialogCustomization.test.ts | 97 ++++++++++---- .../reportDialogCustomization.tsx | 126 ++++++++---------- platform/docs/docs/behaviours/README.md | 5 + .../report-dialog-save-destinations.md | 126 ++++++++++++++++++ .../3p13-to-3p14/report-dialog.md | 59 ++++---- 7 files changed, 297 insertions(+), 168 deletions(-) create mode 100644 platform/docs/docs/behaviours/report-dialog-save-destinations.md diff --git a/extensions/cornerstone-dicom-seg/src/commandsModule.ts b/extensions/cornerstone-dicom-seg/src/commandsModule.ts index c1762420fad..09046a452b0 100644 --- a/extensions/cornerstone-dicom-seg/src/commandsModule.ts +++ b/extensions/cornerstone-dicom-seg/src/commandsModule.ts @@ -322,8 +322,9 @@ const commandsModule = ({ } const { label, predecessorImageId } = segmentation; - const defaultSeriesDescription = - label || (modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'); + // The label is the editable name, and this is the name for an item + // without one. The dialog offers the label first, and this name last. + const defaultSeriesDescription = modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'; const { value: reportName, @@ -337,6 +338,7 @@ const commandsModule = ({ predecessorImageId, title: modality === 'RTSTRUCT' ? 'Save Contours' : 'Save Segmentation', modality, + itemName: label, defaultSeriesDescription, enableDownload: true, }); @@ -363,7 +365,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, }, diff --git a/extensions/default/src/Panels/createReportDialogPrompt.tsx b/extensions/default/src/Panels/createReportDialogPrompt.tsx index cd2306a3093..bcfe8ea5ae9 100644 --- a/extensions/default/src/Panels/createReportDialogPrompt.tsx +++ b/extensions/default/src/Panels/createReportDialogPrompt.tsx @@ -11,9 +11,10 @@ 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 current name of the item, when the user can edit that + * name, such as the segmentation name. A new series offers this name first. + * - `defaultSeriesDescription` is the name for an item that has no other name, + * such as 'Contours' or 'Measurements'. A new series offers this name 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,35 +23,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 three destinations, and says which one is in effect. Each - * destination stores all of the current data as one object; the destination - * decides only which series that object belongs to, and so which instance the - * object supersedes. The dialog merges nothing: - * - `Save as new` creates a new series, with an editable series number - * (defaulting to one past the existing series of this modality) and series - * description. The description starts from the first of three names: the - * description the data was loaded from, then the one last used for this type - * of item, then `defaultSeriesDescription`. All of them are offered as - * completions of what gets typed. - * - `Save to current` stores into the series the data was loaded from, which - * keeps its own series number and description, so neither is editable. The - * dialog offers this destination only when `predecessorImageId` names a - * loaded series. - * - `Replace existing` stores into another loaded series of this modality, - * which the user chooses from a list. That series also keeps its own series - * number and description. The dialog offers this destination only when - * there is another such series. - * - * A series appears as a destination only when this save applies to that series. - * Two things must hold: the series holds the same type of object, which is the - * `modality`; and the series has a `predecessorImageId` value, which names the - * immediate prior object of that type that someone saved into the series. The - * save supersedes that one instance, and the save reads the series and the - * instance number through the image id. A local id, such as `dicomfile:3`, is - * a valid value: the viewer registers an uploaded instance under a local id, so - * a user can save more than once against an uploaded instance. A display set - * that the viewer downloaded and never stored has no value, and the dialog does - * not offer that display set. + * 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 @@ -73,6 +51,7 @@ export default function CreateReportDialogPrompt({ modality = 'SR', minSeriesNumber = 0, predecessorImageId, + itemName = '', defaultSeriesDescription = '', itemType, rememberedDescriptionCount = 5, @@ -111,6 +90,7 @@ export default function CreateReportDialogPrompt({ dataSources: allowMultipleDataSources ? dataSources : undefined, predecessorImageId, minSeriesNumber, + itemName, defaultSeriesDescription, itemType, rememberedDescriptionCount, diff --git a/extensions/default/src/customizations/reportDialogCustomization.test.ts b/extensions/default/src/customizations/reportDialogCustomization.test.ts index e7d03ea8f6d..e33a7782094 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.test.ts +++ b/extensions/default/src/customizations/reportDialogCustomization.test.ts @@ -114,8 +114,7 @@ const UNRELATED_SERIES = { SeriesDescription: 'Axial', }; -// A series that the user uploaded, which the viewer registers under a local id. -// The provider resolves a local id, so this series is a destination. +// An uploaded instance carries a local id, which the provider resolves. const LOCAL_SERIES = { displaySetInstanceUID: 'ds-local', Modality: 'SEG', @@ -125,7 +124,7 @@ const LOCAL_SERIES = { predecessorImageId: 'dicomfile:3', }; -// A series that the viewer downloaded and never stored, so no image id names it. +// The viewer never stored this series, so the series has no predecessor image id. const DOWNLOADED_SERIES = { displaySetInstanceUID: 'ds-downloaded', Modality: 'SEG', @@ -263,9 +262,8 @@ describe('ReportDialog', () => { }); it('counts a series that it does not offer as a destination', () => { - // 3108 belongs to a series that no predecessor image id names, so the - // lists above do not offer that series. A new series must still get a - // number past 3108, or a later save takes a number that is already in use. + // 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(); @@ -352,12 +350,10 @@ describe('ReportDialog', () => { fireEvent.click(tab('replace')); expect(screen.getByText('Spleen')).toBeTruthy(); - // An uploaded instance carries a local id, and the provider resolves that - // id, so the user can save against the uploaded instance more than once. + // An uploaded instance carries a local id, which the provider resolves. expect(screen.getByText('Kidney')).toBeTruthy(); - // A downloaded object has no predecessor image id. The list gave the - // SeriesInstanceUID of that display set before, which is not an image id, - // and the adapter then raised an exception while it made the object. + // 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(); }); @@ -369,9 +365,7 @@ describe('ReportDialog', () => { }); it('stores into an uploaded series through its local image id', () => { - // The viewer registers an uploaded instance under a local id, and the - // provider resolves that id. A user must be able to save more than once - // against an uploaded instance, so a local id is a destination. + // A user must be able to save against an uploaded instance more than once. setDisplaySets([LOCAL_SERIES]); const { onSave } = renderDialog(); @@ -415,13 +409,41 @@ describe('ReportDialog', () => { expect(descriptionField().value).toBe('Right kidney'); fireEvent.click(screen.getByTestId('report-series-description-options')); - // The ones used before, most recent first, then the provided one. + // The ones used before, most recent first, then the generic name. expect(shownDescriptions()).toEqual(['Right kidney', 'Left kidney', 'Segmentation 1']); }); - it('offers the description the data was loaded from before any other', () => { - // A save of this data into a new series usually keeps the name that the - // data already has, so that name comes before the remembered ones. + 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 }); @@ -439,18 +461,15 @@ describe('ReportDialog', () => { }); it('offers the last used description when the caller provides none', () => { - // The findings flow of a fork provides no description. The field started - // from `descriptionOptions[1]` before, which was the name used before the - // last one, because the empty provided name never took the first place. + // 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('offers the last used description when the provided one is the same', () => { - // The deduplication drops the remembered copy of the provided name, which - // moved every later name up one place and offered the second most recent. + 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(); @@ -467,7 +486,7 @@ describe('ReportDialog', () => { fireEvent.change(descriptionField(), { target: { value: ' ' } }); fireEvent.click(saveButton()); - // The name the field offered, and not the provided one behind it. + // The name the field offered, and not the generic name behind it. expect(onSave).toHaveBeenCalledWith(expect.objectContaining({ reportName: 'Right kidney' })); }); @@ -502,10 +521,30 @@ 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 series description is truthy, so it was taken as the one name - // the count of 0 offers, and then dropped for being blank: the field - // opened empty and an emptied field saved an empty name. + // A blank loaded description must fall through to the generic name. setDisplaySets([{ ...CURRENT_SERIES, SeriesDescription: ' ' }]); const { onSave } = renderDialog({ rememberedDescriptionCount: 0, @@ -571,7 +610,7 @@ describe('ReportDialog', () => { fireEvent.keyDown(descriptionField(), { key: 'ArrowDown' }); fireEvent.keyDown(descriptionField(), { key: 'Enter' }); - // The second entry of the list, the field having started from the first. + // The field starts from the first entry, so the arrow key picks the second. expect(descriptionField().value).toBe('Left kidney'); expect(onSave).not.toHaveBeenCalled(); diff --git a/extensions/default/src/customizations/reportDialogCustomization.tsx b/extensions/default/src/customizations/reportDialogCustomization.tsx index 8c15a2e04a9..bfb657d242a 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -22,6 +22,13 @@ import { rememberSeriesDescription, } from '../utils/seriesDescriptionHistory'; +/** + * The dialog that stores a segmentation, a contour set or a measurement report. + * 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`. + */ + type DataSource = { value: string; label: string; @@ -47,11 +54,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,11 +96,13 @@ 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'. - * This is the last of the three names a new series starts from: the name the - * data was loaded from comes first, then the name last used for this type of - * item. + * The current name of the item, when the user can edit that name - the + * segmentation name, for example. A new series offers this name first. + */ + 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; /** @@ -127,6 +132,7 @@ function ReportDialog({ modality = 'SR', predecessorImageId, minSeriesNumber = 3000, + itemName = '', defaultSeriesDescription = '', itemType, rememberedDescriptionCount = 5, @@ -169,27 +175,11 @@ function ReportDialog({ }; }) .filter(series => { - // This save applies to a series when two things hold: the series holds - // the same type of object, which the modality filter above decides; and - // the series has a `predecessorImageId` value, which names the immediate - // prior object of that type that someone saved into the series. The - // save supersedes that one instance, and the `PredecessorSequence` - // provider reads the general image module of the instance through the - // value. - // - // A `SeriesInstanceUID` value meets neither condition, so this list does - // not fall back to one. A UID names a series and not an instance, so it - // names no prior object, and the provider finds no instance for it. The - // provider then raises an exception on `1 + Number(undefined)` while the - // adapter makes the object. A display set that the viewer downloaded and - // never stored has no value, and this list drops that display set. - // - // A local id, such as `dicomfile:3`, meets both conditions and stays in - // this list. The viewer registers an uploaded instance under a local id, - // and the provider resolves that id, so a user can save more than once - // against an uploaded instance. - // - // A dropped series still counts towards the number of a new series below. + // The value must be a `predecessorImageId`, which names the instance + // that this save supersedes. A `SeriesInstanceUID` value names no + // instance, and the provider throws on one - see the behaviour doc. The + // `seen` set keeps the values unique, because two display sets can + // belong to one series. if (!series.value || seen.has(series.value)) { return false; } @@ -214,12 +204,8 @@ function ReportDialog({ ); /** - * The series number offered for a new series - one past the existing ones. - * - * This number counts every loaded series of the modality, and not the series - * that the lists above offer. A series without a `predecessorImageId` value - * is not offered, and a count of the offered series alone therefore gave the - * next new series a number that a loaded series already holds. + * The series number offered for a new series - one past every loaded series of + * the modality, and not only the series that `existingSeries` offers. */ const defaultNewSeriesNumber = useMemo( () => @@ -233,47 +219,41 @@ function ReportDialog({ ); /** - * The series descriptions to offer for a new series, most likely to be wanted - * first. The first of them is the one the field starts with: - * - * 1. the description the data was loaded from, since a save of that same - * data into a new series usually keeps its name; - * 2. the descriptions last used for this type of item, most recent first; - * 3. `defaultSeriesDescription`, the name the caller provided for this data. - * - * The field started from `descriptionOptions[1]` before, which assumed that - * the provided name was always the first option and that the last used one - * therefore came second. Two cases broke that assumption, and both offered - * the *second* most recent name: a caller that provides no name at all, and a - * provided name that the user has already saved something as, which the - * deduplication below removes from the history. + * 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(() => { - // A `rememberedDescriptionCount` of 0 turns the list off, so the field gets - // a name to start from and there is nothing to pick from. The name has to - // be one that survives the filter below: a blank series description is - // truthy, so taking it here and dropping it there left no name at all, and - // an emptied field then saved an empty one. - const offered = !(rememberedDescriptionCount > 0) - ? [currentSeries?.description?.trim() ? currentSeries.description : defaultSeriesDescription] - : [ - currentSeries?.description, - ...getSeriesDescriptionHistory(itemType || modality, rememberedDescriptionCount), - defaultSeriesDescription, - ]; - - const options = offered.filter((option): option is string => !!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) + : []; + + // A blank name drops out here, so a name of spaces cannot hide a later name. + const offered = [ + itemName, + currentSeries?.description, + ...remembered, + defaultSeriesDescription, + ].filter((option): option is string => !!option?.trim()); + + const options = offered.filter( (option, index) => - options.findIndex(other => other.toLowerCase() === option.toLowerCase()) === index + offered.findIndex(other => other.toLowerCase() === option.toLowerCase()) === index ); - }, [currentSeries, defaultSeriesDescription, itemType, modality, rememberedDescriptionCount]); - /** - * The name a new series starts with, and what an emptied field falls back to, - * being the first of the options above. - */ + 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'); 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..741d758d0f9 --- /dev/null +++ b/platform/docs/docs/behaviours/report-dialog-save-destinations.md @@ -0,0 +1,126 @@ +# 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 +provider then throws +`TypeError: Cannot read properties of undefined (reading 'instanceNumber')` while +the adapter makes the object. The dialog offered a UID before, and a save into a +downloaded series then failed with that error. + +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 +count for the number of a new series still includes such a series: the count +reads every loaded series of the modality. A count of the offered series alone +gave a new series a number that a loaded series already held. + +## 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 current name of the item. The user can edit that name, so a + rename before the save reaches the field. `storeSegmentation` passes + `segmentation.label`. +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. + +- `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/migration-guide/3p13-to-3p14/report-dialog.md b/platform/docs/docs/migration-guide/3p13-to-3p14/report-dialog.md index dd2177dd3a4..cc2409fde67 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,8 +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 first of three names - the description the data was - loaded from, the one last used for this type of item, then + 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 @@ -64,7 +64,7 @@ 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 const { value, series, seriesNumber, dataSourceName, action } = @@ -74,19 +74,26 @@ 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 current name of the item, offered first for a new series + itemName: segmentation.label, + // New: the name for an item that has no other, offered last + defaultSeriesDescription: '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 current name of the item being stored, when the user can edit +that name. 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`. + +`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). @@ -97,22 +104,12 @@ 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 save applies to -that series. Two things must hold: the series holds the same type of object, -which is the modality being stored; and the series has a `predecessorImageId` -value, which names the immediate prior object of that type that someone saved -into the series. - -The dialog does not fall back to the `SeriesInstanceUID` value of the display -set, because that value meets neither condition. A `SeriesInstanceUID` value -names a series and not an instance, so it names no prior object, and it is not an -image id: the `PredecessorSequence` provider finds no instance for a UID, and the -provider then raises an exception while the adapter makes the object. A local -id, such as `dicomfile:3`, meets both conditions, and the dialog -offers a series that carries one: the viewer registers an uploaded instance under -a local id, so a user can save more than once against an uploaded instance. A -series that the dialog does not offer still counts towards the number that the -dialog offers for a new series. +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 @@ -199,9 +196,9 @@ Two new optional inputs control this: In the dialog, the `Save as new` description field: -- starts from the first of three names: 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; +- 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** From d5a23b8326a3d3b0602632878c1bd465554819aa Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 9 Sep 2026 14:19:23 -0400 Subject: [PATCH 07/10] fix(ReportDialog): offer a generated segmentation name last, not first A new series offers `itemName` first, and `storeSegmentation` passed `segmentation.label` as `itemName`. The label holds a name that the service invents for a new segmentation: `createSegmentationForViewport`, `_createSegmentationForDisplaySet` and the tmtv command each name one `Segmentation 1`, `Segmentation 2`, and so on. A generated name therefore outranked the descriptions that the user used before, and `Save as new` prefilled `Segmentation 1` instead of the name of the last save. `rememberSeriesDescription` then wrote the generated name into the history, and after five saves the history held generated names only. The migration guide states the opposite, and defect 2 of this branch asks for the opposite. `itemName` now means the name that the user chose. The service records the name that the service invents as `generatedLabel` on the segmentation, in the way that `predecessorImageId` is recorded, and the two callers that invent a name pass `labelIsGenerated: true`. `storeSegmentation` compares the label with `generatedLabel`: an equal label goes to `defaultSeriesDescription` and comes last, and a different label is the name of the user and comes first. A rename makes the two different, so no code has to clear the record. A segmentation that the viewer loaded has no record, so the `SeriesDescription` of that segmentation counts as a chosen name. A generated name stays reachable as the last option, so three new segmentations saved in one session still get three different names when the history is empty. Three new tests cover the record. The behaviour doc and the migration guide state the rule. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/commandsModule.ts | 13 +++-- .../SegmentationService.test.ts | 48 +++++++++++++++++++ .../SegmentationService.ts | 17 +++++++ .../utils/createSegmentationForViewport.ts | 1 + .../src/Panels/createReportDialogPrompt.tsx | 5 +- .../reportDialogCustomization.tsx | 5 +- extensions/tmtv/src/commandsModule.ts | 1 + .../report-dialog-save-destinations.md | 11 +++-- .../3p13-to-3p14/report-dialog.md | 12 +++-- 9 files changed, 97 insertions(+), 16 deletions(-) diff --git a/extensions/cornerstone-dicom-seg/src/commandsModule.ts b/extensions/cornerstone-dicom-seg/src/commandsModule.ts index 09046a452b0..ba997507853 100644 --- a/extensions/cornerstone-dicom-seg/src/commandsModule.ts +++ b/extensions/cornerstone-dicom-seg/src/commandsModule.ts @@ -321,10 +321,13 @@ const commandsModule = ({ throw new Error('No segmentation found'); } - const { label, predecessorImageId } = segmentation; - // The label is the editable name, and this is the name for an item - // without one. The dialog offers the label first, and this name last. - const defaultSeriesDescription = modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'; + const { label, predecessorImageId, generatedLabel } = segmentation; + // The dialog offers the name that the user chose first, ahead of the + // remembered descriptions. A generated name such as `Segmentation 3` is + // not such a name, so a generated name goes last instead. + const chosenLabel = label && label !== generatedLabel ? label : ''; + const defaultSeriesDescription = + (!chosenLabel && label) || (modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'); const { value: reportName, @@ -338,7 +341,7 @@ const commandsModule = ({ predecessorImageId, title: modality === 'RTSTRUCT' ? 'Save Contours' : 'Save Segmentation', modality, - itemName: label, + itemName: chosenLabel, defaultSeriesDescription, enableDownload: true, }); diff --git a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts index d5530dc9654..b6fc99e7563 100644 --- a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts +++ b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.test.ts @@ -1263,6 +1263,54 @@ describe('SegmentationService', () => { expect(retrievedSegmentationId).toEqual(expect.any(String)); }); + describe('generatedLabel', () => { + // `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 + // records the name that the service invents, so the save can tell the two + // apart. + const displaySet = { + imageIds: ['imageId'], + isDynamicVolume: false, + SeriesNumber: 1, + SeriesDescription: 'Series Description', + Modality: 'SEG', + } as unknown as AppTypes.DisplaySet; + + const createWith = async (options?: Record) => { + const stored = { segmentationId: 'created' } as cstTypes.Segmentation; + + jest + .spyOn(imageLoader, 'createAndCacheDerivedLabelmapImages') + .mockReturnValue([{ imageId: 'imageId' }] as csTypes.IImage[]); + jest + .spyOn(cstSegmentation.state, 'getSegmentations') + .mockReturnValue([{ segmentationId: 'segmentationId' }] as cstTypes.Segmentation[]); + jest.spyOn(cstSegmentation.state, 'getSegmentation').mockReturnValue(stored); + jest.spyOn(service, 'addOrUpdateSegmentation').mockReturnValue(undefined); + + await service.createLabelmapForDisplaySet(displaySet, options); + return stored; + }; + + it('records a label that the service invents', async () => { + const stored = await createWith(); + + expect(stored.generatedLabel).toBe('Segmentation 2'); + }); + + it('records nothing for a label that the caller gives', async () => { + const stored = await createWith({ label: 'Liver' }); + + expect(stored.generatedLabel).toBeUndefined(); + }); + + it('records a label that the caller reports as generated', async () => { + const stored = await createWith({ label: 'Segmentation 7', labelIsGenerated: true }); + + expect(stored.generatedLabel).toBe('Segmentation 7'); + }); + }); + it('should create a labelmap for a dynamic volume display set', async () => { const segmentationId = 'segmentationId'; const displaySet = { diff --git a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts index e0a54fe37e7..2633a8f34f7 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 @@ -526,6 +532,17 @@ class SegmentationService extends PubSubService implements ISegmentationServiceI } this.addOrUpdateSegmentation(segmentationPublicInput); + + // `storeSegmentation` compares the label with this name, to tell a name that + // the user chose from a generated one such as `Segmentation 3`. + if (options?.labelIsGenerated || !options?.label) { + const segmentation = this.getSegmentation(segmentationId); + + if (segmentation) { + segmentation.generatedLabel = label; + } + } + return segmentationId; } 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/Panels/createReportDialogPrompt.tsx b/extensions/default/src/Panels/createReportDialogPrompt.tsx index bcfe8ea5ae9..e70587b5fbc 100644 --- a/extensions/default/src/Panels/createReportDialogPrompt.tsx +++ b/extensions/default/src/Panels/createReportDialogPrompt.tsx @@ -11,8 +11,9 @@ 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. - * - `itemName` is the current name of the item, when the user can edit that - * name, such as the segmentation name. A new series offers this name first. + * - `itemName` is the name that the user chose for the item, such as the + * segmentation name. A new series offers this name first. A generated name + * is not such a name, and belongs in `defaultSeriesDescription`. * - `defaultSeriesDescription` is the name for an item that has no other name, * such as 'Contours' or 'Measurements'. A new series offers this name last. * - `itemType` is the type of item being stored, used as the key that the diff --git a/extensions/default/src/customizations/reportDialogCustomization.tsx b/extensions/default/src/customizations/reportDialogCustomization.tsx index bfb657d242a..0f06a25b2b1 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -96,8 +96,9 @@ type ReportDialogProps = { /** Lowest series number to use for a newly created series of this modality. */ minSeriesNumber?: number; /** - * The current name of the item, when the user can edit that name - the - * segmentation name, for example. A new series offers this name first. + * 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; /** 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/docs/docs/behaviours/report-dialog-save-destinations.md b/platform/docs/docs/behaviours/report-dialog-save-destinations.md index 741d758d0f9..22ec7f26ade 100644 --- a/platform/docs/docs/behaviours/report-dialog-save-destinations.md +++ b/platform/docs/docs/behaviours/report-dialog-save-destinations.md @@ -78,9 +78,14 @@ gave a new series a number that a loaded series already held. `Save as new` offers four names, and the field starts from the first name that is not blank: -1. `itemName`, the current name of the item. The user can edit that name, so a - rename before the save reaches the field. `storeSegmentation` passes - `segmentation.label`. +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 records + the invented name as `generatedLabel`. A label that is still equal to + `generatedLabel` goes to `defaultSeriesDescription` instead, so a generated + name cannot outrank the remembered descriptions, and cannot fill the history + with `Segmentation 1`, `Segmentation 2`, and so on. 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 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 cc2409fde67..9793bb7d91f 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 @@ -82,10 +82,14 @@ const { value, series, seriesNumber, dataSourceName, action } = }); ``` -`itemName` is the current name of the item being stored, when the user can edit -that name. 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`. +`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). `defaultSeriesDescription` is the name for an item that has no other name, and a new series offers `defaultSeriesDescription` last. The in-tree callers pass From d94ae189aa994338ea016db4672f2bdda2ca496f Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Wed, 9 Sep 2026 16:32:50 -0400 Subject: [PATCH 08/10] fix(ReportDialog): keep a provided name out of the description history `submit` remembered the description of every new series, and the description of a first save is the name that the caller provides. A new segmentation carries the generated `Segmentation 1`, the field offers that name when the history is empty, and a save then wrote `Segmentation 1` into the history. The dialog offered `Segmentation 1` as the name of the next, unrelated segmentation, whose own generated name is `Segmentation 2`. Commit d5a23b8326 moved a generated name to the back of the offer list, but the name still entered the history. The history now drops a description that is equal to `defaultSeriesDescription`, without regard to case or to the space at each end. The caller supplies that name at every save, and the dialog offers the name as the last of the four, so the history loses nothing and keeps the room for a name that the user chose. The behaviour doc claimed that a generated name cannot reach the history. The claim was wrong for the reason above, and the doc and the migration guide now state the rule that holds. Two new tests cover a save of the provided name and a typed copy of that name. The test of the separate history per type of item now types a name, because a save of `Contours` no longer records anything. Co-Authored-By: Claude Opus 5 --- .../reportDialogCustomization.test.ts | 26 ++++++++++++++++++- .../reportDialogCustomization.tsx | 10 ++++++- .../report-dialog-save-destinations.md | 10 +++++-- .../3p13-to-3p14/report-dialog.md | 7 +++-- 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/extensions/default/src/customizations/reportDialogCustomization.test.ts b/extensions/default/src/customizations/reportDialogCustomization.test.ts index e33a7782094..e41b53a13d0 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.test.ts +++ b/extensions/default/src/customizations/reportDialogCustomization.test.ts @@ -393,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 }); @@ -567,8 +590,9 @@ describe('ReportDialog', () => { 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', () => { diff --git a/extensions/default/src/customizations/reportDialogCustomization.tsx b/extensions/default/src/customizations/reportDialogCustomization.tsx index 0f06a25b2b1..adf04a9e269 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -327,7 +327,14 @@ function ReportDialog({ ? (targetSeries.description ?? '') : newSeriesDescription.trim() || baseSeriesDescription; - if (!targetSeries) { + // The history keeps a name that the user chose. The caller supplies + // `defaultSeriesDescription` at every save, such as the generated + // `Segmentation 3`, and the dialog offers the name in any case, so the + // history drops that name and keeps the room for a chosen one. + const isProvidedName = + storedDescription.toLowerCase() === defaultSeriesDescription.trim().toLowerCase(); + + if (!targetSeries && !isProvidedName) { rememberSeriesDescription( itemType || modality, storedDescription, @@ -352,6 +359,7 @@ function ReportDialog({ seriesNumber, newSeriesDescription, baseSeriesDescription, + defaultSeriesDescription, itemType, modality, rememberedDescriptionCount, diff --git a/platform/docs/docs/behaviours/report-dialog-save-destinations.md b/platform/docs/docs/behaviours/report-dialog-save-destinations.md index 22ec7f26ade..511785e7dcb 100644 --- a/platform/docs/docs/behaviours/report-dialog-save-destinations.md +++ b/platform/docs/docs/behaviours/report-dialog-save-destinations.md @@ -84,8 +84,7 @@ not blank: invents a name such as `Segmentation 3` for a new segmentation, and records the invented name as `generatedLabel`. A label that is still equal to `generatedLabel` goes to `defaultSeriesDescription` instead, so a generated - name cannot outrank the remembered descriptions, and cannot fill the history - with `Segmentation 1`, `Segmentation 2`, and so on. + name does not outrank the remembered descriptions. 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 @@ -108,6 +107,13 @@ 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 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 9793bb7d91f..03b2677d54f 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 @@ -210,8 +210,11 @@ In the dialog, the `Save as new` description field: 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`. From 8489777d8d7a46c165e1d14d23a05d51207163eb Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Thu, 10 Sep 2026 14:25:05 -0400 Subject: [PATCH 09/10] fix(ReportDialog): read the label origin from the state, not from two strings The review of the pull request found six defects. This commit repairs all six, and a companion commit in `@cornerstonejs/tools` repairs the one defect that the viewer cannot repair alone. **The service wrote a private attribute onto the state.** `SegmentationService` wrote `segmentation.generatedLabel` after `addOrUpdateSegmentation` returned. `SegmentationPublicInput.config` carries `labelIsGenerated` now, and `normalizeSegmentationInput` puts the flag on the segmentation. The value is explicit, because this config always carries a label, and the fallback of `normalizeSegmentationInput` reads `!config.label`. **The save compared two strings.** `storeSegmentation` read `label !== generatedLabel`. A user who renames a segmentation to the generated text, or who types that text, chose that name, and the comparison read the name as a name that no user chose: `itemName` went empty, and the description history then dropped the name. `storeSegmentation` reads `labelIsGenerated` now. `updateSegmentation` in `@cornerstonejs/tools` clears the flag on a rename. **The migration guide contradicted its own prose.** The example passed `itemName: segmentation.label` for every label, and passed a fixed `defaultSeriesDescription`. The example follows the in-tree caller now, and a new paragraph describes `labelIsGenerated`. **A null default series description threw.** `defaultSeriesDescription.trim()` ran on the value of the prop, and the default of the prop replaces `undefined` alone. An explicit null from a mode context, or from a customization, made `Save` throw a `TypeError`, and the save then stored nothing. The read is `?.` now. **An offered name kept its outer spaces.** The dialog dropped a name of spaces, and kept the spaces of every other name. An emptied field then stored a `SeriesDescription` with the spaces, and the trimmed history entry disagreed with the stored value. Every offered name loses its outer spaces now. **A comment named a read that moved.** `MetadataProvider` said that the `PredecessorSequence` module reads `generalImage.sopClassUID`. That module reads the SOP Common module after `fix(adapters): take the predecessor reference from the SOP Common module`. The comment gives the module definition as the reason now, in the provider and in the test. The added field itself is unchanged. The behaviour doc follows the same change, in the section on the name of a new series. `extensions/default`, `extensions/cornerstone-dicom-seg`, the two suites of `SegmentationService` and `MetadataProvider` give 137 of 137 tests, and `extensions/default` with `extensions/cornerstone-dicom-seg` gives 125 of 125. This pull request needs the `@cornerstonejs/tools` release that carries `labelIsGenerated`. `extensions/cornerstone/package.json` pins 5.8.2, and someone must raise the pin before the merge. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/commandsModule.ts | 10 +++-- extensions/cornerstone/src/commandsModule.ts | 3 ++ .../SegmentationService.test.ts | 40 +++++++++++-------- .../SegmentationService.ts | 16 +++----- .../reportDialogCustomization.test.ts | 35 ++++++++++++++++ .../reportDialogCustomization.tsx | 19 ++++----- .../core/src/classes/MetadataProvider.test.ts | 8 ++-- platform/core/src/classes/MetadataProvider.ts | 14 +++---- .../report-dialog-save-destinations.md | 10 +++-- .../3p13-to-3p14/report-dialog.md | 21 ++++++++-- 10 files changed, 117 insertions(+), 59 deletions(-) diff --git a/extensions/cornerstone-dicom-seg/src/commandsModule.ts b/extensions/cornerstone-dicom-seg/src/commandsModule.ts index ba997507853..864282d46e1 100644 --- a/extensions/cornerstone-dicom-seg/src/commandsModule.ts +++ b/extensions/cornerstone-dicom-seg/src/commandsModule.ts @@ -321,13 +321,15 @@ const commandsModule = ({ throw new Error('No segmentation found'); } - const { label, predecessorImageId, generatedLabel } = segmentation; + const { label, predecessorImageId, labelIsGenerated } = segmentation; // The dialog offers the name that the user chose first, ahead of the // remembered descriptions. A generated name such as `Segmentation 3` is - // not such a name, so a generated name goes last instead. - const chosenLabel = label && label !== generatedLabel ? label : ''; + // not such a name, so a generated name goes last instead. The state says + // which of the two the label is, so this reads no string against another: + // a user who types the generated name still chose the name. + const chosenLabel = labelIsGenerated ? '' : label || ''; const defaultSeriesDescription = - (!chosenLabel && label) || (modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'); + (labelIsGenerated && label) || (modality === 'RTSTRUCT' ? 'Contours' : 'Segmentation'); const { value: reportName, diff --git a/extensions/cornerstone/src/commandsModule.ts b/extensions/cornerstone/src/commandsModule.ts index 722f3d267a5..c5d0b6115fa 100644 --- a/extensions/cornerstone/src/commandsModule.ts +++ b/extensions/cornerstone/src/commandsModule.ts @@ -1873,6 +1873,9 @@ function commandsModule({ placeholder: i18n.t('Tools:Enter new label'), defaultValue: label, }).then(label => { + // The update clears `labelIsGenerated`, because the user chose this + // label. `storeSegmentation` then offers the label as the first name + // for a new series. 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 b6fc99e7563..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,11 +1265,13 @@ describe('SegmentationService', () => { expect(retrievedSegmentationId).toEqual(expect.any(String)); }); - describe('generatedLabel', () => { + 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 - // records the name that the service invents, so the save can tell the two - // apart. + // 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, @@ -1277,37 +1281,37 @@ describe('SegmentationService', () => { } as unknown as AppTypes.DisplaySet; const createWith = async (options?: Record) => { - const stored = { segmentationId: 'created' } as cstTypes.Segmentation; - jest .spyOn(imageLoader, 'createAndCacheDerivedLabelmapImages') .mockReturnValue([{ imageId: 'imageId' }] as csTypes.IImage[]); jest .spyOn(cstSegmentation.state, 'getSegmentations') .mockReturnValue([{ segmentationId: 'segmentationId' }] as cstTypes.Segmentation[]); - jest.spyOn(cstSegmentation.state, 'getSegmentation').mockReturnValue(stored); - jest.spyOn(service, 'addOrUpdateSegmentation').mockReturnValue(undefined); + const add = jest.spyOn(service, 'addOrUpdateSegmentation').mockReturnValue(undefined); await service.createLabelmapForDisplaySet(displaySet, options); - return stored; + return (add.mock.calls[0][0] as cstTypes.SegmentationPublicInput).config; }; - it('records a label that the service invents', async () => { - const stored = await createWith(); + it('marks a label that the service invents', async () => { + const config = await createWith(); - expect(stored.generatedLabel).toBe('Segmentation 2'); + expect(config.label).toBe('Segmentation 2'); + expect(config.labelIsGenerated).toBe(true); }); - it('records nothing for a label that the caller gives', async () => { - const stored = await createWith({ label: 'Liver' }); + it('marks nothing for a label that the caller gives', async () => { + const config = await createWith({ label: 'Liver' }); - expect(stored.generatedLabel).toBeUndefined(); + expect(config.label).toBe('Liver'); + expect(config.labelIsGenerated).toBe(false); }); - it('records a label that the caller reports as generated', async () => { - const stored = await createWith({ label: 'Segmentation 7', labelIsGenerated: true }); + it('marks a label that the caller reports as generated', async () => { + const config = await createWith({ label: 'Segmentation 7', labelIsGenerated: true }); - expect(stored.generatedLabel).toBe('Segmentation 7'); + expect(config.label).toBe('Segmentation 7'); + expect(config.labelIsGenerated).toBe(true); }); }); @@ -1359,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 2633a8f34f7..e774c800b69 100644 --- a/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts +++ b/extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts @@ -502,6 +502,12 @@ class SegmentationService extends PubSubService implements ISegmentationServiceI }, config: { label, + // The service invents a name such as `Segmentation 3` when the caller + // gives no label. `storeSegmentation` reads the flag, to tell a name + // that the user chose from a name that the service invented. The flag + // is explicit here, because this config always carries a label, and the + // fallback of `normalizeSegmentationInput` reads `!config.label`. + labelIsGenerated: options?.labelIsGenerated ?? !options?.label, fallbackLabel: `S:${displaySet.SeriesNumber} ${displaySet.Modality}`, segments: options?.segments && Object.keys(options.segments).length > 0 @@ -533,16 +539,6 @@ class SegmentationService extends PubSubService implements ISegmentationServiceI this.addOrUpdateSegmentation(segmentationPublicInput); - // `storeSegmentation` compares the label with this name, to tell a name that - // the user chose from a generated one such as `Segmentation 3`. - if (options?.labelIsGenerated || !options?.label) { - const segmentation = this.getSegmentation(segmentationId); - - if (segmentation) { - segmentation.generatedLabel = label; - } - } - return segmentationId; } diff --git a/extensions/default/src/customizations/reportDialogCustomization.test.ts b/extensions/default/src/customizations/reportDialogCustomization.test.ts index e41b53a13d0..4225e0e477c 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.test.ts +++ b/extensions/default/src/customizations/reportDialogCustomization.test.ts @@ -491,6 +491,41 @@ describe('ReportDialog', () => { 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'] }); diff --git a/extensions/default/src/customizations/reportDialogCustomization.tsx b/extensions/default/src/customizations/reportDialogCustomization.tsx index adf04a9e269..3b207a1f720 100644 --- a/extensions/default/src/customizations/reportDialogCustomization.tsx +++ b/extensions/default/src/customizations/reportDialogCustomization.tsx @@ -231,13 +231,12 @@ function ReportDialog({ ? getSeriesDescriptionHistory(itemType || modality, rememberedDescriptionCount) : []; - // A blank name drops out here, so a name of spaces cannot hide a later name. - const offered = [ - itemName, - currentSeries?.description, - ...remembered, - defaultSeriesDescription, - ].filter((option): option is string => !!option?.trim()); + // Every name loses its outer spaces here, so the stored name matches the + // name that the dialog shows, and the remembered name matches both. A + // blank name drops out, so a name of spaces cannot hide a later name. + const offered = [itemName, currentSeries?.description, ...remembered, defaultSeriesDescription] + .map(option => option?.trim()) + .filter((option): option is string => !!option); const options = offered.filter( (option, index) => @@ -330,9 +329,11 @@ function ReportDialog({ // The history keeps a name that the user chose. The caller supplies // `defaultSeriesDescription` at every save, such as the generated // `Segmentation 3`, and the dialog offers the name in any case, so the - // history drops that name and keeps the room for a chosen one. + // history drops that name and keeps the room for a chosen one. A caller + // that passes an explicit null supplies no such name, and `?.` keeps the + // save from throwing on that null. const isProvidedName = - storedDescription.toLowerCase() === defaultSeriesDescription.trim().toLowerCase(); + storedDescription.toLowerCase() === defaultSeriesDescription?.trim().toLowerCase(); if (!targetSeries && !isProvidedName) { rememberSeriesDescription( diff --git a/platform/core/src/classes/MetadataProvider.test.ts b/platform/core/src/classes/MetadataProvider.test.ts index 3a477a82613..a95f6931a5d 100644 --- a/platform/core/src/classes/MetadataProvider.test.ts +++ b/platform/core/src/classes/MetadataProvider.test.ts @@ -88,10 +88,10 @@ describe('MetadataProvider', () => { }; it('gives the SOP Class UID of the instance', () => { - // `@cornerstonejs/metadata` lists SOPClassUID in this module, and the - // adapters read `generalImage.sopClassUID` to build a - // ReferencedSOPClassUID. That Type 1 element was absent from every - // predecessor back pointer while this provider answered `undefined`. + // The DICOM standard lists SOPClassUID in the General Image module, and + // `@cornerstonejs/metadata` lists the attribute as well, so a consumer + // that asks either provider for this module expects the value. This + // provider answered `undefined` for the attribute. expect(metadataProvider.getTagFromInstance('generalImageModule', instance)).toMatchObject({ sopInstanceUID: 'sop-general-image', sopClassUID: '1.2.840.10008.5.1.4.1.1.88.33', diff --git a/platform/core/src/classes/MetadataProvider.ts b/platform/core/src/classes/MetadataProvider.ts index 92ee5a88e9e..c472054b823 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, - // `@cornerstonejs/metadata` lists SOPClassUID in this module, and a - // consumer that reads the module through either provider expects the - // value. The adapters build a ReferencedSOPClassUID from it: the - // PredecessorSequence module of `referencedMetadataProvider` reads - // `generalImage.sopClassUID` for the back pointer of a new revision, - // and that Type 1 element was absent from every such back pointer, - // because this provider answered `undefined` and dcmjs drops an - // undefined key when dcmjs denaturalizes a dataset. + // The DICOM standard lists SOPClassUID in the General Image module, + // and `@cornerstonejs/metadata` lists the attribute as well, so a + // consumer that asks either provider for this module expects the + // value. This provider answered `undefined`, and a consumer that + // reads the value off this module therefore got nothing. The SOP + // Common module of this provider already answers the same value. sopClassUID: instance.SOPClassUID, instanceNumber: toNumber(instance.InstanceNumber), lossyImageCompression: instance.LossyImageCompression, diff --git a/platform/docs/docs/behaviours/report-dialog-save-destinations.md b/platform/docs/docs/behaviours/report-dialog-save-destinations.md index 511785e7dcb..abb228e7335 100644 --- a/platform/docs/docs/behaviours/report-dialog-save-destinations.md +++ b/platform/docs/docs/behaviours/report-dialog-save-destinations.md @@ -81,10 +81,12 @@ 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 records - the invented name as `generatedLabel`. A label that is still equal to - `generatedLabel` goes to `defaultSeriesDescription` instead, so a generated - name does not outrank the remembered descriptions. + 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 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 03b2677d54f..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 @@ -67,6 +67,11 @@ the primary action on the right, rather than the right-aligned cluster `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, @@ -74,10 +79,11 @@ const { value, series, seriesNumber, dataSourceName, action } = title: 'Save Segmentation', modality: 'SEG', predecessorImageId, - // New: the current name of the item, offered first for a new series - itemName: 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: 'Segmentation', + defaultSeriesDescription: (labelIsGenerated && label) || 'Segmentation', enableDownload: true, }); ``` @@ -91,6 +97,15 @@ 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` From 3922393cdd2801ef658309c8cc2a29206c18a75f Mon Sep 17 00:00:00 2001 From: Bill Wallace Date: Thu, 10 Sep 2026 16:57:40 -0400 Subject: [PATCH 10/10] refactor(core): rename getSeriesDateTime to getLatestInstanceDateTime The name said that the function gives the date and the time of the series. 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 function reads `InstanceCreationDate`/`Time`, `ContentDate`/`Time`, `AcquisitionDate`/`Time`, `StructureSetDate`/`Time`, `PresentationCreationDate`/`Time`, and `SeriesDate`/`Time` last. The old name invites a defect. A handler of a derived display set calls the function, and the handler of an image display set does not: that handler writes `instance.SeriesDate` and `instance.SeriesTime` directly. A reader sees the difference, reads the image handler as a mistake, and changes the image handler to call the function. An image instance carries `AcquisitionDate` and `AcquisitionTime`, and those two attributes differ between the instances of one series, so each display set of one split series then gets a different key. The display sets stop tying on `dateTimeSortKey`, they no longer reach `compareSameSeriesDisplaySet`, and every comparison that `addSameSeriesCompare` registers goes inert. No error tells the user. The series list is simply in the wrong order. This commit renames the symbols only. No function body changes, and no test expectation changes. - `getSeriesDateTime` becomes `getLatestInstanceDateTime`. - `getSeriesDateTimeSortKey` becomes `getLatestInstanceDateTimeSortKey`. - The type `SeriesDateTime` becomes `LatestInstanceDateTime`. The two fields keep the names `SeriesDate` and `SeriesTime`, because a handler assigns the two fields to a display set under those names. - The module `platform/core/src/utils/seriesDateTime.ts` becomes `platform/core/src/utils/latestInstanceDateTime.ts`, and the test module moves with the module. The doc comment of the function now states the rule in full: a handler of a derived display set calls the function, the handler of an image display set must not call the function, and the comment gives the reason. The name alone cannot carry that rule. The `SeriesDate` contract of `DisplaySet.ts` said that every handler writes the value with this function, which is wrong for an image display set. That sentence names the two cases now, and the `dateTimeSortKey` comment of `sortStudy.ts` names the two cases as well. The function gets no deprecated alias. The name reached `platform/core` with OHIF/Viewers#6222 on the 3.14.0-beta.25 line, so the name was in no stable release, and no deployment can depend on the name. This commit also deletes `extensions/default/src/utils/getCurrentDicomDateTime.ts`. That file exported a second `getSeriesDateTime`, which gave the current date and time, so two exported functions held one name with opposite meanings. No module imports the file, `extensions/default/src/utils/index.ts` does not re-export the file, and `utils.getCurrentDicomDateTime` of `platform/core` supersedes the other export of the file. `platform/docs/docs/migration-guide/3p13-to-3p14/display-set-ordering.md` tells an integrator the old name, the new name, and the deletion of the file. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/getSopClassHandlerModule.ts | 2 +- .../src/getSopClassHandlerModule.ts | 2 +- .../src/getSopClassHandlerModule.ts | 2 +- .../src/getSopClassHandlerModule.ts | 4 +- .../SOPClassHandlers/chartSOPClassHandler.ts | 4 +- .../thumbnailDetailsCustomization.ts | 6 +- .../src/utils/getCurrentDicomDateTime.ts | 20 ------- .../dicom-pdf/src/getSopClassHandlerModule.js | 2 +- .../src/getSopClassHandlerModule.js | 2 +- .../studyBrowser/derivedDateTime.jsonc | 2 +- platform/core/src/classes/MetadataProvider.ts | 2 +- platform/core/src/types/DisplaySet.ts | 23 +++++--- platform/core/src/utils/index.ts | 14 ++--- ...test.js => latestInstanceDateTime.test.js} | 56 +++++++++---------- ...sDateTime.ts => latestInstanceDateTime.ts} | 34 ++++++++--- platform/core/src/utils/sortStudy.test.js | 4 +- platform/core/src/utils/sortStudy.ts | 17 +++--- .../utils/updateNewInstanceMetadata.test.js | 8 +-- .../src/utils/updateNewInstanceMetadata.ts | 6 +- .../docs/development/notes-requirements.md | 8 +-- .../3p13-to-3p14/display-set-ordering.md | 38 ++++++++++++- .../sampleCustomizations.tsx | 2 +- 22 files changed, 148 insertions(+), 110 deletions(-) delete mode 100644 extensions/default/src/utils/getCurrentDicomDateTime.ts rename platform/core/src/utils/{seriesDateTime.test.js => latestInstanceDateTime.test.js} (87%) rename platform/core/src/utils/{seriesDateTime.ts => latestInstanceDateTime.ts} (86%) 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/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/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/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/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.ts b/platform/core/src/classes/MetadataProvider.ts index 6c0c94a6603..57af76d7345 100644 --- a/platform/core/src/classes/MetadataProvider.ts +++ b/platform/core/src/classes/MetadataProvider.ts @@ -379,7 +379,7 @@ class MetadataProvider { // 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..4d0f564d6c0 100644 --- a/platform/core/src/types/DisplaySet.ts +++ b/platform/core/src/types/DisplaySet.ts @@ -70,16 +70,23 @@ 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. The two + * kinds of handler take the value from two different places, and a handler + * that takes the value from the wrong place breaks the order of the series + * list: * - * - 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`, and + * the handler reads those two attributes 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`. An image handler must not call + * `getLatestInstanceDateTime`, because that function reads + * `AcquisitionDate`/`AcquisitionTime` too, and those two attributes differ + * between the instances of one series. * - 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, and the handler + * calls `getLatestInstanceDateTime` for the value. 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..690cb966163 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,14 @@ 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. A + * handler of an image display set writes the `SeriesDate`/`SeriesTime` of the + * instance directly, and must not call that function - the next paragraph gives + * the reason. * * The key is read from the display set and never from `displaySet.instance`, * for two reasons. @@ -177,7 +180,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/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/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 = {