Skip to content
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

optional params as checkbox with widget #645

Open
tlambert03 opened this issue Apr 22, 2024 · 4 comments
Open

optional params as checkbox with widget #645

tlambert03 opened this issue Apr 22, 2024 · 4 comments

Comments

@tlambert03
Copy link
Member

@cmalinmayor does this for parameters that have Optional[thing]:

Screenshot 2024-04-22 at 4 17 12 PM

i like it, and we could do something similar here

@hanjinliu
Copy link
Collaborator

Hi @tlambert03 ,
Actually, I've already did a very similar thing in magicclass.

メディア1-output

In this example, I had to define a custom Optional type because magicgui has its own type mapping strategy for T | None type, but eventually that can be replaced to typing.Optional.
I could not find the code you were refering to so I don't know how much it's already implemented. Do you think it is useful to directly port this widget to magicgui?

@cmalinmayor
Copy link

Linking code here for reference:
Example of the spinbox: https://github.com/funkelab/motile-napari-plugin/blob/9a35d20ea28653759021de74aa495675f5c61869/src/motile_plugin/widgets/solver_params.py#L35
And the checkbox: https://github.com/funkelab/motile-napari-plugin/blob/9a35d20ea28653759021de74aa495675f5c61869/src/motile_plugin/widgets/solver_params.py#L80
My implementation seems quite similar, with a few differences. It allows default values that aren't None, while still allowing you to uncheck the box to set it to None in the backend (in the UI, it disables the linked SpinBox or in your case slider so it appears greyed out). I didn't use magicgui, so I think my code is more inspiration than an actual starting point. There is also extra stuff in that code example to allow two-way updating (e.g. you want to view a new SolverParams object, and set the spinbox and checkbox accordingly) - which presumably can be ignored for this issue.

@tlambert03
Copy link
Member Author

thank you both. @hanjinliu it would be great if you wanted to adapt your solution and contribute it here..

I think in terms of the GUI: what I'd want is probably something that looks like @cmalinmayor's widget: just a new checkbox on the left of the widget, without any text. When checked, the linked widget is enabled, and when unchecked, the linked widget is disabled and the default value is used. (the checkbox could have a tooltip that shows the literal default value that would be used when unchecked)

It allows default values that aren't None, while still allowing you to uncheck the box to set it to None in the backend

I think the semantics I would go for is:

  • if a parameter is annotated as x: int | None = None, then you get a value widget (slider or spinbox) and the checkbox is unchecked by default, and the value is None. checking the box provides the value
  • if a parameter is annotated as x: int | None = 10, then you get a value widget (slider or spinbox) and the checkbox is checked by default, and the value is 10. unchecking the box returns None
  • (parameters that don't have None as an optional value don't have checkboxes)

I just don't think we should worry about a case where the user wants the checkbox to mean "go back to the default value". Since the end-user can always use the main widget to return the value to the default value, that is where it should be done (not using a checkbox). I can imagine a separate feature that would allow something like a right-click on the widget to return to the default value, but i think checkbox should gate "None-ness" not an implicit setting of the value to a non-null default value.

@hanjinliu
Copy link
Collaborator

hanjinliu commented Apr 30, 2024

Yes, it is how I implemented (I realized now that the sentence "Use default value" was not appropriate).
I think I can send a quick PR that implements the feature.

In terms of the design, do you think the checkbox should be on the left side? It seems not consistent with other labeled widgets. It also makes the behavior of _unify_label_widths complicated. It would be much simpler if Type | None is converted into a LabeledWidget of (OptionalWidget of (widget for Type)), that is, [ label ][[ checkbox ][widget for Type]].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants