Skip to content

COMP: Make EditorEffects base classes available for extensions - #1277

Open
lassoan wants to merge 1 commit into
Slicer:masterfrom
lassoan:expose-segment-editor-effects-library-for-extensions
Open

COMP: Make EditorEffects base classes available for extensions#1277
lassoan wants to merge 1 commit into
Slicer:masterfrom
lassoan:expose-segment-editor-effects-library-for-extensions

Conversation

@lassoan

@lassoan lassoan commented Dec 5, 2019

Copy link
Copy Markdown
Contributor

SlicerMacroBuildModuleQtLibrary macro does not add module include directories to CMake variables that are exposed to extensions. However, we need Segmentations/EditorEffects classes to be available for extensions to allow creating editor effects in C++.

See background in https://discourse.slicer.org/t/unable-to-include-qslicersegmentationseditoreffects-when-building-extension/9371/5

SlicerMacroBuildModuleQtLibrary macro does not add module include directories to CMake variables that are exposed to extensions.
However, we need Segmentations/EditorEffects classes to be available for extensions to allow creating editor effects in C++.
@lassoan
lassoan requested a review from jcfr December 5, 2019 02:20
@lassoan

lassoan commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

@jcfr is there any drawback of using SlicerMacroBuildModuleWidgets macro for exposing Segment Editor effects C++ base classes to extensions? Do you have any alternative suggestions? We used SlicerMacroBuildModuleQtLibrary before but that did not make these classes available for extensions.

@lassoan

lassoan commented Dec 6, 2019

Copy link
Copy Markdown
Contributor Author

@jcfr can you have a look? Just tell me if it's OK to use the macro like this. Thanks!

@jcfr

jcfr commented Jan 7, 2020

Copy link
Copy Markdown
Member

I don't foresee any issues. That said, I suggest we introduce a new CMake function called SlicerMacroBuildModuleEditorEffect

And as discussed, a new module template should be added.

@hjmjohnson

Copy link
Copy Markdown
Member

@lassoan Can this be merged as is with the new macro added later?

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.

3 participants