From 25a926d5cb36dd3b6da17e249c825e7b64338f7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 11 Oct 2022 14:29:50 +0200 Subject: [PATCH] Dogfood term package from go-gh --- cmd/gh/main.go | 4 - go.mod | 4 +- pkg/iostreams/color.go | 25 ------ pkg/iostreams/color_test.go | 121 -------------------------- pkg/iostreams/console.go | 7 +- pkg/iostreams/console_windows.go | 14 +-- pkg/iostreams/iostreams.go | 136 ++++++------------------------ pkg/iostreams/iostreams_test.go | 68 --------------- pkg/iostreams/tty_size.go | 20 ----- pkg/iostreams/tty_size_windows.go | 16 ---- utils/table_printer.go | 4 +- 11 files changed, 38 insertions(+), 381 deletions(-) delete mode 100644 pkg/iostreams/tty_size.go delete mode 100644 pkg/iostreams/tty_size_windows.go diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 750233ba7..f867d7c70 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -64,10 +64,6 @@ func mainRun() exitCode { cmdFactory := factory.New(buildVersion) stderr := cmdFactory.IOStreams.ErrOut - - if spec := os.Getenv("GH_FORCE_TTY"); spec != "" { - cmdFactory.IOStreams.ForceTerminal(spec) - } if !cmdFactory.IOStreams.ColorEnabled() { surveyCore.DisableColor = true } else { diff --git a/go.mod b/go.mod index 08d4c245b..e2cbf9a23 100644 --- a/go.mod +++ b/go.mod @@ -25,7 +25,6 @@ require ( github.com/mattn/go-colorable v0.1.13 github.com/mattn/go-isatty v0.0.16 github.com/mgutz/ansi v0.0.0-20200706080929-d51e80ef957d - github.com/muesli/termenv v0.12.0 github.com/muhammadmuzzammil1998/jsonc v0.0.0-20201229145248-615b0916ca38 github.com/opentracing/opentracing-go v1.1.0 github.com/shurcooL/githubv4 v0.0.0-20220520033151-0b4e3294ff00 @@ -35,7 +34,6 @@ require ( github.com/stretchr/testify v1.7.5 golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 golang.org/x/sync v0.0.0-20210220032951-036812b2e83c - golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 golang.org/x/text v0.3.7 gopkg.in/yaml.v3 v3.0.1 @@ -59,6 +57,7 @@ require ( github.com/mattn/go-runewidth v0.0.13 // indirect github.com/microcosm-cc/bluemonday v1.0.19 // indirect github.com/muesli/reflow v0.3.0 // indirect + github.com/muesli/termenv v0.12.0 // indirect github.com/olekukonko/tablewriter v0.0.5 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/rivo/uniseg v0.2.0 // indirect @@ -70,6 +69,7 @@ require ( github.com/yuin/goldmark-emoji v1.0.1 // indirect golang.org/x/net v0.0.0-20220722155237-a158d28d115b // indirect golang.org/x/oauth2 v0.0.0-20220309155454-6242fa91716a // indirect + golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab // indirect gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 // indirect ) diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index e2c2c1c9c..1d35c05d9 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -2,7 +2,6 @@ package iostreams import ( "fmt" - "os" "strconv" "strings" @@ -25,30 +24,6 @@ var ( } ) -func EnvColorDisabled() bool { - return os.Getenv("NO_COLOR") != "" || os.Getenv("CLICOLOR") == "0" -} - -func EnvColorForced() bool { - return os.Getenv("CLICOLOR_FORCE") != "" && os.Getenv("CLICOLOR_FORCE") != "0" -} - -func Is256ColorSupported() bool { - return IsTrueColorSupported() || - strings.Contains(os.Getenv("TERM"), "256") || - strings.Contains(os.Getenv("COLORTERM"), "256") -} - -func IsTrueColorSupported() bool { - term := os.Getenv("TERM") - colorterm := os.Getenv("COLORTERM") - - return strings.Contains(term, "24bit") || - strings.Contains(term, "truecolor") || - strings.Contains(colorterm, "24bit") || - strings.Contains(colorterm, "truecolor") -} - func NewColorScheme(enabled, is256enabled bool, trueColor bool) *ColorScheme { return &ColorScheme{ enabled: enabled, diff --git a/pkg/iostreams/color_test.go b/pkg/iostreams/color_test.go index e88fe79e3..59fea53ed 100644 --- a/pkg/iostreams/color_test.go +++ b/pkg/iostreams/color_test.go @@ -6,127 +6,6 @@ import ( "github.com/stretchr/testify/assert" ) -func TestEnvColorDisabled(t *testing.T) { - tests := []struct { - name string - NO_COLOR string - CLICOLOR string - CLICOLOR_FORCE string - want bool - }{ - { - name: "pristine env", - NO_COLOR: "", - CLICOLOR: "", - CLICOLOR_FORCE: "", - want: false, - }, - { - name: "NO_COLOR enabled", - NO_COLOR: "1", - CLICOLOR: "", - CLICOLOR_FORCE: "", - want: true, - }, - { - name: "CLICOLOR disabled", - NO_COLOR: "", - CLICOLOR: "0", - CLICOLOR_FORCE: "", - want: true, - }, - { - name: "CLICOLOR enabled", - NO_COLOR: "", - CLICOLOR: "1", - CLICOLOR_FORCE: "", - want: false, - }, - { - name: "CLICOLOR_FORCE has no effect", - NO_COLOR: "", - CLICOLOR: "", - CLICOLOR_FORCE: "1", - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Setenv("NO_COLOR", tt.NO_COLOR) - t.Setenv("CLICOLOR", tt.CLICOLOR) - t.Setenv("CLICOLOR_FORCE", tt.CLICOLOR_FORCE) - - if got := EnvColorDisabled(); got != tt.want { - t.Errorf("EnvColorDisabled(): want %v, got %v", tt.want, got) - } - }) - } -} - -func TestEnvColorForced(t *testing.T) { - tests := []struct { - name string - NO_COLOR string - CLICOLOR string - CLICOLOR_FORCE string - want bool - }{ - { - name: "pristine env", - NO_COLOR: "", - CLICOLOR: "", - CLICOLOR_FORCE: "", - want: false, - }, - { - name: "NO_COLOR enabled", - NO_COLOR: "1", - CLICOLOR: "", - CLICOLOR_FORCE: "", - want: false, - }, - { - name: "CLICOLOR disabled", - NO_COLOR: "", - CLICOLOR: "0", - CLICOLOR_FORCE: "", - want: false, - }, - { - name: "CLICOLOR enabled", - NO_COLOR: "", - CLICOLOR: "1", - CLICOLOR_FORCE: "", - want: false, - }, - { - name: "CLICOLOR_FORCE enabled", - NO_COLOR: "", - CLICOLOR: "", - CLICOLOR_FORCE: "1", - want: true, - }, - { - name: "CLICOLOR_FORCE disabled", - NO_COLOR: "", - CLICOLOR: "", - CLICOLOR_FORCE: "0", - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Setenv("NO_COLOR", tt.NO_COLOR) - t.Setenv("CLICOLOR", tt.CLICOLOR) - t.Setenv("CLICOLOR_FORCE", tt.CLICOLOR_FORCE) - - if got := EnvColorForced(); got != tt.want { - t.Errorf("EnvColorForced(): want %v, got %v", tt.want, got) - } - }) - } -} - func TestColorFromRGB(t *testing.T) { tests := []struct { name string diff --git a/pkg/iostreams/console.go b/pkg/iostreams/console.go index 73126787d..89bdd1daa 100644 --- a/pkg/iostreams/console.go +++ b/pkg/iostreams/console.go @@ -3,8 +3,9 @@ package iostreams -import "errors" +import "os" -func enableVirtualTerminalProcessing(fd uintptr) error { - return errors.New("not implemented") +func hasAlternateScreenBuffer(hasTrueColor bool) bool { + // on non-Windows, we just assume that alternate screen buffer is supported in most cases + return os.Getenv("TERM") != "dumb" } diff --git a/pkg/iostreams/console_windows.go b/pkg/iostreams/console_windows.go index 12f4e2850..ee0238e49 100644 --- a/pkg/iostreams/console_windows.go +++ b/pkg/iostreams/console_windows.go @@ -3,14 +3,8 @@ package iostreams -import ( - "golang.org/x/sys/windows" -) - -func enableVirtualTerminalProcessing(fd uintptr) error { - stdout := windows.Handle(fd) - - var originalMode uint32 - windows.GetConsoleMode(stdout, &originalMode) - return windows.SetConsoleMode(stdout, originalMode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING) +func hasAlternateScreenBuffer(hasTrueColor bool) bool { + // on Windows we just assume that alternate screen buffer is supported if we + // enabled virtual terminal processing, which in turn enables truecolor + return hasTrueColor } diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 1a6399ae8..892f17162 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -8,18 +8,16 @@ import ( "os" "os/exec" "os/signal" - "strconv" "strings" "sync" "time" "github.com/briandowns/spinner" + ghTerm "github.com/cli/go-gh/pkg/term" "github.com/cli/safeexec" "github.com/google/shlex" "github.com/mattn/go-colorable" "github.com/mattn/go-isatty" - "github.com/muesli/termenv" - "golang.org/x/term" ) const DefaultWidth = 80 @@ -40,13 +38,12 @@ type fileReader interface { } type IOStreams struct { + term ghTerm.Term + In fileReader Out fileWriter ErrOut io.Writer - colorEnabled bool - is256enabled bool - hasTrueColor bool terminalTheme string progressIndicatorEnabled bool @@ -63,8 +60,6 @@ type IOStreams struct { stdoutIsTTY bool stderrTTYOverride bool stderrIsTTY bool - termWidthOverride int - ttySize func() (int, int, error) pagerCommand string pagerProcess *os.Process @@ -75,42 +70,33 @@ type IOStreams struct { } func (s *IOStreams) ColorEnabled() bool { - return s.colorEnabled + return s.term.IsColorEnabled() } func (s *IOStreams) ColorSupport256() bool { - return s.is256enabled + return s.term.Is256ColorSupported() } func (s *IOStreams) HasTrueColor() bool { - return s.hasTrueColor + return s.term.IsTrueColorSupported() } // DetectTerminalTheme is a utility to call before starting the output pager so that the terminal background // can be reliably detected. func (s *IOStreams) DetectTerminalTheme() { - if !s.ColorEnabled() { - s.terminalTheme = "none" - return - } - - if s.pagerProcess != nil { + if !s.ColorEnabled() || s.pagerProcess != nil { s.terminalTheme = "none" return } style := os.Getenv("GLAMOUR_STYLE") if style != "" && style != "auto" { + // ensure GLAMOUR_STYLE takes precedence over "light" and "dark" themes s.terminalTheme = "none" return } - if termenv.HasDarkBackground() { - s.terminalTheme = "dark" - return - } - - s.terminalTheme = "light" + s.terminalTheme = s.term.Theme() } // TerminalTheme returns "light", "dark", or "none" depending on the background color of the terminal. @@ -123,7 +109,7 @@ func (s *IOStreams) TerminalTheme() string { } func (s *IOStreams) SetColorEnabled(colorEnabled bool) { - s.colorEnabled = colorEnabled + //s.colorEnabled = colorEnabled } func (s *IOStreams) SetStdinTTY(isTTY bool) { @@ -339,65 +325,13 @@ func (s *IOStreams) RefreshScreen() { } } -// TerminalWidth returns the width of the terminal that stdout is attached to. -// TODO: investigate whether ProcessTerminalWidth could replace all this. +// TerminalWidth returns the width of the terminal that controls the process func (s *IOStreams) TerminalWidth() int { - if s.termWidthOverride > 0 { - return s.termWidthOverride - } - - defaultWidth := DefaultWidth - - if w, _, err := terminalSize(s.Out.Fd()); err == nil { + w, _, err := s.term.Size() + if err == nil && w > 0 { return w } - - if isCygwinTerminal(s.Out.Fd()) { - tputExe, err := safeexec.LookPath("tput") - if err != nil { - return defaultWidth - } - tputCmd := exec.Command(tputExe, "cols") - tputCmd.Stdin = os.Stdin - if out, err := tputCmd.Output(); err == nil { - if w, err := strconv.Atoi(strings.TrimSpace(string(out))); err == nil { - return w - } - } - } - - return defaultWidth -} - -// ProcessTerminalWidth returns the width of the terminal that the process is attached to. -func (s *IOStreams) ProcessTerminalWidth() int { - w, _, err := s.ttySize() - if err != nil { - return DefaultWidth - } - return w -} - -func (s *IOStreams) ForceTerminal(spec string) { - s.colorEnabled = !EnvColorDisabled() - s.SetStdoutTTY(true) - - if w, err := strconv.Atoi(spec); err == nil { - s.termWidthOverride = w - return - } - - ttyWidth, _, err := s.ttySize() - if err != nil { - return - } - s.termWidthOverride = ttyWidth - - if strings.HasSuffix(spec, "%") { - if p, err := strconv.Atoi(spec[:len(spec)-1]); err == nil { - s.termWidthOverride = int(float64(s.termWidthOverride) * (float64(p) / 100)) - } - } + return DefaultWidth } func (s *IOStreams) ColorScheme() *ColorScheme { @@ -427,25 +361,20 @@ func (s *IOStreams) TempFile(dir, pattern string) (*os.File, error) { } func System() *IOStreams { + terminal := ghTerm.FromEnv() + + // we avoid using terminal.IsTerminalOutput() in order to additionally support cygwin stdoutIsTTY := isTerminal(os.Stdout) stderrIsTTY := isTerminal(os.Stderr) var stdout fileWriter = os.Stdout - isVirtualTerminal := false - if stdoutIsTTY { - // enables ANSI escape sequences on modern Windows - if err := enableVirtualTerminalProcessing(os.Stdout.Fd()); err == nil { - isVirtualTerminal = true - } else { - // as a fallback, translate ANSI escape sequences to Windows console syscalls - colorableStdout := colorable.NewColorable(os.Stdout) - if colorableStdout != os.Stdout { - // ensure that the file descriptor of the original stdout is preserved - stdout = &fdWriter{ - fd: os.Stdout.Fd(), - Writer: colorableStdout, - } - } + // On Windows with no virtual terminal processing support, translate ANSI escape + // sequences to console syscalls + if colorableStdout := colorable.NewColorable(os.Stdout); colorableStdout != os.Stdout { + // ensure that the file descriptor of the original stdout is preserved + stdout = &fdWriter{ + fd: os.Stdout.Fd(), + Writer: colorableStdout, } } @@ -453,18 +382,15 @@ func System() *IOStreams { In: os.Stdin, Out: stdout, ErrOut: colorable.NewColorable(os.Stderr), - colorEnabled: EnvColorForced() || (!EnvColorDisabled() && stdoutIsTTY), - is256enabled: isVirtualTerminal || Is256ColorSupported(), - hasTrueColor: isVirtualTerminal || IsTrueColorSupported(), pagerCommand: os.Getenv("PAGER"), - ttySize: ttySize, + term: terminal, } if stdoutIsTTY && stderrIsTTY { io.progressIndicatorEnabled = true } - if stdoutIsTTY && isVirtualTerminal { + if stdoutIsTTY && hasAlternateScreenBuffer(terminal.IsTrueColorSupported()) { io.alternateScreenBufferEnabled = true } @@ -485,9 +411,6 @@ func Test() (*IOStreams, *bytes.Buffer, *bytes.Buffer, *bytes.Buffer) { }, Out: &fdWriter{fd: 1, Writer: out}, ErrOut: errOut, - ttySize: func() (int, int, error) { - return -1, -1, errors.New("ttySize not implemented in tests") - }, } io.SetStdinTTY(false) io.SetStdoutTTY(false) @@ -496,18 +419,13 @@ func Test() (*IOStreams, *bytes.Buffer, *bytes.Buffer, *bytes.Buffer) { } func isTerminal(f *os.File) bool { - return isatty.IsTerminal(f.Fd()) || isatty.IsCygwinTerminal(f.Fd()) + return ghTerm.IsTerminal(f) || isCygwinTerminal(f.Fd()) } func isCygwinTerminal(fd uintptr) bool { return isatty.IsCygwinTerminal(fd) } -// terminalSize measures the viewport of the terminal that the output stream is connected to -func terminalSize(fd uintptr) (int, int, error) { - return term.GetSize(int(fd)) -} - // pagerWriter implements a WriteCloser that wraps all EPIPE errors in an ErrClosedPagerPipe type. type pagerWriter struct { io.WriteCloser diff --git a/pkg/iostreams/iostreams_test.go b/pkg/iostreams/iostreams_test.go index 772df9bf6..eefc07b9c 100644 --- a/pkg/iostreams/iostreams_test.go +++ b/pkg/iostreams/iostreams_test.go @@ -2,79 +2,11 @@ package iostreams import ( "bufio" - "errors" "fmt" - "io" "os" "testing" ) -func TestIOStreams_ForceTerminal(t *testing.T) { - tests := []struct { - name string - iostreams *IOStreams - arg string - wantTTY bool - wantWidth int - }{ - { - name: "explicit width", - iostreams: &IOStreams{}, - arg: "72", - wantTTY: true, - wantWidth: 72, - }, - { - name: "measure width", - iostreams: &IOStreams{ - ttySize: func() (int, int, error) { - return 72, 0, nil - }, - }, - arg: "true", - wantTTY: true, - wantWidth: 72, - }, - { - name: "measure width fails", - iostreams: &IOStreams{ - Out: &fdWriter{ - Writer: io.Discard, - fd: 1, - }, - ttySize: func() (int, int, error) { - return -1, -1, errors.New("ttySize sabotage!") - }, - }, - arg: "true", - wantTTY: true, - wantWidth: 80, - }, - { - name: "apply percentage", - iostreams: &IOStreams{ - ttySize: func() (int, int, error) { - return 72, 0, nil - }, - }, - arg: "50%", - wantTTY: true, - wantWidth: 36, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tt.iostreams.ForceTerminal(tt.arg) - if isTTY := tt.iostreams.IsStdoutTTY(); isTTY != tt.wantTTY { - t.Errorf("IOStreams.IsStdoutTTY() = %v, want %v", isTTY, tt.wantTTY) - } - if tw := tt.iostreams.TerminalWidth(); tw != tt.wantWidth { - t.Errorf("IOStreams.TerminalWidth() = %v, want %v", tw, tt.wantWidth) - } - }) - } -} - func TestStopAlternateScreenBuffer(t *testing.T) { ios, _, stdout, _ := Test() ios.SetAlternateScreenBufferEnabled(true) diff --git a/pkg/iostreams/tty_size.go b/pkg/iostreams/tty_size.go deleted file mode 100644 index c6cbd9c6f..000000000 --- a/pkg/iostreams/tty_size.go +++ /dev/null @@ -1,20 +0,0 @@ -//go:build !windows -// +build !windows - -package iostreams - -import ( - "os" - - "golang.org/x/term" -) - -// ttySize measures the size of the controlling terminal for the current process -func ttySize() (int, int, error) { - f, err := os.Open("/dev/tty") - if err != nil { - return -1, -1, err - } - defer f.Close() - return term.GetSize(int(f.Fd())) -} diff --git a/pkg/iostreams/tty_size_windows.go b/pkg/iostreams/tty_size_windows.go deleted file mode 100644 index 72fb8a7ba..000000000 --- a/pkg/iostreams/tty_size_windows.go +++ /dev/null @@ -1,16 +0,0 @@ -package iostreams - -import ( - "os" - - "golang.org/x/term" -) - -func ttySize() (int, int, error) { - f, err := os.Open("CONOUT$") - if err != nil { - return -1, -1, err - } - defer f.Close() - return term.GetSize(int(f.Fd())) -} diff --git a/utils/table_printer.go b/utils/table_printer.go index 2ccef943c..1dfd8ef25 100644 --- a/utils/table_printer.go +++ b/utils/table_printer.go @@ -40,10 +40,8 @@ func NewTablePrinterWithOptions(ios *iostreams.IOStreams, opts TablePrinterOptio var maxWidth int if opts.MaxWidth > 0 { maxWidth = opts.MaxWidth - } else if ios.IsStdoutTTY() { - maxWidth = ios.TerminalWidth() } else { - maxWidth = ios.ProcessTerminalWidth() + maxWidth = ios.TerminalWidth() } return &ttyTablePrinter{ out: out,