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

windows: enable virtual terminal processing, fixes #169 #173

Closed
wants to merge 1 commit into from
Closed

windows: enable virtual terminal processing, fixes #169 #173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions color_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package color

import (
"os"

"golang.org/x/sys/windows"
)

func init() {
// Opt-in for ansi color support for current process.
Copy link
Contributor

Choose a reason for hiding this comment

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

How to opt-out in case it causes issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "opt-in" comment is in relation to the Microsoft documentation, which calls it to "opt in for color support".
See how ENABLE_VIRTUAL_TERMINAL_PROCESSING is used in their examples at https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences#output-sequences for a better understanding.

I can not comment on possible issues this would cause, as I had no issues with it. I would just argue that it is by recommendation from Microsoft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, see this PR for crossterm, which is a rust crate with color support that dealt with the same issue last year:

crossterm-rs/crossterm#657

Copy link
Owner

Choose a reason for hiding this comment

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

We have a similiar constraint in the package that automatically disabled the coloring based on the environment, see: https://github.com/fatih/color/blob/main/color.go#L22

I wonder if we could move this into a function, and move it there. I think we still want to keep color_windows.go though.

Copy link
Contributor

Choose a reason for hiding this comment

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

How to opt-out in case it causes issues?

I think that in "no color" mode this Windows API should not be called.

The reason is that this code changes the console mode. So the console would have a different "behavior" after the program using github.com/fatih/color exits.

Moreover, there should be a way to revert to the original value of ConsoleMode.

Personally, I propose to simply add this as a code snippet in README.md as a workaround for #169

  1. It is "safer". I think it is better to not break anyone by changing the current console mode just to enable coloring.
  2. It does not seem like a frequent issue.
  3. It does not bring additional dependency 😉

Copy link
Owner

Choose a reason for hiding this comment

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

I think we should include this into the library. I like adding instructions to Readme, but this is a fundamental fix, and not including it breaks coloring for people using Windows. So we should make it a priority for them.

Copy link
Contributor Author

@martinlindhe martinlindhe Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@pellared and @fatih

I re-did this PR as #186 because I accidentally deleted the source fork. I still didn't address your last 2 comments. Is there a consensus about opting out on this?

Personally, I think opt-out from this code path is unneeded because the majority of Windows users will not exercise this code anyway as they will not be running in a 3rd party shell. Windows Terminal and Windows PowerShell both does this "opt in" on color themselves, so apps there don't need to perform the signal. Also it does not seem to be easy to "opt out" on colors in either of those.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, so I think this should be opt-in. It's only for people who use windows and if it causes an issue, we can always push a bugfix version. I like option for customization, but not to make things work the way it should. So if coloring doesn't work, nobody should be hunting down configuration in the readme to make it work. It's not very user friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I propose to simply add this as a code snippet in README.md as a workaround for #169

The issue with such solution is that every user of fatih/color will need to apply this workaround in order to fix the issue, where this PR simply fixes it for good.

// https://learn.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences#output-sequences
var outMode uint32
out := windows.Handle(os.Stdout.Fd())
if err := windows.GetConsoleMode(out, &outMode); err == nil {
_ = windows.SetConsoleMode(out, outMode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING)
Copy link
Contributor

@pellared pellared Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Any thought if ENABLE_PROCESSED_OUTPUT should be set as well?

Characters written by the WriteFile or WriteConsole function or echoed by the ReadFile or ReadConsole function are parsed for ASCII control sequences, and the correct action is performed. Backspace, tab, bell, carriage return, and line feed characters are processed. It should be enabled when using control sequences or when ENABLE_VIRTUAL_TERMINAL_PROCESSING is set.

Reference: https://learn.microsoft.com/en-us/windows/console/setconsolemode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any thought if ENABLE_PROCESSED_OUTPUT should be set as well?

I don't know. It was not needed in order to get color output in git bash.

Copy link
Owner

Choose a reason for hiding this comment

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

It looks we need to set ENABLE_PROCESSED_OUTPUT, from the docs:

ScreenShot-2023-03-01-at-10 05 20@2x

Copy link
Owner

Choose a reason for hiding this comment

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

@martinlindhe could you add this flag as well? Let's make sure to follow the doc rules, there are probably some edge cases we're not aware yet, but it's probably only fixed if ENABLE_PROCESSED_OUTPUT is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fatih Fixed in #186

}
}
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ go 1.13
require (
github.com/mattn/go-colorable v0.1.13
github.com/mattn/go-isatty v0.0.16
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not v0.5.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No good reason, maybe it was current when I opened the PR.

I accidentally deleted my branch so I would need to create a new PR in order to change the dependency version. Do you want me to do that?

Copy link
Owner

Choose a reason for hiding this comment

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

@martinlindhe could you update the dependency please? Let's use the latest and greatest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in #186.

)