From cf28afeeb622836883b4bce84740f4b05d496428 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 29 Dec 2019 13:09:45 +0100 Subject: [PATCH] readline,repl: improve history up/previous MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reaching the history end caused the last entry to be persistent. That way there's no actualy feedback to the user that the history end is reached. Instead, visualize the original input line and keep the history index at the history end in case the user wants to go back again. PR-URL: https://github.com/nodejs/node/pull/31112 Reviewed-By: Michaël Zasso Reviewed-By: James M Snell --- lib/readline.js | 6 +-- test/parallel/test-readline-interface.js | 20 ++++++--- test/parallel/test-repl-history-navigation.js | 14 ++++--- test/parallel/test-repl-persistent-history.js | 41 +++++++++++++++---- .../test-repl-programmatic-history.js | 37 ++++++++++++++--- 5 files changed, 90 insertions(+), 28 deletions(-) diff --git a/lib/readline.js b/lib/readline.js index 0856d8aa087e61..62565b60701d6e 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -718,7 +718,7 @@ Interface.prototype._historyNext = function() { }; Interface.prototype._historyPrev = function() { - if (this.historyIndex < this.history.length) { + if (this.historyIndex < this.history.length && this.history.length) { const search = this[kSubstringSearch] || ''; let index = this.historyIndex + 1; while (index < this.history.length && @@ -727,9 +727,7 @@ Interface.prototype._historyPrev = function() { index++; } if (index === this.history.length) { - // TODO(BridgeAR): Change this to: - // this.line = search; - return; + this.line = search; } else { this.line = this.history[index]; } diff --git a/test/parallel/test-readline-interface.js b/test/parallel/test-readline-interface.js index d0a9b3589c8354..f141fe125d5224 100644 --- a/test/parallel/test-readline-interface.js +++ b/test/parallel/test-readline-interface.js @@ -462,12 +462,20 @@ function isWarned(emitter) { fi.emit('keypress', '.', { name: 'up' }); // 'baz' assert.strictEqual(rli.historyIndex, 2); assert.strictEqual(rli.line, 'baz'); - fi.emit('keypress', '.', { name: 'up' }); // 'baz' - assert.strictEqual(rli.historyIndex, 2); - assert.strictEqual(rli.line, 'baz'); - fi.emit('keypress', '.', { name: 'up' }); // 'baz' - assert.strictEqual(rli.historyIndex, 2); - assert.strictEqual(rli.line, 'baz'); + fi.emit('keypress', '.', { name: 'up' }); // 'ba' + assert.strictEqual(rli.historyIndex, 4); + assert.strictEqual(rli.line, 'ba'); + fi.emit('keypress', '.', { name: 'up' }); // 'ba' + assert.strictEqual(rli.historyIndex, 4); + assert.strictEqual(rli.line, 'ba'); + // Deactivate substring history search and reset history index. + fi.emit('keypress', '.', { name: 'right' }); // 'ba' + assert.strictEqual(rli.historyIndex, -1); + assert.strictEqual(rli.line, 'ba'); + // Substring history search activated. + fi.emit('keypress', '.', { name: 'up' }); // 'ba' + assert.strictEqual(rli.historyIndex, 0); + assert.strictEqual(rli.line, 'bat'); rli.close(); } diff --git a/test/parallel/test-repl-history-navigation.js b/test/parallel/test-repl-history-navigation.js index 6b0813e1fae36e..b1cdc5cbdfc41e 100644 --- a/test/parallel/test-repl-history-navigation.js +++ b/test/parallel/test-repl-history-navigation.js @@ -78,8 +78,7 @@ const tests = [ }, { env: { NODE_REPL_HISTORY: defaultHistoryPath }, - checkTotal: true, - test: [UP, UP, UP, UP, UP, DOWN, DOWN, DOWN, DOWN], + test: [UP, UP, UP, UP, UP, DOWN, DOWN, DOWN, DOWN, DOWN], expected: [prompt, `${prompt}Array(100).fill(1).map((e, i) => i ** 2)`, prev && '\n// [ 0, 1, 4, 9, 16, 25, 36, 49, 64, 81, 100, 121, ' + @@ -92,6 +91,8 @@ const tests = [ `${prompt}555 + 909`, prev && '\n// 1464', `${prompt}let ab = 45`, + prompt, + `${prompt}let ab = 45`, `${prompt}555 + 909`, prev && '\n// 1464', `${prompt}{key : {key2 :[] }}`, @@ -138,9 +139,12 @@ const tests = [ // UP - skipping const foo = true '\x1B[1G', '\x1B[0J', '> 555 + 909', '\x1B[12G', - // UP, UP, ENTER. UPs at the end of the history have no effect. - '\r\n', - '1464\n', + // UP, UP + // UPs at the end of the history reset the line to the original input. + '\x1B[1G', '\x1B[0J', + '> 55', '\x1B[5G', + // ENTER + '\r\n', '55\n', '\x1B[1G', '\x1B[0J', '> ', '\x1B[3G', '\r\n' diff --git a/test/parallel/test-repl-persistent-history.js b/test/parallel/test-repl-persistent-history.js index bb10085eccfcf6..1d1261a3752365 100644 --- a/test/parallel/test-repl-persistent-history.js +++ b/test/parallel/test-repl-persistent-history.js @@ -10,6 +10,7 @@ const assert = require('assert'); const fs = require('fs'); const path = require('path'); const os = require('os'); +const util = require('util'); const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); @@ -51,6 +52,7 @@ ActionStream.prototype.readable = true; // Mock keys const UP = { name: 'up' }; +const DOWN = { name: 'down' }; const ENTER = { name: 'enter' }; const CLEAR = { ctrl: true, name: 'u' }; @@ -90,20 +92,42 @@ const tests = [ }, { env: {}, - test: [UP, '\'42\'', ENTER], - expected: [prompt, '\'', '4', '2', '\'', '\'42\'\n', prompt, prompt], + test: [UP, '21', ENTER, "'42'", ENTER], + expected: [ + prompt, + '2', '1', '21\n', prompt, prompt, + "'", '4', '2', "'", "'42'\n", prompt, prompt + ], clean: false }, { // Requires the above test case env: {}, - test: [UP, UP, ENTER], - expected: [prompt, `${prompt}'42'`, '\'42\'\n', prompt] + test: [UP, UP, CLEAR, ENTER, DOWN, CLEAR, ENTER, UP, ENTER], + expected: [ + prompt, + `${prompt}'42'`, + `${prompt}21`, + prompt, + prompt, + `${prompt}'42'`, + prompt, + prompt, + `${prompt}21`, + '21\n', + prompt, + ] }, { env: { NODE_REPL_HISTORY: historyPath, NODE_REPL_HISTORY_SIZE: 1 }, - test: [UP, UP, CLEAR], - expected: [prompt, `${prompt}'you look fabulous today'`, prompt] + test: [UP, UP, DOWN, CLEAR], + expected: [ + prompt, + `${prompt}'you look fabulous today'`, + prompt, + `${prompt}'you look fabulous today'`, + prompt + ] }, { env: { NODE_REPL_HISTORY: historyPathFail, @@ -169,6 +193,8 @@ function runTest(assertCleaned) { const opts = tests.shift(); if (!opts) return; // All done + console.log('NEW'); + if (assertCleaned) { try { assert.strictEqual(fs.readFileSync(defaultHistoryPath, 'utf8'), ''); @@ -193,6 +219,7 @@ function runTest(assertCleaned) { output: new stream.Writable({ write(chunk, _, next) { const output = chunk.toString(); + console.log('INPUT', util.inspect(output)); // Ignore escapes and blank lines if (output.charCodeAt(0) === 27 || /^[\r\n]+$/.test(output)) @@ -207,7 +234,7 @@ function runTest(assertCleaned) { next(); } }), - prompt: prompt, + prompt, useColors: false, terminal: true }, function(err, repl) { diff --git a/test/parallel/test-repl-programmatic-history.js b/test/parallel/test-repl-programmatic-history.js index 7eda401a7c386b..5307ae0556ae74 100644 --- a/test/parallel/test-repl-programmatic-history.js +++ b/test/parallel/test-repl-programmatic-history.js @@ -8,6 +8,7 @@ const assert = require('assert'); const fs = require('fs'); const path = require('path'); const os = require('os'); +const util = require('util'); const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); @@ -49,6 +50,7 @@ ActionStream.prototype.readable = true; // Mock keys const UP = { name: 'up' }; +const DOWN = { name: 'down' }; const ENTER = { name: 'enter' }; const CLEAR = { ctrl: true, name: 'u' }; @@ -88,20 +90,40 @@ const tests = [ }, { env: {}, - test: [UP, '\'42\'', ENTER], - expected: [prompt, '\'', '4', '2', '\'', '\'42\'\n', prompt, prompt], + test: [UP, '21', ENTER, "'42'", ENTER], + expected: [ + prompt, + // TODO(BridgeAR): The line is refreshed too many times. The double prompt + // is redundant and can be optimized away. + '2', '1', '21\n', prompt, prompt, + "'", '4', '2', "'", "'42'\n", prompt, prompt + ], clean: false }, { // Requires the above test case env: {}, - test: [UP, UP, ENTER], - expected: [prompt, `${prompt}'42'`, '\'42\'\n', prompt] + test: [UP, UP, UP, DOWN, ENTER], + expected: [ + prompt, + `${prompt}'42'`, + `${prompt}21`, + prompt, + `${prompt}21`, + '21\n', + prompt + ] }, { env: { NODE_REPL_HISTORY: historyPath, NODE_REPL_HISTORY_SIZE: 1 }, - test: [UP, UP, CLEAR], - expected: [prompt, `${prompt}'you look fabulous today'`, prompt] + test: [UP, UP, DOWN, CLEAR], + expected: [ + prompt, + `${prompt}'you look fabulous today'`, + prompt, + `${prompt}'you look fabulous today'`, + prompt + ] }, { env: { NODE_REPL_HISTORY: historyPathFail, @@ -167,6 +189,8 @@ function runTest(assertCleaned) { const opts = tests.shift(); if (!opts) return; // All done + console.log('NEW'); + if (assertCleaned) { try { assert.strictEqual(fs.readFileSync(defaultHistoryPath, 'utf8'), ''); @@ -192,6 +216,7 @@ function runTest(assertCleaned) { output: new stream.Writable({ write(chunk, _, next) { const output = chunk.toString(); + console.log('INPUT', util.inspect(output)); // Ignore escapes and blank lines if (output.charCodeAt(0) === 27 || /^[\r\n]+$/.test(output))