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

readline: keypress event trigger for escape character #7382

Closed

Conversation

princejwesley
Copy link
Contributor

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

readline

Description of change

Fixes: #7379

@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 23, 2016
@@ -149,3 +154,8 @@ addTest('\x1b[31ma\x1b[39ma', [
{ name: 'undefined', sequence: '\x1b[39m', code: '[39m' },
{ name: 'a', sequence: 'a' },
]);

// escape character
addTest('\x1b', [
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test for multiple escape characters one after another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@princejwesley princejwesley force-pushed the repl-keypress-escape branch 2 times, most recently from ad7bf8f to 9509910 Compare June 23, 2016 06:50
function onData(b) {
if (stream.listenerCount('keypress') > 0) {
var r = stream[KEYPRESS_DECODER].write(b);
if (r) {
if (timeoutRef) {
timeoutRef = clearTimeout(timeoutRef);
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not return anything. See https://github.com/nodejs/node/blob/ede98a77677afcc5e75a126043a11a720f7ae28b/lib/timers.js -- you can use just use clearTimeout(timeoutRef) without the if or re-assigning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Fishrock123 Yes, I just assigned the undefined return value to timeout object. I'll remove that

@princejwesley princejwesley force-pushed the repl-keypress-escape branch 4 times, most recently from 435a870 to bcb95b7 Compare June 23, 2016 15:44
@silverwind
Copy link
Contributor

Looking good on the timeout value part from my side. By the way: changes over time are much easier to review if you don't force push after every change.

// that can be called to run tests in sequence
const addKeyIntervalTest = (sequences, expectedKeys, interval = 550,
assertDelay = 550) => {
return (next) => () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just add a comment stating which functions these are mocking?

@Fishrock123 Fishrock123 self-assigned this Jun 30, 2016
@jasnell
Copy link
Member

jasnell commented Jul 5, 2016

Local testing shows this doing ok. LGTM with green CI but would like others to review also.

@princejwesley
Copy link
Contributor Author

@silverwind
Copy link
Contributor

silverwind commented Aug 2, 2016

Gave it a try. While it seems to work on single escape presses, 2 quick presses seem to get swallowed, 3 quick presses gives me:

undefined { sequence: '\u001b\u001b\u001b',
  name: 'escape',
  ctrl: false,
  meta: true,
  shift: false }
 { sequence: '',
  name: '\u0000',
  ctrl: true,
  meta: false,
  shift: false }

@princejwesley
Copy link
Contributor Author

@silverwind Do we need to reduce timeout value below 500ms?

@silverwind
Copy link
Contributor

I don't think the timeout value is the issue, it's what happens when another escape arrives during the timeout.

@princejwesley
Copy link
Contributor Author

@nodejs/collaborators Is it good to merge?

@jasnell
Copy link
Member

jasnell commented Aug 6, 2016

I'm fine with this as is but would like @silverwind to be comfortable with it also.

@silverwind
Copy link
Contributor

Not really happy that it prints that garbage \u0000 after three escape presses, I'd like to see that fixed at least.

@princejwesley
Copy link
Contributor Author

@silverwind I'll fix it

@princejwesley
Copy link
Contributor Author

@silverwind Please try now

@princejwesley
Copy link
Contributor Author

@silverwind
Copy link
Contributor

Looking good. Only nit I have a are ESC+ESC and ESC+ESC+ESC entered during the timeout which are given as these keypress events:

{ sequence: '\u001b\u001b',
  name: undefined,
  ctrl: false,
  meta: false,
  shift: false }
{ sequence: '\u001b\u001b\u001b',
  name: undefined,
  ctrl: false,
  meta: false,
  shift: false }

Is ESC+ESC a valid escape sequence? If not, maybe we should emit individual events per keypress.

Above issue isn't really something that should prevent this from landing. LGTM.

@princejwesley
Copy link
Contributor Author

princejwesley commented Aug 14, 2016

@silverwind As per the existing implementation, meta flag is set for the invalid escape sequences. Updated PR to set meta flag.

CI: https://ci.nodejs.org/job/node-test-pull-request/3659/

For the invalid escape sequence, we may need a new PR to emit individual events per keypress as its a breaking change.

@princejwesley
Copy link
Contributor Author

CI is green.

princejwesley added a commit that referenced this pull request Aug 17, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@princejwesley
Copy link
Contributor Author

Landed in 4b883a3fb45

@silverwind
Copy link
Contributor

For the invalid escape sequence, we may need a new PR to emit individual events per keypress as its a breaking change.

I'd say leave it as is. I can't find any reference that ESC+ESC is not a escape sequence of its own.

evanlucas pushed a commit that referenced this pull request Aug 20, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Fixes: #7379
PR-URL: #7382
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readline keypress event is triggered only after pressing Esc key 3 times
7 participants