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

Ditch smkx in favor of custom bindkey extension #5113

Closed
wants to merge 1 commit into from
Closed

Ditch smkx in favor of custom bindkey extension #5113

wants to merge 1 commit into from

Conversation

apjanke
Copy link
Contributor

@apjanke apjanke commented May 24, 2016

Motivation

More than 2 years ago, Oh My Zsh merged the Pull Request #1355, which introduced a fix to be able to use terminfo sequences to specify key codes to bindkey, with the intention to make it standard for everyone and not having to change bindkey sequences all the time.

That experiment failed, and it's been obviously clear for quite a while. Since then lots of issues have popped up where certain combinations of keys didn't work (see list of Fixes below), and the culprit of that is that terminfo databases are not maintained all that well and there is not consistent support among Terminal Emulators.

This is why we're choosing to drop that experiment, and use a custom wrapper around bindkey named omz_bindkey were you'll be able to choose whichever modifier keys you want ─ CTRL, ALT, SHIFT, META ─ instead of having to use an obscure character sequence like '^[[OH'.

Here's where you come into play: since this is a big change, we need a broad number of users to test in their respective configurations, with as many Terminal Emulators as we can, before we can safely merge this PR knowing that there won't be many users affected.

Thanks for taking the time to read this; in this GitHub guide you'll find how you can check out this PR locally. After that, restart your terminal or run exec zsh to reload Oh My Zsh.

Please report back if there are any keyboard shortcuts broken in your system with the following information:

  • Zsh version: run echo $ZSH_VERSION.
  • Terminal Emulator and its version: e.g. iTerm2, Terminal.app, Gnome Terminal, Guake, ...
  • TERM variable: run echo $TERM.
  • The broken keyboard shortcut and its character sequence which you can get following this tutorial I've made.
  • Your current bindkey configuration: run bindkey and post its output.
  • Anything else that you think is relevant.

That is all. Thanks again for participating in this! ─ @mcornella


Description

I think it's time to abandon smkx/rmkx and stick with "local" keypad mode, even if it requires us to hardcode a few character sequences instead of using terminfo for everything. This PR does so, adding an omz_bindkey function that allows us to still use terminfo capability codes in key bindings, and supports cursor keys with modifiers. This should fix a whole slew of these recurring cursor-key terminal issues.

Fixes #4916 – Num Pad can't input
Fixes #4872 – Home and End not working in MobaXterm
Fixes #4784 – Home and End not working in PhpStorm terminal in Mac OS/X
Fixes #3757 – Home/End broken in PuTTY
Fixes #3733 – Home/End broken in CentOS
Fixes #3495 – Home/End broken in CentOS
Fixes #3061 – Home/End broken in Ubuntu
Probably fixes #2750 – Cursor key bindings broken in urxvt on openSUSE – or at least makes it moot
Fixes #2735 – history-substring-search key bindings broken after terminfo changeover
Fixes #2698 – Home/End broken in gnome-terminal
Fixes #2654 – Num keypad broken on OS X
Fixes #2369 – Change to option-arrow behavior on OS X
Fixes #5149 – Plugin 'safe-paste' messes with 'history-substring-search'
Fixes #5228 – Application mode not working on xterm.js
Fixes #5237 - Safe-paste plugin breaks Home/End
Avoids xtermjs/xterm.js#151 - zsh / oh-my-zsh - no scroll
...and all those other issues about Home/End or the numeric keypad.

Discussion

Switching to use smkx/rmkx application mode and terminfo for portability was a good idea: you're supposed to use portability libraries for stuff like this. But I think it's time to call it a failed experiment. Not because the implementation in OMZ was bad, but because terminfo and the various terminal programs out there don't have sufficient support for it.

I dug pretty deep in to this issue, and I think that the smkx/rmkx+terminfo solution cannot be made to work for everyone. And you'll continue to see issues reported for it. But local mode with a few hardcoded sequences can work.

The smkx/rmkx approach is defeated by a few factors:

  • Terminal emulators vary in their application mode behavior, and don't necessarily match xterm, which is what most people use for their $TERM, even if they're really running a different program
  • terminfo conflates the mode setting for cursor keypads and the numeric keypads: [sr]mkx changes both, but we only want the cursor keypads changed
  • the program-specific terminfo database entries for PuTTY, Terminal.app, and other programs may not be installed on a given system, or may be incomplete or out of date. We don't want to require custom installation of terminfo entries if we can possibly avoid it.

Using smkx/rmkx is also complicated because:

  • Most terminal-related code snippets on forums, StackOverflow, and the rest of the internet are for local mode, so they won't work in OMZ if we use [sr]mkx
  • bash only supports local mode, so bash cursor code can't be used under [sr]mkx
  • Most people don't know about [sr]mkx, so educating people about it becomes part of every bug report for keypad problems
  • Using [sr]mkx requires using the zle-line-init and zle-line-finish hooks, which conflicts with anything else that wants to use them (including some OMZ plugins).

zle-line-init/finish hooks

As part of this PR's changes, Oh My Zsh completely takes over the zle-line-init and zle-line-finish hooks, replacing them with functions that provide support for multiple hook functions to be registered. This is done for two independent reasons. Primarily, it's to clobber and remove any zle-line-init/finish hooks installed by zsh configuration files supplied by the system. Some Linux distributions define hooks which do the [sr]mkx thing, and we need to make sure that doesn't happen. Secondly, it allows for multiple OMZ plugins to install line-init/finish hooks without conflicting with each other.

As part of this, we're essentially having OMZ declare zle-line-init/finish to be completely its territory. I think this is okay.

Implementation

The terminfo-based key binding code was a lot easier to read, because it used mnemonic terminfo capability codes instead of literal terminal control sequences. So this PR adds an omz_bindkey extension on top of bindkey that allows you to still use those codes. (It also adds logging support, which would make diagnosing key binding issues a lot easier, because you could see which plugins or user code is doing which binding.) I think this makes the key binding code a lot more readable, both as it is, and in the diffs for future PRs.

The omz_bindkey layer keeps a supplemental little database of character sequences based on xterm's local mode behavior (which is not in terminfo), and uses those for cursor keys. It also includes a couple hacks for specific known misbehaving or variant terminal programs. And it has support for binding modifier-key (alt/ctrl/shift) cursor keys, which terminfo doesn't itself provide.

Future considerations

If you like this approach and merge it, I'll follow it up with a PR for multiple keymap support in the key bindings, which will take care of the issues caused by flipping between the emacs and vi keymaps during the OMZ startup sequence.

@mcornella mcornella added Status: testers needed Pull Requests that are waiting for testers to merge Area: core Issue or PR related to core parts of the project Topic: bindkey Pull Request or issue regarding keyboard shortcuts labels May 29, 2016
@mcornella
Copy link
Member

Thanks for this Andrew, hopefully I'll have it checked out in less than a week. I agree on the failed experiment sentiment, I think this is the only way forward.

@mcornella mcornella mentioned this pull request May 29, 2016
@mcornella
Copy link
Member

The history-substring-search plugin stops working after this PR. I think we have to also the terminfo-style key bindings.

Terminfo kcuu1 and kcud1 are "^[OA" and "^[OB" respectively, while using CTRL+V for UP / DOWN arrow I get ^[[A and ^[[B respectively.

What I don't get is why it works for you, perhaps you have overwritten bindkeys?

My infocmp:

#   Reconstructed via infocmp from file: /lib/terminfo/x/xterm-256color
xterm-256color|xterm with 256 colors,
    am, bce, ccc, km, mc5i, mir, msgr, npc, xenl,
    colors#256, cols#80, it#8, lines#24, pairs#32767,
    acsc=``aaffggiijjkkllmmnnooppqqrrssttuuvvwwxxyyzz{{||}}~~,
    bel=^G, blink=\E[5m, bold=\E[1m, cbt=\E[Z, civis=\E[?25l,
    clear=\E[H\E[2J, cnorm=\E[?12l\E[?25h, cr=^M,
    csr=\E[%i%p1%d;%p2%dr, cub=\E[%p1%dD, cub1=^H,
    cud=\E[%p1%dB, cud1=^J, cuf=\E[%p1%dC, cuf1=\E[C,
    cup=\E[%i%p1%d;%p2%dH, cuu=\E[%p1%dA, cuu1=\E[A,
    cvvis=\E[?12;25h, dch=\E[%p1%dP, dch1=\E[P, dim=\E[2m,
    dl=\E[%p1%dM, dl1=\E[M, ech=\E[%p1%dX, ed=\E[J, el=\E[K,
    el1=\E[1K, flash=\E[?5h$<100/>\E[?5l, home=\E[H,
    hpa=\E[%i%p1%dG, ht=^I, hts=\EH, ich=\E[%p1%d@,
    il=\E[%p1%dL, il1=\E[L, ind=^J, indn=\E[%p1%dS,
    initc=\E]4;%p1%d;rgb\:%p2%{255}%*%{1000}%/%2.2X/%p3%{255}%*%{1000}%/%2.2X/%p4%{255}%*%{1000}%/%2.2X\E\\,
    invis=\E[8m, is2=\E[!p\E[?3;4l\E[4l\E>, kDC=\E[3;2~,
    kEND=\E[1;2F, kHOM=\E[1;2H, kIC=\E[2;2~, kLFT=\E[1;2D,
    kNXT=\E[6;2~, kPRV=\E[5;2~, kRIT=\E[1;2C, kb2=\EOE,
    kbs=\177, kcbt=\E[Z, kcub1=\EOD, kcud1=\EOB, kcuf1=\EOC,
    kcuu1=\EOA, kdch1=\E[3~, kend=\EOF, kent=\EOM, kf1=\EOP,
    kf10=\E[21~, kf11=\E[23~, kf12=\E[24~, kf13=\E[1;2P,
    kf14=\E[1;2Q, kf15=\E[1;2R, kf16=\E[1;2S, kf17=\E[15;2~,
    kf18=\E[17;2~, kf19=\E[18;2~, kf2=\EOQ, kf20=\E[19;2~,
    kf21=\E[20;2~, kf22=\E[21;2~, kf23=\E[23;2~,
    kf24=\E[24;2~, kf25=\E[1;5P, kf26=\E[1;5Q, kf27=\E[1;5R,
    kf28=\E[1;5S, kf29=\E[15;5~, kf3=\EOR, kf30=\E[17;5~,
    kf31=\E[18;5~, kf32=\E[19;5~, kf33=\E[20;5~,
    kf34=\E[21;5~, kf35=\E[23;5~, kf36=\E[24;5~,
    kf37=\E[1;6P, kf38=\E[1;6Q, kf39=\E[1;6R, kf4=\EOS,
    kf40=\E[1;6S, kf41=\E[15;6~, kf42=\E[17;6~,
    kf43=\E[18;6~, kf44=\E[19;6~, kf45=\E[20;6~,
    kf46=\E[21;6~, kf47=\E[23;6~, kf48=\E[24;6~,
    kf49=\E[1;3P, kf5=\E[15~, kf50=\E[1;3Q, kf51=\E[1;3R,
    kf52=\E[1;3S, kf53=\E[15;3~, kf54=\E[17;3~,
    kf55=\E[18;3~, kf56=\E[19;3~, kf57=\E[20;3~,
    kf58=\E[21;3~, kf59=\E[23;3~, kf6=\E[17~, kf60=\E[24;3~,
    kf61=\E[1;4P, kf62=\E[1;4Q, kf63=\E[1;4R, kf7=\E[18~,
    kf8=\E[19~, kf9=\E[20~, khome=\EOH, kich1=\E[2~,
    kind=\E[1;2B, kmous=\E[M, knp=\E[6~, kpp=\E[5~,
    kri=\E[1;2A, mc0=\E[i, mc4=\E[4i, mc5=\E[5i, meml=\El,
    memu=\Em, op=\E[39;49m, rc=\E8, rev=\E[7m, ri=\EM,
    rin=\E[%p1%dT, ritm=\E[23m, rmacs=\E(B, rmam=\E[?7l,
    rmcup=\E[?1049l, rmir=\E[4l, rmkx=\E[?1l\E>, rmso=\E[27m,
    rmul=\E[24m, rs1=\Ec, rs2=\E[!p\E[?3;4l\E[4l\E>, sc=\E7,
    setab=\E[%?%p1%{8}%<%t4%p1%d%e%p1%{16}%<%t10%p1%{8}%-%d%e48;5;%p1%d%;m,
    setaf=\E[%?%p1%{8}%<%t3%p1%d%e%p1%{16}%<%t9%p1%{8}%-%d%e38;5;%p1%d%;m,
    sgr=%?%p9%t\E(0%e\E(B%;\E[0%?%p6%t;1%;%?%p5%t;2%;%?%p2%t;4%;%?%p1%p3%|%t;7%;%?%p4%t;5%;%?%p7%t;8%;m,
    sgr0=\E(B\E[m, sitm=\E[3m, smacs=\E(0, smam=\E[?7h,
    smcup=\E[?1049h, smir=\E[4h, smkx=\E[?1h\E=, smso=\E[7m,
    smul=\E[4m, tbc=\E[3g, u6=\E[%i%d;%dR, u7=\E[6n,
    u8=\E[?1;2c, u9=\E[c, vpa=\E[%i%p1%dd,

@mcornella mcornella closed this Jun 3, 2016
@mcornella mcornella reopened this Jun 3, 2016
@apjanke
Copy link
Contributor Author

apjanke commented Jun 4, 2016

Ah, yes: that will need to be changed to use omz_bindkey instead of plain bindkey, and make sure omz_bindkey supports -t in conjunction with -M. I'll update this PR to do so.

I didn't notice it because I have some additional key bindings set in my ~/.zshrc and I forgot to test it against a plain ~/.zshrc too.

@apjanke
Copy link
Contributor Author

apjanke commented Jun 6, 2016

Okay, I've amended the PR to handle -M and have the history-substring-search and dircycle plugins use the new omz_bindkey mechanism.

@mcornella
Copy link
Member

The dircycle plugin stopped working because the omz_bindkey function uses shift-ctrl while the plugin uses ctrl-shift.

If only one of these should work, I think ctrl-shift is a much more common occurrence. Another option is to support both permutations, but then all other key permutations should follow the same rule.

My suggestion is to use ctrl-shift, ctrl-alt and ctrl-alt-shift. The meta-* permutations should also change in the same fashion.

@apjanke
Copy link
Contributor Author

apjanke commented Jun 17, 2016

Good catch.

I think it'll be more readable if we stick to a canonical ordering. (Alternately, the omz_bindkey function itself could do string splitting and re-order the inputs to a canonical ordering, but I'd rather not spend the time to implement that now.)

I've switched it to using your suggested canonical ordering. The dircycle plugin now works in my testing (with Xterm).

@mcornella
Copy link
Member

Ok, this appeared to fix #5228, which I've added to the list of fixes. I'll see if we can accelerate the testing of this PR so we can merge it right away.

@snussbaumer
Copy link

FYI I've switched to this PR for now, as this solves keybinding problems with the home/end/ctrl+left and ctrl+right keys in IntelliJ IDEA 2016.2.4 (on Ubuntu 16.04). I see no problem at this moment, but will log any issue here if I find one. Let me know if this is not the proper place for "testers".

@mcornella
Copy link
Member

@snussbaumer yes this is the proper place, thanks for commenting. Did you have to do anything extra after applying this PR, i.e. delete some bindkey commands from your zshrc file?

@snussbaumer
Copy link

I could remove the following bindkey :

bindkey '^[[H' beginning-of-line
bindkey '^[[F' end-of-line

but I still need :

bindkey '^[[5D' backward-word
bindkey '^[[5C' forward-word

Note : these backward-word (CTRL+LEFT) and forward-word (CTRL+RIGHT) bindings wouldn't work at all before applying the PR.

@snussbaumer
Copy link

snussbaumer commented Sep 19, 2016

Another note : in IntelliJ terminal showkey gives me the following bindings :

  • ^[[5D for CTRL+LEFT
  • ^[[5C for CTRL+RIGHT

Whereas when I'm in GNU terminal I get :

  • ^[[1;5D for CTRL+LEFT
  • ^[[1;5C for CTRL+RIGHT

Luckily my changes in zshrc seem to be ok in both environments ... but still this is quite confusing


ok I get it, it looks as if you can have mulitple bindings for the same command, that's why it's working :

bindkey | grep 'backward-word'
"^[B" backward-word
"^[[1;5D" backward-word
"^[[5D" backward-word
"^[b" backward-word

@apjanke
Copy link
Contributor Author

apjanke commented Sep 19, 2016

Good to hear.

That sounds like IntelliJ is behaving oddly for Ctrl-modified arrow keys. That ^[[5D means "five spaces left" instead of "one space left, modified by Ctrl". The GNU terminal behavior is more normal for terminal emulators. That would explain why you still need bindings for ^[[5D and ^[[5C; they're non-standard.

Someone should report this as a bug upstream to IntelliJ.

@mcornella
Copy link
Member

For the ^[[5D and ^[[5C you could also use the wrapper omz_bindkey introduced in this PR. Here's how you would use it:

omz_bindkey -c ctrl left backward-word
omz_bindkey -c ctrl right forward-word

@apjanke
Copy link
Contributor Author

apjanke commented Sep 19, 2016

For the ^[[5D and ^[[5C you could also use the wrapper omz_bindkey introduced in this PR. Here's how you would use it:

I don't think that will work. The omz_bindkey modified-cursor support is based on the standard xterm modifier-key control sequences. The IntelliJ terminal is deviating from that behavior, so the ctrl left and ctrl right bindings won't match it. terminfo doesn't have support for modified-cursor patterns, either. You'll need to continue to hardcode the sequences for IntelliJ to work around this.

I suspect this is a bug in the IntelliJ terminal plugin. It looks like it's naively inserting a 5 for "ctrl" into the ^[[D etc sequences without realizing it has to add an explicit 1; before it in the case of the cursor keys.

@mcornella
Copy link
Member

Oh right, my mistake, I meant the other two sequences.

@Daemoen
Copy link

Daemoen commented Oct 27, 2018

Curious why this is still open and unmerged? Is it because there are still more tests needed before feeling it's safe to merge? to this date, even using master, vi-mode is still broken in numerous cases, as is safe-paste.

@robbyrussell
Copy link
Member

@mcornella Do you think this is close to merge-able?

@HyP3r-
Copy link

HyP3r- commented May 7, 2019

Also from my perspective: why is this problem still not solved? For a terminal extension that is used by so many people (87,000 stars) it is embarrassing if simple things like crtl-left or the Numpad don't work. Please fix it! @robobenklein @mcornella @apjanke
If this is solved, probably my issue is also solved: #7484

@robbyrussell
Copy link
Member

We could really use some testers on this. Thanks in advance!

@HyP3r-
Copy link

HyP3r- commented Dec 29, 2019

I have merged this pull request into my fork and have not encountered any problems with it. On these systems and platforms:
Debian Jessie, Stretch and Buster and Ubuntu 18.04.3 LTS and 19.10 on the Local terminals as well as via SSH on Windows with PuTTy 0.63 and 0.73 and Remote Desktop Manager 2019.2.21.0 and on Linux with ssh in the terminal. As well as on cygwin 3.1.2 with the local terminal.

therealjumbo added a commit to therealjumbo/dotfiles that referenced this pull request Feb 18, 2020
The safe-paste plugin apparently breaks up-arrow / down-arrow fuzzy
search for reasons... see ohmyzsh/ohmyzsh#5113
once that is merged the problem should be fixed.
@larson-carter
Copy link
Member

@apjanke

Could you please upstream this, by getting rid of the conflicts? If so we would then be able to continue our reviews on it.

Thank You!

@ohmyzsh ohmyzsh bot added the Status: conflicts Pull Request that has conflicts with the master branch label May 6, 2020
@apjanke
Copy link
Contributor Author

apjanke commented May 6, 2020

Rebased and resolved conflicts; I think I got it right.

@larson-carter
Copy link
Member

@apjanke Thank you.

@mcornella Do we want to proceed with a PR review?

@mcornella mcornella removed the Status: conflicts Pull Request that has conflicts with the master branch label May 9, 2020
@pgimalac
Copy link
Contributor

pgimalac commented Jun 8, 2020

I've encountered a bug with this PR: when I type a command and go up to search in my history all commands starting with the first word appear (not necessarily the full command).
For example when I type "git pull" and up there are commands like "git push" or "git add" that can appear, while I would expect only commands like "git pull origin master".

@apjanke
Copy link
Contributor Author

apjanke commented Jun 16, 2020

Hmmmm... I can't reproduce your issue. When I check out my ~/.oh-my-zsh against this PR and do "git pull" and hit up arrow, it only grabs the items from my history that specifically start with "git pull".

git-pull-and-do-up-arrow-under-ditch-smkx

Maybe you've got some other autocompletion configuration stuff going on? Are you all rebased on master and stuff?

@pgimalac
Copy link
Contributor

@apjanke I tried again and it didn't happen anymore !
I guess I did something wrong last time...
Sorry for the false alarm.

@apjanke
Copy link
Contributor Author

apjanke commented Jun 16, 2020

Okay, good to hear!

@TheTechRobo
Copy link
Contributor

What is the status on this PR?

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