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.

Sign up for GitHub

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

readline: add reverse search #24335

Closed
wants to merge 1 commit into from
Closed

readline: add reverse search #24335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 97 additions & 4 deletions lib/readline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ const {
kClearScreenDown
} = CSI;

// stuff used for reverse search
const kReverseSearchPrompt = "(reverse-i-search)`':";
const kFailedReverseSearchPrompt = '(failed-reverse-i-search)`';

// Lazy load StringDecoder for startup performance.
let StringDecoder;

Expand All @@ -75,6 +79,13 @@ function createInterface(input, output, completer, terminal) {
return new Interface(input, output, completer, terminal);
}

function buildReverseSearchPrompt(text, match) {
if (text === undefined)
return kReverseSearchPrompt;
if (match === '')
return `${kFailedReverseSearchPrompt}${text}':`;
return `(reverse-i-search)\`${text}': ${match}`;
}

function Interface(input, output, completer, terminal) {
if (!(this instanceof Interface)) {
Expand All @@ -90,6 +101,9 @@ function Interface(input, output, completer, terminal) {
this._previousKey = null;
this.escapeCodeTimeout = ESCAPE_CODE_TIMEOUT;

this.inReverseSearch = false;
this.reverseSearchIndex = 0;
Copy link
Member

@BridgeAR BridgeAR Dec 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.


EventEmitter.call(this);
var historySize;
var removeHistoryDuplicates = false;
Expand Down Expand Up @@ -347,7 +361,8 @@ Interface.prototype._addHistory = function() {

Interface.prototype._refreshLine = function() {
// line length
var line = this._prompt + this.line;
const line = this._prompt + this.line;

var dispPos = this._getDisplayPos(line);
var lineCols = dispPos.cols;
var lineRows = dispPos.rows;
Expand Down Expand Up @@ -383,6 +398,8 @@ Interface.prototype._refreshLine = function() {
}

this.prevRows = cursorPos.rows;

searchHistory.call(this);
Copy link
Member

Choose a reason for hiding this comment

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

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.

};


Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

};

function appendSearchResult(result) {
// this.previewResult = result;

// Cursor to left edge.
cursorTo(this.output, 0);
clearScreenDown(this.output);

// Based on the line and match result
// write the data
if (result !== '') {
this.output.write(buildReverseSearchPrompt(this.line, result));
cursorTo(this.output, this.cursor + kReverseSearchPrompt.length - 2);
} else if (this.line === '') {
this.output.write(buildReverseSearchPrompt());
cursorTo(this.output, this.cursor + kReverseSearchPrompt.length - 2);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1. I guess this is very good idea!

} else {
this.output.write(buildReverseSearchPrompt(this.line, ''));
cursorTo(this.output, this.cursor + kFailedReverseSearchPrompt.length);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

}

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

@BridgeAR BridgeAR Jan 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

this.reverseSearchIndex++) {
if (this.line.trim() !== '' &&
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this.history[this.reverseSearchIndex].includes(this.line)) {
result = this.history[this.reverseSearchIndex++];
break;
}
}

return result;
}

function searchHistory() {
if (this.inReverseSearch) {
let result = searchText.call(this);
const historySet = new Set(this.history);
if (this.reverseSearchIndex >= [...historySet].length) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

this.reverseSearchIndex = 0;

result = searchText.call(this);
}
appendSearchResult.call(this, result);
}
}

Interface.prototype._tabComplete = function(lastKeypressWasTab) {
var self = this;

Expand Down Expand Up @@ -768,16 +837,25 @@ Interface.prototype._moveCursor = function(dx) {
}
};

function breakOutOfReverseSearch() {
this.inReverseSearch = false;
this._refreshLine();
}

// handle a write from the tty
Interface.prototype._ttyWrite = function(s, key) {
const previousKey = this._previousKey;
key = key || {};
this._previousKey = key;

// Ignore escape key, fixes
// https://github.com/nodejs/node-v0.x-archive/issues/2876.
if (key.name === 'escape') return;
if (key.name === 'escape') {
if (this.inReverseSearch) {
breakOutOfReverseSearch.call(this);
}
// Else, ignore escape key. Fixes:
// https://github.com/nodejs/node-v0.x-archive/issues/2876.
return;
}

if (key.ctrl && key.shift) {
/* Control and shift pressed */
Expand All @@ -802,6 +880,11 @@ Interface.prototype._ttyWrite = function(s, key) {
// This readline instance is finished
this.close();
}

if (this.inReverseSearch) {
breakOutOfReverseSearch.call(this);
}

break;

case 'h': // delete left
Expand Down Expand Up @@ -897,6 +980,11 @@ Interface.prototype._ttyWrite = function(s, key) {
case 'right':
this._wordRight();
break;

case 'r':
if (!this.inReverseSearch)
this.inReverseSearch = true;
searchHistory.call(this);
}

} else if (key.meta) {
Expand Down Expand Up @@ -931,6 +1019,11 @@ Interface.prototype._ttyWrite = function(s, key) {
switch (key.name) {
case 'return': // carriage return, i.e. \r
this._sawReturnAt = Date.now();
if (this.inReverseSearch) {
this.line = this.history[this.reverseSearchIndex - 1];
this.inReverseSearch = false;
this.reverseSearchIndex = 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

this._line();
break;

Expand Down
156 changes: 156 additions & 0 deletions test/parallel/test-repl-reverse-search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
'use strict';

// Flags: --expose-internals

const common = require('../common');
const stream = require('stream');
const REPL = require('internal/repl');
const assert = require('assert');
const fs = require('fs');
const path = require('path');

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

const defaultHistoryPath = path.join(tmpdir.path, '.node_repl_history');

// Create an input stream specialized for testing an array of actions
class ActionStream extends stream.Stream {
run(data) {
const _iter = data[Symbol.iterator]();
const doAction = () => {
const next = _iter.next();
if (next.done) {
// Close the repl. Note that it must have a clean prompt to do so.
this.emit('keypress', '', { ctrl: true, name: 'd' });
return;
}
const action = next.value;

if (typeof action === 'object') {
this.emit('keypress', '', action);
} else {
this.emit('data', `${action}`);
}
setImmediate(doAction);
};
setImmediate(doAction);
}
resume() {}
pause() {}
}
ActionStream.prototype.readable = true;


// Mock keys
const ENTER = { name: 'enter' };
const CLEAR = { ctrl: true, name: 'u' };
const ESCAPE = { name: 'escape' };
const SEARCH = { ctrl: true, name: 'r' };

const prompt = '> ';
const reverseSearchPrompt = '(reverse-i-search)`\':';


const wrapWithSearchTexts = (code, result) => {
return `(reverse-i-search)\`${code}': ${result}`;
};
const tests = [
{ // creates few history to search for
env: { NODE_REPL_HISTORY: defaultHistoryPath },
test: ['\' search\'.trim()', ENTER, 'let ab = 45', ENTER,
'555 + 909', ENTER, '{key : {key2 :[] }}', ENTER],
expected: [],
clean: false
},
{
env: { NODE_REPL_HISTORY: defaultHistoryPath },
test: [SEARCH, 's', ESCAPE, CLEAR],
expected: [reverseSearchPrompt,
wrapWithSearchTexts('s', '\' search\'.trim()')]
},
{
env: { NODE_REPL_HISTORY: defaultHistoryPath },
test: ['s', SEARCH, ESCAPE, CLEAR],
expected: [wrapWithSearchTexts('s', '\' search\'.trim()')]
},
{
env: { NODE_REPL_HISTORY: defaultHistoryPath },
test: ['5', SEARCH, SEARCH, ESCAPE, CLEAR],
expected: [wrapWithSearchTexts('5', '555 + 909'),
wrapWithSearchTexts('5', 'let ab = 45')]
},
{
env: { NODE_REPL_HISTORY: defaultHistoryPath },
test: ['*', SEARCH, ESCAPE, CLEAR],
expected: ['(failed-reverse-i-search)`*\':'],
clean: true
}
];

function cleanupTmpFile() {
try {
// Write over the file, clearing any history
fs.writeFileSync(defaultHistoryPath, '');
} catch (err) {
if (err.code === 'ENOENT') return true;
throw err;
}
return true;
}

const numtests = tests.length;

const runTestWrap = common.mustCall(runTest, numtests);

function runTest() {
const opts = tests.shift();
if (!opts) return; // All done

const env = opts.env;
const test = opts.test;
const expected = opts.expected;

REPL.createInternalRepl(env, {
input: new ActionStream(),
output: new stream.Writable({
write(chunk, _, next) {
const output = chunk.toString();

if (!output.includes('reverse-i-search')) {
return next();
}

if (expected.length) {
assert.strictEqual(output, expected[0]);
expected.shift();
}

next();
}
}),
prompt: prompt,
useColors: false,
terminal: true
}, function(err, repl) {
if (err) {
console.error(`Failed test # ${numtests - tests.length}`);
throw err;
}

repl.once('close', () => {
if (opts.clean)
cleanupTmpFile();

if (expected.length !== 0) {
throw new Error(`Failed test # ${numtests - tests.length}`);
}
setImmediate(runTestWrap, true);
});

repl.inputStream.run(test);
});
}

// run the tests
runTest();