Disable terminal echo while State Tool is running. #2882
Conversation
mitchell-as
force-pushed
the
mitchell/dx-2214-2
branch
3 times, most recently
from
97cb81d to
5dbd4bf
Compare
The exception is for prompts and subshells (which temporarily re-enable echoing).
mitchell-as
force-pushed
the
mitchell/dx-2214-2
branch
from
5dbd4bf to
7c45563
Compare
|
|
||
| func (v *SubShell) TurnOffEcho() { | ||
| if runtime.GOOS == "windows" { | ||
| return // not supported |
There was a problem hiding this comment.
Windows does not have access to the termios functions needed to disable echo in emulated shells like Git Bash (which is not interactive anyway).
internal/subshell/bash/bash.go
Outdated
| if runtime.GOOS == "windows" { | ||
| return // not supported | ||
| } | ||
| termecho.Off() |
There was a problem hiding this comment.
Shouldn't these, and the other TurnOnEcho functions be calling termecho.On()?
There was a problem hiding this comment.
Oh, how embarrassing. Thanks for catching this.
| // Disable terminal echo while State Tool is running. | ||
| // Other than in prompts and subshells (which temporarily re-enable echo), user typing should | ||
| // not interfere with output (e.g. runtime progress bars). | ||
| sshell.TurnOffEcho() | ||
| defer sshell.TurnOnEcho() |
There was a problem hiding this comment.
Does this also disable ctrl+c and other similar inputs? I don't believe that's something we want to disallow.
There was a problem hiding this comment.
Also, is it possible that if the state tool errors mid-execution, we could leave the user's shell in a state where they cannot echo?
There was a problem hiding this comment.
You didn't see this, but in my original PR (#2768), I noted that echo is restored after ^C, so no problem there. The defer does its job.
If there is a hard crash, echo will probably be lost, but this is the case for any terminal program that messes with termios, so the only resolution is for the user to exit this terminal and start up a new one.
There was a problem hiding this comment.
It looks like the defer will run even on a panic as long as it's on the function call stack so we should be good even in the event of a crash as this defer is placed ahead of the actual command logic.
|
As mentioned in the PR description, the survey library automatically handles the flipping on/off of terminal echo. I don't know exactly how it does it, but during testing it did. |
The exception is for prompts and subshells (which temporarily re-enable echoing).
Note: the survey package does this automatically, so no changes to the prompt package are required.