-
Notifications
You must be signed in to change notification settings - Fork 565
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
BUG: Make translatable Editor Effects titles in Segmentations #7216
Conversation
a896a5e
to
934e344
Compare
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 is very good, just added a few comments, mostly should be easy to fix.
The only topic that may need some more discussion is how to initialize the title (in the constructor).
Modules/Loadable/Segmentations/EditorEffects/Python/SegmentEditorDrawEffect.py
Outdated
Show resolved
Hide resolved
Modules/Loadable/Segmentations/EditorEffects/Python/SegmentEditorFillBetweenSlicesEffect.py
Outdated
Show resolved
Hide resolved
Modules/Loadable/Segmentations/EditorEffects/qSlicerSegmentEditorAbstractEffect.cxx
Outdated
Show resolved
Hide resolved
void qSlicerSegmentEditorAbstractEffect::setTitle(QString title) | ||
{ | ||
Q_UNUSED(title); | ||
qCritical() << Q_FUNC_INFO << ": Cannot set effect title by method, only in constructor!"; |
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.
If you don't want to make effect title editable then remove this method.
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 method is required by scripted effects so that their title
property is writable.
/// This property stores the title of the effect | ||
/// Cannot be empty. | ||
/// \sa title(), \sa setTitle() | ||
Q_PROPERTY(QString title READ title WRITE setTitle) |
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.
Q_PROPERTY(QString title READ title WRITE setTitle) | |
Q_PROPERTY(QString title READ title) |
virtual QString title()const; | ||
/// Set the title of the effect | ||
/// NOTE: title must be defined in constructor in C++ effects, this can only be used in python scripted ones | ||
virtual void setTitle(QString title); |
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.
Why Python and C++ effects behave differently? We should keep the behavior the same for both (either both required to set the title in the constructor or neither).
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.
Why Python and C++ effects behave differently?
I made this to be coherent with the current definition of the class in which name
and perSegment
properties are meant, in C++ effects, to only be initialized from the constructor, not through a setter.
The different behaviour is justified by the fact that the setTitle
method is required for scripted effects to make the title
property writable (even in the constructor). What is not the case for C++ effects in which initialization may be done in constructors without using the setter.
I will however do what you suggest and keep the same setter's behaviour for all effects types. It should not be a problem since that property is not as critical as the name
which is used in other contexts.
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.
Yes, we want to keep the C++ and Python APIs as similar as possible to help people more easily go back and forth between languages.
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.
Thank you for your feedback, that's what is done in the last submitted commit.
setTitle
is now only defined in qSlicerSegmentEditorAbstractEffect
base class and inherited by all other derived effect classes.
Modules/Loadable/Segmentations/EditorEffects/qSlicerSegmentEditorAbstractEffect.h
Outdated
Show resolved
Hide resolved
Modules/Loadable/Segmentations/EditorEffects/qSlicerSegmentEditorAbstractEffect.h
Outdated
Show resolved
Hide resolved
5f2e1dc
to
080eaa2
Compare
Parent folder name is added to the context name is if the folder contains To resolve this, I'll reorganize the folder structure in the source tree to match the install tree. Do not merge until then. |
080eaa2
to
d948581
Compare
Enables fixing of these issues: