ENH: Add Gui Connection support for a subset of ctk Widgets

[HarryDC]

Adds support for : ctkCheckbox, ctkComboBox, ctkDoubleSlider, ctkDoubleSpinBox, ctkDoubleRangeSlider

Depends on commontk/CTK#1158

Adds some of the widgets from #7308

[HarryDC]

There was an interface change, the RangeWidget now requires a Default annotation. It seemed more consistent than let the user see the error message that comes up when requested range does not contain [0,0] which is the default value for a FloatRange. Optionally we could just soften this to only require a default annotation if the given range doesn't include [0,0]

At the moment connecting something like rangeParam: Annotated[FloatRange, RangeBounds(5, 10)] to a RangeWidget will fail with a validation error saying that [0,0] (the range default value) is not in the range [5,10] the value of the RangeBounds validator

[lassoan]

Thanks for working on this! If default value, range, etc. are not specified in Python, are the values specified in Qt Designer used?

[HarryDC]

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

[lassoan]

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.

[HarryDC]

@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

[lassoan]

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.

we can't really discern if a property is "set" it's just a normal python value, it always has a value

This is very good. Use that value if it is not specified already in type annotations.

I don't think this is in the scope of this PR

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

[HarryDC]

Thanks, I'll also talk this through with @jcfr and @sjh26 in the new year, i'll be off for the rest of the year. Happy Holidays !

[HarryDC]

@sjh26 @allemangD Added the last "simple" widgets, changed the requirement for Default in the range widget to only require it when the given FloatRange does not contain the [0,0] range.

This is ready for review.

[allemangD]

ctkDoubleRangeSlider doesn't provide setRange, so minimum and maximum need to be set separately for that widget. It causes ctkRangeWidgetToRangeConnector to fail tests.

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 ctkDoubleRangeSlider.

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 [0, 99] to [200, 300], the intermediate state [200, 99] is invalid. However I tested it and it looks like ctkDoubleRangeSlider automatically updates the other bound, so the intermediate state is [200, 200]. Maybe this was something that only affected the built-in qt widgets?

https://github.com/Slicer/Slicer/pull/7499/files#diff-d723feed0f367e90d79c12b367958ba3b5ad3085f1d0c786b95ac9733fd34427L512-R540

[HarryDC]

@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

[allemangD]

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.

[allemangD]

Yep, rebase fixes the issue. It should be fine to rebase-merge from GitHub.