ENH: Migrate ctkDICOMModalities to ctkDICOMDatabase accessors (stacked on #1418) - #1411
ENH: Migrate ctkDICOMModalities to ctkDICOMDatabase accessors (stacked on #1418)#1411hjmjohnson wants to merge 4 commits into
Conversation
fedd09d to
dd62b37
Compare
|
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
|
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
dd62b37 to
fb41b04
Compare
|
@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 Follow-up PR (Phase B) will migrate Before I write Phase B, one design question on the migration: several internal call sites that read these lists today (e.g.
Happy to follow whichever pattern matches the existing conventions in |
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
|
@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 Phase B is now pushed as three commits on top of the framework commit:
Local Qt5 build of I'll let CI run and address any platform-specific issues. Ready for another look whenever you have time. |
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
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
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
Migrates the
ctkDICOMModalitiesnamespace from a header-only definition to deprecated free functions, with the supported-modality lists moved ontoctkDICOMDatabaseas the new home. Stacked on #1418 (theCTK_DEPRECATED_SINCEframework).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
ENH: Add modality list accessors to ctkDICOMDatabase— addssupportedModalities(),defaultModalities(), andmodalitiesExcludedFromThumbnailGeneration()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).ENH: Migrate DICOM modality call sites to ctkDICOMDatabase accessors— switches every in-tree caller to the newctkDICOMDatabaseaccessors.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.cppso they no longer pollute the header. External consumers continue to compile until they raiseCTK_DISABLE_DEPRECATED_BEFORE.Stacking / merge order
CTK_DEPRECATED_SINCEframework onlyctkDICOMModalities→ctkDICOMDatabasemigrationBoth 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.