Skip to content

2316: Refactoring preferences#2358

Merged
wpotrzebowski merged 16 commits into
mainfrom
2313-save-on-ok
Nov 21, 2022
Merged

2316: Refactoring preferences#2358
wpotrzebowski merged 16 commits into
mainfrom
2313-save-on-ok

Conversation

@krzywon
Copy link
Copy Markdown
Contributor

@krzywon krzywon commented Oct 31, 2022

Description

This hopes to combine parts of what was done in #2306 and #2321, specifically related to formalizing the preferences panel and widget class structures.

As asked for in #2316, the following has been added to this PR:

  • Settings are staged in the preferences panel until they are accepted and only then is the config updated (using the OK or Apply buttons)
  • All staged changes are discarded if they are rejected (using the Close button or X)
  • Restore defaults resets the active widget to the default values as read in from the configuration
  • Added 'Cancel' and 'Apply' buttons
  • Updated the help documentation to include expected behaviors and information on specific

Fixes #2312
Fixes #2313

How Has This Been Tested?

  • Unit tests that are in this PR
  • From run.py:
    • Open preferences, modify settings, click Cancel, open preferences, and be sure previous changes are gone
    • Open preferences, modify settings, click X, open preferences, and be sure previous changes are gone
    • Open preferences, modify settings, click Ok, open preferences, and be sure previous changes are persistent
    • Open preferences, modify settings, click Apply, click Close and/or X, open preferences, and be sure previous changes are persistent
    • Open preferences, modify settings, click Apply, click Close and/or X, open preferences, click Restore Defaults, and be sure values reset to defaults

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)

Copy link
Copy Markdown
Contributor

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

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

You could use a QFormLayout to make things line up.

@lucas-wilkins
Copy link
Copy Markdown
Contributor

I've added type checking to config setting because currently the text fields are not validated (will add a config type checker next). This causes some pretty bad issues if you set an int to a string, for example (I tested it).

@lucas-wilkins
Copy link
Copy Markdown
Contributor

config will now raise a TypeError if you try and give it the wrong type. This would still need to be handled on the gui side (use .validate(name, value) to just test an entry

@krzywon
Copy link
Copy Markdown
Contributor Author

krzywon commented Oct 31, 2022

Thanks for the info. I'll add Qt validators for ints and floats and apply them to the appropriate inputs.

@gonzalezma
Copy link
Copy Markdown
Contributor

I think that the default option for Plotting Options/Use full-width ... should be that it is not checked. Otherwise one gets often really ugly plots with the legend appearing just in the middle of the screen. The alternative would be that for each plot the legend size is resized to the length of the longer label, but this will be harder to implement. With this option unchecked we recover the same behaviour as before, while given the user the possibility to modify that whenever the data labels are too large. Also after playing with the options and changing the legend length a couple of times I got an error:
File "C:\Users\gonzalezm\Git\sasview\src\sas\qtgui\MainWindow\GuiManager.py", line 1294, in showPlot
self.filesWidget.displayData(plot, id)
File "C:\Users\gonzalezm\Git\sasview\src\sas\qtgui\MainWindow\DataExplorer.py", line 1087, in displayData
self.plotData([(plot_item, plot_to_show)])
File "C:\Users\gonzalezm\Git\sasview\src\sas\qtgui\MainWindow\DataExplorer.py", line 1156, in plotData
new_plot.plot(plot_set, transform=transform)
File "C:\Users\gonzalezm\Git\sasview\src\sas\qtgui\Plotting\Plotter.py", line 243, in plot
if len(l)> config.FITTING_PLOT_LEGEND_MAX_LINE_LENGTH:
TypeError: '>' not supported between instances of 'int' and 'str'

So probably the plotting options need more testing until finding the best default params.

@gonzalezma
Copy link
Copy Markdown
Contributor

In the doc, I suppose this is not a % scaling. If so remove percent from
QT Screen Scale Factor: A percent scaling factor ...
And trying to change this factor, I got the following error:
Cannot set set variable 'QT_SCALE_FACTOR', improper type (Cannot coerce <class 'str'> to bool)
Could this be related to the type checking?

@lucas-wilkins
Copy link
Copy Markdown
Contributor

@gonzalezma

'QT_SCALE_FACTOR', improper type (Cannot coerce <class 'str'> to bool) - Could this be related to the type checking?

Yes, that would be - though indirectly. Not sure why it would say bool, unless something is wired up wrong. It should be able to coerce a string to a float if it is valid (I think)

@lucas-wilkins
Copy link
Copy Markdown
Contributor

@gonzalezma

Nope.

  1. it wont convert strings to floats
  2. the error message for this says bool, oops

@krzywon
Copy link
Copy Markdown
Contributor Author

krzywon commented Nov 1, 2022

I've added validators to the float and integer inputs and ensured the values are coerced before hitting the config

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 7, 2022
@butlerpd butlerpd requested a review from pbeaucage November 8, 2022 14:35
@lucas-wilkins lucas-wilkins self-requested a review November 8, 2022 14:36
@butlerpd
Copy link
Copy Markdown
Member

butlerpd commented Nov 8, 2022

moving legend location to be put into a new PR. Lucas to do proper review and hopefully approve

@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 8, 2022
@wpotrzebowski wpotrzebowski merged commit 47bac4c into main Nov 21, 2022
@wpotrzebowski wpotrzebowski deleted the 2313-save-on-ok branch November 21, 2022 14:50
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 immediately saves the values, even if OK isn't clicked Restore Defaults in the Preferences window does nothing

5 participants