readline: turn emitKeys into a streaming parser #1601
Conversation
|
What about having a timer that starts when you see |
I think readline behavior should be consistent with other shells. I just tested |
|
Sorry, I was referring to ncurses-based programs and such, not so much shells or other REPLs. |
|
|
||
| if (ch === '\x1b') { | ||
| s += (ch = yield); | ||
| } |
There was a problem hiding this comment.
Maybe use a while (ch === '\x1b'), so it will catch as many escapes as one could possibly feed into it?
|
Clever use of a generator here. I like it. Would prefer if someone who better understands the parsing requirements could also take a look though. |
|
There're a few unused consts after this commit, probaby best to remove these (unless there's future use for them): |
|
Tested it a bit. Escape sequences all work as expected, the code looks well documented, nice work! |
| } else { | ||
| s = s.toString(stream.encoding || 'utf-8'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Does all the existing Buffer functionality still work?
There was a problem hiding this comment.
No, it's not, and it didn't work before either.
I'd appreciate another look at this, but I think this Buffer handling was a dead code since this commit: 3c91a7a.
Function emitKeys never receives any buffers in the first place, because string_decoder.write() returns strings only.
|
LGTM, but maybe cc @piscisaureus? |
Done. Some of rhe consts are used in |
| if ((match = cmd.match(/^(\d\d?)(;(\d))?([~^$])$/))) { | ||
| code += match[1] + match[4]; | ||
| modifier = (match[3] || 1) - 1; | ||
| } else if ((match = cmd.match(/^((\d;)?(\d))?([A-Za-z])$/))) { |
There was a problem hiding this comment.
should we put these into consts then? Does it make a difference?
There was a problem hiding this comment.
Are you talking about regular expressions?
I think the only difference is readability, so if regexps are large enough, it'd make sense to move them out (or get rid of them since large regular expressions are a source of trouble usually).
But those are small, so they could be inlined.
V8 should create each regexp from regexp literal only once anyway.
In certain environments escape sequences could be splitted into multiple chunks. For example, when user presses left arrow, `\x1b[D` sequence could appear as two keypresses (`\x1b` + `[D`). Fixes: nodejs#1403
|
Rebased on top of master branch and added generators to |
|
LGTM |
In certain environments escape sequences could be splitted into multiple chunks. For example, when user presses left arrow, `\x1b[D` sequence could appear as two keypresses (`\x1b` + `[D`). PR-URL: #1601 Fixes: #1403 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
|
Thanks! Landed in aed6bce |
In certain environments escape sequences could be splitted into multiple chunks. For example, when user presses left arrow, `\x1b[D` sequence could appear as two keypresses (`\x1b` + `[D`). PR-URL: nodejs#1601 Fixes: nodejs#1403 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
see #1403
I changed
emitKeysto a generator that receives data character by character and emitskeypressevent whenever an escape sequence is complete.So now behavior is similar to
bashandpythonshells I tested so far. For example, you can sequentally pressESC + [ + D, and it will be handled the same as "left arrow", because the data that's coming to node.js is the same in both cases:\x1b[D.Possible side effect:
escapekey itself will never be emitted. If we get\x1b, parser will treat it as a start of an escape sequence no matter what. Our REPL never rely on that, but maybe other readline users are (xkcd spacebar heating yep).