-
Notifications
You must be signed in to change notification settings - Fork 128
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
Better feedback for changes in the new settings widget #38799
base: main
Are you sure you want to change the base?
Conversation
e8de202
to
65694fa
Compare
65694fa
to
cf92efb
Compare
To answer this, the precommit hook relies on searching for a call to |
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.
Works well, just a quick Q in the code, but nothing that needs changing. I did notice that there was a fair amount of Qt stuff and widgets being called directly in the presenter. That's not ideal, but fixing it definitely isn't in the scope of this PR.
Functionally working well. Apply button enables/disabled properly when making changes to the settings. Settings are saved as expected between runs of workbench. Nice work. This is a really nice usability feature.
from workbench.widgets.settings.base_classes.config_settings_changes_model import ConfigSettingsChangesModel | ||
|
||
|
||
class SettingsPresenterBase: |
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.
As this is a base class, are we ever expecting to make usable objects of this class? If not, it might be worth using the abc
package to make this an explicit abstract base class.
I have this issue to keep track of some ways we'd like to further improve the settings widget I'm very happy for you to leave a comment / edit it with any further changes you think are outside of the scope of this pr |
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.
Functional testing described above. Code looks good to me now. Nice work! 🚀
Description of work
It was requested that the 'Apply' button on the new settings widget is greyed out when the user has made no changes to the settings, and enabled when there is a pending change.
The other thing I discovered in this PR is that the test files that were added in the original PR (#38373) were not added to cmake so haven't been running. I thought the search for missing pytest files pre-commit job was supposed to flag this. They've been added in this PR (along with new tests).
Summary of work
Design-wise I thought it best that the model of the settings widget tabs didn't know about the presenter. Instead, the presenter emits a signal when it adds a change in the model which is picked up by the parent presenter. For this I added a QObject base class of the presenters.
There is no associated issue.
Further detail of work
To test:
Play around in the settings widget, change settings and change them back again. 'Apply' button should enable and disable as you expect.
Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeepers
will take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.