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

Handles <ESC>[J and <ESC>M #68

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Handles <ESC>[J and <ESC>M #68

merged 6 commits into from
Sep 15, 2020

Conversation

wasv
Copy link
Contributor

@wasv wasv commented Dec 24, 2019

Clears lines below current line on [J, clears entire scrollback buffer on [2J or [3J.
Also includes support for M, reverse linefeed, which is used alongside [J in zsh.

Fixes #2

Similar to up, used as inverse to <ESC>[J in zsh sometimes.
@ticky
Copy link
Contributor

ticky commented Jan 7, 2020

Hi @wasv, thanks for this! Are you able to provide us with some examples of using this behaviour so we can implement some tests?

@wasv
Copy link
Contributor Author

wasv commented Jan 7, 2020

I saw this issue most often was when using autocomplete in zsh. I've attached an example, captured using the script command, and the html file generated from that example by the patched version of the code.

zsh-autocomplete.html.txt

zsh-autocomplete.txt

@wasv
Copy link
Contributor Author

wasv commented Jul 12, 2020

A lot has happened in the world since I made this PR and #67. I can understand why they may have fallen into the backlog. However, I was wondering if they could be merged at some point? That way I can use the upstream instead of building my fork each time.

If any additional work is needed, please let me know.

@ticky
Copy link
Contributor

ticky commented Sep 8, 2020

Hey @wasv, I’m sorry that this one’s not been a high priority and has fallen a bit by the wayside.

Given the potentially high-impact nature of changes to this for us, we’d really prefer them to have test coverage, and while I initially had intended to add that myself, it’s been a very weird year and other things have pulled our collective attention elsewhere.

If you’d be able to add some specs for each of the escape codes you’ve added I’m sure we could get this merged, though!

@ticky
Copy link
Contributor

ticky commented Sep 15, 2020

I wound up down a very deep rabbit hole of reading specs and testing terminal behaviours with this one today! 😂 I’ve pushed a few commits, and I hope they meet with your approval.

  1. Given \x1b[2J clears the screen, I’ve updated it to reset the size of the screen and the position of the cursor. The cursor is reset in all the terminals I’ve tested, so I believe this is correct.
  2. I’ve implemented \x1b[1J, because everywhere I could find reference for these, that one was mentioned, and I figured it was worth adding!
  3. The \x1b[J handler didn’t remove the following lines, just blanked them out, meaning you could end up with erroneously “long” output; I’ve updated it to remove the lines instead
  4. I’ve added some simple tests which should cover all of the stuff we’ve added

Wherever possible, I’ve followed the lead of xterm, iTerm 2 and Apple Terminal, and all seem to agree with the implementation I’ve ended up with. I’ve also sourced some of the explanations of codes, and borrowed some names, from https://www.real-world-systems.com/docs/ANSIcode.html

I will be passing this to some golang-speaking colleagues for their input and merge approval, but I’d also love your thoughts on this. Apologies again for taking so long to get here, and thanks so much for caring enough to open this! 😃

@ticky ticky requested review from lox, chloeruka, pda and yob September 15, 2020 01:25
@wasv
Copy link
Contributor Author

wasv commented Sep 15, 2020

Thank you for looking into this. Your commits look good to me, however, I actually don't know Go. I learned it for this PR. I'll check if this fixes the issues I was having with zsh autocomplete.

Copy link
Contributor

@lox lox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks legit to me @ticky! Thanks @wasv!

@wasv wasv mentioned this pull request Sep 15, 2020
@wasv
Copy link
Contributor Author

wasv commented Sep 15, 2020

I was able to get this code to build and run, and it does fix the issues I was having with zsh autocomplete. However, I ran into some unrelated issues with the Dockerfile, which I described in a new issue.

@ticky ticky merged commit 4f1fb07 into buildkite:master Sep 15, 2020
@ticky
Copy link
Contributor

ticky commented Sep 15, 2020

Thanks for this, @wasv!

ticky added a commit that referenced this pull request Feb 4, 2021
An issue introduced by an oversight in #68, it was possible for the line clearing instructions to fail if there weren't any lines present to clear in their respective positions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle <ESC>[J, <ESC>[1J, <ESC>[2J
3 participants