Allow flow control also in the shell #7708
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
(I'm not familiar with how fish tests work) FWIW if you can run This will work on Linux for sure, I don't know about macOS. |
|
I confirm that the latest changes address my comment #7704 (comment) Thanks :) |
zanchey
reviewed
Feb 12, 2021
faho
force-pushed
the
flow-control-2
branch
3 times, most recently
from
dd4e399
to
995f90f
Compare
Since, unlike e.g. OPOST, this can sometimes be useful, just copy whatever flow control settings the terminal ends up with. We still *default* flow control to off (because it's an awful default and allows us to bind ctrl-s), but if the user decides to enable it so be it. Note that it's _possible_ flow control ends up enabled accidentally, I doubt this happens much and it won't render the shell unusable (and good terminals might even tell you you've stopped the app). Fixes fish-shell#7704
Without flow control enabled these won't be interpreted any way.
We actually restore those before exit, so this would force-disable flow control whenever fish exits.
I have no idea how to see that flow control has worked otherwise
This is a bit of an interesting pexpect test, but honestly pexpect works quite well! I'm happy with it!
This fails on FreeBSD on sr.ht and NetBSD on my own VM, but it works manually. It also fails on macOS but I have no way to confirm. I think it might be a problem in pexpect's platform support? Either way, the test is valuable so just skip it there and solve it later.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This specifically copies IXON/IXOFF from the terminal modes after a command to the shell-modes, so you can use
to enable flow control (and
stty -ixon -ixoffto disable it again). This brings with it the obvious issues that you lose two keychords (ctrl-s and ctrl-q by default, typically) and that it might be confusing if you're not familiar with it (unless your terminal tells you what happened, like e.g. konsole does).It's technically possible someone ends up with flow control enabled accidentally - this doesn't check if the command was
sttyafter all - but I'm assuming that's highly unlikely, and even if it did the shell would still basically work.It also still disables flow control on startup so you specifically have to ask for it - we don't want to give up ctrl-s.
Fixes issue #7704.
TODOs:
Not sure how to test this - can we just send ctrl-s from pexpect? How do we then know it worked? Confirm that no output happened? Does this require waiting for a timeout?