-
Notifications
You must be signed in to change notification settings - Fork 521
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 Gui Connection support for a subset of ctk Widgets #7499
base: main
Are you sure you want to change the base?
Conversation
de494fc
to
8bb99ec
Compare
There was an interface change, the RangeWidget now requires a At the moment connecting something like |
Thanks for working on this! If default value, range, etc. are not specified in Python, are the values specified in Qt Designer used? |
With regards to the default in Qt designer i'd have to check, it all depends on the order and if we're doing the first write from the parameternode to the gui or vice versa (and I don't know which way it is). But the latter hides the danger that the gui could overwrite an already set parameter. I haven't inspected the connect call at all, but I don't think that we can distinguish between a field of a parameter node that has been set and one that hasn't. We can distinguish between one that has a default value and one that hasn't. But i think it would be more transparent to let the parameter node be the carrier of it's value rather than have the gui determine the default value otherwise it could happen that you overwrite already set values just because the GUI has different defaults |
I see that implementation of parameter node wrapper would be easier if Qt designer inputs could be ignored. However, we cannot do that, because it would be just too embarrassing to explain developers that all the properties they set in Qt designer are ignored if those properties can be also set in the parameter node wrapper. They would need to look up the documentation for every property to know if they can set it in Qt designer or they must set it in Python. If a property is set in both QT designer and in Python then it is OK that the one specified in Python is used. But if a property is only set in Qt designer then it must not be overwritten by some defaults in the parameter node wrapper. |
@lassoan i'd have to look if this can be accomplished, right now we can't really discern if a property is "set" it's just a normal python value, it always has a value. I don't think this is in the scope of this PR |
This PR introduces a new error: "Cannot have a connection to a range widget where the float is unbounded. Add a RangeBounds annotation", which is really confusing. The developer has already set up the widget in Qt designer (either explicitly setting the property or leaving the default) and now the GUI connector is complaining that the range has not been specified. The GUI connector has access to the widget, so if the range has not been explicitly set in the parameter node wrapper then the GUI connector should just get the range from the widget. This is especially true because the range Is only required because of the widget.
This is very good. Use that value if it is not specified already in type annotations.
This PR is already an improvement: it forces the user to implement a workaround (to specify range again in the parameter node wrapper) and this workaround will remain functional after reading of defaults from the widget is implemented. So, I agree that it is acceptable to merge this PR and fix the remaining issues later - then just to make sure we don't forget about this, please add a task to an existing issue or submit a new issue, and add a link to it from here (maybe also add a link as a content in the code where we now display the error message about requiring the range). Thank you for your hard work on this and sorry for being a bit demanding. I may feel strongly about this because I recently gave a training to developers new to Slicer and saw how hard it was for them to grasp how all this parameter definition works. I would really like to remove all unnecessary redundancies and inconsistencies to improve the experience for new developers (even at the cost of more work for us, Slicer core developers). |
a51228d
to
56ed7e5
Compare
@sjh26 @allemangD Added the last "simple" widgets, changed the requirement for This is ready for review. |
I'm not sure if it would be better to set minimum and maximum separately for all range widgets, or to include a special case for just I recall some issue from discussions with Connor about setting limits on ranges to be sure that intermediate states are valid; for example if we change limits from |
@allemangD Did you update slicer ? JC implemented the setRange in commontk/CTK#1158 that;s been merged ? Although maybe Slicer CTK needs to be updated as well |
I just checked out this PR and clean build. Nothing special otherwise. Looks like CTK is updated in Slicer yesterday, #7544, so probably easiest to just rebase this branch on main. I'll do that and test locally. In any case it's certainly better to have a consistent interface for the widgets than to work around it in Python. |
Yep, rebase fixes the issue. It should be fine to rebase-merge from GitHub. |
ctkCheckbox, ctkComboBox, ctkDoubleSlider, ctkDoubleSpinBox ctkDoubleRangeSlider
also refactor the tests for clarity
27b6f56
to
73250c5
Compare
Adds support for :
ctkCheckbox
,ctkComboBox
,ctkDoubleSlider
,ctkDoubleSpinBox
,ctkDoubleRangeSlider
Depends on commontk/CTK#1158
Adds some of the widgets from #7308