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 issue #2898: close app nicely on CTRL+C #2985

Closed
wants to merge 1 commit into from

Conversation

kinow
Copy link
Member

@kinow kinow commented Mar 11, 2019

Fix for #2898 .

Encloses GTK main loop in a try/catch, to wait for KeyboardInterrupt and quit nicely.

Another workaround is to use CTRL + \, which sends SIGKILL, but not a well known short-cut (nor really nice/polite to go SIGKILLing processes around 😬

Tested locally, but alas going with no unit/functional tests, so Codecov won't be happy.

Bruno

@kinow kinow requested a review from hjoliver March 11, 2019 06:39
@kinow
Copy link
Member Author

kinow commented Mar 11, 2019

Pull request was ready over 3 hours ago. But Travis kept failing with flaky tests. Kicked it four times (finish a chapter in some book, come back and check Travis). This would be really off-putting to new contributors 😢

But finally Travis is happy 🎉

@matthewrmshin matthewrmshin added this to the next-release milestone Mar 11, 2019
@matthewrmshin matthewrmshin added the bug Something is wrong :( label Mar 11, 2019
@matthewrmshin
Copy link
Contributor

(Not sure if you can re-target this PR to point to the 7.8.x branch or not. If not, I'll just incorporate that into the cherry-picks of #2947.)

@kinow
Copy link
Member Author

kinow commented Mar 11, 2019

@matthewrmshin , can't re-target I think, but checked out the 7.8.x branch, then cherry-picked the commit and created a new branch. Should I close this one and open a new pull request from here? Or 2 pull requests?

Or if it's easier for you to cherry-pick in #2947, all good for me.

@matthewrmshin
Copy link
Contributor

Hi @kinow please close this one and open a new one targeting the cylc/7.8.x branch. GUI logic is going to get blasted by #2966 on master (hopefully some time this week).

@kinow
Copy link
Member Author

kinow commented Mar 11, 2019

Consider it done. Thanks @matthewrmshin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants