-
Notifications
You must be signed in to change notification settings - Fork 47
Implemented Close Project functionality #3757
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
Conversation
|
Just quickly looking, I am getting error |
|
It was triggered when I tried to "Close Project" |
|
I don't think it has anything to do with the changes? |
krzywon
left a comment
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.
The P(r) perspective needs to be fixed before I can approve.
| <addaction name="actionClose_Project"/> | ||
| <addaction name="separator"/> |
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.
Logically, this would make more sense to be above the Preferences option.
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 don't think I fully agree - closing and quitting actions are usually on the bottom of the list, cf. Window menu.
For File you choose to either Close Project or Quit - these two should belong to the same "category".
|
I think the issue @wpotrzebowski saw was related to #3664. This should now be resolved with the latest sasmodels changes. Note - I am not seeing this error locally, though I haven't tried the installer. |
|
@rozyczko - I would give an updated review, but I don't see any new commits since yesterday. |
sorry, did git commit, forgot to push it. |
krzywon
left a comment
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.
P(r) resetting works as expected now. I think the save on close project issue I found should be fixed, but otherwise, this looks ready to go.
| "All unsaved changes will be lost if you don't save.", | ||
| QMessageBox.Save | QMessageBox.Discard | QMessageBox.Cancel, | ||
| QMessageBox.Cancel) | ||
| if reply == QMessageBox.Save: |
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.
If the saving project dialog is cancelled, the project reset still happens with no project saved, which may not be ideal.
Maybe if (reply == QMessageBox.Save and self.actionSave_Project()) or reply == QMessageBox.Discard:? This would require the save project to return something Truthy on success.
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.
Errr.. nope :) we have
if reply == QMessageBox.Save:
...
elif reply == QMessageBox.Discard:
....
# else Cancel, do nothingThe else branch is just noop but with comment added
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 don't think my original point was clear. Try the following steps:
- Open a project or do some work in SasView
- Choose File -> Close Project
- Click the 'Save' button
- In the resulting file browser dialog, click
Cancel - The result is no saved project, and all work cleared from SasView
I don't think users will appreciate all of their work being thrown away without warning.
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.
Ah! This makes sense. Indeed cancelling the "Save Project" does the wrong thing. thanks!
Description
Added
Close Projectaction to theFilemenu. This action removes all loaded data, plots and content of all perspectives.This effectively puts SasView in the "Just opened" state, with minor caveats of calculator window content and certain options.
Fixes #3312
How Has This Been Tested?
Local Win10
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licensing (untick if necessary)