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

fix flashing during erase() on windows terminal #116

Merged
merged 3 commits into from Jun 3, 2021

Conversation

AskAlice
Copy link
Contributor

@AskAlice AskAlice commented Jun 3, 2021

This is a fix for windows terminal.
You can see when they implemented this feature here https://github.com/microsoft/terminal/pull/5181/files#diff-e3429f3447a1f078e953e22ed6556dfbffd1c584ec362f1be455d98589c38e79R108

before:

https://emu.bz/before.webm

after:

https://emu.bz/after.webm

for some reason this did not work with WSL in windows terminal, but it never worked in that situation anyway.
I can't detect if it's in windows terminal on WSL because the WT_TERMINAL env variable doesn't exist in that environment, but i could check for /etc/wsl.conf if needed. If it is in windows terminal on WSL, it can't use the cursor position escape sequence in combination with a carriage return in the clear function.

fmt.Fprintf(s.Writer, "\r\033[K") // erases to end of line

would need to be either

fmt.Fprintf(s.Writer, "\r") // erases to end of line

or

fmt.Fprintf(s.Writer, "\033[K") // erases to end of line

in that environment

in a975c3c, simply writing \033[K instead of \r\033[K in the clear function seems to do the trick, but I think I will need some verification that this works as intended in native linux environments.

@AskAlice AskAlice changed the title fix flashing during erase() on windows. fix flashing during erase() on windows terminal Jun 3, 2021
@briandowns
Copy link
Owner

Thanks for this PR! Do you have any Mac or Linux envs to test these changes? I'm curious if there'll be any regressions.

@AskAlice
Copy link
Contributor Author

AskAlice commented Jun 3, 2021

Thanks for this PR! Do you have any Mac or Linux envs to test these changes? I'm curious if there'll be any regressions.

running go test on my repo on a laptop running ubuntu groovy, it passed. Could you try and test more with your own env? I assume you don't use windows.

I can test on mac later today

@briandowns
Copy link
Owner

Thanks for this PR! Do you have any Mac or Linux envs to test these changes? I'm curious if there'll be any regressions.

running go test on my repo on a laptop running ubuntu groovy, it passed. Could you try and test more with your own env? I assume you don't use windows.

I can test on mac later today

I can check it out on Mac at some point today.

@briandowns
Copy link
Owner

Alright, had a free second. LGTM. Thank you so much for this fix!

@briandowns briandowns merged commit 2f0aa90 into briandowns:master Jun 3, 2021
@AskAlice
Copy link
Contributor Author

AskAlice commented Jun 3, 2021

Thanks for the quick merge! 💖
Our customers are going to appreciate this

@briandowns
Copy link
Owner

Happy to get more Windows support and if it's client based, I can try to give it some more attention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants