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

Vi-mode doesn't play well with multi-line prompts #3481

Closed
paradox460 opened this issue Oct 23, 2016 · 41 comments
Closed

Vi-mode doesn't play well with multi-line prompts #3481

paradox460 opened this issue Oct 23, 2016 · 41 comments
Labels
bug Something that's not working as intended vi-mode
Milestone

Comments

@paradox460
Copy link

paradox460 commented Oct 23, 2016

I have a multi-line prompt, where there is information displayed on one line, and the user input goes on the second. Basically handled through something like this:

echo -n -e -s (pwd) (__fish_git_prompt)"\n$"

This has worked without flaw in vi-mode in the past. As of version 2.3.1-667-g668de88, and likely caused by #3215, switching modes results in the whole prompt being pushed down, not redrawn over the existing prompt.

<insert mode:>
[ i ] ~/developer/docker
$ docker-compose start api

<switching to normal mode>
[ i ] ~/developer/docker
[ n ] ~/developer/docker
$ docker compose start api

Additionally, the cursor does not redraw as a bar/underscore, but remains how its configured

System info

This issue is new, and cannot be reproduced in the stable (2.3.1) version of fish

@faho
Copy link
Member

faho commented Oct 23, 2016

What happens if you save an empty fish_vi_cursor function, i.e. add the following to config.fish:

function fish_vi_cursor; end

?

@faho
Copy link
Member

faho commented Oct 23, 2016

I cannot confirm this with konsole (which uses the same cursor sequences).

@paradox460
Copy link
Author

Adding function fish_vi_cursor; end to my config fixes the redrawing issue. Calling echo -en '\e]50;CursorShape=1\x7' manually, however, does change the cursor shape.

Here's what the envars referenced in fish_vi_cursor are set to on my machine:

TERM=xterm-256color
ITERM_PROFILE=Default

@paradox460
Copy link
Author

Additionally, overloading __fish_cursor_konsole and __fish_cursor_xterm with an echo statement specific to each one shows that __fish_cursor_konsole is being called.

@faho
Copy link
Member

faho commented Oct 23, 2016

What are your $fish_cursor variables set to? Specifically $fish_cursor_insert and $fish_cursor_default?

@faho faho added bug Something that's not working as intended needs more info labels Oct 23, 2016
@paradox460
Copy link
Author

paradox460 commented Oct 23, 2016

Only unknown appears to be set

$ set | grep cursor
fish_cursor_unknown 'block'  'blink'

@faho
Copy link
Member

faho commented Oct 23, 2016

What happens when you set them? Is there any difference between "block" and "line" and "underscore"? (Any other value should disable this function)

@paradox460
Copy link
Author

Setting them makes the cursor change work, but the newline bug is still there.

I've noticed a few behaviors of the newline bug:

  • it only occurs when transitioning from insert to normal mode
  • transitioning from normal to insert or visual, or from visual to normal, behaves as expected
  • transitioning when the cursor is at the start of the line in insert mode behaves as expected. the bug is only reproducible if the cursor is anywhere else in the line, including the end

@faho
Copy link
Member

faho commented Oct 23, 2016

it only occurs when transitioning from insert to normal mode

That's good to know. The escape binding in insert mode is a bit complicated to allow canceling in the pager and to facilitate vi's weird behavior of moving the cursor back.

Can you try bind -M insert -m default \e cancel or "force-repaint" instead of cancel?

@paradox460
Copy link
Author

paradox460 commented Oct 24, 2016

Force repaint fixes the issue for transitioning the cursor at its current position between modes, but in testing it I found another oddity: whenever I use operations that both move the cursor and change normal to insert (eg: I, A) the bug is reproduced. This applies even with force-repaint in play

@faho
Copy link
Member

faho commented Oct 26, 2016

Force repaint fixes the issue for transitioning the cursor at its current position between modes

If you mean that the cursor moves back when you enter normal mode, that's on purpose, because that's what vi does.

whenever I use operations that both move the cursor and change normal to insert (eg: I, A) the bug is reproduced. This applies even with force-repaint in play

My guess is iTerm doesn't handle that particular sequence of escape sequences correctly.

Can you record a session with script (ie. execute script, reproduce the problem and then exit, this will create a file called typescript which you should upload somewhere - I'm not sure if github's comment system will mangle it)?

Can you try it with an empty fish_mode_prompt function?

@paradox460
Copy link
Author

I meant bind -M insert -m default \e force-repaint works as I believe it should; as it used to before this bug cropped up.

I've attached the typescript in a zip so as to prevent github from messing it up
typescript.zip

@zanchey
Copy link
Member

zanchey commented Oct 27, 2016

@paradox460, from my reading it doesn't look like this affects 2.4b1 - are you able to test that version?

@paradox460
Copy link
Author

paradox460 commented Nov 1, 2016

Just installed and tested 2.4b1 (via brew reinstall fish --devel)

The error is still manifesting itself, and I verified that the shell was running 2.4b1


However, it does seem to be fixed in 2.3.1-712-g19e12e3

@faho
Copy link
Member

faho commented Nov 1, 2016

@paradox460: Is your cursor shape still changing with 2.3.1-712-g19e12e3?

There aren't that many differences between 2.4b1 and master (i.e. 19e12e3). There are some lint fixes that should not change anything, there are some unrelated script changes that I'm confident won't change this, and there's 2a5ad19.

Which is also in the Integration_2.4.0 branch (as c8fe0e5), but it should still change the cursor in your case, at least when the variables are set to something. That is unless I've misunderstood something about your configuration - are you using GNU screen or something similar?

And if it still changes the cursor, I'm kinda confused as to how this should be fixed.

Can you test the Integration_2.4.0 branch (which should become 2.4.0) so we can rule out the other differences?

@paradox460
Copy link
Author

paradox460 commented Nov 8, 2016

Sorry for the delayed response.

Running fish, version 2.4.0-108-gd7283cd this morning, added set fish_cursor_insert "line"; set fish_cursor_default "underscore" to my config.fish. No cursor changes, but the newline bug I originally reported does not manifest itself.


Running fish, version 2.4.0, with the same variables set in my config, produces the same behavior. Cursor moves fine, no mutli-line redrawing issues, but it will not change.

I am not using screen or similar multiplexers.


Running __fish_cursor_konsole 'line' manually on the command-line changes the cursor to a line

@faho
Copy link
Member

faho commented Nov 8, 2016

I'd imagine tput Ss on your system returns > 0, in which case we don't run the cursor stuff.

@paradox460
Copy link
Author

Yep, returns 4 with unknown terminfo capability 'Ss'

@krader1961
Copy link
Contributor

Does this still need more info? If not, and we still believe this is an unresolved issue, that label should be removed. Otherwise it should be closedd.

@faho
Copy link
Member

faho commented Mar 29, 2017

Does this still need more info?

Yes. Specifically, it needs someone with a bit of expertise to look at it. It requires iTerm2 (which is why I'm unable to check), and vi-mode with vi_cursor with the tput Ss check patched out.

@krader1961
Copy link
Contributor

With the if not tput Ss... block in place the fish_vi_cursor_handle_preexec and fish_vi_cursor_handle functions are not defined. With that block removed they are defined. The latter being defined thusly:

function fish_vi_cursor_handle --on-variable fish_bind_mode --on-event fish_postexec
    set -l varname fish_cursor_$fish_bind_mode
    if not set -q $varname
        set varname fish_cursor_unknown
    end

    __fish_cursor_1337 $$varname
end

The problem appears to be with this block of code in share/functions/fish_vi_cursor.fish:

and not string match -q "konsole*" -- $TERM
or not begin set -q KONSOLE_PROFILE_NAME

If I change that or to an and then this works as expected. You basically have this in the current version of that script:

if cond1
and cond2
and cond3
or cond4
    return  # terminal does not support console shapes
end

The problem is that cond2 is true and cond4 is false inside iTerm2 on macOS. That cond4 is false doesn't matter because cond2 is true. But cond4 is clearly meant to keep from executing the return.

Note that simply changing the or cond4 to and cond4 is not correct. It just happens to "fix" the problem in this specific situation due to how cond4 is constructed given that iTerm2 exports TERM=xterm and also ITERM_PROFILE.

I hate to say "I told you so" but I did in fact warn that this extremely hard to understand block of chained conditionals was likely wrong when I reviewed the change which introduced it. And that decomposing it into independent tests which are evaluated in the opposite order would be safer. That is, first test for the whitelisted scenarios that we know work despite things like tput Ss failing. Such as having TERM=xterm and ITERM_PROFILE defined. Only proceed to the more generic blacklist tests if we know the environment isn't whitelisted.

@faho
Copy link
Member

faho commented Mar 30, 2017

@krader1961: I know tput Ss is wrong here. What I need to know is if you can still reproduce the issue at hand, i.e. vi-cursor interfering with multi-line prompts in iTerm.

If you can't, we can remove the tput entirely. If you can, we need to figure out why it occurs.

@krader1961
Copy link
Contributor

Hmmm. Last night I couldn't reproduce the problem. However, I suspect that may be due to using tmux. I can reproduce the problem on a bare iTerm2 without tmux. Using script to capture what is being written to the terminal shows that switching from insert to normal mode sends the ^[]1337;CursorShape=2^G to switch the cursor to an underscore. Then the prompt is repainted twice. The first time without repositioning the cursor to the first line of the prompt. The second time is does send the ^[[A sequence to move the cursor up one line.

Switching from normal to insert mode sends the ^[]1337;CursorShape=1^G sequence to switch the cursor back to a vertical line. Then the prompt is repainted once beginning with the ^[[A sequence to move the cursor up one line.

So this is not a iTerm2 specific problem AFAICT. I'll do more testing to try and pinpoint what is causing the transition from insert to normal mode to repaint twice with the first one not properly repositioning the cursor.

@krader1961
Copy link
Contributor

This also affects macOS Terminal.app (which uses __fish_cursor_xterm) and xterm (from XQuartz). Both are using __fish_cursor_xterm rather than __fish_cursor_1337 used for iTerm2. So it isn't specific to iTerm2 or the particular cursor script.

Any output from fish_vi_cursor_handle to the terminal causes the problem. If I instead redirect the output to a file or define it as an empty function the problem does not occur.

Also, not sure what I was doing different last night but I can now reproduce this both with and without tmux between fish and the terminal.

Finally, I can reproduce the problem using konsole on Ubuntu 16.10 but not reliably. I find that typing random combinations of \e, i, a, and other characters will sometimes cause the problem. So there is clearly something wrong with fish's idea of the state of the screen that is causing this issue. Or possibly a race condition.

Running infocmp -1 xterm-256color on macOS and Ubuntu shows many differences. But I've tried using the terminfo definition from each OS on the other without making any noticeable change in behavior. There is clearly a bug somewhere in fish but since I no longer use vi key bindings, and this is a cosmetic issue, I'm not motivated to dig deeper.

I still assert, @faho, that the block beginning with if not tput Ss >/dev/null ^/dev/null should be refactored. The lengthy compound conditional is far too hard to understand.

rouge8 added a commit to rouge8/dotfiles that referenced this issue Mar 20, 2021
This patch enables fish's vi-mode cursors (e.g. a line in insert mode, a
block in normal mode, an underscore in replace mode) and attempts to
workaround fish-shell/fish-shell#3481. It doesn't quite work however; it
only fixes the transition from insert -> normal mode. Replace -> normal,
normal -> append, and probably others are still broken. It also doesn't
work with `fish_mode_prompt`...
rouge8 added a commit to rouge8/dotfiles that referenced this issue Mar 20, 2021
This patch enables fish's vi-mode cursors (e.g. a line in insert mode, a
block in normal mode, an underscore in replace mode) and attempts to
workaround fish-shell/fish-shell#3481. It doesn't quite work however; it
only fixes the transition from insert -> normal mode. Replace -> normal,
normal -> append, and probably others are still broken. It also doesn't
work with `fish_mode_prompt`...
@rouge8
Copy link
Contributor

rouge8 commented Mar 20, 2021

I was almost able to get this to work by adding commandline -f repaint to __fish_cursor_xterm. That stops the duplicate prompt when transitioning from insert to normal mode, but not from replace to normal or normal to append or etc. It also behaves weirdly with fish_mode_prompt on the first line of a multiline prompt -- when transitioning from insert to normal mode, the cursor is moved to the far left of the prompt:

cursor over fish_mode_prompt on the far left

Posting this here in case it gives anyone any ideas. 🤷

@lilyball
Copy link
Contributor

I don't think this is a problem with the cursor escape at all, I think it's a problem with printing anything from an on-variable hook invoked at prompt time. I say this because defining an --on-variable fish_bind_mode function that simply runs tput sgr0 reproduces the issue for me. I suspect fish isn't even trying to check the output for escape codes and is simply assuming that printing anything requires repainting, and it's repainting over the current line instead of over the start of the multi-line prompt.

I can also reproduce the commandline -f repaint issue, by defining my function as tput sgr0; commandline -f repaint. I'm very curious as to why the behavior differs between modes, I wouldn't have expected the prompt code to care.


Here's a fun workaround that seems to work: define fish_mode_prompt to set the cursor. In a quick test right now, this allow for toggling the cursor without reproducing this issue. The downside is having this function defined means the regular prompt doesn't seem to be redrawn, which is a problem if the bind mode is indicated as part of the regular prompt (e.g. using starship).

But it looks like we can combine the two. Define the function to set the cursor and have it end by calling commandline -f repaint. fish 3.3.1 seems to avoid recursively invoking fish_mode_prompt.

@lilyball
Copy link
Contributor

Oops I just found fish_mode_prompt mentioned in #3696 (comment), but I think my usage of commandline -f repaint to allow the regular prompt to repaint too is new.

@lilyball
Copy link
Contributor

Hmm, after writing up code to hack fish_vi_cursor to implement exactly this, the commandline -f repaint trick has stopped working. I don't see anything different between what I was doing before and what I'm doing now though.

Also, I hadn't tested visual mode before, and testing it now the cursor is jumping to the beginning of the line when entering/exiting visual mode. Ugh.

rouge8 added a commit to rouge8/dotfiles that referenced this issue Jan 4, 2022
What I really want is for `fish_vi_cursor` to work, but alas I still run
into fish-shell/fish-shell#3481.

In the meantime, the cursor still changes in vim/nvim which is most of
what I would want.
@page-down
Copy link
Contributor

Recently there are feedbacks from kitty users about the fish vi cursor issue. Continue the discussion here.

@faho
Here are the complete steps to reproduce this issue. To standardize the environment, I use Kubuntu 21.10 LiveCD as an example.

  • Boot kubuntu-21.10-desktop-amd64.iso with a virtual machine.
  • Install fish 3.3.1
sudo apt-add-repository ppa:fish-shell/release-3
sudo apt install fish
  • config.fish
function fish_prompt; printf 'fish\n>'; end

set -g fish_key_bindings fish_vi_key_bindings

set -g fish_cursor_default block blink
set -g fish_cursor_insert line blink
set -g fish_cursor_replace_one underscore blink
set -g fish_cursor_visual block blink
  • Konsole 21.08.1
  • fish
  • Type sometext in insert mode, move cursor to end of line.
  • Press a and esc repeatedly at intervals of approximately 500ms.
[N] fish
[N] fish
[I] fish
[I] fish
[I] fish
>sometext

Also notice that the cursor is sometimes block in insertion mode and does not change to line.

You can also install the latest version of kitty, which reproduces the same issue.

@faho
Copy link
Member

faho commented Jan 24, 2022

Okay, it turns out when you press them slower it reproduces more. Possibly because you're then not hitting the escape timeout?

It seems the issue is some overlap between the event handler and the cursor movement.

Try this:

bind -M insert \e 'if commandline -P; commandline -f cancel; else; set fish_bind_mode default; commandline -f repaint-mode backward-char; end'
bind -m insert a repaint-mode forward-single-char

All this does is let the bindings for "a" in normal mode and escape in insert mode repaint the mode prompt first, before doing cursor movement. This seems to avoid triggering the issue, but it's also possible it just makes the already small window even smaller.

@page-down
Copy link
Contributor

page-down commented Jan 24, 2022

I tried the above configuration. It does improve a lot, but I still reproduce the issue.

So, with the current event mechanism, is it possible that it could be triggered during the execution of other operations and lead to unintended results?

EDIT:
example:

printi\e[5 q]ng text
\e]2;set-w\e[5 qindow-title\a

I'm not familiar with the code, is there a mechanism like a queue or something to solve the same kind of issue?

EDIT2:
I looked at the issues that have been posted and it looks like the following are relevant to this issue. They are all related to prompt redraws where the cursor is moved down a line (to redraw).

I saw in kitty's command output debug log that it's not actually a cursor down move.
In contrast to the normal vi cursor case, fish is missing a command to move the cursor up one line.

(1)

Prompt redraw bugs out when editing with a multiline prompt with fzf # 5945
https://github.com/fish-shell/fish-shell/issues/5945

> from mk12:
> ... it seems more that fish is just drawing the prompt in the wrong place, one line lower than it should.

(2)
I used something like the fish_mode_prompt method mentioned at the end here (below, # 3696) in kitty's PR to output the cursor escape sequence in prompt, instead of using function --on-variable fish_bind_mode.

This approach currently can solve the issue that occurs in kitty.

Support fish_vi_cursor on iTerm2 # 3696
https://github.com/fish-shell/fish-shell/issues/3696

(3)
Another issue reported for moving down one line when redrawing.

commandline operations in binding mess up multi-line prompts # 3074
https://github.com/fish-shell/fish-shell/issues/3074

> When using a binding that manipulates the command line after returning from a curses display,
> Fish repeats all prompt lines but the first on screen.

@Diaoul
Copy link

Diaoul commented May 15, 2022

@page-down have you managed to find a satisfying workaround?

@lilyball
Copy link
Contributor

With Fish 3.4 enabling vi cursors in Terminal.app I'm now hitting this all the time. In particular, I've noticed that pasting text from insert mode causes the prompt to move down two lines. This is presumably caused by fish_bind_mode being changed during the bracketed paste though I haven't dug into it. It's extremely annoying though, far more so than just the shift when switching modes manually as I paste more often than I leave insert mode, to the point that I'm ready to turn off cursors just to fix this even though I really want the cursor.

@ridiculousfish
Copy link
Member

ridiculousfish commented Jun 5, 2022

The tty timestamps are changing unexpectedly, leading fish to believe that some other process has been messing with the tty and then abandoning the line. We need to figure out what's causing the tty timestamps to change.

edit: it's the echoes for setting the cursor shape.

@ridiculousfish
Copy link
Member

Fixed by just re-fstating the tty after running external commands from key bindings.

@ridiculousfish ridiculousfish modified the milestones: fish-future, fish 3.5.0 Jun 6, 2022
@faho
Copy link
Member

faho commented Jun 7, 2022

Okay, someone on macOS will want to confirm that vi cursor shaping works on iTerm with multiline prompts now and then we can remove the awkward hack that disables it.

@ridiculousfish
Copy link
Member

iTerm2 seems OK, however there's still a bug where pasting in vi mode with a multi line prompt will cause the line to move down, probably the same root cause. Reopening to fix that.

ridiculousfish added a commit that referenced this issue Jun 12, 2022
The recent improvements to multiline prompts and vi-mode in #3481 appear
to be sufficient to make iTerm2 well behaved, so remove our hack which
disabled it by default.

Fixes #3696
@ridiculousfish
Copy link
Member

iTerm2 looks OK to me in some medium-length testing with vi mode and multi-line prompts so I enabled it by default (#3696).

rouge8 added a commit to rouge8/dotfiles that referenced this issue Jun 16, 2022
This patch enables fish's vi-mode cursors (e.g. a line in insert mode, a
block in normal mode, an underscore in replace mode) now that
fish-shell/fish-shell#3481 has been fixed.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something that's not working as intended vi-mode
Projects
None yet
Development

No branches or pull requests

10 participants