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.

Sign up for GitHub

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

Jump to bottom

cmd/age: fix terminal escape sequences on Windows #475

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cmd/age: fix terminal escape sequences on Windows #475

wants to merge 1 commit into from

Conversation

BuriedInTheGround
Copy link
Contributor

With this PR, if possible, we enable virtual terminal processing, which is necessary for using terminal escape sequences on instances of the Windows Console. When enabling virtual terminal processing fails, we completely avoid using escape sequences to prevent weird characters to be printed to the console.

Please see comments on code for more info.

Fixes #474

P.S.: I'm happy to explain eventually obscure choices I made.

@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:03 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:03 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:03 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:03 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:03 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:03 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:03 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:06 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:06 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:06 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:06 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:06 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:06 — with GitHub Actions Inactive
@BuriedInTheGround BuriedInTheGround temporarily deployed to Build, sign, release binaries December 26, 2022 23:06 — with GitHub Actions Inactive
@BuriedInTheGround @FiloSottile
cmd/age: fix terminal escape sequences on Windows
4570c49 
If possible, we enable virtual terminal processing, which is necessary
for using terminal escape sequences on instances of the Windows Console.
When enabling virtual terminal processing fails, we completely avoid
using escape sequences to prevent weird characters to be printed to the
console.

Fixes FiloSottile#474
@FiloSottile FiloSottile temporarily deployed to Build, sign, release binaries April 22, 2023 11:21 — with GitHub Actions Inactive
@FiloSottile FiloSottile temporarily deployed to Build, sign, release binaries April 22, 2023 11:21 — with GitHub Actions Inactive
@FiloSottile FiloSottile temporarily deployed to Build, sign, release binaries April 22, 2023 11:21 — with GitHub Actions Inactive
@FiloSottile FiloSottile temporarily deployed to Build, sign, release binaries April 22, 2023 11:21 — with GitHub Actions Inactive
@FiloSottile FiloSottile temporarily deployed to Build, sign, release binaries April 22, 2023 11:21 — with GitHub Actions Inactive
@FiloSottile FiloSottile temporarily deployed to Build, sign, release binaries April 22, 2023 11:21 — with GitHub Actions Inactive
@FiloSottile FiloSottile temporarily deployed to Build, sign, release binaries April 22, 2023 11:21 — with GitHub Actions Inactive
FiloSottile
FiloSottile reviewed Apr 22, 2023
View reviewed changes
cmd/age/tui_windows.go

mode |= ENABLE_PROCESSED_OUTPUT
mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING
ret, _, _ := setConsoleMode.Call(uintptr(syscall.Stdout), uintptr(mode))
Copy link
Owner

Choose a reason for hiding this comment

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

Thank you for this and sorry about the review lag!

I made a couple small style changes, but then realized this shouldn't be unconditionally be stdout, as we might have opened CONOUT$ as a terminal instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking the time to review!

Shouldn't this work anyway by using syscall.Stdout? Go initializes syscall.Stdout with GetStdHandle(STD_OUTPUT_HANDLE) (see here), and STD_OUTPUT_HANDLE defaults to CONOUT$, as described in the Windows Console docs.

Maybe I'm confusing or missing something. If so, could you please elaborate on the situation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FiloSottile FiloSottile FiloSottile left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Weird characters in place of terminal escape sequences on Windows Console
2 participants
@BuriedInTheGround @FiloSottile