Skip to content

ENH: Migrate ctkDICOMModalities to ctkDICOMDatabase accessors (stacked on #1418) - #1411

Open
hjmjohnson wants to merge 4 commits into
commontk:masterfrom
BRAINSia:fix-dicom-modalities-deprecation-api
Open

ENH: Migrate ctkDICOMModalities to ctkDICOMDatabase accessors (stacked on #1418)#1411
hjmjohnson wants to merge 4 commits into
commontk:masterfrom
BRAINSia:fix-dicom-modalities-deprecation-api

Conversation

@hjmjohnson

@hjmjohnson hjmjohnson commented Apr 2, 2026

Copy link
Copy Markdown
Contributor

Migrates the ctkDICOMModalities namespace from a header-only definition to deprecated free functions, with the supported-modality lists moved onto ctkDICOMDatabase as the new home. Stacked on #1418 (the CTK_DEPRECATED_SINCE framework).

This PR was previously the four-commit "framework + migration" series. The framework commit has been split out to #1418 to make review easier; this PR now contains only the three migration commits and will rebase to a clean series once #1418 merges.

What this PR changes
  1. ENH: Add modality list accessors to ctkDICOMDatabase — adds supportedModalities(), defaultModalities(), and modalitiesExcludedFromThumbnailGeneration() as the new authoritative home for the lists, per @lassoan's review on the original draft of ENH: Migrate ctkDICOMModalities to ctkDICOMDatabase accessors (stacked on #1418) #1411 (comment).
  2. ENH: Migrate DICOM modality call sites to ctkDICOMDatabase accessors — switches every in-tree caller to the new ctkDICOMDatabase accessors.
  3. ENH: Deprecate ctkDICOMModalities namespace and move definitions out-of-line — wraps the legacy free functions in #if CTK_DEPRECATED_SINCE(0, 1) and moves their bodies to a .cpp so they no longer pollute the header. External consumers continue to compile until they raise CTK_DISABLE_DEPRECATED_BEFORE.
Stacking / merge order
PR Scope Status
#1418 CTK_DEPRECATED_SINCE framework only depends on nothing; merge first
this PR (#1411) ctkDICOMModalitiesctkDICOMDatabase migration depends on #1418

Both PRs target master. While #1418 is open, the framework commit appears in both PRs' commit lists; once #1418 merges, GitHub will rebase this branch and only the three migration commits will remain.

@lassoan

lassoan commented Apr 3, 2026

Copy link
Copy Markdown
Member

Having a clear deprecation framework for Qt classes would be nice.

I would choose a different solution for the new implementation of the deprecated methods, as ctkDICOMModalities should be completely phased out in the long term:

  • AllModalities and CommonImagingModalities: the contents of these lists are application-dependent. There should be a way developers can change this list. They should be methods in ctkDICOMDatabase because that is already a place where application-specific behavior can be configured by registering ctkDICOMDisplayedFieldGenerator rules and various other settings. -> We should replace ctkDICOMModalities::AllModalities and ctkDICOMModalities::CommonImagingModalities by ctkDICOMDatabase::supportedModalities and ctkDICOMDatabase::defaultModalities.
  • ExcludedFromThumbnailGeneration: only the thumbnail generators know what modalities they can process, so it does not make sense to statically hardcode the values in another class. -> We should replace ctkDICOMModalities::excludedFromThumbnailGeneration by ctkDICOMDatabase::modalitiesExcludedFromThumbnailGeneration.

Introduces a versioned API deprecation system modeled after Qt's
QT_DEPRECATED_SINCE so that future PRs can evolve CTK public APIs
without breaking external projects.

New header Libs/ctkDeprecated.h provides:

  CTK_VERSION_CHECK(major, minor, patch)
  CTK_DEPRECATED_SINCE(major, minor)

CMakeLists.txt adds a CTK_DISABLE_DEPRECATED_BEFORE cache variable
(hex 0xMMmmpp, default 0x000000 = keep all). ctkMacroBuildLib.cmake
propagates the configured value as a PUBLIC compile definition so
downstream consumers automatically inherit the project-wide setting,
and adds ${CTK_SOURCE_DIR}/Libs to each library's include path so
that ctkDeprecated.h can be included from anywhere in the build tree.

Libs/ctkDeprecated.h is installed to ${CTK_INSTALL_INCLUDE_DIR} so
that installed CTK headers can use the macros against external
consumers.

This commit only adds the framework. No CTK API is migrated yet.
Follow-up PRs will use CTK_DEPRECATED_SINCE() to evolve specific
APIs (e.g. ctkDICOMModalities) one at a time.

Assisted-by: Claude Code -- mechanical refactor of commontk#1411 to isolate
  the deprecation framework from the ctkDICOMModalities migration
@hjmjohnson
hjmjohnson force-pushed the fix-dicom-modalities-deprecation-api branch from dd62b37 to fb41b04 Compare April 27, 2026 12:51
@hjmjohnson hjmjohnson changed the title ENH: Add CTK_DEPRECATED_SINCE infrastructure and migrate ctkDICOMModalities public API ENH: Add CTK_DEPRECATED_SINCE API deprecation framework Apr 27, 2026
@hjmjohnson

Copy link
Copy Markdown
Contributor Author

@lassoan thanks for the review. I've split this PR into two phases per your feedback:

This PR (Phase A) is now framework-only: it just adds the CTK_DEPRECATED_SINCE macro infrastructure (in a new standalone Libs/ctkDeprecated.h, not in ctkExport.h.in). No CTK API is migrated.

Follow-up PR (Phase B) will migrate ctkDICOMModalities per your design — ctkDICOMDatabase::supportedModalities / defaultModalities / modalitiesExcludedFromThumbnailGeneration as the new home, with the existing ctkDICOMModalities::* static lists kept behind CTK_DEPRECATED_SINCE(0,1) for source compatibility during the transition.

Before I write Phase B, one design question on the migration: several internal call sites that read these lists today (e.g. ctkDICOMSeriesModel, ctkDICOMPatientModel, ctkDICOMStudyModel) don't currently hold a ctkDICOMDatabase*. Options for those callers:

  1. Add a setDatabase(ctkDICOMDatabase*) to each model and require it be set before use.
  2. Keep a fallback to the static defaults when no database is attached.
  3. Something else you'd prefer.

Happy to follow whichever pattern matches the existing conventions in ctkDICOMDatabase callers.

Per @lassoan's review on PR commontk#1411, modality lists are
application-dependent and should be configurable on
ctkDICOMDatabase rather than baked into a static namespace
header.

Adds three Q_PROPERTY-backed list accessors:

  supportedModalities()                       <- AllModalities
  defaultModalities()                         <- CommonImagingModalities
  modalitiesExcludedFromThumbnailGeneration() <- ExcludedFromThumbnailGeneration

with matching setters, change-notification signals, and
Q_INVOKABLE bindings so applications can override the lists at
runtime (e.g. to extend supported modalities with project-specific
codes or to restrict the default UI selection).

Each list is seeded in ctkDICOMDatabasePrivate's constructor with
the historical defaults from ctkDICOMModalities so existing
callers see no behavior change. ctkDICOMModalities itself remains
in place for now; subsequent commits will migrate internal callers
and then deprecate the static namespace.

Assisted-by: Claude Code -- mechanical refactor of commontk#1411 per
  lassoan's design feedback
All five internal callers of ctkDICOMModalities now consult the
attached ctkDICOMDatabase first and fall back to the static
ctkDICOMModalities lists only when no database is yet attached:

  Libs/DICOM/Core/ctkDICOMSeriesModel.cpp     (ctor seed)
  Libs/DICOM/Core/ctkDICOMPatientModel.cpp    (ctor seed)
  Libs/DICOM/Core/ctkDICOMStudyModel.cpp      (ctor seed)
  Libs/DICOM/Core/ctkDICOMScheduler.cpp       (thumbnail-exclusion check)
  Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp (5 sites)

The fallback is required because the model classes' DicomDatabase
member is only assigned after construction via setDicomDatabase().
At construction time the static lists are still the correct
defaults; once a database is attached, applications can override
the lists per ctkDICOMDatabase via setSupportedModalities() etc.

Behavior is unchanged from the user's point of view: the database
is seeded with the historical static defaults, so the migrated
code paths produce the same lists they did before unless an
application explicitly overrides them.

Assisted-by: Claude Code -- mechanical migration of the call sites
  from commontk#1411 per lassoan's design feedback
…of-line

Marks the ctkDICOMModalities::AllModalities,
ExcludedFromThumbnailGeneration, and CommonImagingModalities
namespace-scope variables as deprecated since CTK 0.1, guarded by
CTK_DEPRECATED_SINCE(0, 1) so that downstream projects can opt out
of the deprecated API by configuring with
-DCTK_DISABLE_DEPRECATED_BEFORE=0x000100.

The previous header definitions used `static const QStringList X = {...}`
inside the namespace, which gave each translation unit including the
header its own internally-linked heap-backed copy of every list (~200
QString allocations per TU). clazy reported these as
non-pod-global-static (SIOF risk).

Both problems are fixed by:

  1. Moving the data into Meyers' singletons in the new
     ctkDICOMModalities.cpp (one shared copy across the whole
     CTKDICOMCore library).

  2. Replacing the in-header definitions with
     `extern const QStringList&` references that bind to those
     singletons. References to function-local statics have trivial
     namespace-scope construction, so clazy's warning no longer
     fires.

Source compatibility is preserved: any external caller that still
writes `ctkDICOMModalities::AllModalities` continues to compile and
gets a deprecation warning pointing at the ctkDICOMDatabase
replacement.

Internal callers were already migrated to the ctkDICOMDatabase
accessors in the previous commit, so this commit changes no
behavior other than removing the per-TU duplication and emitting
deprecation diagnostics.

Assisted-by: Claude Code -- mechanical refactor of commontk#1411 per
  lassoan's design feedback
@hjmjohnson

Copy link
Copy Markdown
Contributor Author

@lassoan I went ahead with option 2 — fall back to the static defaults when no database is attached — as I think that's the correct choice here. It preserves existing behavior 1:1 (the model classes initialize their ModalityFilter member at construction, before any setDicomDatabase() call, and the database is seeded with the historical static defaults anyway), while letting applications override the lists per ctkDICOMDatabase once one is attached.

Phase B is now pushed as three commits on top of the framework commit:

Commit What
2dc4c7e0c Add supportedModalities() / defaultModalities() / modalitiesExcludedFromThumbnailGeneration() accessors (with setters, change-notification signals, and Q_PROPERTY bindings) to ctkDICOMDatabase, seeded from the existing static defaults.
fd2b206c3 Migrate the five internal call sites (ctkDICOMSeriesModel, ctkDICOMPatientModel, ctkDICOMStudyModel, ctkDICOMScheduler, ctkDICOMVisualBrowserWidget) to query the database first and fall back to ctkDICOMModalities::* only when no DB is attached.
148c9e2c7 Mark ctkDICOMModalities::AllModalities / ExcludedFromThumbnailGeneration / CommonImagingModalities as deprecated since CTK 0.1, move the data into Meyers' singletons in a new ctkDICOMModalities.cpp, and replace the in-header definitions with extern const QStringList&. Source compatibility is preserved; clazy's non-pod-global-static warning is gone (one shared copy across the whole library instead of one per TU).

Local Qt5 build of CTKDICOMCore and CTKDICOMWidgets is clean.

I'll let CI run and address any platform-specific issues. Ready for another look whenever you have time.

@hjmjohnson
hjmjohnson marked this pull request as ready for review April 29, 2026 23:15
@hjmjohnson hjmjohnson changed the title ENH: Add CTK_DEPRECATED_SINCE API deprecation framework ENH: Migrate ctkDICOMModalities to ctkDICOMDatabase accessors (stacked on #1418) May 5, 2026
hjmjohnson added a commit to BRAINSia/CTK that referenced this pull request May 25, 2026
Introduces a versioned API deprecation system modeled after Qt's
QT_DEPRECATED_SINCE so that future PRs can evolve CTK public APIs
without breaking external projects.

New header Libs/ctkDeprecated.h provides:

  CTK_VERSION_CHECK(major, minor, patch)
  CTK_DEPRECATED_SINCE(major, minor)

CMakeLists.txt adds a CTK_DISABLE_DEPRECATED_BEFORE cache variable
(hex 0xMMmmpp, default 0x000000 = keep all). ctkMacroBuildLib.cmake
propagates the configured value as a PUBLIC compile definition so
downstream consumers automatically inherit the project-wide setting,
and adds ${CTK_SOURCE_DIR}/Libs to each library's include path so
that ctkDeprecated.h can be included from anywhere in the build tree.

Libs/ctkDeprecated.h is installed to ${CTK_INSTALL_INCLUDE_DIR} so
that installed CTK headers can use the macros against external
consumers.

This commit only adds the framework. No CTK API is migrated yet.
Follow-up PRs will use CTK_DEPRECATED_SINCE() to evolve specific
APIs (e.g. ctkDICOMModalities) one at a time.

Assisted-by: Claude Code -- mechanical refactor of commontk#1411 to isolate
  the deprecation framework from the ctkDICOMModalities migration
lassoan pushed a commit to BRAINSia/CTK that referenced this pull request Jun 2, 2026
Introduces a versioned API deprecation system modeled after Qt's
QT_DEPRECATED_SINCE so that future PRs can evolve CTK public APIs
without breaking external projects.

New header Libs/ctkDeprecated.h provides:

  CTK_VERSION_CHECK(major, minor, patch)
  CTK_DEPRECATED_SINCE(major, minor)

CMakeLists.txt adds a CTK_DISABLE_DEPRECATED_BEFORE cache variable
(hex 0xMMmmpp, default 0x000000 = keep all). ctkMacroBuildLib.cmake
propagates the configured value as a PUBLIC compile definition so
downstream consumers automatically inherit the project-wide setting,
and adds ${CTK_SOURCE_DIR}/Libs to each library's include path so
that ctkDeprecated.h can be included from anywhere in the build tree.

Libs/ctkDeprecated.h is installed to ${CTK_INSTALL_INCLUDE_DIR} so
that installed CTK headers can use the macros against external
consumers.

This commit only adds the framework. No CTK API is migrated yet.
Follow-up PRs will use CTK_DEPRECATED_SINCE() to evolve specific
APIs (e.g. ctkDICOMModalities) one at a time.

Assisted-by: Claude Code -- mechanical refactor of commontk#1411 to isolate
  the deprecation framework from the ctkDICOMModalities migration
lassoan pushed a commit that referenced this pull request Jun 2, 2026
Introduces a versioned API deprecation system modeled after Qt's
QT_DEPRECATED_SINCE so that future PRs can evolve CTK public APIs
without breaking external projects.

New header Libs/ctkDeprecated.h provides:

  CTK_VERSION_CHECK(major, minor, patch)
  CTK_DEPRECATED_SINCE(major, minor)

CMakeLists.txt adds a CTK_DISABLE_DEPRECATED_BEFORE cache variable
(hex 0xMMmmpp, default 0x000000 = keep all). ctkMacroBuildLib.cmake
propagates the configured value as a PUBLIC compile definition so
downstream consumers automatically inherit the project-wide setting,
and adds ${CTK_SOURCE_DIR}/Libs to each library's include path so
that ctkDeprecated.h can be included from anywhere in the build tree.

Libs/ctkDeprecated.h is installed to ${CTK_INSTALL_INCLUDE_DIR} so
that installed CTK headers can use the macros against external
consumers.

This commit only adds the framework. No CTK API is migrated yet.
Follow-up PRs will use CTK_DEPRECATED_SINCE() to evolve specific
APIs (e.g. ctkDICOMModalities) one at a time.

Assisted-by: Claude Code -- mechanical refactor of #1411 to isolate
  the deprecation framework from the ctkDICOMModalities migration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants