Fix input prompt repeating value when value is longer than terminal width. Closes #214 #239
Conversation
| * @return {Number} - The string length | ||
| */ | ||
|
|
||
| var REGEX_SYMBOLS = /([\0-\u02FF\u0370-\u1DBF\u1E00-\u20CF\u2100-\uD7FF\uDC00-\uFE1F\uFE30-\uFFFF]|[\uD800-\uDBFF][\uDC00-\uDFFF]|[\uD800-\uDBFF])([\u0300-\u036F\u1DC0-\u1DFF\u20D0-\u20FF\uFE20-\uFE2F]+)/g; |
There was a problem hiding this comment.
This line gives a long line warning from the linter. What can we do about it?
There was a problem hiding this comment.
Most linters can be configured to ignore lines with regular expressions in, for that exact reason. You can use the constructor function, but then you're compromising code quality, which isn't great.
|
Is there any way you can add a test for this? |
|
@callumacrae Let me see what can I do. |
| tty.lines = function() { | ||
| var value = this.rl.line || this.rl.history[0]; | ||
| var lineLength = utils.getUnicodeStringLength( this.rl._prompt + value ); | ||
| var result = lineLength / cliWidth(); |
There was a problem hiding this comment.
cliWidth can return 0 which is definitively not what we want.
There was a problem hiding this comment.
@SBoudrias In which cases can it return 0?
There was a problem hiding this comment.
In this case: https://github.com/knownasilya/cli-width/blob/master/index.js#L17
Pretty sure all the time on Windows.
There was a problem hiding this comment.
Hm, I guess my fix will not work in Windows in that case. Let me borrow a PC and give it a try there. Maybe there is an alternative way to get the terminal width in Windows.
There was a problem hiding this comment.
You can get free Windows VM to test on https://www.modern.ie/en-us/virtualization-tools#downloads
Pretty sure you can install git and checkout inquirer with them.
There was a problem hiding this comment.
@SBoudrias I've tested in Windows 8.1 and cliWidth returns the correct width on that platform. I had to implement some windows-specific workarounds for my patch to work fine, but I can confirm it works as expected on cmd.exe.
There still exist the possibility of cliWidth() returning 0, maybe in older Windows versions? I propose checking the return value of cliWidth, and if it is zero, don't make use of my patch, thus resulting in the actual behaviour. What do you think?
There was a problem hiding this comment.
Seems like a potential workaround.
Another is to manually handle line returns so we stay consistent. But that might just bring new kinds of issues.
There was a problem hiding this comment.
@callumacrae @SBoudrias Workaround implemented. tty.lines now returns 1 if cliWidth is 0, which, preserves the original behaviour.
|
Works great in Ubuntu as well. |
…idth. Closes #214 The problem happens because inquirer cleans only the current line on each key stroke, without paying attention to the actual length of the value, which might be longer than one line.
|
I was able to write some unit tests that check that the corresponding number of lines are cleaned, and that my patch is reverted if |
| done(); | ||
| }.bind(this)); | ||
|
|
||
| this.rl.line = "hello"; |
There was a problem hiding this comment.
For some reason, I have to assign this.rl.line directly, as emitting a line event doesn't assign the actual line to neither this.rl.line or this.rl.history, where tty expects them.
There was a problem hiding this comment.
That is correct as we use a mocked readline. So there's no usual Readline behavior during testing.
|
@SBoudrias Is there anything else needed for this PR to be merged? |
|
Hey, I'd have a couple comments on the code changes, but I'll hold onto them for now because this fix is very specific to the input prompt and I have a solution in mind that would fix this issue for every prompt type. I'm experimenting with it ATM and just need some more time to make sure it'll work fine. |
|
I'm going forward with the rendering process redesign. I'll close this PR as I don't intend to merge it. |
|
seriously, please, please merge this PR. We're still having problems with it 6 years later. |
The problem happens because inquirer cleans only the current line on
each key stroke, without paying attention to the actual length of the
value, which might be longer than one line.
This PR was only tested on OS X. Help from people with other OSes would be appreciated.