windows: enable virtual terminal processing, fixes #169 #173
Conversation
|
Seems that github closed this PR when I deleted my fork. Reopening. |
| ) | ||
|
|
||
| func init() { | ||
| // Opt-in for ansi color support for current process. |
There was a problem hiding this comment.
How to opt-out in case it causes issues?
There was a problem hiding this comment.
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.
Additionally, see this PR for crossterm, which is a rust crate with color support that dealt with the same issue last year:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
- It is "safer". I think it is better to not break anyone by changing the current console mode just to enable coloring.
- It does not seem like a frequent issue.
- It does not bring additional dependency 😉
There was a problem hiding this comment.
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.
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.
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.
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.
| @@ -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.
| 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 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It looks we need to set ENABLE_PROCESSED_OUTPUT, from the docs:
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 ENABLE_PROCESSED_OUTPUT is set.
|
Fixed with #186 |
This patch fixes color display in Windows, when not running in a shell that takes care of this automatically.
If you run your go app thru Windows Powershell or Windows Terminal, they both take care of this automatically and you would not need this patch.
However, if you chose to use other shells such as bash, then without this patch you will only see ansi codes. More info in #169 (comment)