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

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

Merged
merged 1 commit into from Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions color_windows.go
@@ -0,0 +1,18 @@
package color

import (
"os"

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

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

@pellared pellared Mar 1, 2023

Choose a reason for hiding this comment

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

Can we use NO_COLOR env var and NoColor = true global variable to opt-out?
In such configuration this code should not be executed.

#173 (comment)

Copy link
Owner

Choose a reason for hiding this comment

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

the NO_COLOR env already changes the NoColor value. So we can just check for the NoColor variable and it'll cover everything.

Copy link
Contributor

@pellared pellared Mar 1, 2023

Choose a reason for hiding this comment

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

So I guess it is just a matter of adding

if NoColor {
  return
}

right? 😉

@fatih let me know if you want me to address my own comments

Copy link
Owner

@fatih fatih Mar 1, 2023

Choose a reason for hiding this comment

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

The problem though is, it wouldn't work as expected. Imagine this situation:

  1. You import color and enable NO_COLOR.
  2. The init() is called, it check's NoColor, if it's false it will not set the configuration
  3. After a while though, you set NoColor = false.
  4. It still doesn't work, because init() will not run anymore. So we need a separate enviornment variable. NoColor is working fine as it's now, but it wouldn't work for opt-out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding #2. init() is run after all var () blocks, so NoColor would be set before init() is called.

// 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 {
outMode |= windows.ENABLE_PROCESSED_OUTPUT | windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING
_ = windows.SetConsoleMode(out, outMode)
}
Comment on lines +14 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent Error Flow

Suggested change
if err := windows.GetConsoleMode(out, &outMode); err == nil {
outMode |= windows.ENABLE_PROCESSED_OUTPUT | windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING
_ = windows.SetConsoleMode(out, outMode)
}
if err := windows.GetConsoleMode(out, &outMode); err != nil {
return
}
outMode |= windows.ENABLE_PROCESSED_OUTPUT | windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING
_ = windows.SetConsoleMode(out, outMode)

reference: https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pellared I sent yet another PR for your last 2 comments. See #187

}
2 changes: 1 addition & 1 deletion go.mod
Expand Up @@ -7,4 +7,4 @@ require (
github.com/mattn/go-isatty v0.0.17
)

require golang.org/x/sys v0.3.0 // indirect
require golang.org/x/sys v0.5.0
4 changes: 2 additions & 2 deletions go.sum
Expand Up @@ -4,5 +4,5 @@ github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/
github.com/mattn/go-isatty v0.0.17 h1:BTarxUcIeDqL27Mc+vyvdWYSL28zpIhv3RoTdsLMPng=
github.com/mattn/go-isatty v0.0.17/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.3.0 h1:w8ZOecv6NaNa/zC8944JTU3vz4u6Lagfk4RPQxv92NQ=
golang.org/x/sys v0.3.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU=
golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=