readline: add reverse search #24335

antsmartian · 2018-11-13T10:02:58Z

Hello Team,

I wanted to add reverse search to repl:

Reverse search is supported in linux and unix terminals.
User can press ctrl+r to search their history
When user types in text, we search through node_repl_history and show off the matching results.
When user press enter we run the matching command on our repl.
When more results found for a string, pressing ctrl+r again will navigate the search results one by one in a circular fashion (if we reach end of the search result).

It's a work in progress need tests, doc etc.

Initial mode of working is ready.

Need thoughts from community on the idea. But I would love to land this on repl 😍

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

Fishrock123 · 2018-11-15T22:52:27Z

Not sure I understand what this does, could you elaborate?

antsmartian · 2018-11-16T01:07:08Z

@Fishrock123 Oh, I'm sorry, should have given it in PR description, but didn't. Modified now.

Trott · 2018-11-28T03:25:07Z

@nodejs/repl

Fishrock123 · 2018-11-29T00:10:40Z

This sounds fine to me if it is fully implemented. @antsmartian Did you need anything to progress here?

antsmartian · 2018-11-29T01:10:45Z

@Fishrock123 Not really. I just wanted repl team & community confirmation on this idea, before I can go ahead with few changes and add test cases to land this. Thanks for your time on this.

antsmartian · 2018-12-08T09:02:14Z

@nodejs/repl @Fishrock123 PTAL..

BridgeAR

Great that you implement this! It is something I am definitely looking forward to!

I personally would change the output to the same as it's used in bash:

(reverse-i-search)'$input': $match and (failed reverse-i-search)'input': $lastMatch.

That way the $match is not duplicated.

lib/readline.js

BridgeAR · 2018-12-08T13:34:36Z

lib/readline.js

+  this.reverseSearchPrompt = "(reverse-i-search)`' ";
+  this.inReverseSearch = false;
+  this.reverseSearchResults = [];
+  this.reverseSearchIndex = 0;


I might be wrong but I believe it is possible to combine reverseSearchIndex and inReverseSearch by setting reverseSearchIndex to -2 when not searching and -1 when nothing is found in reverseSearch.

lib/readline.js

antsmartian · 2018-12-12T15:25:46Z

Thanks @BridgeAR. Will take a look at your comments. One quick question, do you think this will be valuable for the user:

(failed reverse-i-search)'input': $lastMatch

I guess it can slightly confuse the user if we show $lastMatch here, with the wrong input. Thoughts?

BridgeAR · 2018-12-12T15:31:52Z

@antsmartian I just suggested to mirror the way it's known to me. Showing $lastMatch is definitely not really helpful and just showing (failed reverse-i-search)'input': should likely even be better.

antsmartian · 2018-12-14T04:10:39Z

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

antsmartian · 2018-12-14T05:54:16Z

@BridgeAR @nodejs/repl PTAL..

antsmartian · 2018-12-20T07:16:02Z

Gentle remainder.. cc @nodejs/repl @BridgeAR

lib/readline.js

antsmartian · 2019-01-17T13:42:48Z

@BridgeAR Thanks for your review. Will have a look at it and close this. Regarding the color check, are there any existing function that we use for repl? I tried searching the codebase, but no luck till now.

BridgeAR

Thanks for following up on the comments! It's on a good way but it needs some further polishing.

Since this feature is mainly a readline feature and not a repl feature, we should likely also add a couple readline tests. Please also add tests for more special cases.

BridgeAR · 2019-01-20T21:01:40Z

lib/readline.js

@@ -474,8 +491,60 @@ Interface.prototype._insertString = function(c) {
    // a hack to get the line refreshed if it's needed
    this._moveCursor(0);
  }
+
+  searchHistory.call(this);


This should be obsolete as _refreshLine() has just been called before and that calls searchHistory().

I guess searchHistory is required, but we should move up to the else block (where the refresh line is not called in that case.

BridgeAR · 2019-01-20T21:08:16Z

lib/readline.js

+  const historySet = new Set(this.history);
+  for (;this.reverseSearchIndex < [...historySet].length;
+    this.reverseSearchIndex++) {
+    if (this.line.trim() !== '' &&


The trimming could be done once upfront instead of for each history entry but I would rather not do this at all. It should be fine to search for e.g., multiple whitespace.

Hmmm, searching for whitespaces might be useful. Let me check on this.

BridgeAR · 2019-01-20T21:18:46Z

lib/readline.js

+          this.line = this.history[this.reverseSearchIndex - 1];
+          this.inReverseSearch = false;
+          this.reverseSearchIndex = 0;
+        }


Did you test all other "special" keys like left, right, up, down, tab, etc? I personally would just mimic the behavior in the bash which ends the reverse search but if it has no side effects here, we could keep this. I just expect some possible input to have unintended side effects.

E.g. ctrl + a: where does the cursor go in that case? The beginning of the line should not be zero in this case. We could keep the functionality working but in that case we would have to move the cursor to the correct position and we have to make sure this works with all input (and that's not that easy due to lots of cases).

Good point, yes I did test on this. Actually, all the keys like left, right, ctrl + a are working as expected (as the whole reverse search is on the same stream and cursor set correctly). In this case, I guess it's good enough to leave these as is, rather than writing a layer which prevents all these key combinations when search is on. What say? Thoughts?

BridgeAR · 2019-01-20T21:33:51Z

lib/readline.js

+  if (this.inReverseSearch) {
+    let result = searchText.call(this);
+    const historySet = new Set(this.history);
+    if (this.reverseSearchIndex >= [...historySet].length) {


I do not understand this code part, can you please elaborate? How could the found index be bigger than the set length? It could at max be equal but that should be legit, no?

BridgeAR · 2019-01-20T21:53:56Z

lib/readline.js

+    cursorTo(this.output, this.cursor + kReverseSearchPrompt.length - 2);
+  } else {
+    this.output.write(buildReverseSearchPrompt(this.line, ''));
+    cursorTo(this.output, this.cursor + kFailedReverseSearchPrompt.length);


The this.cursor part confuses me in all of these. Are you certain it is the correct position?

Yes. But can you explain me a bit, why this is confusing you?

BridgeAR · 2019-01-20T22:15:25Z

lib/readline.js

+    cursorTo(this.output, this.cursor + kReverseSearchPrompt.length - 2);
+  } else if (this.line === '') {
+    this.output.write(buildReverseSearchPrompt());
+    cursorTo(this.output, this.cursor + kReverseSearchPrompt.length - 2);


We could move the cursor to the position where we matched the entry. That way we also use the same behavior as the bash and it's helpful for the user.

+1. I guess this is very good idea!

BridgeAR · 2019-01-20T22:29:21Z

lib/readline.js

@@ -383,6 +398,8 @@ Interface.prototype._refreshLine = function() {
  }

  this.prevRows = cursorPos.rows;
+
+  searchHistory.call(this);


This calls the searchHistory more often than it's necessary. Right now it would trigger this with each e.g. cursor move but we should only trigger the search if the input letters changed.

BridgeAR · 2019-01-20T22:29:26Z

lib/readline.js

+function searchText() {
+  let result = '';
+  const historySet = new Set(this.history);
+  for (;this.reverseSearchIndex < [...historySet].length;


There is no need to convert the set back to an array. Just use .size instead. The same below.

However, this approach does not work in all cases. The unique set could have less entries than the original history and the entry to look for might be at the end of the history which would not be reached here anymore.

E.g.: The set of [1, 1, 1, 2] is { 1, 2 } and if we look for 2 it would only check history entry 0 and 1 and would give up afterwards.

I personally would just iterate directly over the array.

// At the top of the file: const kReverseMatches = Symbol('kReverseMatches'); const kRreverseSearchIndex = Symbol('kRreverseSearchIndex'); // In the constructor: Object.defineProperty(this, kRreverseSearchIndex, { value: -2, writable: true, configurable: true, enumerable: false }); Object.defineProperty(this, kReverseMatches, { value: null, writable: true, configurable: true, enumerable: false }); // Actual logic: function reverseSearch(self) { while (++self[kRreverseSearchIndex] < self.history.length) { const entry = self.history[self[kRreverseSearchIndex]]; if (self[kReverseMatches].has(entry)) continue; self[kReverseMatches].add(entry); const start = entry.indexOf(self.line) if (start !== -1) { const output = `(reverse-i-search)\`${self.line}': `; self.output.write(`${output}${entry}`); cursorTo(self.output, output.length + start); return; } } self[kRreverseSearchIndex] = -1; const output = `(failed-reverse-i-search)\`${text}':`; self.output.write(output); cursorTo(self.output, output.length); } // Further below: case 'r': if (this[kRreverseSearchIndex] === -2) { this[kRreverseSearchIndex] = -1; this[kReverseMatches] = new Set(); } reverseSearch(this); // Further below: case 'return': // carriage return, i.e. \r this._sawReturnAt = Date.now(); if (this[kRreverseSearchIndex] !== -2) { if (this[kRreverseSearchIndex] !== -1) this.line = this.history[this[kRreverseSearchIndex]]; this[kRreverseSearchIndex] = -2; this[kReverseMatches] = null; }

The code is untested but it should roughly work like that.

Btw: the bash would change the historyIndex which this implementation does not but I think it's fine as it is.

antsmartian · 2019-03-03T08:47:25Z

@BridgeAR Thanks for your time on this, sorry was quite busy hence late reply from my side. Will try to update the PR with comments addressed.

antsmartian · 2019-12-25T10:33:32Z

Taken care by: #31006

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Nov 13, 2018

antsmartian force-pushed the multiline-strip-comments branch 3 times, most recently from 5d0498e to 10579f5 Compare November 13, 2018 12:49

BridgeAR added the wip Issues and PRs that are still a work in progress. label Nov 13, 2018

antsmartian force-pushed the multiline-strip-comments branch 3 times, most recently from 5a61791 to 35ab75c Compare December 8, 2018 07:52

antsmartian removed the wip Issues and PRs that are still a work in progress. label Dec 8, 2018

antsmartian changed the title ~~[WIP]: Add reverse search to repl~~ repl: add reverse search to repl Dec 8, 2018

BridgeAR reviewed Dec 8, 2018

View reviewed changes

antsmartian force-pushed the multiline-strip-comments branch from 35ab75c to e7f7afd Compare December 14, 2018 04:09

BridgeAR reviewed Dec 20, 2018

View reviewed changes

lib/readline.js Outdated Show resolved Hide resolved

lib/readline.js Outdated Show resolved Hide resolved

lib/readline.js Outdated Show resolved Hide resolved

lib/readline.js Outdated Show resolved Hide resolved

lib/readline.js Outdated Show resolved Hide resolved

antsmartian force-pushed the multiline-strip-comments branch from e7f7afd to bdb624c Compare January 19, 2019 06:12

lib: support reverse search in repl

75d46ab

antsmartian force-pushed the multiline-strip-comments branch from bdb624c to 75d46ab Compare January 20, 2019 09:09

BridgeAR reviewed Jan 20, 2019

View reviewed changes

BridgeAR changed the title ~~repl: add reverse search to repl~~ readline: add reverse search Jan 20, 2019

BridgeAR mentioned this pull request Jul 5, 2019

repl: substring based search history #28437

Closed

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

BridgeAR mentioned this pull request Dec 17, 2019

repl: add reverse search #31006

Closed

4 tasks

antsmartian closed this Dec 25, 2019

kgryte mentioned this pull request Mar 27, 2024

[RFC]: add reverse-search support in the REPL stdlib-js/stdlib#2069

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: add reverse search #24335

readline: add reverse search #24335

antsmartian commented Nov 13, 2018 •

edited

Loading

Fishrock123 commented Nov 15, 2018

antsmartian commented Nov 16, 2018 •

edited

Loading

Trott commented Nov 28, 2018

Fishrock123 commented Nov 29, 2018

antsmartian commented Nov 29, 2018 •

edited

Loading

antsmartian commented Dec 8, 2018

BridgeAR left a comment

BridgeAR Dec 8, 2018 •

edited

Loading

antsmartian commented Dec 12, 2018

BridgeAR commented Dec 12, 2018

antsmartian commented Dec 14, 2018

antsmartian commented Dec 14, 2018

antsmartian commented Dec 20, 2018

antsmartian commented Jan 17, 2019

BridgeAR left a comment

BridgeAR Jan 20, 2019

antsmartian Mar 3, 2019

BridgeAR Jan 20, 2019

antsmartian Mar 3, 2019

BridgeAR Jan 20, 2019

antsmartian Mar 3, 2019

BridgeAR Jan 20, 2019

BridgeAR Jan 20, 2019

antsmartian Mar 3, 2019

BridgeAR Jan 20, 2019

antsmartian Mar 3, 2019

BridgeAR Jan 20, 2019

BridgeAR Jan 20, 2019 •

edited

Loading

antsmartian commented Mar 3, 2019

antsmartian commented Dec 25, 2019

readline: add reverse search #24335

readline: add reverse search #24335

Conversation

antsmartian commented Nov 13, 2018 • edited Loading

Checklist

Fishrock123 commented Nov 15, 2018

antsmartian commented Nov 16, 2018 • edited Loading

Trott commented Nov 28, 2018

Fishrock123 commented Nov 29, 2018

antsmartian commented Nov 29, 2018 • edited Loading

antsmartian commented Dec 8, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Dec 8, 2018 • edited Loading

Choose a reason for hiding this comment

antsmartian commented Dec 12, 2018

BridgeAR commented Dec 12, 2018

antsmartian commented Dec 14, 2018

antsmartian commented Dec 14, 2018

antsmartian commented Dec 20, 2018

antsmartian commented Jan 17, 2019

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

antsmartian commented Mar 3, 2019

antsmartian commented Dec 25, 2019

antsmartian commented Nov 13, 2018 •

edited

Loading

antsmartian commented Nov 16, 2018 •

edited

Loading

antsmartian commented Nov 29, 2018 •

edited

Loading

BridgeAR Dec 8, 2018 •

edited

Loading

BridgeAR Jan 20, 2019 •

edited

Loading