Skip to content
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

Merged

Conversation

cpinter
Copy link
Member

@cpinter 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

Copy link
Member

@lassoan lassoan left a 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.

@cpinter
Copy link
Member Author

cpinter commented May 27, 2021

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?

@lassoan
Copy link
Member

lassoan commented May 27, 2021

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.

@cpinter
Copy link
Member Author

cpinter commented May 27, 2021

OK no problem. Once we agree on this factory class I'll make the same fixes there.

@cpinter
Copy link
Member Author

cpinter commented May 27, 2021

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
@cpinter cpinter force-pushed the dicom-displayed-field-rules-factory branch from 3dcd225 to 321545e Compare May 28, 2021 15:19
@cpinter
Copy link
Member Author

cpinter commented May 28, 2021

@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).

Copy link
Member

@lassoan lassoan left a 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!

cpinter added a commit to cpinter/Slicer that referenced this pull request May 28, 2021
- 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
@lassoan lassoan merged commit 01ab373 into commontk:master May 28, 2021
cpinter added a commit to Slicer/Slicer that referenced this pull request May 28, 2021
- 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
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