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

Fix mass ws deletion bug #18914

Merged
merged 4 commits into from Feb 20, 2017
Merged

Fix mass ws deletion bug #18914

merged 4 commits into from Feb 20, 2017

Conversation

NickDraper
Copy link
Contributor

@NickDraper NickDraper commented Feb 17, 2017

Fixes #17729

Added a DeleteWorkspaces algorithm, use it in GUI when deleting multiple workspaces, all it does is iterate over the list deleting workspaces.

To test:

* Test that deleting many workspaces does not crash*

  1. Create many workspaces with:
for i in range(3000):
    name = "WS_" + str(i)
    CreateSampleWorkspace(OutputWorkspace=name)
  1. Select all in the ADS
  2. Press the delete button
    • Confirm that Mantid does not crash and that the time it takes to clear the 3000 workspaces is acceptable.

* Test that deleting few very large data sets does not disturb the user experience (too much)*

  1. Find a sample data set here: smb://olympic/babylon5/Scratch/Anton/SampleWS . The relevant file is MDEvent_ellipPeaks_new.nxs
  2. If you have a 16GB computer run the follow script, else you have to modify the times you can load the file.
for i in range(10):
    name = "WS_" + str(i)
    Load(Filename='PATH_TO/MDEvent_ellipPeaks_new.nxs', OutputWorkspace=name)
  1. Select all workspace and delete them.
    • Confirm that the time taken is acceptable and that the screen does not freeze. It will be unusable though.

RELEASE NOTES

docs/source/release/framework.rst


Reviewer

Please comment on the following (full description):

Code Review
  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards? Is it well structured with small focussed classes/methods/functions?
  • Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?
  • If there are changes in the release notes then do they describe the changes appropriately?
Functional Tests
  • Do changes function as described? Add comments below that describe the tests performed?

  • How do the changes handle unexpected situations, e.g. bad input?

  • Has the relevant documentation been added/updated?

  • Is user-facing documentation written in a user-friendly manner?

  • Has developer documentation been updated if required?

  • Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

@NickDraper NickDraper added Framework Issues and pull requests related to components in the Framework Component: GUI labels Feb 17, 2017
@NickDraper NickDraper added this to the Release 3.10 milestone Feb 17, 2017
@NickDraper NickDraper added Patch Candidate Urgent issues that must be included in a patch following a release High Priority An issue or pull request that if not addressed is severe enough to postponse a release. labels Feb 17, 2017
@NickDraper NickDraper changed the title Fix mass ws deletetion bug Fix mass ws deletion bug Feb 17, 2017
@samueljackson92 samueljackson92 self-assigned this Feb 20, 2017
Copy link
Contributor

@samueljackson92 samueljackson92 left a comment

Choose a reason for hiding this comment

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

Tested with example scripts and played with tying to pass invalid input (e.g. no workspaces) to algorithm. All seems well. See comment below from code review but this is basically ready to go I think.

}
alg->setProperty("WorkspaceList", vecWsNames);
executeAlgorithmAsync(alg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should put all this in a private method? void MantidUI::deleteWorkspaces() or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it already is!

Copy link
Contributor

Choose a reason for hiding this comment

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

So it is!

@samueljackson92
Copy link
Contributor

@NickDraper Also checked this with Giovanni's Mantid project and I cannot reproduce.

@samueljackson92
Copy link
Contributor

LGTM :shipit:

@AndreiSavici AndreiSavici merged commit 854a93e into master Feb 20, 2017
@AndreiSavici AndreiSavici deleted the 17729_prevent_mass_delete_crash branch February 20, 2017 19:05
martyngigg pushed a commit that referenced this pull request Feb 23, 2017
add DeleteWorkspaces algorithm, use it in GUI

re #17729

(cherry picked from commit 9953f7c)

clang format and release notes

re #17729

(cherry picked from commit b6ba0e5)

Resolve cppcheck warning

re #17729

(cherry picked from commit 8827cbd)

Resolve small syntax error
(cherry picked from commit 1891682)

Add release note for mass workspace fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues and pull requests related to components in the Framework High Priority An issue or pull request that if not addressed is severe enough to postponse a release. Patch Candidate Urgent issues that must be included in a patch following a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants