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

readline,repl: add substring history search #31112

Closed

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 27, 2019

Please see the individual commit messages for details.

Fixes: #28437
Fixes: #25272

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 27, 2019
@BridgeAR BridgeAR force-pushed the 2019-12-27-repl-substring-history-search branch 2 times, most recently from 5b21b6c to a36f1a8 Compare January 4, 2020 08:44
@BridgeAR BridgeAR marked this pull request as ready for review January 4, 2020 11:14
@BridgeAR
Copy link
Member Author

BridgeAR commented Jan 4, 2020

@nodejs/repl @nodejs/readline PTAL. I included a couple of smaller things that are all somewhat connected. I can split this into multiple PRs if requested.

This

  • adds substring history search,
  • improves unicode support,
  • adds support for tabs,
  • improves previews of very long input,
  • refactors code for simplicity and
  • improves the performance of the GetColumnWidth function for some inputs.

@nodejs-github-bot

This comment has been minimized.

doc/api/repl.md Outdated Show resolved Hide resolved
doc/api/repl.md Outdated Show resolved Hide resolved
lib/internal/repl/utils.js Outdated Show resolved Hide resolved
src/node_i18n.cc Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR force-pushed the 2019-12-27-repl-substring-history-search branch from 97ecd26 to 6c73393 Compare January 8, 2020 09:57
The preview had an off by one error in case colors where deactivated
and did not take fullwidth unicode characters into account when
displaying the preview.
This improves the current history search feature by adding substring
based history search similar to ZSH. In case the `UP` or `DOWN`
buttons are pressed after writing a few characters, the start string
up to the current cursor is used to search the history.

All other history features work exactly as they used to.

Fixes: nodejs#28437
Skip history entries that are identical to the currently visible line
to improve the user experience.
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.
@BridgeAR BridgeAR force-pushed the 2019-12-27-repl-substring-history-search branch from 6c73393 to 9e958f3 Compare January 8, 2020 11:00
@nodejs-github-bot

This comment has been minimized.

Simplify getStringWidth by removing dead code (the options were
unused) and by refactoring the logic.
This moves the charLengthLeft() and charLengthAt() into the internal
readline file. This allows sharing the functions internally with
other code.
This improves the performance in GetColumnWidth for full width
characters.
The option is now set to true by default. Most terminals do not have
full emoji support and visualize emojis with zero width joiners as
individual emojis.
Also verify that at least one argument is always passed through to the
function and remove support for passing through code points. Only
accept strings from now on to simplify the API.
This improves the completion previews by activating them for lines
that exceed the current terminal columns.
As a drive-by fix it also simplifies some statements.
This also adds a test to verify that changed writer options also
change the preview output depending on the options.
@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v12.x labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
The preview had an off by one error in case colors where deactivated
and did not take fullwidth unicode characters into account when
displaying the preview.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This improves the current history search feature by adding substring
based history search similar to ZSH. In case the `UP` or `DOWN`
buttons are pressed after writing a few characters, the start string
up to the current cursor is used to search the history.

All other history features work exactly as they used to.

PR-URL: nodejs#31112
Fixes: nodejs#28437
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Skip history entries that are identical to the currently visible line
to improve the user experience.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
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: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
1. Simplify the getStringWidth function used by Intl builds by removing
   dead code (the options were unused) and by refactoring the logic.
2. Improve the getStringWidth unicode handling used by non-Intl builds.
   The getStringWidth function returned the wrong width for multiple
   inputs. It's now improved by supporting various zero width characters
   and more full width characters.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This moves the charLengthLeft() and charLengthAt() into the internal
readline file. This allows sharing the functions internally with
other code.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This improves the performance in GetColumnWidth for full width
characters.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
The option is now set to true by default. Most terminals do not have
full emoji support and visualize emojis with zero width joiners as
individual emojis.
Also verify that at least one argument is always passed through to the
function and remove support for passing through code points. Only
accept strings from now on to simplify the API.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#31112
Fixes: nodejs#25272
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This improves the completion previews by activating them for lines
that exceed the current terminal columns.
As a drive-by fix it also simplifies some statements.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This also adds a test to verify that changed writer options also
change the preview output depending on the options.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
The preview had an off by one error in case colors where deactivated
and did not take fullwidth unicode characters into account when
displaying the preview.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This improves the current history search feature by adding substring
based history search similar to ZSH. In case the `UP` or `DOWN`
buttons are pressed after writing a few characters, the start string
up to the current cursor is used to search the history.

All other history features work exactly as they used to.

PR-URL: #31112
Fixes: #28437
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
Skip history entries that are identical to the currently visible line
to improve the user experience.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
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: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
1. Simplify the getStringWidth function used by Intl builds by removing
   dead code (the options were unused) and by refactoring the logic.
2. Improve the getStringWidth unicode handling used by non-Intl builds.
   The getStringWidth function returned the wrong width for multiple
   inputs. It's now improved by supporting various zero width characters
   and more full width characters.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This moves the charLengthLeft() and charLengthAt() into the internal
readline file. This allows sharing the functions internally with
other code.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This improves the performance in GetColumnWidth for full width
characters.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
The option is now set to true by default. Most terminals do not have
full emoji support and visualize emojis with zero width joiners as
individual emojis.
Also verify that at least one argument is always passed through to the
function and remove support for passing through code points. Only
accept strings from now on to simplify the API.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #31112
Fixes: #25272
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This improves the completion previews by activating them for lines
that exceed the current terminal columns.
As a drive-by fix it also simplifies some statements.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
This also adds a test to verify that changed writer options also
change the preview output depending on the options.

PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

repl: substring based search history Tabs in the repl cause display issues
6 participants