-
Notifications
You must be signed in to change notification settings - Fork 155
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
Wait cursor only shows when Glue is really doing something #1097
Conversation
@astrofrog , I tried this with loading a big data file and the cursor appears to change properly. I didn't try saving/loading Glue session files. Perhaps you want to take this for a spin before merging. |
self, "Confirm Close", | ||
"Are you sure you want to reset the session? " | ||
"This will close all datasets, subsets, and data viewers", | ||
buttons=buttons, defaultButton=QMessageBox.Cancel) |
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.
In general, I prefer not to change the line length in cases like this when it actually makes the code less readable - would you mind disabling the line length PEP8 warnings or changing the line length to 100 or 120?
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.
Ok, no problem. I'll keep that in mind for the future.
Wait cursor only shows when Glue is really doing something
(merged manually since this required resolving conflicts) |
How did you "merge manually" and still get the PR to be listed as "merged" instead of "closed without merging"? I'm just curious. I have seen this across different repos. There must be some kind of git-fu that I have yet learned. |
@pllim - I think it's because I made the merge commit message look exactly like what GitHub normally does (I changed the default merge message) |
@astrofrog -- I added a note in my commit indicating I was not sure if a context manager should belong in a module named "decorators" (OCD speaking again!), but I couldn't find another module that is appropriate and didn't want to create a new |
This fixes #1092 (the second commit).