-
Notifications
You must be signed in to change notification settings - Fork 497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Add DICOM displayed field generator rule factory #970
ENH: Add DICOM displayed field generator rule factory #970
Conversation
cpinter
commented
May 25, 2021
- DICOM displayed rules are created for every new database using a factory class
- Allows the indexer to use an up-to-date list of generator rules when updating the displayed fields after importing new data
- The displayed field generator sets up the displayed field rules when setting the database, and also on each startUpdate so that it always has an up-to-date rule list
- The factory uses a new DisplayedFieldGeneratorRules table in the database for providing the proper rule set
- The table defines for each rule (by name) whether they are enabled and may provide an options string (for later use)
- Database schema version has been increased to 0.7.0
- If the table does not exist (e.g. using another or a custom schema) or if there is no entry for a rule (omitted from the list for any reason), then the rule is treated as enabled by default. So basically it is enough to specify the disabled rules in this table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks very good, just added a few small comments.
Thanks for the comments @lassoan ! I took the factory class basically as is from the recently integrated pluggable markups infrastructure in Slicer, assuming that after the lengthy review process it can be considered perfect. Would you like me to make these changes in that class as well? |
Yes, please. That pull request contained 200+ file changes, so I'm quite sure many things needed further fine-tuning, but it was not possible to hold up the entire build wgile these were sorted out. |
OK no problem. Once we agree on this factory class I'll make the same fixes there. |
I just noticed that apparently the markups widget factory was cloned from qSlicerSegmentEditorEffectFactory, so I'll fix that one as well :) Update: Also qSlicerSubjectHierarchyPluginHandler. If these changes do not lead far I can fix these, but if there are complications I might not have the capacity. |
- DICOM displayed rules are created for every new database using a factory class - Allows the indexer to use an up-to-date list of generator rules when updating the displayed fields after importing new data - The displayed field generator sets up the displayed field rules when setting the database, and also on each startUpdate so that it always has an up-to-date rule list - The factory uses a new DisplayedFieldGeneratorRules table in the database for providing the proper rule set - The table defines for each rule (by name) whether they are enabled and may provide an options string (for later use) - Database schema version has been increased to 0.7.0 - If the table does not exist (e.g. using another or a custom schema) or if there is no entry for a rule (omitted from the list for any reason), then the rule is treated as enabled by default. So basically it is enough to specify the disabled rules in this table
3dcd225
to
321545e
Compare
@lassoan I made the requested changes, and it works. I propagated those to the factory classes in Slicer I mentioned before (that were basically identical), however two out of three do not work yet (the decorator class does not seem to be considered). I think this could be merged if it looks good. I need to spend a bit more time on the Slicer factories, however, that is a different story and I can do it in the meantime (and maybe connect it with the CTK hash update). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thank you!
- Remove public setInstance function and do the cleanup in a private cleanup function - Add PythonQtDecorator classes to be able to use the instance function as static in Python to create the factory class - Make constructor and destructor private to prevent accidental wrong use (add PythonQtWrapper generated class as friend to allow this) - Update usage of these factories that used the workaround to fix non-static instance function Re commontk/CTK#970
- Remove public setInstance function and do the cleanup in a private cleanup function - Add PythonQtDecorator classes to be able to use the instance function as static in Python to create the factory class - Make constructor and destructor private to prevent accidental wrong use (add PythonQtWrapper generated class as friend to allow this) - Update usage of these factories that used the workaround to fix non-static instance function Re commontk/CTK#970