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

changed escape sequence for alt-arrow to work on bash on os x #417

Merged
merged 2 commits into from
Dec 19, 2016

Conversation

BobReid
Copy link

@BobReid BobReid commented Dec 16, 2016

These are the escape sequences that make alt-arrow work on os x bash.

I have just started poking around the code base and am not super familiar with it. Any guidance on how to make this cross platform / cross shell would be appreciated.

@BobReid
Copy link
Author

BobReid commented Dec 16, 2016

Fix for #225

@@ -2501,7 +2501,7 @@ Terminal.prototype.evaluateKeyEscapeSequence = function(ev) {
// HACK: Make Alt + left-arrow behave like Ctrl + left-arrow: move one word backwards
// http://unix.stackexchange.com/a/108106
if (result.key == '\x1b[1;3D') {
result.key = '\x1b[1;5D';
result.key = '\x1bb';
Copy link
Member

Choose a reason for hiding this comment

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

You can wrap this in a platform check by using if (browser.isMac). This change also fails a test which will need its own if statement.

@parisk do you expect this to break anything? I'm concerned about how #225 (comment) could not repro and other it may not behave in other shells like zsh.

Copy link
Author

Choose a reason for hiding this comment

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

I just tested with zsh.
I actually reprod the issue with zsh as well with the old escape codes. The new escape codes actually works with both bash and zsh on my mac which actually caught me by surprise.

I will wrap the changes in the isMac check and update the tests

Copy link
Author

Choose a reason for hiding this comment

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

@Tyriar as for the test I could tackle it in two ways.

  1. Add a simple if check to the tests to check the platform. downside being the test is now dependent on the platform it runs on
  2. Add additional tests and mock out browser check so all cases are tested regardless of platform.

What is preferable to you?

Copy link
Author

Choose a reason for hiding this comment

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

I see that there are already some test that mock out the browser check so I will go with that for now.

@BobReid
Copy link
Author

BobReid commented Dec 16, 2016

Updated the PR.
Added the browser check and fixed tests

@parisk
Copy link
Contributor

parisk commented Dec 19, 2016

Thanks @BobReid!

@parisk parisk merged commit 593a4ec into xtermjs:master Dec 19, 2016
@BobReid BobReid deleted the os_x_bash_arrow_fix branch December 19, 2016 15:23
@aliatsis
Copy link

this still doesn't fix alt+Backspace to delete words on mac.
should that be a separate issue?

@BobReid
Copy link
Author

BobReid commented Dec 19, 2016

alt+Backspace does not have a special function on my mac (bash & zsh on stock terminal and iTerm)

@parisk
Copy link
Contributor

parisk commented Dec 20, 2016

On my tests Alt + Backspace just deletes a character in bash.

@aliatsis what macOS version are you running currently? Also are you using the stock terminal with bash?

@aliatsis
Copy link

@parisk I'm on Sierra (10.12).. it was the same in Yosemite before I upgraded.
Yes, I am using the stock terminal with bash

Tyriar added a commit to microsoft/vscode that referenced this pull request Dec 28, 2016
The terminal should be a lot faster at processing output now thanks to
xtermjs/xterm.js#422

Also of note: xtermjs/xterm.js#417: changed escape sequence for alt-arrow to work on bash on os x
@Tyriar Tyriar modified the milestone: 2.3.0 Feb 2, 2017
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.

4 participants