windows: enable virtual terminal processing, fixes #169 #173
There are no files selected for viewing
| 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. | ||
| // 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) | ||
|
There was a problem hiding this comment. Any thought if
Reference: https://learn.microsoft.com/en-us/windows/console/setconsolemode There was a problem hiding this comment.
I don't know. It was not needed in order to get color output in git bash. There was a problem hiding this comment. It looks we need to set
There was a problem hiding this comment. @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 |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
There was a problem hiding this comment. 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? There was a problem hiding this comment. @martinlindhe could you update the dependency please? Let's use the latest and greatest. There was a problem hiding this comment. Updated in #186. |
||
| ) | ||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.gothough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/colorexits.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.mdas a workaround for #169There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.