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

Fixes issue #371 (As much as is possible) #373

Merged
merged 2 commits into from
Jan 28, 2020
Merged

Fixes issue #371 (As much as is possible) #373

merged 2 commits into from
Jan 28, 2020

Conversation

sharnoff
Copy link
Contributor

@sharnoff sharnoff commented Jan 25, 2020

The key codes for newline ('\n') and Ctrl+J are both 0xA - this is given by the terminal, and there's not much we can do about that particular fact. As it turns out, terminals operating in "canonical mode" - i.e. not raw mode - will convert any carriage returns ('\r') into newline characters - even though the user doesn't directly input newlines.

When we enter raw mode, one of the things that gets disabled is this automatic conversion. Now, when we get newlines, there's no ambiguity; it must be Ctrl+J. Terminal apps (like vim) are then able to handle Ctrl+J separately because they make that distinction once in raw mode.

More info: https://viewsourcecode.org/snaptoken/kilo/02.enteringRawMode.html#fix-ctrl-m; ICRNL bitflag in termios

This commit (roughly speaking) changes changes event parsing of newlines to only be enabled while the terminal isn't in raw mode. Outside of raw mode, it doesn't seem possible to handle Enter and Ctrl+J separately without using some of the same features of termios.h that are used to enable raw mode.

Copy link
Member

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and for looking into this! I will have to run this branch. The input layer is not very useful when raw mode is not enabled therefore wouldn't it be pointless to check for this?

// newlines as input is because the terminal converts \r into \n for us. When we
// enter raw mode, we disable that, so \n no longer has any meaning - it's better to
// use Ctrl+J. Waiting to handle it here means it gets picked up later
b'\n' if !crate::terminal::sys::is_raw_mode_enabled() => Ok(Some(InternalEvent::Event(
Copy link
Member

Choose a reason for hiding this comment

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

Though the input layer isn't very useful if the raw mode is not enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I get what you're saying - does parse_event only run while in raw mode?

@sharnoff
Copy link
Contributor Author

I will have to run this branch. The input layer is not very useful when raw mode is not enabled therefore wouldn't it be pointless to check for this?

I don't know when this is used, but I built a version of a personal project with this change, and it fixed the issue (I'd previously noticed it there). This issue might also extend to Windows, depending on how the terminal is configured, but I haven't looked into that

@TimonPost
Copy link
Member

The windows terminal does not use those codes. It is UNIX only that parses those keys. Secondly, raw mode will make input bytes be written to the terminal buffer directly. And will prevent the bytes from being displayed by the user and allow the terminal to handle the bytes. This is what we do, without raw mode we are not able to fetch the input bytes.

@TimonPost
Copy link
Member

It can be realistic that CTRL + J is not possible to handle. There are limitations to some keys we can support. I will give it a try in the evening, and check with other libraries if they support this. So I will report back in some hours.

@TimonPost
Copy link
Member

image

CONTROL + J is working for me on Linux. It seems to work!

Copy link
Member

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

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

Thanks, I tested it and it worked. I will do a release this evening because broot reported this issue originally and is waiting for a fix.

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.

None yet

2 participants