-
Notifications
You must be signed in to change notification settings - Fork 41
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
Add a preferences window #2167
Add a preferences window #2167
Conversation
…eptions are thrown
Hey, I'm pretty close to having the config stuff updated. There's a lot of changes, but it should be pretty simple to use. Just ironing out bugs that are only detected when building the installers. |
self.restoreDefaultMethods = [] | ||
# Set defaults values for the list and stacked widgets | ||
self.stackedWidget.setCurrentIndex(0) | ||
self.listWidget.setCurrentRow(0) |
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.
why is this necessary in the __init__
method? Aren't current indices/rows set to 0 anyway on instantiation?
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.
It's not necessary, but a good practice to ensure the two widgets are at the same point.
elif btn.text() == 'OK': | ||
self.close() | ||
elif btn.text() == 'Help': | ||
self.help() |
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.
it's probably better to create separate slots for all the buttons rather than compare with the text
property. Those texts can change and updating the handlers is not an obvious follow up.
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.
Agreed. This was a placeholder.
@krzywon is this PR still in DRAFT state? |
I have updated the window to account for changes made to the configuration system in PR #2168. There are a few outstanding modifications needed, as noted by @rozyczko, otherwise, this is ready for review. Question: There are at least three ways forward from here - which one is preferred? I plan to work on 2 and 3 (and beyond) during the code camp if that makes a difference:
|
Regarding option 2:
I'm not sure how many of these need to be in a preferences panel. I fact, I'm not sure some of them belong in the config, I just wasn't sure about removing them from it at the time. |
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.
I think it is good enough to give basis for Preferences windows. I've vote for merging it and move on with remaining functionality later
One potential issue is with multiple config files design. Should we not expect tab-per-config file view rather than have all config items thrown into a single window/view? |
My vision is to have separate tabs for separate preference types. Plotting options would be separate from fitting optimizers, which would be separate from GPU options, etc. Calling the config from different tabs is straightforward, but would be cumbersome for the user to have everything bundled together. |
I merged main, updated my unit tests, and added tests for the preferences widget generator. Assuming CI succeeds, this should now be ready to merge |
This adds a preferences panel that can be opened from the File menu. There is a helper class for adding property widgets, a help file, and some basic unit tests.
I'm opening this as a draft because @lucas-wilkins is actively working on the config files for #2164 and we plan to combine our efforts once both are ready.
Fixes #2166