-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support alt+enter key signal by default #2718
Conversation
@jerch If this implementation is correct, we need add the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thats almost there 👍. I think we should change a few things though:
- rename the new flag to something like
altEscMode
- extend the flag evaluation to other keys as well
macOptionIsMeta
handling- needs a test for flag on/off
@Tyriar How does macOptionIsMeta
play into this? Will it always treat Option as Alt if set or just for some key combinations? What does Alt in that case on macs?
src/common/input/Keyboard.ts
Outdated
if (altEnterMode && modifiers === 2) { | ||
result.key = C0.ESC + C0.CR; | ||
} | ||
else { | ||
result.key = C0.CR; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not tested xterm's behavior, but I think DECSET 1039 is meant to switch the leading ESC on/off for any key? Thus we would have to extend the flag to other keys as well.
alt = option on macs, I guess to answer the question we'd be best to check other terminals that support this.
This feature has always confused me a little, we could really do with a solid use case of where this is actually used so we can study the behavior of other terminals. #2364 mentions it's used in "some REPLs". |
@Tyriar @kumaran-14 Yes we should try to get a hold of the uses, maybe start with a simple script to check what keys are affected by that setting in xterm. A second test maybe can eval what @NHDaly, @pfitzseb Do you have some insights for us, how |
See note about how Alt+Enter is used in Fish shell. |
Julia's REPL works the same way -- Alt-Enter inserts a newline regardless of whether a block is closed or not. |
@PerBothner, @pfitzseb Thx for the pointers. Yes xterm has by default this 8bit mode active for Alt+key and for Alt+Enter the fullscreen switch, at least in Ubuntu/Debian. Those key setup settings are somewhat complicated in xterm due to its additional 8bit key mode. @Tyriar, @kumaran-14 So my current suggestion is to go with a slimmer implementation:
Any other ideas? |
@jerch I just saw the recent comment. |
@kumaran-14 Yes I think we currently dont need the additional DECSET mode. It just complicates things. Sorry for the the forth and back, the needed bits are abit unclear and differ alot across emulators. |
We should check how iTerm in particular handles this case. |
@Tyriar @jerch I tried to find out how few terminals behave.
|
@kumaran-14 Thx for testing. Well, this is quite a naming mess. I think our
iTerm2 calls this "Esc+" mode, I have no clue what iTerm's "Meta" mode is about, so the name is abit misleading here. We are after "Esc+" in iTerm's terms. Also this seems to be in line with emacs docs and their recommendations regarding keyboard setup (https://www.emacswiki.org/emacs/MetaKeyProblems#toc15). But now the global Basically we should follow here iTerm's behavior:
What is still unclear to me - is there any other key active on mac keyboards as Meta key, if @Tyriar Plz correct my assumptions/ideas above, the whole |
Sorry about super slow response. @jerch making |
@Tyriar if it is so, i think the changes in the pr now should be right. |
src/common/services/CoreService.ts
Outdated
@@ -9,6 +9,7 @@ import { IDecPrivateModes, ICharset } from 'common/Types'; | |||
import { clone } from 'common/Clone'; | |||
|
|||
const DEFAULT_DEC_PRIVATE_MODES: IDecPrivateModes = Object.freeze({ | |||
altEscMode: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kumaran-14 yep, except for this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tyriar updated.
Good to merge when @jerch signs off. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go from my side 👍
Thx for bearing with us @kumaran-14 and the bumpy ride. Opened another issue to track the Meta/option key handling for Mac (#2831).
Fixes #2498.