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

repl: add reverse search #31006

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 17, 2019

This adds a reverse search similar to zsh. Please check the commit messages for details.

This is still WIP. It needs tests and one line length issue must be fixed.
Update: done

See it in action: https://asciinema.org/a/shV3YOFu74BcBakJcvY4USNqv

Supersedes #24335

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Dec 17, 2019
This adds support for very long input lines to still display the
input preview correct.
@BridgeAR BridgeAR force-pushed the 2019-12-16-repl-reverse-search branch from 385ef1a to c7ecd5d Compare December 18, 2019 14:40
Add a reverse search that works similar to the ZSH one. It is
triggered with <ctrl> + r and <ctrl> + s. It skips duplicated history
entries and works with multiline statements. Matching entries indicate
the search parameter with an underscore and cancelling with <ctrl> + c
or escape brings back the original line.
Multiple matches in a single history entry work as well and are
matched in the order of the current search direction. The cursor is
positioned at the current match position of the history entry.
Changing the direction immediately searches checks for the next entry
in the expected direction from the current position on.
Entries are accepted as soon any button is pressed that doesn't
correspond with the reverse search.
The behavior is deactivated for simple terminals. They do not support
most ANSI escape codes that are necessary for this feature.
This just removes some redundant work and some other small things.
The cursor move functions accept a callback. It was possible that
`undefined` was returned in case there was no error instead of null.
@BridgeAR BridgeAR force-pushed the 2019-12-16-repl-reverse-search branch from c7ecd5d to 7f38f0a Compare December 19, 2019 10:06
@BridgeAR BridgeAR marked this pull request as ready for review December 19, 2019 10:07
@BridgeAR BridgeAR requested a review from targos December 19, 2019 10:07
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 19, 2019

const alreadyMatched = new Set();
const labels = {
r: 'bck-i-search: ',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these prompts be less abbreviated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have a suggestion? I thought it's nice to keep it aligned with ZSH and to also keep the length of both strings identical. backward-i-search and forward-i-search has a one character length difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it OK to land this as is? We can also change the name later on in a patch.

@Fishrock123
Copy link
Contributor

This appears to work as advertised. 👍

lib/internal/repl/utils.js Show resolved Hide resolved
lib/internal/repl/utils.js Outdated Show resolved Hide resolved
lib/internal/repl/utils.js Outdated Show resolved Hide resolved
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Left a few nits and I'm not sure we should support this - but code wise this is good enough to land IMO :]

@BridgeAR
Copy link
Member Author

I just addressed the nits, fixed one very unlikely issue (search lines exceeding the current terminal columns) and added a big TODO entry. Resizing the terminal will hide the overlay and fixing that requires significant work inside of readline. This should definitely not be a blocker though.

@benjamingr in what way are you uncertain that we should support reverse-i-search? It is one of my biggest missed features :D (I also plan on adding more features soonish).

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

changes lgtm

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 20, 2019
@Trott
Copy link
Member

Trott commented Dec 21, 2019

documentation update checkbox is checked, but no doc files have been updated I don't think?

@BridgeAR
Copy link
Member Author

@Trott I added a specific section that documents the behavior. We should probably revive #20825. Not all key bindings are actually fully implemented but so far we lack any key binding documentation.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
@antsmartian antsmartian mentioned this pull request Jan 8, 2020
4 tasks
@targos targos added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Jan 14, 2020
@BridgeAR BridgeAR deleted the 2019-12-16-repl-reverse-search branch January 20, 2020 12:08
@targos targos removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This adds support for very long input lines to still display the
input preview correct.

PR-URL: nodejs#31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Add a reverse search that works similar to the ZSH one. It is
triggered with <ctrl> + r and <ctrl> + s. It skips duplicated history
entries and works with multiline statements. Matching entries indicate
the search parameter with an underscore and cancelling with <ctrl> + c
or escape brings back the original line.
Multiple matches in a single history entry work as well and are
matched in the order of the current search direction. The cursor is
positioned at the current match position of the history entry.
Changing the direction immediately checks for the next entry in the
expected direction from the current position on.
Entries are accepted as soon any button is pressed that doesn't
correspond with the reverse search.
The behavior is deactivated for simple terminals. They do not support
most ANSI escape codes that are necessary for this feature.

PR-URL: nodejs#31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This just removes some redundant work and some other small things.

PR-URL: nodejs#31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
The cursor move functions accept a callback. It was possible that
`undefined` was returned in case there was no error instead of null.

PR-URL: nodejs#31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This adds support for very long input lines to still display the
input preview correct.

PR-URL: #31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
Add a reverse search that works similar to the ZSH one. It is
triggered with <ctrl> + r and <ctrl> + s. It skips duplicated history
entries and works with multiline statements. Matching entries indicate
the search parameter with an underscore and cancelling with <ctrl> + c
or escape brings back the original line.
Multiple matches in a single history entry work as well and are
matched in the order of the current search direction. The cursor is
positioned at the current match position of the history entry.
Changing the direction immediately checks for the next entry in the
expected direction from the current position on.
Entries are accepted as soon any button is pressed that doesn't
correspond with the reverse search.
The behavior is deactivated for simple terminals. They do not support
most ANSI escape codes that are necessary for this feature.

PR-URL: #31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This just removes some redundant work and some other small things.

PR-URL: #31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
The cursor move functions accept a callback. It was possible that
`undefined` was returned in case there was no error instead of null.

PR-URL: #31006
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable-change PRs with changes that should be highlighted in changelogs. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants