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

Update fish_vi_key_bindings.fish #7908

Closed
wants to merge 1 commit into from
Closed

Update fish_vi_key_bindings.fish #7908

wants to merge 1 commit into from

Conversation

xiruizhao
Copy link
Contributor

Description

add bindings for undo and redo

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

add bindings for undo and redo
@faho
Copy link
Member

faho commented Apr 7, 2021

These are already bound to history-search-backward/forward:

bind -s --preset u history-search-backward
bind -s --preset \cr history-search-forward

Given that undo is also not bound in readline's vi-mode I prefer not to change the default here. Sorry!

@faho faho closed this Apr 7, 2021
@faho faho added this to the will-not-implement milestone Apr 7, 2021
@xiruizhao
Copy link
Contributor Author

In readline 8.1, u is bound to vi-undo, although C-R is bound to reverse-search-history. Conforming to vim bindings is a better idea.

@krobelus
Copy link
Member

krobelus commented Apr 7, 2021

In readline 8.1, u is bound to vi-undo

I think it makes sense to do the same; the history search one is not a big loss given that k already does up-or-search.

It doesn't feel nice: after typing echo , pressing u once doesn't move the cursor.

I also tried binding undo to \c_ in insert mode (like readline) but hit #7910.

@faho
Copy link
Member

faho commented Apr 7, 2021

In that case, we should however remove the previous bindings.

@xiruizhao
Copy link
Contributor Author

What about binding previous ones to / and ? ? Removing them is also fine.

@faho
Copy link
Member

faho commented Apr 7, 2021

A vi-bindings user would have to decide that. I don't know what makes sense here.

(Also note that my bash with readline 8.1 does not have "u" bound to vi-undo - INPUTRC=/dev/null bash -c 'bind -Pm vi' says nothing about vi-undo or "u")

@xiruizhao
Copy link
Contributor Author

xiruizhao commented Apr 7, 2021

I just tried, u actually binds to vi-undo in bash 5.1.4 (this is also reflected in source code https://github.com/bminor/bash/blob/f3a35a2d601a55f337f8ca02a541f8c033682247/lib/readline/vi_keymap.c#L165), probably a bug of bind?

@faho
Copy link
Member

faho commented Apr 7, 2021

Doesn't happen for me. u doesn't appear to be doing anything. Probably some compile-time settings again (I've checked my local customization, I have very little of it and nothing that should apply to vi-mode let alone u).

@faho
Copy link
Member

faho commented Apr 7, 2021

Ah, no undo is just weirder than I thought - it doesn't undo entering text, but if I dw then it'll undo something. Still not displayed in bind -P.

@xiruizhao
Copy link
Contributor Author

The bind bug is probably due to https://github.com/bminor/bash/blob/f3a35a2d601a55f337f8ca02a541f8c033682247/lib/readline/funmap.c#L59 not having vi-undo. I will notify readline's maintainer.

@xiruizhao
Copy link
Contributor Author

xiruizhao commented Apr 7, 2021

Since j and k include forward/backward history search functionality, I deleted previous bindings. You can reopen this PR and review it. @faho

@krobelus
Copy link
Member

krobelus commented Apr 8, 2021

@xiruizhao GitHub doesn't allow to reopen after a force push, but I'll cherry-pick your commit and add a changelog entry.

@xiruizhao
Copy link
Contributor Author

Ah, no undo is just weirder than I thought - it doesn't undo entering text, but if I dw then it'll undo something. Still not displayed in bind -P.

It would be actually nice to undo insertions/paste.

@zanchey
Copy link
Member

zanchey commented Apr 12, 2021

@xiruizhao GitHub doesn't allow to reopen after a force push, but I'll cherry-pick your commit and add a changelog entry.

That's f66f999 from https://github.com/xiruizhao/fish-shell/tree/patch-1 I take it?

@krobelus
Copy link
Member

Sorry for the delay, cherry-picked as 8bbb06b

owarai added a commit to owarai/fzf.fish that referenced this pull request Apr 16, 2021
owarai added a commit to owarai/fzf.fish that referenced this pull request Apr 16, 2021
owarai added a commit to owarai/fzf.fish that referenced this pull request May 14, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants