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 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
@BridgeAR BridgeAR force-pushed the BridgeAR:2019-12-27-repl-substring-history-search branch 2 times, most recently from 5b21b6c to a36f1a8 Dec 31, 2019
@BridgeAR BridgeAR marked this pull request as ready for review Jan 4, 2020
@BridgeAR

This comment has been minimized.

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.
@BridgeAR BridgeAR requested review from targos, Trott, benjamingr, jasnell and srl295 Jan 4, 2020
@nodejs-github-bot

This comment was marked as outdated.

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 was marked as outdated.

@BridgeAR BridgeAR force-pushed the BridgeAR:2019-12-27-repl-substring-history-search branch from 97ecd26 to 6c73393 Jan 8, 2020
BridgeAR added 4 commits Dec 26, 2019
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: #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 BridgeAR:2019-12-27-repl-substring-history-search branch from 6c73393 to 9e958f3 Jan 8, 2020
@nodejs-github-bot

This comment was marked as outdated.

BridgeAR added 8 commits Dec 31, 2019
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.
Fixes: #25272
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.
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 2020
PR-URL: #31112
Fixes: #25272
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 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>
BridgeAR added a commit that referenced this pull request Jan 10, 2020
PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Jan 10, 2020

Landed in b4ae0cb...b0a7621 🎉

@BridgeAR BridgeAR closed this Jan 10, 2020
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 2020
PR-URL: #31112
Fixes: #25272
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 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>
MylesBorins added a commit that referenced this pull request Jan 16, 2020
PR-URL: #31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
@BridgeAR BridgeAR deleted the BridgeAR:2019-12-27-repl-substring-history-search branch Jan 20, 2020
@todo todo bot mentioned this pull request Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.