-
Notifications
You must be signed in to change notification settings - Fork 1
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
Switch/Optional widget fields setter/getter #148
Changes from all commits
9288132
3507b6e
7db8ba9
81fd146
27b7b8a
9373eb0
8970630
4647678
18b2eb1
08152f8
3ee9571
eca46f7
0a0a0b1
4d3efe9
4807716
b056134
44c01d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,10 +14,12 @@ | |
FilenameParameter, | ||
MultiFilenameParameter, | ||
Parameter, | ||
Vector3dParameter, | ||
parameter_registry, | ||
) | ||
from ess.reduce.ui import ResultBox, WorkflowWidget, workflow_widget | ||
from ess.reduce.widgets import OptionalWidget, SwitchWidget, create_parameter_widget | ||
from ess.reduce.widgets._base import WidgetWithFieldsProtocol, get_fields, set_fields | ||
from ess.reduce.workflow import register_workflow, workflow_registry | ||
|
||
SwitchableInt = NewType('SwitchableInt', int) | ||
|
@@ -173,6 +175,46 @@ def test_switchable_optional_parameter_switchable_first() -> None: | |
assert isinstance(dummy_widget.wrapped, OptionalWidget) | ||
|
||
|
||
def test_optional_widget_set_value_get_fields() -> None: | ||
optional_param = IntParam('a', 'a', 1, optional=True) | ||
optional_widget = create_parameter_widget(optional_param) | ||
assert isinstance(optional_widget, WidgetWithFieldsProtocol) | ||
assert isinstance(optional_widget, OptionalWidget) | ||
# Check initial state | ||
assert optional_widget._option_box.value is None | ||
assert get_fields(optional_widget) == {'essreduce-opted-out': True, 'value': 1} | ||
# Update the value of the wrapped widget and check the fields | ||
set_fields(optional_widget, {'value': 2}) | ||
assert optional_widget.value is None # Opted-out is not changed | ||
assert get_fields(optional_widget) == {'essreduce-opted-out': True, 'value': 2} | ||
optional_widget.value = 3 | ||
assert get_fields(optional_widget) == {'essreduce-opted-out': False, 'value': 3} | ||
|
||
|
||
def test_optional_widget_set_fields_get_fields() -> None: | ||
optional_param = Vector3dParameter( | ||
'a', 'a', sc.vector([1, 2, 3], unit='m'), optional=True | ||
) | ||
optional_widget = create_parameter_widget(optional_param) | ||
assert isinstance(optional_widget, WidgetWithFieldsProtocol) | ||
assert isinstance(optional_widget, OptionalWidget) | ||
# Check initial state | ||
assert optional_widget._option_box.value is None | ||
expected = {'essreduce-opted-out': True, 'x': 1, 'y': 2, 'z': 3, 'unit': 'm'} | ||
assert optional_widget.get_fields() == expected | ||
# Update the value of the wrapped widget | ||
optional_widget.set_fields( | ||
{'essreduce-opted-out': True, 'x': 4, 'y': 5, 'z': 6, 'unit': 'm'} | ||
) | ||
assert optional_widget.value is None # Opted-out is not changed | ||
optional_widget.set_fields({'essreduce-opted-out': False}) | ||
assert optional_widget.value == sc.vector([4, 5, 6], unit='m') | ||
# Check the fields and the option box value | ||
expected = {'essreduce-opted-out': False, 'x': 4, 'y': 5, 'z': 6, 'unit': 'm'} | ||
assert optional_widget.get_fields() == expected | ||
assert optional_widget._option_box.value == optional_param.name | ||
|
||
|
||
def test_optional_widget_dispatch() -> None: | ||
optional_param = Parameter('a', 'a', 1, optional=True) | ||
assert isinstance(create_parameter_widget(optional_param), OptionalWidget) | ||
|
@@ -431,6 +473,76 @@ def run_workflow(): | |
assert was_called | ||
|
||
|
||
def test_switchable_widget_set_values() -> None: | ||
param = IntParam('a', 'a', 1, switchable=True) | ||
widget = create_parameter_widget(param) | ||
assert isinstance(widget, SwitchWidget) | ||
assert not widget.enabled | ||
widget.set_fields({'enabled': True}) | ||
assert widget.enabled | ||
widget.set_fields({'enabled': False}) | ||
assert not widget.enabled | ||
widget.set_fields({'enabled': True, 'value': 2}) | ||
assert widget.enabled | ||
assert widget.value == 2 | ||
widget.set_fields({'enabled': False, 'value': 3}) | ||
assert not widget.enabled | ||
assert widget.value == 3 | ||
widget.set_fields({'value': 4}) | ||
assert not widget.enabled | ||
assert widget.value == 4 | ||
|
||
|
||
def test_switchable_widget_get_fields_only_value() -> None: | ||
param = IntParam('a', 'a', 1, switchable=True) | ||
widget = create_parameter_widget(param) | ||
assert isinstance(widget, SwitchWidget) | ||
assert widget.get_fields() == {'enabled': False, 'value': 1} | ||
widget.enabled = True | ||
assert widget.get_fields() == {'enabled': True, 'value': 1} | ||
widget.value = 2 | ||
assert widget.get_fields() == {'enabled': True, 'value': 2} | ||
widget.enabled = False | ||
assert widget.get_fields() == {'enabled': False, 'value': 2} | ||
|
||
|
||
def test_switchable_widget_set_fields() -> None: | ||
param = Vector3dParameter('a', 'a', sc.vector([1, 2, 3], unit='m'), switchable=True) | ||
widget = create_parameter_widget(param) | ||
assert isinstance(widget, SwitchWidget) | ||
assert not widget.enabled | ||
assert widget.value == sc.vector([1, 2, 3], unit='m') | ||
widget.set_fields({'enabled': True, 'x': 4, 'y': 5, 'z': 6, 'unit': 'm'}) | ||
assert widget.enabled | ||
assert widget.value == sc.vector([4, 5, 6], unit='m') | ||
widget.set_fields({'x': 7, 'y': 8}) | ||
assert widget.enabled | ||
assert widget.value == sc.vector([7, 8, 6], unit='m') | ||
|
||
|
||
def test_switchable_widget_get_fields_sub_fields() -> None: | ||
param = Vector3dParameter('a', 'a', sc.vector([1, 2, 3], unit='m'), switchable=True) | ||
widget = create_parameter_widget(param) | ||
assert isinstance(widget, SwitchWidget) | ||
assert widget.get_fields() == { | ||
'enabled': False, | ||
'x': 1, | ||
'y': 2, | ||
'z': 3, | ||
'unit': 'm', | ||
} | ||
widget.enabled = True | ||
assert widget.get_fields() == {'enabled': True, 'x': 1, 'y': 2, 'z': 3, 'unit': 'm'} | ||
widget.set_fields({'enabled': False, 'x': 4, 'y': 5, 'unit': 'mm'}) | ||
assert widget.get_fields() == { | ||
'enabled': False, | ||
'x': 4, | ||
'y': 5, | ||
'z': 3, | ||
'unit': 'mm', | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably a stupid question but do we really need the switchable and optional widgets? Can we do without? It would simplify the code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the The usability indeed needs improvement, so we have a issue about that #74 But I don't think we can't simply remove them to support those use cases I mentioned at the beginning. If you want to remove them from the specific workflow, you can just set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I still don't really understand the difference between optional and switchable. They seem very similar: if enabled, use the value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nvaytet No worries, we should have had a better documentation from the first place...
|
||
|
||
|
||
FilenameSampleRun = NewType('FilenameSampleRun', str) | ||
parameter_registry[FilenameSampleRun] = FilenameParameter.from_type( | ||
FilenameSampleRun, default="SampleRun.hdf" | ||
|
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.
Should we only fill the rest of the fields if the widget is enabled, or do we fill in anyway?
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.
We should restore the state even if they are disabled. So that user can easily switch it on and see the restored status.