ENH: Create roles instead of hard-coded DICOM display fields #963
Conversation
Libs/DICOM/Core/ctkDICOMDisplayedFieldGeneratorLastStudyDateRule.cpp
Outdated
Show resolved
Hide resolved
Libs/DICOM/Core/ctkDICOMDisplayedFieldGeneratorLastStudyDateRule.cpp
Outdated
Show resolved
Hide resolved
Libs/DICOM/Core/ctkDICOMDisplayedFieldGeneratorLastStudyDateRule.cpp
Outdated
Show resolved
Hide resolved
cpinter
force-pushed
the
roles-instead-of-hardcoded-display-fields
branch
from
65b8ad0
to
8d16d91
Compare
|
Comment here if the code is ready for review (and remove "WIP" from the subject). |
cpinter
force-pushed
the
roles-instead-of-hardcoded-display-fields
branch
from
8d16d91
to
da49319
Compare
cpinter
changed the title
|
I wanted to do some more extensive testing first. Everything seems good, so if you agree then it's ready to be merged. |
|
One thing I am still thinking about is the best way to update the displayed fields after removing entries from the database. I created a ticket for it where we can discuss this: This work is not in the scope of this PR, I was just thinking ahead because now I have a pretty good understanding about how these displayed fields work, and when I get to doing the update on removal it will be old memory. |
Libs/DICOM/Core/ctkDICOMDatabase.cpp
Outdated
| @@ -3070,7 +2970,8 @@ void ctkDICOMDatabase::updateDisplayedFields() | |||
| Q_D(ctkDICOMDatabase); | |||
|
|
|||
| // Get the files for which the displayed fields have not been created yet (DisplayedFieldsUpdatedTimestamp is NULL) | |||
| //TODO: handle cases when the values actually changed; now we only cover insertion and schema update | |||
| // Note: The per-instance update only covers insertion and schema update. If fields on the series/study/patient level need to be | |||
| // updated on the insertion of a new instance, then it can be handled using the startUpdate/endUpdate functions of the roles. | |||
There was a problem hiding this comment.
| // updated on the insertion of a new instance, then it can be handled using the startUpdate/endUpdate functions of the roles. | |
| // updated on the insertion of a new instance, then it can be handled using the startUpdate/endUpdate functions of the rules. |
There was a problem hiding this comment.
Apparently I made this mistake at many places, including the branch name and commit message. I fixed them all except for the branch name.
| ctkDICOMDatabase* DICOMDatabase; | ||
| }; | ||
| #endif |
There was a problem hiding this comment.
why all the lines are marked as changed?
There was a problem hiding this comment.
Not sure why line endings in some files are converted to Windows style. Thanks for noticing.
There are DICOM display fields that do not concern only one instance, but a higher level such as series (number of images), study (number of series), or patient (last study date, number of studies). These fields were calculated using hard-coded functions. Now these have their own rule classes that do the necessary processing in their endUpdate functions.
cpinter
force-pushed
the
roles-instead-of-hardcoded-display-fields
branch
from
da49319
to
be87628
Compare
|
I made the changes. Please merge if there is nothing else. Thank you! |
There are DICOM display fields that do not concern only one instance, but a higher level such as series (number of images), study (number of series), or patient (last study date, number of studies). These fields were calculated using hard-coded functions. Now these have their own role classes that do the necessary processing in their endUpdate functions.