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

Additional alt and control sequences #1231

Merged
merged 14 commits into from
Feb 21, 2018
Merged

Conversation

saamalik
Copy link
Contributor

@saamalik saamalik commented Jan 22, 2018

  • Add punctuation alt sequences (both shift/non-shift versions)
  • Add support for ctrl+alt sequences

Follow up to #1225.
Fixes #1267

Sample bash key sequences to try:

  • ctrl+alt+y: yank first argument from previous command
  • alt+.: yank last argument previous command

* Add punctuation alt sequences
* Add support for ctrl+alt sequences
src/Terminal.ts Outdated
} else if (ev.keyCode >= 48 && ev.keyCode <= 57) {
result.key = C0.ESC + (ev.keyCode - 48);
} else {
const t = (p,s) => !ev.shiftKey ? p : s;
switch (ev.keyCode) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW another possible implementation is having a map structure, e.g:

{
   186: [';', ':'],
   187: ['=', '+'],
   188: [',', '<'],
 .....
}

And then essentially just do a lookup in map to get actual code.

Let me know if you guys prefer the map instead and I can re-implement.

src/Terminal.ts Outdated
} else if (ev.keyCode >= 48 && ev.keyCode <= 57) {
result.key = C0.ESC + (ev.keyCode - 48);
} else {
const t = (p, s) => !ev.shiftKey ? p : s;
switch (ev.keyCode) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW another possible implementation is having a map structure, e.g:

{
   186: [';', ':'],
   187: ['=', '+'],
   188: [',', '<'],
 .....
}

And then essentially just do a lookup in map to get actual code.

Let me know if you guys prefer the map instead and I can re-implement.

@saamalik
Copy link
Contributor Author

saamalik commented Jan 23, 2018

@Tyriar what are your thoughts on this PR? I don't mind dropping the whole branch and starting afresh if you give me some high-level guidance/pointers.

Ideally we could solve the following two use-cases:

  • ctrl + alt + [a-z]: these are useful in bash (ctrl-alt-y) and also in VIM
  • alt + {special-char}: useful in bash (alt+.) and again in ViM.

For the alt+{special-char} implementation, I essentially mimicked what ITerm2 emits. If you don't like the giant switch (or the proposed map) implementation maybe we could reduce the list to just the known character sets; or maybe find ranges for which we could lookup key via basic artithmetic, e.g: ev.keyCode - 144--though the shift characters would need to be dropped.

@saamalik saamalik closed this Jan 23, 2018
@saamalik saamalik reopened this Jan 23, 2018
@saamalik saamalik closed this Jan 23, 2018
@saamalik saamalik reopened this Jan 23, 2018
@Tyriar
Copy link
Member

Tyriar commented Jan 25, 2018

Sorry about not getting to this, a few things:

  • 👍 to the map (which should let you remove the t function)
  • Does ctrl+alt+[a-z] do anything at the moment?
  • I'm assuming alt+[special char] only works when macOptionIsMeta is true?
  • What's up with the tilde key? Can we do nothing there instead?
  • How does iTerm2/Terminal.app behave with these new keybindings?

@saamalik
Copy link
Contributor Author

@Tyriar -- cool, will re-implement using the map and provide answers to your questions in a bit.

@saamalik
Copy link
Contributor Author

saamalik commented Jan 27, 2018

Does ctrl+alt+[a-z] do anything at the moment?

Tried with both mcOptionIsMeta on and off, but the ctrl+alt+[a-z] sequences are completely ignored. Even with t.setOption('debug', true) there is nothing printed to the console.

alt+[special char] only works when macOptionIsMeta is true?

Yup, on Mac OSX both the alt+[special char] and alt+shift+[special char] only works when macOptionIsMeta; otherwise completely ignored.

Same with alt+[digit] (enabled on Win/Linux or when macOptionIsMeta). However, alt+shift+[digit] also outputs the same sequence as alt+[digit], which is incorrect. As part of the PR I'll also fix the alt+shift+[digit] to output the same sequence as iTerm/Terminal.app. With this fix, on bash pressing alt+shift+* will expand all matches on command-line. It'll bring the same sequences which are supported on iTerm/Terminal.app.

What's up with the tilde key? Can we do nothing there instead?

This tool shows the tilde as a DEADKEY; however iTerm2 outputs appropriate distinct sequences ^[` and ^[~ for alt+` and alt+shift+~, respectively. PR will add support for both sequences and be consistent with other keys.

How does iTerm2/Terminal.app behave with these new keybindings?

All of the new mappings are exactly mirroring the sequences emitted with iTerm2/Terminal.app. This was verified using the ctrl+v and entering the appropriate key sequence. There might be other keyboard sequences, but at least the ones captured in the PR will match iTerm2/Terminal.app and other terminals.

* Reimplemented reg + shift alt digits and characters using a map
* Add test cases for digits
@saamalik
Copy link
Contributor Author

saamalik commented Jan 27, 2018

@Tyriar this PR is ready for review. Keycode -> key mapping has been re-implemented using a map (actually an Object, but let me know if you prefer an actual Map even though syntax will look ugly with double, and triple square-brackets).

As I mentioned in the previous comment, all key -> key sequences introduced in this PR match exactly what iTerm2 emits. On bash, here are some of the mappings you can try (enable macOptionIsMeta if on OSX):

$ touch p1 p2 p3
$ echo a b c d
$ <ctrl+alt+y> # a
$ <alt+.> # d
$ ls p<alt+shift+/> #list glob-matching files
$ ls p<alt+shift+8> # insert glob-matching files as arguments
$ echo bad command do not run <alt+shift+3> # comment out command

Let me know if you have any questions or would like further changes. Thanks!

@saamalik
Copy link
Contributor Author

@Tyriar friendly ping. What are your thoughts on the PR?

@Tyriar
Copy link
Member

Tyriar commented Jan 31, 2018

@saamalik I like it, just might take some time to give it a full review/test. I don't expect many more changes needed to get it merged. I'm hoping to get it in for 3.1.0 😃

@saamalik
Copy link
Contributor Author

saamalik commented Feb 1, 2018

@Tyriar btw let me know if you'd like me to squash all commits on top of origin/master.

@Tyriar
Copy link
Member

Tyriar commented Feb 2, 2018

@saamalik should be fine without that

@saamalik
Copy link
Contributor Author

@Tyriar just another friendly ping to see if we could please get this merged-in?

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delay, super busy working on terminal splitting for VS Code 😅

This looks good and also seems to work as advertised so I'll merge. Thanks a bunch for the PR!

@Tyriar Tyriar merged commit 62cf093 into xtermjs:master Feb 21, 2018
@Tyriar Tyriar added this to the 3.2.0 milestone Feb 21, 2018
@Tyriar Tyriar self-assigned this Feb 21, 2018
@saamalik
Copy link
Contributor Author

@Tyriar wohoo -- thanks!

@saamalik saamalik deleted the ctrl-alt-sequences branch March 11, 2018 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants