readline: keypress event trigger for escape character by princejwesley · Pull Request #7382 · nodejs/node · GitHub

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

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

sure

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.

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

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.

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

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

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

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 }

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

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

@nodejs/collaborators Is it good to merge?

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

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

Fixes: nodejs#7379

@silverwind I'll fix it

@silverwind Please try now

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

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.

@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.

CI is green.

Fixes: #7379 PR-URL: #7382 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Landed in 4b883a3fb45

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.

Fixes: #7379 PR-URL: #7382 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 23, 2016

MylesBorins reviewed Jun 23, 2016
View reviewed changes

princejwesley force-pushed the repl-keypress-escape branch 2 times, most recently from ad7bf8f to 9509910 Compare June 23, 2016 06:50

Fishrock123 reviewed Jun 23, 2016
View reviewed changes

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

Fishrock123 reviewed Jun 24, 2016
View reviewed changes

Fishrock123 self-assigned this Jun 30, 2016

princejwesley added 2 commits August 13, 2016 10:10

readline: keypress event trigger for escape character

ccfc2c4

Fixes: nodejs#7379

readline: Add more comments

0be2947

Fix garbage

9327a39

princejwesley force-pushed the repl-keypress-escape branch from e2476c7 to 9327a39 Compare August 13, 2016 05:36

Consistent handling of invalid sequence

78ca577

princejwesley added a commit that referenced this pull request Aug 17, 2016

readline: keypress trigger for escape character

4b883a3

Fixes: #7379 PR-URL: #7382 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

princejwesley closed this Aug 17, 2016

evanlucas pushed a commit that referenced this pull request Aug 20, 2016

readline: keypress trigger for escape character

fbc5805

Fixes: #7379 PR-URL: #7382 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

evanlucas mentioned this pull request Aug 24, 2016

2016-08-26, Version 6.5.0 (Current) #8253

Merged

MylesBorins pushed a commit that referenced this pull request Sep 30, 2016

readline: keypress trigger for escape character

68a2979

Fixes: #7379 PR-URL: #7382 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins added the land-on-v4.x label Sep 30, 2016

rvagg pushed a commit that referenced this pull request Oct 18, 2016

readline: keypress trigger for escape character

0911a02

Fixes: #7379 PR-URL: #7382 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins pushed a commit that referenced this pull request Oct 26, 2016

readline: keypress trigger for escape character

c235708

Fixes: #7379 PR-URL: #7382 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

MylesBorins mentioned this pull request Oct 26, 2016

V4.6.2 proposal #9298

Closed

readline: keypress event trigger for escape character by princejwesley · Pull Request #7382 · nodejs/node · GitHub

Uh oh!

Uh oh!

readline: keypress event trigger for escape character - #7382

readline: keypress event trigger for escape character#7382princejwesley wants to merge 4 commits intonodejs:masternodejs/node:masterfrom princejwesley:repl-keypress-escapeprincejwesley/node:repl-keypress-escapeCopy head branch name to clipboard

Conversation

princejwesley commented Jun 23, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

MylesBorins Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

princejwesley Jun 23, 2016

Choose a reason for hiding this comment

Uh oh!

silverwind commented Jun 23, 2016

Uh oh!

Fishrock123 Jun 24, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 5, 2016

Uh oh!

princejwesley commented Aug 2, 2016

Uh oh!

silverwind commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

princejwesley commented Aug 2, 2016

Uh oh!

silverwind commented Aug 2, 2016

Uh oh!

princejwesley commented Aug 6, 2016

Uh oh!

jasnell commented Aug 6, 2016

Uh oh!

silverwind commented Aug 8, 2016

Uh oh!

princejwesley commented Aug 13, 2016

Uh oh!

princejwesley commented Aug 13, 2016

Uh oh!

princejwesley commented Aug 13, 2016

Uh oh!

silverwind commented Aug 13, 2016

Uh oh!

princejwesley commented Aug 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

princejwesley commented Aug 17, 2016

Uh oh!

princejwesley commented Aug 17, 2016

Uh oh!

silverwind commented Aug 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

readline: keypress event trigger for escape character#7382
princejwesley wants to merge 4 commits into
nodejs:masterfrom
princejwesley:repl-keypress-escape

silverwind commented Aug 2, 2016 •
edited
Loading

princejwesley commented Aug 14, 2016 •
edited
Loading