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

2389: Validate text/int/float inputs within the preferences panel #2476

Merged
merged 26 commits into from
Jun 20, 2023

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Mar 28, 2023

Description

A validator method was added to the preferences widget to check that text input boxes are valid before staging any changes. If the value becomes invalid, any previously staged value for that input is removed.

Fixes #2389
Fixes #2521

How Has This Been Tested?

New (but minimal) unit tests and locally running the GUI to ensure invalid entries are not stored in the config.

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Apr 10, 2023
@butlerpd
Copy link
Member

This needs to wait for the pyside merge which will require some more changes.

@wpotrzebowski
Copy link
Contributor

@krzywon to look at it again

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label May 9, 2023
@krzywon
Copy link
Contributor Author

krzywon commented May 17, 2023

This should be ready for review now

@wpotrzebowski wpotrzebowski added Discuss At The Call Issues to be discussed at the fortnightly call and removed Discuss At The Call Issues to be discussed at the fortnightly call labels May 23, 2023
@wpotrzebowski
Copy link
Contributor

@rozyczko to look at it

Copy link
Member

@rozyczko rozyczko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, minor questions raised to address outside the scope of this review.

Functionality:

  • leaving a textbox in yellow state stil allows Apply and OK. Clicking either button updates the staging stack with the incorrect value

Additionally, I am getting failures on clicking "Accept" on the Preferences panel. This seems to be a leftover from merging the PySide6 branch into main. We might as well create a separate ticket for this.

09:02:11 - sas.qtgui.Perspectives.Fitting.FittingWidget - ERROR - Traceback (most recent call last):
  File "D:\projects\sasview\src\sas\qtgui\Utilities\Preferences\PreferencesPanel.py", line 100, in _okClicked
    self._saveStagedChanges()
  File "D:\projects\sasview\src\sas\qtgui\Utilities\Preferences\PreferencesPanel.py", line 106, in _saveStagedChanges
    self.stackedWidget.widget(i).applyNonConfigValues()
  File "D:\projects\sasview\src\sas\qtgui\Perspectives\Fitting\FittingOptions.py", line 177, in applyNonConfigValues
    color = line_edit.palette().color(QtGui.QPalette.Background).name()
AttributeError: type object 'PySide6.QtGui.QPalette' has no attribute 'Background'

Also, the Default Fit Algorithm combobox is squashed.

Untitled

src/sas/qtgui/Utilities/Preferences/PreferencesWidget.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/Preferences/PreferencesWidget.py Outdated Show resolved Hide resolved
@krzywon
Copy link
Contributor Author

krzywon commented May 24, 2023

I went ahead and covered all of your suggestions here. They are all related to validation and made the system better

@rozyczko
Copy link
Member

Thanks! The code and widgets look good now.

The functionality is almost there.

  1. The Fit Optimizers page: changing its parameters doesn't trigger the OK/Apply check. I can modify the Fit Algorithms combobox and any numerical value and OK/Apply buttons don't enable.

  2. Moving away from an invalid textbox leaves the textbox in an invalid state. Having yellow bg for the users is good, but maybe the control should revert to its original content when focus is lost on bad content?
    Yes - the bad value isn't saved but when the widget is closed and reopened, the invalid textbox is still shown, despite the underlying value (in the model) being correct.

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Jun 5, 2023
@wpotrzebowski
Copy link
Contributor

@krzywon and @rozyczko to double check if it works

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Jun 6, 2023
@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Jun 19, 2023
@butlerpd
Copy link
Member

@rozyczko to check

@rozyczko
Copy link
Member

There are still some minor glitches but we need this functionality.

@rozyczko rozyczko merged commit 131c1ac into main Jun 20, 2023
36 checks passed
@krzywon krzywon deleted the 2389-validate-inputs branch June 27, 2023 19:11
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 1, 2023
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

Successfully merging this pull request may close these issues.

Preferences Panel Missing GPU and Optimizer Options Empty Numerical Preference Values Throw Errors
4 participants