fix: move cursor left when exiting vi insert mode with Escape

[andrewgazelka]

Summary

• In vim, pressing Escape to exit insert mode moves the cursor left by one position
• This is because insert mode positions the cursor between characters, while normal mode positions it on a character
• Without this fix, repeatedly pressing i <esc> i <esc> would not move the cursor backward as expected

Test plan

• Added test esc_from_insert_mode_moves_cursor_left
• Updated existing test to explicitly test Esc from normal mode
• All 756 tests pass

[blindFS]

Thanks for trying to improve vi keybindings.

I see the point of aligning the behaviors of reedline vi-mode with those of vim/neovim.

But I don't think this specific one is very useful. It somehow reveals that we may have a bigger issue of wrong calculations of cursor positions within vi mode, see also #927. It would be better to fix them all-together with some fundamental changes/redesigning than this kind of case-wise tuning.

[andrewgazelka]

Hmmm makes sense.

[andrewgazelka]

Thanks for trying to improve vi keybindings.

I see the point of aligning the behaviors of reedline vi-mode with those of vim/neovim.

But I don't think this specific one is very useful. It somehow reveals that we may have a bigger issue of wrong calculations of cursor positions within vi mode, see also #927. It would be better to fix them all-together with some fundamental changes/redesigning than this kind of case-wise tuning.

you want literally 1:1? what must be added

[blindFS]

Thanks for trying to improve vi keybindings.

I see the point of aligning the behaviors of reedline vi-mode with those of vim/neovim.

But I don't think this specific one is very useful. It somehow reveals that we may have a bigger issue of wrong calculations of cursor positions within vi mode, see also #927. It would be better to fix them all-together with some fundamental changes/redesigning than this kind of case-wise tuning.

you want literally 1:1?

Not necessarily, just taking those off-by-1 issues like #766 into account. And you may find a better way to fix them.

what must be added

Actually I don't have a clear view of how to do it.

[blindFS]

@fdncred @ysthakur any thought?

[ysthakur]

I agree with @blindFS that we should try to fix the source of the off-by-ones rather than fix individual issues downstream.

By the way, we already had a PR for this with a very similar implementation: #699. It got reverted in #773 because it broke history search somehow: #772. I'm not sure if your implementation suffers from the same bug, but make sure to test that.

[fdncred]

I thought this looked familiar. Thanks yash! I agree with yash and bfs.

[andrewgazelka]

btw I currently do not have capacity to make improvements to this so feel free to take over if this impl is best fit.