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

Executing fg via binding makes keybindings not work the second time #2114

Closed
jonhoo opened this issue Jun 1, 2015 · 21 comments
Closed

Executing fg via binding makes keybindings not work the second time #2114

jonhoo opened this issue Jun 1, 2015 · 21 comments
Labels
bug
Milestone

Comments

@jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Jun 1, 2015

With this binding in config.fish:

function fish_user_key_bindings
    bind \cr 'fg >/dev/null ^/dev/null'
end

Do the following commands in a new shell:

$ vim
# <press Ctrl+Z inside vim to send to background>
# <press Ctrl+r, vim (correctly) restored>
# <press Ctrl+Z again>
# <press Ctrl+r, ^R is printed to terminal, vim is not restored>

In particular, no key bindings work at this point until enter or ^J is pressed.

@ridiculousfish ridiculousfish added this to the fish-future milestone Jun 1, 2015
@faho
Copy link
Member

@faho faho commented Jul 16, 2015

I can reproduce this.

Some more info:

  • It's not just \cr, it appears to be binding fg that's causing the issue
  • It's not just vim, less also works
  • Running fg manually works
  • It's also an issue if you don't bg again but quit
  • Syntax highlighting and suggestion are also non-functional, only entering basic text seems to work
  • Entering commands works, but echos them again on the next line, highlighted

In other words:

$ touch halibut
$ less halibut
<\cz>
<\cr>
<q>
$ echo barracuda # in white
echo barracuda
barracuda # dundundundundundundundundun dun dun

@jonhoo
Copy link
Contributor Author

@jonhoo jonhoo commented Jul 16, 2015

I get almost the same behavior as @faho on 97edc96, except that the scary "dundundundundundundundundun dun dun" part seems to be missing.

@faho
Copy link
Member

@faho faho commented Jul 16, 2015

the scary "dundundundundundundundundun dun dun" part

HAhAHAHAhA.... that was my attempt at a reference... 😆

@jonhoo
Copy link
Contributor Author

@jonhoo jonhoo commented Jul 16, 2015

Hehe, yes, I got that 😉

@jonhoo
Copy link
Contributor Author

@jonhoo jonhoo commented Feb 16, 2016

This is really unfortunate, as I run into this multiple times a day.
@ridiculousfish: Any pointers as to where I might start looking to fix it?

@faho
Copy link
Member

@faho faho commented Feb 16, 2016

@jonhoo: It's possible fish isn't resetting the terminal mode correctly. For me with a ToT fish just executing a single command (like true) interactively helps.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 17, 2016

Simply pressing enter after suspending the command the second time fixes the fish input state. I'm confident this is not a tty mode problem. You don't even need to use a full-screen program like vim or less. Simply running sleep 9999 then doing the \cZ\cR\cZ dance will reproduce the problem.

Frankly, I'm not sure this should even be allowed. If you've already started typing another command then press \cR to magically foreground a job what do you expect to happen to the characters you've already entered? If we do want to support this the fix probably lies in having the code that responds to \cZ (or whatever the suspend key is) forcibly redraw the command line and otherwise ensure it is in a sane state.

@jonhoo, I'm curious why you created that binding? Why not just type fg? Which is not to imply the issue you identified shouldn't be fixed one way or another. It just seems like a rather odd binding.

@jonhoo
Copy link
Contributor Author

@jonhoo jonhoo commented Feb 17, 2016

@krader1961: I don't think there's something magical about \cR with regards to output. The same could be said for \cZ and then manually typing fg, no? I use this binding because it is faster than typing fg<Enter>, and I do it a lot.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 17, 2016

@jonhoo: You misunderstood me. I agree there is nothing magical about the specific key sequence you bind the fg command to. You could use an arbitrary sequence like bind \eforeground fg. My point is that trying to execute the fg command from a key binding seemed weird as it saves maybe 100 ms of typing. Yes, it should "just work" in keeping with the fish philosophy; or, at least be disallowed with an explicit error message if there is no reasonable way to make this work reliably.

@jonhoo
Copy link
Contributor Author

@jonhoo jonhoo commented Feb 17, 2016

@krader1961: Oh, sorry. Yes, I'd be fine with this being disallowed if you were in the middle of typing a command. However, it should work fine if you haven't yet typed anything. While you're right it doesn't save much time, it certainly interrupts the flow somewhat if I'm flicking back and forth between, say, vim and my terminal, typing in something based on the command I just ran. I could use multiple terminal tabs/windows, but anything involving tab is fairly slow, as it's far from home row.

@faho faho added the bug label Feb 26, 2016
@faho faho changed the title Subsequent binds not executed if first command leaves fish Executing fg via binding makes keybindings not work the second time Feb 26, 2016
@rbong
Copy link

@rbong rbong commented Feb 26, 2016

I believe it should work if something is already typed in if at all possible. Imagine if you're looking at a man page or another reference and wish to switch to/from the manual; it could be very useful.

In addition, fg could be only part of a larger binding. It is certainly annoying and prone to error to type the binding and thent typing in fg and enter. It's yet more characters if you wish to, say, run a command after fg separated by a semicolon, or type a space before fg to not log the command. In addition, it clutters up the terminal after exiting from the command. I am trying to do all of these things, as anyone trying to make fg part of a larger command might need to do.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 26, 2016

@rbong, I'm afraid I don't understand any of the scenarios you describe. Let's start with the first one:

Imagine if you're looking at a man page or another reference and wish to switch to/from the manual;

If I've typed man some-comand and pressed [enter] so that I'm looking at that man page the man command is now the foreground process. Which means fish won't receive any key presses and won't run the magic fg binding. Or are you suggesting that while running man some-command you type [ctrl-Z] to suspend it then start typing some-command --opt1 --opt2 and before pressing [enter] want to be able to invoke the fg binding to bring the man command to the foreground?

If the latter wouldn't it be simpler to simply open another shell and run the man command in parallel? That's what I normally do. That is, I normally have one or two terminals that I use for looking at documentation or source code while I'm actually editing code or entering other commands in a different window. That's more simpler and more general than what you seem to be trying to do.

run a command after fg separated by a semicolon

Huh? Under what circumstances does it make sense to put a process in the background then type something like fg; new-command? Granted, that should "just work" but I've never seen anyone do so in real life or in documentation, blog articles, etc.

or type a space before fg to not log the command

Double huh? Entering a space before fg will only keep fg from being logged. It will have absolutely no effect on whether the command being foregrounded was logged.

@rbong
Copy link

@rbong rbong commented Feb 27, 2016

Sorry for my confusing scenarios @krader1961

If the latter wouldn't it be simpler to simply open another shell and run the man command in parallel?

It is the latter scenario. I admittedly also open another window... however, I can see how someone would find this useful. On systems with smaller screens restricted to the terminal, the user is forced to use a multiplexer, as I have been many times.

With this scenario, I meant to illustrate that it is in fact useful to have just fg in a keybinding and to use that keybinding in the middle of writing a command.

Huh? Under what circumstances does it make sense to put a process in the background then type something like fg; new-command?

In this scenario where the user is going from and to some foreground process (a very good reason for binding fg to a keystroke), the user may need to clutter up the window with fg commands. A user will eventually run

<space>fg; clear

Or,

<space>clear; fg

Or, wrap clear and fg in a function. It's probably not a good idea to name a function only one or two letters, meaning it's still more than a few extra keystrokes.

Entering a space before fg will only keep fg from being logged

This is what I mean. fg will not be logged. If the user desires fg to be in a keybinding along with some other functionality, they probably wish to hide the command.

I am just illustrating that it's more than just one extra keystroke because of the extra shell management required to recieve the same result.

@krader1961
Copy link
Contributor

@krader1961 krader1961 commented Feb 27, 2016

This is what I mean. fg will not be logged.

Entering [space]fg might make sense in shells which log each command. But, FWIW, it's not a useful optimization in fish because fish will only ever have one instance of each unique fg command in the history file.

I've been programming since the days when 80 column IBM/Hollerith cards were still being used and a 80x24 CRT terminal was bleeding edge. Having two such terminals was considered a luxury. And yet I can't ever recall typing fg and bg so many times that it made me frustrated. But I'll agree that others may have different styles of working and/or concerns that makes binding fg to a key sequence a reasonable optimization for them. So I'm not arguing against making this work. I'm just perplexed that it's such a big deal.

@rbong
Copy link

@rbong rbong commented Feb 27, 2016

@krader1961 It's not a big deal, just a slight nuisance. If my scenarios seem weird, it's because I am using remote key presses in tmux to suspend any foreground processes, do something, and then go back without worrying about what just happened. It's two keypresses if fg works in a keybinding, which is fast uninterruptable and clean, but otherwise I have to worry about the state of various other things in the shell like I mentioned. I think I failed to generalize my situation.

@bruce-hill
Copy link

@bruce-hill bruce-hill commented Jun 15, 2019

This problem is caused by commit b67526a, which disabled the code that would otherwise restore the correct terminal mode after returning from a job. Reverting that change allows bind \cF fg to work correctly. From testing on linux, the cd .; ftp test case mentioned in the comment appears to work correctly even with the change reverted, so perhaps the root cause of the original bug (#121) was fixed by one of the many changes since 2012, but I don't have midnight commander, so I can't test the exact original repro steps.

*Edit: the original bug reappears if the change is rolled back. I don't know exactly how to make everything work, but it definitely has to do with the block of code in b67526a. A reliable test case for me is ls | sh -c 'read </dev/tty >/dev/tty && echo "You said: $REPLY"'

@bruce-hill
Copy link

@bruce-hill bruce-hill commented Jun 15, 2019

I believe this patch fixes the issue without reintroducing the original bug #121:

1035c1035
< static bool terminal_return_from_job(job_t *j) {
---
> static bool terminal_return_from_job(job_t *j, int restore_attrs) {
1060,1072c1060,1070
< // Disabling this per
< // https://github.com/adityagodbole/fish-shell/commit/9d229cd18c3e5c25a8bd37e9ddd3b67ddc2d1b72 On
< // Linux, 'cd . ; ftp' prevents you from typing into the ftp prompt. See
< // https://github.com/fish-shell/fish-shell/issues/121
< #if 0
< // Restore the shell's terminal modes.
< if (tcsetattr(STDIN_FILENO, TCSADRAIN, &shell_modes) == -1) {
< if (errno == EIO) redirect_tty_output();
< debug(1, _(L"Could not return shell to foreground"));
< wperror(L"tcsetattr");
< return false;
< }
< #endif
---
>     // Need to restore the terminal's attributes or `bind \cF fg` will put the
>     // terminal into a broken state (until "enter" is pressed).
>     // See: https://github.com/fish-shell/fish-shell/issues/2114
>     if (restore_attrs) {
>         if (tcsetattr(STDIN_FILENO, TCSADRAIN, &shell_modes) == -1) {
>             if (errno == EIO) redirect_tty_output();
>             debug(1, _(L"Could not return shell to foreground"));
>             wperror(L"tcsetattr");
>             return false;
>         }
>     }
1091c1089,1092
<             terminal_return_from_job(this);
---
>             // Only restore terminal attrs if we're continuing a job. See:
>             // https://github.com/fish-shell/fish-shell/issues/121
>             // https://github.com/fish-shell/fish-shell/issues/2114
>             terminal_return_from_job(this, send_sigcont);

@zanchey
Copy link
Member

@zanchey zanchey commented Aug 31, 2019

@ridiculousfish @faho @mqudsi would you be able to take a look at the comments and patch from @bruce-hill and see if this is something we should take for 3.1?

@ridiculousfish ridiculousfish removed this from the fish-future milestone Sep 1, 2019
@ridiculousfish ridiculousfish added this to the fish 3.1.0 milestone Sep 1, 2019
@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Sep 1, 2019

Tentatively pulling into 3.1 so we don't lose it.

@ridiculousfish
Copy link
Member

@ridiculousfish ridiculousfish commented Sep 2, 2019

This fix looks good and makes sense, fixed as 9fd9f70 . Thanks @bruce-hill !

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 19, 2020
Prior to this change, when a process resumes because it is brought back
to the foreground, we would reset the terminal attributes to shell mode.
This fixed fish-shell#2114 but subtly introduced fish-shell#7483.

This backs out 9fd9f70, re-introducing fish-shell#2114 and re-fixing fish-shell#7483.
A followup fix will re-fix fish-shell#2114; these are broken out separately for
bisecting purposes.

Fixes fish-shell#7483.
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 20, 2020
Prior to this change, when a process resumes because it is brought back
to the foreground, we would reset the terminal attributes to shell mode.
This fixed fish-shell#2114 but subtly introduced fish-shell#7483.

This backs out 9fd9f70, re-introducing fish-shell#2114 and re-fixing fish-shell#7483.
A followup fix will re-fix fish-shell#2114; these are broken out separately for
bisecting purposes.

Fixes fish-shell#7483.
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 21, 2020
Prior to this change, when a process resumes because it is brought back
to the foreground, we would reset the terminal attributes to shell mode.
This fixed fish-shell#2114 but subtly introduced fish-shell#7483.

This backs out 9fd9f70, re-introducing fish-shell#2114 and re-fixing fish-shell#7483.
A followup fix will re-fix fish-shell#2114; these are broken out separately for
bisecting purposes.

Fixes fish-shell#7483.
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2020
Prior to this change, when a process resumes because it is brought back
to the foreground, we would reset the terminal attributes to shell mode.
This fixed fish-shell#2114 but subtly introduced fish-shell#7483.

This backs out 9fd9f70, re-introducing fish-shell#2114 and re-fixing fish-shell#7483.
A followup fix will re-fix fish-shell#2114; these are broken out separately for
bisecting purposes.

Fixes fish-shell#7483.
mqudsi referenced this issue Mar 5, 2021
Prior to this fix, when key binding is a script command (i.e. not a
readline command), fish would run that key binding using fish's shell
tty modes. Switch to using the external tty modes. This re-fixes
issue #2214.
@faho
Copy link
Member

@faho faho commented Mar 10, 2021

Reopening because the fix for #7770 reintroduces this

@faho faho reopened this Mar 10, 2021
@faho faho removed this from the fish 3.1.0 milestone Mar 10, 2021
@faho faho added this to the fish-future milestone Mar 10, 2021
ridiculousfish added a commit to ridiculousfish/fish-shell that referenced this issue Mar 11, 2021
Restore terminal modes after running key bindings with external commands

This concerns the behavior when running an external command from a key
binding. The history is:

Prior to 5f16a29, fish would run these external commands in shell
modes. This meant that fish would pick up any tty changes from external
commands (see fish-shell#2114).

After 5f16a29, fish would save and restore its shell modes around
these external commands. This introduced a regression where anything the
user typed while a bound external command was executing would be echoed,
because external command mode has ECHO set in c_lflag. (This can be
reproed easily with `bind -q 'sleep 1'` and then pressing q and typing).
So 5f16a29 was reverted in fd93559.

This commit partially reverts fd93559. It has it both ways: external
commands are launched with shell modes, and also shell modes are restored
after the external command completes. This supports commands which muck
with the tty, but does not trigger ECHO mode.

Fixes fish-shell#7770. Fixes fish-shell#2114 (for the third time!)

This partially reverts commit fd93559.
@zanchey zanchey removed this from the fish-future milestone Mar 11, 2021
@zanchey zanchey added this to the fish 3.2.1 milestone Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug
Projects
None yet
Development

No branches or pull requests

7 participants