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: undo previous edit when get key code 0x1F #41392

Merged
merged 2 commits into from
Jan 21, 2022
Merged

readline: undo previous edit when get key code 0x1F #41392

merged 2 commits into from
Jan 21, 2022

Conversation

rayw000
Copy link
Contributor

@rayw000 rayw000 commented Jan 4, 2022

  1. Undo previous edit on keystroke ctrl - (emit 0x1F)
  2. unit tests
  3. documentation

Fix: #41308

By now, we have ctrl - to perform undo. In many terminals, all of ctrl -, ctrl _, ctrl / and ctrl ? send key code 0x1F, so we cannot distinguish if we are pressing shift to perform redo.

My questions are:

  1. If we could use a new keyboard shortcut, which one would be appropriate?
  2. Or, how could I set my terminal, e.g. iterm2 or the macOS default Terminal.app, to send key code other than 0x1F on keystroke ctrl _?

Could someone help me out? Thanks!

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. labels Jan 4, 2022
lib/internal/readline/interface.js Outdated Show resolved Hide resolved
lib/internal/readline/interface.js Outdated Show resolved Hide resolved
lib/internal/readline/interface.js Outdated Show resolved Hide resolved
lib/internal/readline/interface.js Outdated Show resolved Hide resolved
lib/internal/readline/interface.js Outdated Show resolved Hide resolved
lib/internal/readline/interface.js Outdated Show resolved Hide resolved
lib/internal/readline/interface.js Outdated Show resolved Hide resolved
lib/internal/readline/interface.js Show resolved Hide resolved
1. Undo previous edit on keystroke `ctrl -` (emit 0x1F)
2. unittests
3. documentation
@rayw000
Copy link
Contributor Author

rayw000 commented Jan 18, 2022

This PR and #41301 are conflicting. Once one of them is accepted, I'll update the other.

@Ayase-252
Copy link
Member

Which PR do you want to land first? this or #41301?

@rayw000
Copy link
Contributor Author

rayw000 commented Jan 21, 2022

Which PR do you want to land first? this or #41301?

Either would be OK. Just let me know. Thank you!

@Ayase-252
Copy link
Member

Okay, I plan to land this PR first, then #41301.

@Ayase-252 Ayase-252 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2022
@nodejs-github-bot
Copy link
Collaborator

@Ayase-252 Ayase-252 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jan 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41392
✔  Done loading data for nodejs/node/pull/41392
----------------------------------- PR info ------------------------------------
Title      readline: undo previous edit when get key code `0x1F` (#41392)
Author     Ray  (@rayw000)
Branch     rayw000:feature-undo-redo -> nodejs:master
Labels     readline, author ready, needs-ci
Commits    2
 - readline: undo previous edit when get key code 0x1F
 - readline: remove redundant code
Committers 1
 - Ray Wang 
PR-URL: https://github.com/nodejs/node/pull/41392
Fixes: https://github.com/nodejs/node/issues/41308
Reviewed-By: James M Snell 
Reviewed-By: Qingyu Deng 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41392
Fixes: https://github.com/nodejs/node/issues/41308
Reviewed-By: James M Snell 
Reviewed-By: Qingyu Deng 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 04 Jan 2022 06:23:39 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41392#pullrequestreview-854802340
   ✔  - Qingyu Deng (@Ayase-252): https://github.com/nodejs/node/pull/41392#pullrequestreview-859251418
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-01-21T03:57:00Z: https://ci.nodejs.org/job/node-test-pull-request/42063/
- Querying data for job/node-test-pull-request/42063/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 41392
From https://github.com/nodejs/node
 * branch                  refs/pull/41392/merge -> FETCH_HEAD
✔  Fetched commits as ef3517552797..c3475d0637dc
--------------------------------------------------------------------------------
Auto-merging doc/api/readline.md
[master 1809a4668c] readline: undo previous edit when get key code 0x1F
 Author: Ray Wang 
 Date: Wed Jan 5 20:06:26 2022 +0800
 3 files changed, 96 insertions(+)
[master dee886c43c] readline: remove redundant code
 Author: Ray Wang 
 Date: Tue Jan 18 15:48:42 2022 +0800
 1 file changed, 2 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
readline: undo previous edit when get key code 0x1F

  1. Undo previous edit on keystroke ctrl - (emit 0x1F)
  2. unittests
  3. documentation

PR-URL: #41392
Fixes: #41308
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Qingyu Deng i@ayase-lab.com

[detached HEAD 355abeff91] readline: undo previous edit when get key code 0x1F
Author: Ray Wang rayw.public@gmail.com
Date: Wed Jan 5 20:06:26 2022 +0800
3 files changed, 96 insertions(+)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
readline: remove redundant code

PR-URL: #41392
Fixes: #41308
Reviewed-By: James M Snell jasnell@gmail.com
Reviewed-By: Qingyu Deng i@ayase-lab.com

[detached HEAD d3bf794f0f] readline: remove redundant code
Author: Ray Wang rayw.public@gmail.com
Date: Tue Jan 18 15:48:42 2022 +0800
1 file changed, 2 deletions(-)

Successfully rebased and updated refs/heads/master.

ℹ Use --fixupAll option, squash the PR manually or land the PR from the command line.

https://github.com/nodejs/node/actions/runs/1727762702

@Ayase-252 Ayase-252 added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jan 21, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 21, 2022
@nodejs-github-bot nodejs-github-bot merged commit 271725a into nodejs:master Jan 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 271725a

@rayw000 rayw000 deleted the feature-undo-redo branch January 21, 2022 11:37
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
1. Undo previous edit on keystroke `ctrl -` (emit 0x1F)
2. unittests
3. documentation

PR-URL: #41392
Fixes: #41308
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
1. Undo previous edit on keystroke `ctrl -` (emit 0x1F)
2. unittests
3. documentation

PR-URL: nodejs#41392
Fixes: nodejs#41308
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Qingyu Deng <i@ayase-lab.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams
Copy link
Contributor

@rayw000 there are test failures when landing this in v16.x-staging. Do you mind investigating and backporting if needed? Thank you.

@rayw000
Copy link
Contributor Author

rayw000 commented Feb 28, 2022

No problem. @danielleadams

@rayw000
Copy link
Contributor Author

rayw000 commented Feb 28, 2022

@danielleadams I didn't see something unusual in my local env. What kind of failures did you encounter? Would you please provide more details? Thank you.

@danielleadams
Copy link
Contributor

danielleadams commented Mar 2, 2022

@rayw000 For sure. I cherry-picked 271725a onto the v16.x-staging branch for the next 16.x release, and when I ran make test, I get the following test failure:

=== release test-readline-interface ===
Path: parallel/test-readline-interface
node:assert:123
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
+ actual - expected

+ ','
- 'the quick brown'
    at Object.<anonymous> (/Users/danielleadams/Code/nodejs/node/test/parallel/test-readline-interface.js:739:10)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: ',',
  expected: 'the quick brown',
  operator: 'strictEqual'
}
Command: out/Release/node --expose-internals /Users/danielleadams/Code/nodejs/node/test/parallel/test-readline-interface.js

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

@rayw000 If you have successfully cherry-picked your commit on top of v16.x-staging and tests are passing on your local env, you can open a backport PR so this can land on the next v16.x release.

@rayw000
Copy link
Contributor Author

rayw000 commented Mar 2, 2022

@danielleadams @aduh95

Now I can repro this on v16.x-staging. This PR requires FakeInput to be able to emit key sequence in some way like this

fakeInput.emit('keypress', '.', { sequence: '\x1F' });

Cherry-picking 271725a misses its dependencies. The function call mentioned above emits nothing but only a key press .. I'm still trying to find out a minimum commits patchset which could make this PR work.

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

If that helps you can open a backport PR for more than one commit – there's probably another commit (or maybe several other commits) that landed on master before this one that you should cherry-pick before this one for the tests to pass.

@rayw000
Copy link
Contributor Author

rayw000 commented Mar 2, 2022

If that helps you can open a backport PR for more than one commit – there's probably another commit (or maybe several other commits) that landed on master before this one that you should cherry-pick before this one for the tests to pass.

OK. Does it mean that I can leave that backport PR open (or keep it a draft) to avoid this PR being merged into this release?

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2022

Danielle added backport-requested-v16.x label on this PR, so that means it won't land on v16.x unless someone opens a backport PR. You don't have to open the backport PR now (or even at all if that ends up being too much work for you) to block the backport, the label is sufficient.

@rayw000
Copy link
Contributor Author

rayw000 commented Mar 2, 2022

Danielle added backport-requested-v16.x label on this PR, so that means it won't land on v16.x unless someone opens a backport PR. You don't have to open the backport PR now (or even at all if that ends up being too much work for you) to block the backport, the label is sufficient.

Got it. Thank you!

@rayw000
Copy link
Contributor Author

rayw000 commented Apr 11, 2022

Hi @danielleadams

This PR should also be labeled as don't-land-to-v16.x for the same reason as #41662 and #41392.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. 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: undo typing in REPL
7 participants