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

Jump to bottom

repl: add reverse search #31006

Closed
wants to merge 8 commits into from
Closed

repl: add reverse search #31006

wants to merge 8 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 17, 2019 •
edited

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
@BridgeAR
repl: fix preview of lines that exceed the terminal columns
e997e86 
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
@BridgeAR
repl: implement reverse search
be0fe19 
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.
@BridgeAR
readline: small refactoring
4a41ffc 
This just removes some redundant work and some other small things.
@BridgeAR
readline: set null as callback return in case there's no error
7f38f0a 
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.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@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
Fishrock123
Fishrock123 reviewed Dec 19, 2019
View reviewed changes
lib/internal/repl/utils.js

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

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
Member

This appears to work as advertised. 👍

benjamingr
benjamingr reviewed Dec 20, 2019
View reviewed changes
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
benjamingr
benjamingr approved these changes Dec 20, 2019
View reviewed changes
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.

Sign in to view
benjamingr
benjamingr approved these changes Dec 20, 2019
View reviewed changes
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.

Sign in to view
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
BridgeAR added a commit that referenced this pull request Jan 7, 2020
@BridgeAR
2020-01-07, Version v13.6.0 (Current)
170ec8a 
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
@BridgeAR
2020-01-07, Version v13.6.0 (Current)
1a552f6 
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
@BridgeAR
2020-01-07, Version v13.6.0 (Current)
e908323 
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
Closed
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
This was referenced Mar 15, 2020
Closed
Closed
@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
@BridgeAR @targos
repl: fix preview of lines that exceed the terminal columns
8208aa7 
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
@BridgeAR @targos
repl: implement reverse search
0520b3d 
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
@BridgeAR @targos
readline: small refactoring
e677438 
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
@BridgeAR @targos
readline: set null as callback return in case there's no error
76f82fc 
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
@BridgeAR @targos
repl: fix preview of lines that exceed the terminal columns
85f8654 
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
@BridgeAR @targos
repl: implement reverse search
936c85c 
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
@BridgeAR @targos
readline: small refactoring
3946cad 
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
@BridgeAR @targos
readline: set null as callback return in case there's no error
e894eeb 
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>
@targos targos mentioned this pull request May 2, 2020
Merged
@kgryte kgryte mentioned this pull request Mar 27, 2024
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fishrock123 Fishrock123 Fishrock123 left review comments

@Trott Trott Trott approved these changes

@antsmartian antsmartian antsmartian approved these changes

@benjamingr benjamingr benjamingr approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@BridgeAR @nodejs-github-bot @Fishrock123 @Trott @antsmartian @benjamingr @targos