cantino / mcfly

Fly through your shell history. Great Scott!
MIT License
6.77k stars 176 forks source link

Terminal settings are not restored after cancelling chosen command with ctrl-c in bash (has workaround) #240

Open kaldonir opened 2 years ago

kaldonir commented 2 years ago

Overview

After choosing a command with mcfly, and cancelling the command with ctrl-c, terminal settings are not restored correctly for me. This manifests in characters being no longer echoed to stdout when typing until I run stty echo. The terminal settings are correctly restored when the chosen command runs to completion.

How to reproduce

  1. <ctrl-r>
  2. sleep 100
  3. <return>
  4. <ctrl-c>
  5. observe that characters are no longer echoed to stdout when typing

Relevant software versions

cantino commented 2 years ago

Thanks for the report. I've never seen this happen, but I also don't use NixOS.

kaldonir commented 2 years ago

If it's not reproducible in other setups, then I'll have to investigate some more where this might be coming from. Can you point me to the part of the code that changes these terminal settings?

cantino commented 2 years ago

If the interface isn't being cleaned up right, perhaps it's an issue with termion's AlternateScreen implementation, initialized here.

kaldonir commented 2 years ago

I poked around a bit, and apparently this issue only occurs when mcfly search is run via the hotkey bound with bind -x. This doesn't happen when I directly execute mcfly search.

I'll try to build a minimal example with the AlternateScreen functionality to figure out what's the issue here, thanks for the pointer!

kaldonir commented 2 years ago

I have tracked this down to the fake typer with TIOCSTI, which seems to trigger this when used in conjunction with bash's bind. I didn't find too much information about this, but someone on stackoverflow has encountered a similar problem: https://stackoverflow.com/questions/56246726/why-does-tiocsti-injection-disable-stty-echo-in-bash-when-used-with-bind

Sadly there's no helpful answer there. I'll dig deeper.

kaldonir commented 2 years ago

This turned out to be a bug in bash and has been fixed already.

I don't think there is anything to do in this repository.

cantino commented 2 years ago

Thank you for investigating it though!

cantino commented 1 year ago

Thanks for investigating it though!

On Mon, Mar 14, 2022 at 2:47 AM kaldonir @.***> wrote:

This turned out to be a bug in bash https://lists.gnu.org/archive/html/bug-bash/2022-03/msg00019.html and has been fixed already.

I don't think there is anything to do in this repository.

— Reply to this email directly, view it on GitHub https://github.com/cantino/mcfly/issues/240#issuecomment-1066576773, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUO65HJX5AZJMFZB3D7EDU74DLRANCNFSM5PK2WSKA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

thijsputman commented 3 months ago

Apologies for bumping an old issue; in case anyone runs across this and is unable to upgrade to a version of Bash without this issue, the below might help.

Change your McFly initialisation to something along the lines of:

eval "$(mcfly init bash)"

if [[ $- =~ .*i.* &&
  ${BASH_VERSINFO[0]} -eq 5 && ${BASH_VERSINFO[1]} -lt 2 ]]; then

  mcfly_search() {
    echo "#mcfly: ${READLINE_LINE[*]}" >> "$MCFLY_HISTORY"
    READLINE_LINE=
    mcfly search
    stty sane
  }

  bind -x '"\C-r": mcfly_search'

fi

This overwrites the original bind -x with an identical one that adds stty sane add the end. This restores the terminal to sane defaults. Not ideal as it overwrites any customisations you might have made, but at the very least it restores +echo...

Seems to resolve the issue on my Ubuntu 22.04 / Bash 5.1.16 system.

@cantino, does it make sense for me to propose a PR to fix this properly in mcfly.bash, or is the official line everyone should just upgrade to Bash 5.2? 😉

cantino commented 2 months ago

Thanks @thijsputman. I don't think we'd want this fix globally since it wipes out a user's settings. Maybe we add to the README?

thijsputman commented 2 months ago

@cantino, it seems I was quick to jump the gun...

After running with the above "solution" for a while, it turned out to have a major caveat: It breaks the functionality of most "special" keys (e.g. arrow-left/-right; home; end) in the shell. As such it wasn't really an improvement over the original situation.

I resigned myself to doing a (blind) stty sane every now and then, and waiting for Ubuntu 24.04 to get released.

mernen commented 2 months ago

@thijsputman Hi, you may be interested in the workaround I was using. I added the following to my bashrc, after the mcfly setup:

# Work around bash bug when pressing ^C after TIOCSTI
PROMPT_COMMAND="${PROMPT_COMMAND%;};stty echo icrnl icanon"

(Quick explanation: ${var%;} will strip any ; suffix from the variable, to avoid an accidental double-semicolon if the last command that modified your $PROMPT_COMMAND left a trailing one)

It's been a few years, but IIRC I got those specific flags by comparing stty output before and after the bug was triggered. This should be less impactful than a full stty sane, and it runs after mcfly is already done, so it doesn't disrupt its operation either. The command is very fast, and I noticed no slowdown on my prompt (something I take very seriously!).

cantino commented 2 months ago

Leaving this here for others to find. Thanks @mernen!

thijsputman commented 2 months ago

Thanks @mernen, that indeed did the trick! 😃

thijsputman commented 1 month ago

Sorry to keep bumping this issue, but there's a slight caveat to @mernen's solution too: It "masks" the exit-code of the previous command (ie echo $? will return 0 regardless of the previous command's status).

Here's an updated workaround that mitigates this:

function _mcfly_workaround() {
  rc=$?
  stty echo icrnl icanon
  return $rc
}

PROMPT_COMMAND="${PROMPT_COMMAND%;};_mcfly_workaround"
tkanarsky commented 1 month ago

+1 for the workarounds in the chat! Thanks. Sadly I'm stuck on Ubuntu 20.04...