-
Notifications
You must be signed in to change notification settings - Fork 129
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
Unhide the cursor before exiting for interrupt signals #155
base: master
Are you sure you want to change the base?
Conversation
This solves my biggest problem with this library! Unfortunately, it doesn't compile on Windows: |
@briandowns please consider this |
I'd consider this if it can be achieved without breaking windows and also scoping the change to the code without all of the other updates. It's a fine change but this PR is a bit bloated and breaks too many users. |
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.
Unfortunately, it doesn't compile on Windows:
spinner/spinner.go:359:14: undefined: syscall.Kill
@kernel-sanders Thanks for giving this a test run 🙏 I can confirm this was happening with the following commands and have also made changes to fix this in fbe5c85:
$ gh repo clone zimeg/spinners
$ cd spinners
$ GOOS=windows make build
Lot's more learnings about syscall
and the strangenesses within, but I'll leave a comment with the important change 📚
I'd consider this if it can be achieved without breaking windows and also scoping the change to the code without all of the other updates. It's a fine change but this PR is a bit bloated and breaks too many users.
@briandowns And thanks for giving this a look 👾 I made a few changes to tidy things with support for Windows, but please do let me know if other changes make sense!
@@ -328,13 +330,29 @@ func (s *Spinner) Start() { | |||
} | |||
|
|||
s.active = true | |||
signal.Notify(s.stopChan, syscall.Signal(0x0), os.Interrupt) |
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.
TIL os.Interrupt
is the only signal available across all os
packages! Reference 📚 ✨
Instead of checking unique syscall
values that might not exist, including SIGQUIT
, only the standard interrupt is caught here.
This means the cursor won't be unhidden for all termination signals, but IMO covering the ^C
case seems ideal 🤖
Summary
This PR unhides the cursor after an interrupt signal is sent while the spinner is active to prevent the cursor from disappearing in some terminals.
All of the signals listened for before termination are
os.Interrupt
and a customsyscall.Signal(0x0)
.The
syscall.Signal(0x0)
is used to signal a successful.Stop()
of the spinner and can safely be used as a termination signal since it doesn't send an actual signal, it only checks that the current process exists.Preview
On some terminals, pressing
CTRL-C
would quit the program without unhiding the cursor. This example is shown withANSI
terminal:Before changes
before.mov
After changes
after.mov
Reviewers
A small program to test with is ready in this repo to verify various exit codes and conditions in certain cases. Different testing cases are listed in
SIGNALS.md
if you want to try these yourself!While multiple signals are noted in the link above, only the interrupt -
^C
- is supported in these changes.