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

modify Keys and Events to detect Esc key presses #45

Merged
merged 3 commits into from
Oct 26, 2016

Conversation

iamcodemaker
Copy link
Contributor

The strategy used here is to read two bytes at a time, going on the
assumption that escape sequences will consist of multi byte reads and
solitary Esc key presses will consist of single byte reads.

Tests had to be modified to account for these new multi byte reads by
including dummy bytes when a single byte was previously expected.

Fixes ticki/termion#43

@ticki
Copy link
Contributor

ticki commented Aug 5, 2016

Interesting approach. Does this affect sequential key presses?

@iamcodemaker
Copy link
Contributor Author

It doesn't seem to affect sequential key presses in my limited testing on my macbook, but in theory it could. If there is more than one key waiting to be read in the OS buffer, then the second one will be skipped. This could happen if the program reads input, then stops reading and does some work while the user continues to type. When the program reads again, multiple keys will be queued up and only every other keypress will be honored. I tested this using std::thread::sleep between calls to Keys::next() and keys are skipped.

Ideally, when the ESC key is pressed, the program could wait for a few ms to see if any other keys follow, then treat it as ESC if no escape sequence is detected. The Read trait doesn't support a timed read though. There may be a way to configure the terminal's buffer using VTIME and VMIN parameters. Or the code could be modified to keep an internal buffer and read into and from that two keys at a time (instead of just discarding the second key).

@iamcodemaker
Copy link
Contributor Author

I tweaked the way this works so that extra bytes are stored for the next time we need them instead of discarded. This avoids the problem of having extra bytes stored in the terminal's buffer. Though it will make single ESC key press detection fail in the buffered keypress case. That's better than the way it worked before.

The design here is less encapsulated than it could be. A better way to do this would be to implement the read_event function completely inside of the Events iterator. I'll make that change.

The strategy used here is to read two bytes at a time, going on the
assumption that escape sequences will consist of multi byte reads and
solitary Esc key presses will consist of single byte reads.

Tests had to be modified to account for these new multi byte reads by
including dummy bytes when a single byte was previously expected.

Fixes ticki/termion#43
@iamcodemaker
Copy link
Contributor Author

Squashed.

@ticki
Copy link
Contributor

ticki commented Aug 7, 2016

I'll test and merge this when I give time. Cool work, I appreciate it.

@jmacdonald
Copy link
Contributor

@ticki any chance you can set aside some time to review (and merge, if appropriate)? Would love to see this functionality make it in. 😄

@ticki
Copy link
Contributor

ticki commented Oct 3, 2016

Yeah, I'll have some time later today. Sorry for the delay.

@ticki
Copy link
Contributor

ticki commented Oct 25, 2016

I tested it but I didn't leave a comment. I don't know why.

This is not going to get merged because it delays the characters into bulks. So, unfortunately I am going to close.

@ticki ticki closed this Oct 25, 2016
@iamcodemaker
Copy link
Contributor Author

I don't fully understand the issue here, but do you have any suggestions on how this could be implemented?

@ticki
Copy link
Contributor

ticki commented Oct 26, 2016

Conditionally reading the next byte?

@ticki
Copy link
Contributor

ticki commented Oct 26, 2016

You know what. This is better than nothing, so I'm going to merge anyway.

@ticki ticki reopened this Oct 26, 2016
@ticki ticki merged commit ea06c6f into redox-os:master Oct 26, 2016
@iamcodemaker iamcodemaker deleted the esc branch September 7, 2017 17:23
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.

3 participants