Params GUI Refactor #109
Params GUI Refactor #109
Conversation
|
@AlisterD I'm going to ask you to review this one with me. To do this, you'll need to checkout the @lawhead I promise to have the reviews back to you by Friday evening! Thanks for the patience. |
| @@ -83,7 +83,7 @@ | |||
| "readableName": "Offline Analysis Alert Tone", | |||
| "helpTip": "Specifies the path to an audio file to be played as an alert tone when offline analysis is complete. Default: bcipy/static/sounds/beep.wav", | |||
| "recommended_values": "", | |||
| "type": "directorypath" | |||
| "type": "filepath" | |||
There was a problem hiding this comment.
A nice refactor! It works well for me locally. A few suggestions:
- Move the generic classes to gui_main
- Move the form to its own module (like
gui/parameters/param_form.py). I'm not sure if we'll add more there, but it feels clean to have only the main, BCInterface, and README at GUI module root. We might add a README with use instructions there later?
I'll still have Alister review tomorrow morning, but I don't forsee any major comments from that!
bcipy/gui/params_form.py
Outdated
| #--- Input widgets | ||
|
|
||
|
|
||
| class FormInput(QWidget): |
There was a problem hiding this comment.
These are generic enough to be used elsewhere and would be useful to the experiment /field UI needs. What do you think about adding the classes to gui_main?
There was a problem hiding this comment.
I can see how these would be useful in the context of the experiment UI. We probably also want to pull out the method that selects the FormInput (subclass) based on the Parameter type. That code is currently in the ParamsForm widget but could be extracted out into a module function.
There was a problem hiding this comment.
I agree! If it's not too much effort in this PR let's do it. I'm okay making another ticket otherwise.
bcipy/gui/params_form.py
Outdated
| form_input = TextInput(param) | ||
| self.controls[key] = form_input | ||
|
|
||
| def do_layout(self): |
There was a problem hiding this comment.
Thanks, good catch.
|
@sci-tab, I moved the generic classes into gui_main and put the params_form into its own module. Do you mind taking another quick look before I merge? |
bcipy/gui/gui_main.py
Outdated
| """ | ||
|
|
||
| def __init__(self, | ||
| parameter: Parameter, |
There was a problem hiding this comment.
I don't think this should be parameter specific. Same with the functions inheriting it below. The SearchInput is generic and should be here for certain.
Let's say I want to use the boolean input to query the user for something other than a parameter (ex. an experiment field). I do think it can be usefully abstracted and reused. I also don't want to extend your PR. I'd be okay leaving them in here or with the params_form and addressing with the experiment tickets. You call!
There was a problem hiding this comment.
Yeah, we should discuss in planning. Alternatively experiment fields could use the same format as a Parameter. We would just need to add a required boolean property.
…uilding other GUIs
Overview
This PR is a rewrite of the parameters GUI from WxPython to PyQt5. It also includes additional functionality to search for parameters.
Ticket
https://www.pivotaltracker.com/story/show/175543725
Contributions
Test
of parameters. Saved changes should persist.