Skip to content

2317: Prompt user to restart when preferences change#2381

Merged
butlerpd merged 8 commits into
mainfrom
2316-prompt-restart
Dec 6, 2022
Merged

2317: Prompt user to restart when preferences change#2381
butlerpd merged 8 commits into
mainfrom
2316-prompt-restart

Conversation

@krzywon
Copy link
Copy Markdown
Contributor

@krzywon krzywon commented Nov 9, 2022 •
edited by wpotrzebowski
Loading

Description

This PR prompts the user to restart SasView if changes are made that only take effect at the next startup of SasView.

NOTE This PR is currently chained off 2313-save-on-ok. Once #2358 is merged, I will rebase this to main.

Fixes #2317

How Has This Been Tested?

Run SasView, open the preferences window and change one of the display parameters that require a restart.

  • If 'Cancel' is clicked, ensure no message dialog is generated
  • If 'Ok' or 'Apply' is clicked, ensure a message is displayed with a description of changes that will only take effect on restart.

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)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

lucas-wilkins
lucas-wilkins reviewed Nov 9, 2022
View reviewed changes
Comment thread src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py
lucas-wilkins
lucas-wilkins approved these changes Nov 9, 2022
View reviewed changes
Comment thread src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py Outdated
Comment thread src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py
rozyczko
rozyczko approved these changes Nov 11, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Works fine, except of one potential issue mentioned.
This is, however, just a mild nuisance for users and can be made into a separate issue.

Comment thread src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py
widget.restoreDefaults()

def stageSingleChange(self, key, value):
def stageSingleChange(self, key: str, value: ConfigType, config_restart_message: Optional[str] = ""):
Copy link
Copy Markdown
Member

@rozyczko rozyczko Nov 11, 2022

Choose a reason for hiding this comment

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

There is no unstaging taking place on option change.
This means when I click twice on Automatic Scale Factor checkbox, I still get prompted to restart even though the preferences haven't changed.

Copy link
Copy Markdown
Contributor

@lucas-wilkins lucas-wilkins Nov 11, 2022

Choose a reason for hiding this comment

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

yeah, need to check if anything has actually changed at some point.

Comment thread src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py
msgBox.show()
if msgBox.exec() == QMessageBox.Yes:
self.parent.guiManager.quitApplication()
os.execl(sys.executable, os.path.abspath(__file__), *sys.argv)
Copy link
Copy Markdown
Member

@rozyczko rozyczko Nov 11, 2022

Choose a reason for hiding this comment

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

Interestingly, this works fine in the installed version, despite __file__ pointing to relative location of sasview.exe in pyinstaller generated setups.
If you open the python shell in the installed version, the __file__ attribute is not even defined.
I would probably use sys.executable instead of os.path.abspath(__file__), since in the installed version it points to location of the exe file (without the need to expand the path with abspath) and in the dev build, it points to python.exe, with sys.argv containing run.py and other arguments.

But since this is working as is, I wouldn't put too much importance in the abve.

@wpotrzebowski wpotrzebowski self-requested a review November 15, 2022 10:24
wpotrzebowski
wpotrzebowski approved these changes Nov 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@wpotrzebowski wpotrzebowski left a comment

Choose a reason for hiding this comment

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

Works as expected. Then only comment I have is that messange in the pop up window can be better aligned?
Screenshot 2022-11-15 at 11 29 41

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 21, 2022
Base automatically changed from 2313-save-on-ok to main November 21, 2022 14:49
@lucas-wilkins
Copy link
Copy Markdown
Contributor

lucas-wilkins commented Nov 21, 2022

Works as expected. Then only comment I have is that messange in the pop up window can be better aligned? Screenshot 2022-11-15 at 11 29 41

Minor complaint. I don't think "factoring" is the right word here.

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 22, 2022
@rozyczko
Copy link
Copy Markdown
Member

rozyczko commented Nov 22, 2022

One more issue - when the scaling value is empty, the parser throws:

15:48:40 - ERROR: Traceback (most recent call last):
  File "D:\projects\sasview_new\src\sas\qtgui\Utilities\Preferences\PreferencesWidget.py", line 21, in set_config_value
    value = dtype(value)
ValueError: could not convert string to float: ''

Hitting OK with empty line text doesn't trigger the warning and resets the value to default

@krzywon krzywon mentioned this pull request Nov 22, 2022
Closed
@krzywon
Copy link
Copy Markdown
Contributor Author

krzywon commented Nov 22, 2022

One more issue - when the scaling value is empty, the parser throws:

Ticketed in #2389

This was referenced Dec 2, 2022
Closed
Merged
@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Dec 6, 2022
@butlerpd butlerpd merged commit 985cfb6 into main Dec 6, 2022
@krzywon krzywon deleted the 2316-prompt-restart branch December 9, 2022 16:50
@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

Reviewers

@lucas-wilkins lucas-wilkins lucas-wilkins approved these changes

@wpotrzebowski wpotrzebowski wpotrzebowski approved these changes

@rozyczko rozyczko rozyczko approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Changing preferences should prompt user to restart SasView

5 participants

@krzywon @lucas-wilkins @rozyczko @wpotrzebowski @butlerpd