RFC: Share some (but not all) bindings between vi- and emacs-mode #3068

Closed
wants to merge 9 commits into
from

Projects

None yet

7 participants

@faho
Member
faho commented May 25, 2016 edited

This undoes the inheritance since it shared too much.

Of course we need some discussion about which bindings precisely should be shared - I've tried to only pick ones which aren't related to text editing as such. Bindings without modifiers (alt/escape or control) are also likely to be superseded by improvements to vi-mode so probably aren't good candidates.

@krader1961: I'm particularly interested in your thoughts as you were vehemently against inheritance and use vi-mode.

(Note: This currently does not include #3061)

  • Documentation updated
  • Release notes updated
  • Fix the test failures
@faho faho Share some bindings between vi- and emacs-mode
This undoes the inheritance since it shared too much.
31578eb
@faho faho added the enhancement label May 25, 2016
@krader1961 krader1961 commented on an outdated diff May 26, 2016
share/functions/__fish_shared_key_bindings.fish
@@ -0,0 +1,75 @@
+function __fish_shared_key_bindings -d "Bindings shared between emacs and vi mode"
+ # These are some bindings that are supposed to be shared between vi mode and default/emacs mode
@krader1961
krader1961 May 26, 2016 Member

We should probably start chipping away at the idea that the "default" binding mode is somehow distinct from supporting emacs bindings. There are probably a handful of places in the documentation where "default == emacs" is meaningful. But this isn't one of them.

@krader1961 krader1961 commented on an outdated diff May 26, 2016
share/functions/__fish_shared_key_bindings.fish
@@ -0,0 +1,75 @@
+function __fish_shared_key_bindings -d "Bindings shared between emacs and vi mode"
+ # These are some bindings that are supposed to be shared between vi mode and default/emacs mode
+ # They are supposed to be unrelated to text-editing
+ # This takes $argv so the vi-bindings can pass the mode they are valid in
@krader1961
krader1961 May 26, 2016 Member

Please use typical punctuation here and the sentences above. That is, these lines are meant to be read as sentences and should thus be terminated by a period.

@krader1961
krader1961 May 26, 2016 Member

Yes, I realize my prior comment is pedantic. But one of the things about the fish source code that is annoying is the weird mixing of sentences and sentence fragments.

@krader1961 krader1961 commented on an outdated diff May 26, 2016
share/functions/fish_default_key_bindings.fish
@@ -11,30 +11,19 @@ function fish_default_key_bindings -d "Default (Emacs-like) key bindings for fis
bind --erase --all
end
+ # These are shelly bindings that we share with vi mode
@krader1961
krader1961 May 26, 2016 Member

s/shelly/shell specific/

@krader1961 krader1961 commented on the diff May 26, 2016
share/functions/fish_vi_key_bindings.fish
@@ -17,14 +17,13 @@ function fish_vi_key_bindings --description 'vi-like key bindings for fish'
set init_mode $argv[1]
end
- # Inherit default key bindings.
+ # Inherit shared key bindings.
@krader1961
krader1961 May 26, 2016 Member

I like this wording since it makes it clearer that the bindings are shared by the two opposite bindings (i.e., emacs vs vi).

@faho faho Incorporate review feedback
Comment-changes only.

[ci skip]
c89c0a2
@krader1961 krader1961 commented on the diff May 30, 2016
share/functions/__fish_shared_key_bindings.fish
+ bind $argv \eO3B history-token-search-forward
+ bind $argv \e\[3A history-token-search-backward
+ bind $argv \e\[3B history-token-search-forward
+ bind $argv \e\[1\;3A history-token-search-backward
+ bind $argv \e\[1\;3B history-token-search-forward
+ bind $argv \e\[1\;9A history-token-search-backward # iTerm2
+ bind $argv \e\[1\;9B history-token-search-forward # iTerm2
+ # Bash compatibility
+ # https://github.com/fish-shell/fish-shell/issues/89
+ bind $argv \e. history-token-search-backward
+
+ bind $argv \el __fish_list_current_token
+ bind $argv \ew 'set tok (commandline -pt); if test $tok[1]; echo; whatis $tok[1]; commandline -f repaint; end'
+ bind $argv \cl 'clear; commandline -f repaint'
+ bind $argv \cc __fish_cancel_commandline
+ bind $argv \cu backward-kill-line
@krader1961
krader1961 May 30, 2016 Member

This one is a questionable in vi mode. It's already bound to c^ in vi mode. So is the next one which doesn't have a vi mode binding. On the other hand neither of these has a direct correlate in Vim. So I'm okay with keeping them in this script.

@krader1961
Member

LGTM

@terlar
Contributor
terlar commented May 30, 2016

I haven't tried out this branch yet, but I really like how things are right now. Personally I am a heavy user of both the vi and emacs bindings depending on context. Perhaps I do it wrong and end up being less in vi-mode due to this, but I think it would be nice to be able to without not too much hassle be able to use emacs bindings when in input mode of vi mode.

@krader1961
Member

@terlar, The problem is the current behavior is not "friendly". I know that when I started using fish I was repeatedly surprised that accidentally pressing a control key did something perfectly reasonable for emacs mode but should have been a no-op for vi mode. I've seem comments from others to the same effect. Being able to have a hybrid of the two modes should be something an experienced (i.e., power) user can easily implement for themselves but should not be the default behavior.

In an offline conversation I had with @faho he suggested

function my_bindings
    fish_default_key_bindings default
    fish_vi_key_bindings
end
set fish_key_bindings my_bindings

I've tested that and it appears to implement the hybrid bindings you want. It just needs to be added to the documentation before this change is merged.

@faho
Member
faho commented Jun 5, 2016

It just needs to be added to the documentation before this change is merged.

Not quite. That function will work once, but it will result in fish_key_bindings being set to fish_vi_key_bindings - this is because our binding functions will set the variable if necessary. The exception being the default bindings, which allow for a mode argument to allow for inheritance.

I need to figure out how to do that and also investigate the test failures. I've written some docs and rebased this, so I could force-push now, but I'm not sure how helpful that is right now.

@faho
Member
faho commented Jun 7, 2016

Okay,, I've managed to fix the tests. It turns out I had missed three rather important bindings (self-insert, cancel and execute) so fish was pretty much non-functional - no way to enter anything, no way to switch modes and no way to execute.

@krader1961: I'm assuming self-insert and execute should only be bound in insert-mode and cancel (the escape thing) should be bound always?

@krader1961
Member

The self-insert binding only makes sense for insert mode. The execute binding makes sense for insert, default and visual modes. What's bound to a bare \e is more complicated in the vi bindings because some of the vi submodes (e.g., visual) should do something other than cancel.

@floam floam added the RFC label Jul 5, 2016
@krader1961
Member

Ping? This is now two months old. Anything we can do to help, @faho?

@faho
Member
faho commented Jul 25, 2016

Ping? This is now two months old. Anything we can do to help, @faho?

This mainly needs to be rebased on top of 7365b6b. I'll try to get this done in the next couple of days, but I'm doing #3219 first. Also, if you'd like to do a last check that the choice of bindings is good that'd be swell.

@faho
Member
faho commented Aug 3, 2016

Okay, I've rebased this now (which required a bit of work). See my "shared-bindings-rebase" branch. The only intended change there was to move the clipboard bindins (\cx and \cv)) to the shared bindings.

Now to review the changes again. There's none in default mode, but a few in vi-mode. Here's a sorted diff without context (which seems like a good way to review this):

--- viold   2016-08-03 15:14:17.968020584 +0200
+++ vinew   2016-08-03 15:13:45.369032220 +0200
-bind \b backward-delete-char
-bind \ca beginning-of-line
-bind \cb backward-char
-bind \ce end-of-line
-bind \cf forward-char
-bind \ck kill-line
-bind \cn down-or-search
-bind \cp up-or-search
-bind \ct transpose-chars
-bind \e\[1\;5C forward-word
-bind \e\[1\;5D backward-word
-bind \e\[1\~ beginning-of-line
-bind \e\[4\~ end-of-line
-bind \e� backward-kill-word
-bind \eb backward-word
-bind \e\< beginning-of-buffer
-bind \ec capitalize-word
-bind \ed kill-word
+bind \ed 'set -l cmd (commandline); if test -z "$cmd"; echo; dirh; commandline -f repaint; else; commandline -f kill-word; end'
-bind \e\> end-of-buffer
-bind \ef forward-word
-bind \e\[I 'begin;end'
-bind \e\[O 'begin;end'
-bind \et transpose-words
-bind \eu upcase-word
-bind -k end end-of-line
-bind -k home beginning-of-line
-bind -k npage end-of-history
-bind -k ppage beginning-of-history
-bind -M insert \b backward-delete-char
-bind -M insert \ca beginning-of-line
-bind -M insert \cb backward-char
-bind -M insert \ce end-of-line
-bind -M insert \cf forward-char
-bind -M insert \ck kill-line
-bind -M insert \cn down-or-search
-bind -M insert \cp up-or-search
-bind -M insert \ct transpose-chars
-bind -M insert \cx end-of-line
+bind -M insert \cx fish_clipboard_copy
-bind -M insert \e\[1\;5C forward-word
-bind -M insert \e\[1\;5D backward-word
-bind -M insert \e\[1\~ beginning-of-line
-bind -M insert \e\[3\;2\~ backward-delete-char
-bind -M insert \e\[4\~ end-of-line
-bind -M insert \e� backward-kill-word
-bind -M insert \eb backward-word
-bind -M insert \e\< beginning-of-buffer
-bind -M insert \ec capitalize-word
-bind -M insert \e\> end-of-buffer
-bind -M insert \e\[F end-of-line
-bind -M insert \ef forward-word
-bind -M insert \e\[H beginning-of-line
-bind -M insert \et transpose-words
-bind -M insert \eu upcase-word
-bind -M insert -k backspace backward-delete-char
-bind -M insert -k dc delete-char
-bind -M insert -k end end-of-line
-bind -M insert -k home beginning-of-line
-bind -M insert -k npage end-of-history
-bind -M insert -k ppage beginning-of-history
+bind -M visual \cd delete-or-exit
+bind -M visual \cl 'clear; commandline -f repaint'
+bind -M visual \cu backward-kill-line
+bind -M visual \cv fish_clipboard_paste
+bind -M visual \cw backward-kill-path-component
+bind -M visual \cx fish_clipboard_copy
+bind -M visual \cy yank
+bind -M visual \e\[1\;3A history-token-search-backward
+bind -M visual \e\[1\;3B history-token-search-forward
+bind -M visual \e\[1\;3C nextd-or-forward-word
+bind -M visual \e\[1\;3D prevd-or-backward-word
+bind -M visual \e\[1\;9A history-token-search-backward
+bind -M visual \e\[1\;9B history-token-search-forward
+bind -M visual \e\[1\;9C nextd-or-forward-word
+bind -M visual \e\[1\;9D prevd-or-backward-word
+bind -M visual \e\[3A history-token-search-backward
+bind -M visual \e\[3B history-token-search-forward
+bind -M visual \e\[3C nextd-or-forward-word
+bind -M visual \e\[3D prevd-or-backward-word
+bind -M visual \e\[A up-or-search
+bind -M visual \e\[B down-or-search
+bind -M visual \ed 'set -l cmd (commandline); if test -z "$cmd"; echo; dirh; commandline -f repaint; else; commandline -f kill-word; end'
+bind -M visual \e\e\[A history-token-search-backward
+bind -M visual \e\e\[B history-token-search-forward
+bind -M visual \e\e\[C nextd-or-forward-word
+bind -M visual \e\e\[D prevd-or-backward-word
+bind -M visual \e\eOA history-token-search-backward
+bind -M visual \e\eOB history-token-search-forward
+bind -M visual \e\eOC nextd-or-forward-word
+bind -M visual \e\eOD prevd-or-backward-word
+bind -M visual \eh __fish_man_page
+bind -M visual \e. history-token-search-backward
+bind -M visual \el __fish_list_current_token
+bind -M visual \e\n commandline\ -i\ \\n
+bind -M visual \eO3A history-token-search-backward
+bind -M visual \eO3B history-token-search-forward
+bind -M visual \eO3C nextd-or-forward-word
+bind -M visual \eO3D prevd-or-backward-word
+bind -M visual \ep __fish_paginate
+bind -M visual \e\r commandline\ -i\ \\n
+bind -M visual \ew 'set tok (commandline -pt); if test $tok[1]; echo; whatis $tok[1]; commandline -f repaint; end'
+bind -M visual \ey yank-pop
+bind -M visual -k btab complete-and-search
+bind -M visual -k down down-or-search
+bind -M visual -k f1 __fish_man_page
+bind -M visual -k up up-or-search
+bind -M visual \t complete
@krader1961
Member

This has gotten so complex that I can't provide a definitive LGTM. Nonetheless it does LGTM. I could find only a couple of possible errors. For example, the equivalent of bind \eb backward-word works in Vim. Yet bind \ef forward-word does not. So in theory the former binding should be retained. While I don't know what to make of that inconsistency in Vi behavior I don't think we should be emulating it. So I am okay with this change.

@faho
Member
faho commented Aug 4, 2016

I've now merged this as 93b9e74. Let's just try it out.

@faho faho closed this Aug 4, 2016
@faho faho added this to the next-2.x milestone Aug 4, 2016
@terlar
Contributor
terlar commented Sep 1, 2016

This bit me during the last upgrade since I relied on the combined behavior, I think I have got it to work now, by doing:

function my_key_bindings
    fish_vi_key_bindings insert
    fish_default_key_bindings -M insert
end

set -U fish_key_bindings my_key_bindings

It seems I had to run the default keybindings after the vi method otherwise it didn't work, also since this command will set the method if running without arguments I have to pass the insert argument.

Another caveat is that the default fish mode prompt queries for fish_key_bindings = fish_vi_key_bindings so I had to replace that also the same reason why I have to give arguments to the other methods as well, since they query for fish_key_bindings = fish_default_key_bindings respectively fish_vi_key_bindings, otherwise it will set the mode. So having a mode wrapper like this confuses these functions.

Was this documented somewhere? Sorry for not keeping up with the latest, have been busy.

@krader1961 krader1961 modified the milestone: fish 2.4.0, next-2.x Sep 3, 2016
@faho
Member
faho commented Sep 3, 2016

It seems I had to run the default keybindings after the vi method otherwise it didn't work,
[...]
Another caveat is that the default fish mode prompt queries for fish_key_bindings = fish_vi_key_bindings so I had to replace that also

@terlar: 635f9046eb6db0663751802e9bb1ab1ad5bcf236 should fix that.

Was this documented somewhere? Sorry for not keeping up with the latest, have been busy.

After that commit, it is.

since this command will set the method if running without arguments I have to pass the insert argument.

That is still needed.

The issue is that the binding functions should reset all your bindings by default, leaving you with a clean set (of $mode + user). So you need to somehow specify that you don't want that.

There might be some better way to do it, but this should work for the time being.

@terlar
Contributor
terlar commented Sep 9, 2016 edited

@faho I didn't find that commit specifically but I saw you had some other commits there and I checked out the latest commit from master and could still not get it to work following the instructions.

So doing as instructed in the documentation. E.g.

function hybrid_bindings --description "Vi-style bindings that inherit emacs-style bindings in all modes"
    for mode in default insert visual
        fish_default_key_bindings -M $mode
    end
    fish_vi_key_bindings --no-erase
end
set -g fish_key_bindings hybrid_bindings

I don't get the default key bindings, the ones I tried out for example C-a and C-e in insert mode does nothing. With my earlier mentioned approach it still works. The interesting thing is that the vi mode doesn't override these specific key-bindings, so if --no-erase worked I guess it should have been okay.

Now also my previous approach doesn't work, so I don't have a work-around to combine the default key-bindings with vi-mode.

@michael-younkin

Hey, I just installed the new fish shell release today, and a bunch of the key bindings I used to use no longer work. I'm more of a "casual" user (ie. not setting up my own keybindings, etc.) so I can't tell for sure, but it looks like this PR is what changed it?

--insert the usual confusion and anger about people making breaking changes here--

Does anyone know how to get back to the old behavior? Really if I could just have Ctrl-f back that would be great.

@jwkvam
jwkvam commented Nov 8, 2016

Ran into same thing. Not sure if this is the right way, but this got me the key binds I was sorely missing.

function custom_key_binds
    fish_vi_key_bindings

    bind -M insert \ca beginning-of-line
    bind -M insert \ce end-of-line
    bind -M insert \cf accept-autosuggestion
end
set -g fish_key_bindings custom_key_binds
@floam
Member
floam commented Nov 8, 2016 edited

I too, find vi-mode harder to use after these changes. Particularly, for whatever reason, I really wanted \ca and \ce to work.

@krader1961
Member

@michael-younkin and @jwkvam: The 2.4.0 documentation hasn't yet been pushed to fishshell.com. When it is what you'll see is a section titled "Command line editor" which says to do

function hybrid_bindings --description "Vi-style bindings that inherit emacs-style bindings in all modes"
    for mode in default insert visual
        fish_default_key_bindings -M $mode
    end
    fish_vi_key_bindings --no-erase
end
set -g fish_key_bindings hybrid_bindings

If you want just a handful, rather than all, of the emacs (default) mode bindings then the proper solution is to create a fish_user_key_bindings function that has just the bindings you want in addition to those provided by fish_vi_key_bindings or fish_default_key_bindings. For example, this is mine:

function fish_user_key_bindings
    bind \ee edit_command_buffer
    bind \ev edit_command_buffer
    bind \e# toggle-comment-cmd-buffer
    bind \e3 toggle-comment-cmd-buffer
    # [meta-right] to accept the auto-suggestion one word at a time
    bind \e\e\[C forward-bigword forward-word
    bind \cf forward-bigword forward-word
end
@michael-younkin

@krader1961 thanks for the response!

I'm not a contributor, so idk whether not you're open to suggestions on how to improve the release process, but, if you are, then here are a few notes:

  • Since one of the main reasons people pick one shell over another is efficiency, so IMO changing something like the shell's keyboard shortcuts is a major breaking change. Actually, I really don't expect projects to ever change their keyboard shortcuts. The overhead for learning new keyboard shortcuts is very high, after all.
  • As an end-user, I'd much prefer for highly visible changes like this to appear in major version number update releases since that's how most other projects work these days (in other words, I would expect this change when going from fish 2.X -> 3.X, not 2.3 -> 2.4)
  • I don't know whether or not it was intended that the docs would be out of sync with the actual released shell, but again as an end user this is very confusing. I didn't go out of my way to get the new version, I just did brew update and brew upgrade and it installed, and I expect other people are having similar experiences. So I sort of expect the docs to be up to date if the latest version is already in package repositories.

I know you're probably working on this project in your spare time, and it's free, and I appreciate that! But if you guys make many more breaking changes like this then I'll probably switch to a different shell because I just don't have time to relearn things and tweak my config files all the time.

@sagebind sagebind referenced this pull request in oh-my-fish/oh-my-fish Nov 15, 2016
Closed

Ctrl-F gets unmapped with fish 2.4 #441

@dead10ck

I couldn't agree more with @michael-younkin , changing peoples' key bindings is a major breaking change, not a point release. Not only that, but it seems like it was a pretty arbitrary decision. This is what loses users.

@krader1961 krader1961 referenced this pull request Nov 15, 2016
Open

fish_vi_mode $ command issues with autocomplete #3299

2 of 2 tasks complete
@krader1961
Member

The suggested hybrid_bindings function approach can't work at the moment due to the fish_vi_key_bindings doing set fish_key_bindings fish_vi_key_bindings. Which causes that function to be run a second time without the --no-erase flag. This definitely needs to be corrected. But rather than continue increasing the length of this issue I'm going to open a new issue; especially since it is now too late to change the 2.4.0 behavior. The time to do that was in the months leading up to the release when the change was already merged. Sadly no one complained which suggests no one building from git head or PPA nightly builds is using vi mode.

@krader1961 krader1961 added a commit to krader1961/fish-shell that referenced this pull request Nov 15, 2016
@krader1961 krader1961 make it easy to get the legacy hybrid key bindings
The changes related to issue #3068 removed most of the emacs bindings
from vi mode. However, since fish 2.4.0 was released several people have
pointed out that the directions for reinstating the legacy hybrid key
bindings don't work. This change fixes that and makes it easier to use
the legacy hybrid bindings.

Fixes #3556
e75b598
@krader1961 krader1961 added a commit that referenced this pull request Nov 17, 2016
@krader1961 krader1961 make it easy to get the legacy hybrid key bindings
The changes related to issue #3068 removed most of the emacs bindings
from vi mode. However, since fish 2.4.0 was released several people have
pointed out that the directions for reinstating the legacy hybrid key
bindings don't work. This change fixes that and makes it easier to use
the legacy hybrid bindings.

Fixes #3556
e8a31a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment