Fix prompt reset on some terminals

[System-Glitch]

fixes #300

PreviousLine() prints \e[1F, which is apparently not recognized by Konsole. I didn't find this control code anywhere in VT100 and ANSI references.
On the other hand, Up() writes \e[1A, which works as expected. Using this makes it work perfectly in both the VSCode integrated terminal and Konsole.

This would require a little bit of testing on Windows and MacOS though, but I don't expect this change to break anything.

[AlecAivazis]

Thanks for hunting this down. I'm going to pull it onto my windows machine and test it out now.

[AlecAivazis]

I confirmed that Select prompts still render correctly with this change

[AlecAivazis]

If i'm following what you found, it seems like cursor.PreviousLine method should be removed entirely since it isn't uniformly supported. I would hate for me to forget this was a problem and bring up the issue in another spot.

Thoughts?

[System-Glitch]

We could either remove it entirely or leave a comment saying it is deprecated since it is not supported by all terminals. Deprecating this method would avoid having to move the module to v3, because removing the method would be a breaking change.

[System-Glitch]

@AlecAivazis
I made a bit more testing to make sure other inputs still work properly, especially the ones using PreviousLine(). All inputs appear to work properly for me.

PreviousLine() still appears to be required. It is used in many other places, so I wouldn't remove it nor mark it as deprecated.

[AlecAivazis]

Sorry if i'm missing something obvious, but I thought the whole source of this problem is that Konsole doesn't support PreviousLine. How does it work in some situations but not in others?

[System-Glitch]

Honestly I'm not sure. But if that works correctly like it is now, I would avoid removing things that may break the lib on other terminals. One hypothesis that I have and that I would need to test out would be that PreviousLine() moves the cursor to the end of the previous line, and doesn't keep the column on Konsole.

Edit: I tested this and no, Konsole doesn't do anything with PreviousLine(). I think it's still working because resetPrompt() moves the cursor up anyway.
I also found the \e[F code on wikipedia. It says "Not ANSI.SYS", which may explain why Konsole doesn't support it.

[System-Glitch]

Any news?

[AlecAivazis]

Hey @System-Glitch! I'm still a little uneasy about the ramifications of this change. I have a feeling more things about going to be broken in Konsole than we know because of this. Can you see if you can make this change everywhere and things stay the same? I'd like to deprecate this function at the very least so people don't end up using it in the future

[System-Glitch]

Ok I will test it out again.

[System-Glitch]

Test Results

Note: these tests have been done on my patch-erase-line branch.

Removing PreviousLine() everywhere

Konsole: Everything appears to be working correctly.

VSCode: The basic Input doesn't erase the previous line, the rest of the inputs appear to be working correctly. If I re-enable PreviousLine() just for Input, everything appear to be working as expected.

Replacing PreviousLine() by Up() everywhere

Konsole: Input goes up one too many times and erases the previous line ($ go run ... if it's the first question, erases the previous question otherwise). Password erases itself entirely after input. Other inputs appear to be working correctly.

VSCode: Everything appears to be working correctly.

Digging deeper

These test results suggest that RuneReader.ReadLine() was responsible. I noticed that this function was using NextLine(), which prints \e[1E. This control code is Non-ANSI as well. The inconsistency between the number of lines the cursor went up and down made sense. So I tried to replace NextLine() with Down(). I tested again and everything appears to be working properly on both Konsole and the VSCode integrated terminal.

Conclusion

I think PreviousLine() and NextLine() can be replaced with Up() and Down() respectively and then entirely removed. I will update the Pull Request accordingly.

[AlecAivazis]

Alright cool, thank you for being so thorough with this. Since it is a core function for the core prompts, i wanted to make sure we were certain what was going on. I think with the new deprecation notice this should be good to go. Will make one last pass and then bring it.

Thanks again!

[AlecAivazis]

Ran through the tests on my windows and mac laptop

[AlecAivazis]

Thanks again @System-Glitch!

[System-Glitch]

@AlecAivazis Can you tag v2.1.2 so this fix is available in a release, please?

[AlecAivazis]

hey @System-Glitch - this was included in the v2.2.0 release that just went out. lemme know if you need anything else :)

[AlecAivazis]

@System-Glitch it seems this PR broke the longlist.go example on windows. i'm going to revert this change

[System-Glitch]

Ok, I will look into it and try to fix it. Long list worked for me when I tested though, so I may not be able to reproduce.