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: fix position computation #28272

Conversation

BenoitZugmeyer
Copy link
Contributor

The implementation of _getDisplayPos, used to compute the cursor position and to find out how many lines to clear up when re-rendering the readline output, was counting each line (except the last one) from the input as one row, even if they were wraping. This caused some rendering issues when the 'prompt' have at least one wide line ending with a newline char, duplicating the lines at the top of the prompt when calling _refreshLine (ex: when the user hits backspace).

This patch fixes the issue by computing the real rows count for each new line in the input string.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

The implementation of _getDisplayPos, used to compute the cursor
position and to find out how many lines to clear up when re-rendering
the readline output, was counting each line (except the last one) from
the input as one row, even if they were wraping.  This caused some
rendering issues when the 'prompt' have at least one wide line ending
with a newline char, duplicating the lines at the top of the prompt when
calling _refreshLine (ex: when the user hits backspace).

This patch fixes the issue by computing the real rows count for each new
line in the input string.
@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 17, 2019
@Fishrock123
Copy link
Contributor

1 similar comment
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jun 30, 2019

@nodejs/readline

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 30, 2019
@addaleax
Copy link
Member

addaleax commented Jul 3, 2019

Landed in 46a73b9, and sorry for the delay! 🎉

@addaleax addaleax closed this Jul 3, 2019
pull bot pushed a commit to tkamenoko/node that referenced this pull request Jul 3, 2019
The implementation of _getDisplayPos, used to compute the cursor
position and to find out how many lines to clear up when re-rendering
the readline output, was counting each line (except the last one) from
the input as one row, even if they were wraping.  This caused some
rendering issues when the 'prompt' have at least one wide line ending
with a newline char, duplicating the lines at the top of the prompt when
calling _refreshLine (ex: when the user hits backspace).

This patch fixes the issue by computing the real rows count for each new
line in the input string.

PR-URL: nodejs#28272
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jul 20, 2019
The implementation of _getDisplayPos, used to compute the cursor
position and to find out how many lines to clear up when re-rendering
the readline output, was counting each line (except the last one) from
the input as one row, even if they were wraping.  This caused some
rendering issues when the 'prompt' have at least one wide line ending
with a newline char, duplicating the lines at the top of the prompt when
calling _refreshLine (ex: when the user hits backspace).

This patch fixes the issue by computing the real rows count for each new
line in the input string.

PR-URL: #28272
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced Jul 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants