From 346fab212b177386de12beb8a6dc0f7406b96b88 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 31 Mar 2025 11:19:51 -0400 Subject: [PATCH 1/7] Accessible color config boilerplate, colors update This commit is focused on incorporating cli/go-gh accessible colors configuration setting into GitHub CLI experience along with new color function to supersede ColorScheme.Gray and ColorScheme.Grayf. Originally, I was considering having all use of ColorScheme.Gray and ColorScheme.Grayf fallback to the new muted logic if accessible colors were enabled, however I decided not being that it exceeds the acceptance criteria. This means that every command using ColorScheme.Gray needs to be updated to use ColorScheme.Muted --- internal/config/config.go | 18 ++++++++++++++ internal/gh/gh.go | 2 ++ internal/gh/mock/config.go | 44 ++++++++++++++++++++++++++++++++++ pkg/cmd/factory/default.go | 3 +++ pkg/iostreams/color.go | 47 ++++++++++++++++++++++++++++++------- pkg/iostreams/color_test.go | 36 ++++++++++++++-------------- pkg/iostreams/iostreams.go | 15 +++++++++--- 7 files changed, 135 insertions(+), 30 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 476154d66..51116df6c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -12,9 +12,11 @@ import ( o "github.com/cli/cli/v2/pkg/option" ghauth "github.com/cli/go-gh/v2/pkg/auth" ghConfig "github.com/cli/go-gh/v2/pkg/config" + xcolor "github.com/cli/go-gh/v2/pkg/x/color" ) const ( + accessibleColorsKey = xcolor.AccessibleColorsSetting aliasesKey = "aliases" browserKey = "browser" editorKey = "editor" @@ -108,6 +110,11 @@ func (c *cfg) Authentication() gh.AuthConfig { return &AuthConfig{cfg: c.cfg} } +func (c *cfg) AccessibleColors(hostname string) gh.ConfigEntry { + // Intentionally panic if there is no user provided value or default value (which would be a programmer error) + return c.GetOrDefault(hostname, accessibleColorsKey).Unwrap() +} + func (c *cfg) Browser(hostname string) gh.ConfigEntry { // Intentionally panic if there is no user provided value or default value (which would be a programmer error) return c.GetOrDefault(hostname, browserKey).Unwrap() @@ -532,6 +539,8 @@ aliases: http_unix_socket: # What web browser gh should use when opening URLs. If blank, will refer to environment. browser: +# Preference for accessible colors that can be customized. This is a global config that cannot be overridden by hostname. Supported values: enabled, disabled +accessible_colors: disabled ` type ConfigOption struct { @@ -602,6 +611,15 @@ var Options = []ConfigOption{ return c.Browser(hostname).Value }, }, + { + Key: accessibleColorsKey, + Description: "toggle preference for accessible colors that can be customized", + DefaultValue: "disabled", + AllowedValues: []string{"enabled", "disabled"}, + CurrentValue: func(c gh.Config, hostname string) string { + return c.AccessibleColors(hostname).Value + }, + }, } func HomeDirPath(subdir string) (string, error) { diff --git a/internal/gh/gh.go b/internal/gh/gh.go index 8e640c41a..4f3ece204 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -35,6 +35,8 @@ type Config interface { // Set provides primitive access for setting configuration values, optionally scoped by host. Set(hostname string, key string, value string) + // AccessibleColors returns the configured accessible colors preference, optionally scoped by host. + AccessibleColors(hostname string) ConfigEntry // Browser returns the configured browser, optionally scoped by host. Browser(hostname string) ConfigEntry // Editor returns the configured editor, optionally scoped by host. diff --git a/internal/gh/mock/config.go b/internal/gh/mock/config.go index 569af1fac..ecb8814b8 100644 --- a/internal/gh/mock/config.go +++ b/internal/gh/mock/config.go @@ -19,6 +19,9 @@ var _ gh.Config = &ConfigMock{} // // // make and configure a mocked gh.Config // mockedConfig := &ConfigMock{ +// AccessibleColorsFunc: func(hostname string) gh.ConfigEntry { +// panic("mock out the AccessibleColors method") +// }, // AliasesFunc: func() gh.AliasConfig { // panic("mock out the Aliases method") // }, @@ -71,6 +74,9 @@ var _ gh.Config = &ConfigMock{} // // } type ConfigMock struct { + // AccessibleColorsFunc mocks the AccessibleColors method. + AccessibleColorsFunc func(hostname string) gh.ConfigEntry + // AliasesFunc mocks the Aliases method. AliasesFunc func() gh.AliasConfig @@ -118,6 +124,11 @@ type ConfigMock struct { // calls tracks calls to the methods. calls struct { + // AccessibleColors holds details about calls to the AccessibleColors method. + AccessibleColors []struct { + // Hostname is the hostname argument value. + Hostname string + } // Aliases holds details about calls to the Aliases method. Aliases []struct { } @@ -190,6 +201,7 @@ type ConfigMock struct { Write []struct { } } + lockAccessibleColors sync.RWMutex lockAliases sync.RWMutex lockAuthentication sync.RWMutex lockBrowser sync.RWMutex @@ -207,6 +219,38 @@ type ConfigMock struct { lockWrite sync.RWMutex } +// AccessibleColors calls AccessibleColorsFunc. +func (mock *ConfigMock) AccessibleColors(hostname string) gh.ConfigEntry { + if mock.AccessibleColorsFunc == nil { + panic("ConfigMock.AccessibleColorsFunc: method is nil but Config.AccessibleColors was just called") + } + callInfo := struct { + Hostname string + }{ + Hostname: hostname, + } + mock.lockAccessibleColors.Lock() + mock.calls.AccessibleColors = append(mock.calls.AccessibleColors, callInfo) + mock.lockAccessibleColors.Unlock() + return mock.AccessibleColorsFunc(hostname) +} + +// AccessibleColorsCalls gets all the calls that were made to AccessibleColors. +// Check the length with: +// +// len(mockedConfig.AccessibleColorsCalls()) +func (mock *ConfigMock) AccessibleColorsCalls() []struct { + Hostname string +} { + var calls []struct { + Hostname string + } + mock.lockAccessibleColors.RLock() + calls = mock.calls.AccessibleColors + mock.lockAccessibleColors.RUnlock() + return calls +} + // Aliases calls AliasesFunc. func (mock *ConfigMock) Aliases() gh.AliasConfig { if mock.AliasesFunc == nil { diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 923651487..598be0747 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -19,6 +19,7 @@ import ( "github.com/cli/cli/v2/pkg/cmd/extension" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + xcolor "github.com/cli/go-gh/v2/pkg/x/color" ) var ssoHeader string @@ -292,6 +293,8 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io.SetPager(pager.Value) } + io.SetAccessibleColorsEnabled(xcolor.IsAccessibleColorsEnabled()) + return io } diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 07fdcd79f..3e2d08ac2 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -30,7 +30,9 @@ var ( greenBold = ansi.ColorFunc("green+b") highlightStart = ansi.ColorCode(highlightStyle) highlight = ansi.ColorFunc(highlightStyle) + darkThemeMuted = ansi.ColorFunc("white+d") darkThemeTableHeader = ansi.ColorFunc("white+du") + lightThemeMuted = ansi.ColorFunc("black+h") lightThemeTableHeader = ansi.ColorFunc("black+hu") noThemeTableHeader = ansi.ColorFunc("default+u") @@ -42,20 +44,22 @@ var ( // NewColorScheme initializes color logic based on provided terminal capabilities. // Logic dealing with terminal theme detected, such as whether color is enabled, 8-bit color supported, true color supported, // and terminal theme detected. -func NewColorScheme(enabled, is256enabled, trueColor bool, theme string) *ColorScheme { +func NewColorScheme(enabled, is256enabled, trueColor, accessibleColors bool, theme string) *ColorScheme { return &ColorScheme{ - enabled: enabled, - is256enabled: is256enabled, - hasTrueColor: trueColor, - theme: theme, + enabled: enabled, + is256enabled: is256enabled, + hasTrueColor: trueColor, + accessibleColors: accessibleColors, + theme: theme, } } type ColorScheme struct { - enabled bool - is256enabled bool - hasTrueColor bool - theme string + enabled bool + is256enabled bool + hasTrueColor bool + accessibleColors bool + theme string } func (c *ColorScheme) Enabled() bool { @@ -73,6 +77,29 @@ func (c *ColorScheme) Boldf(t string, args ...interface{}) string { return c.Bold(fmt.Sprintf(t, args...)) } +func (c *ColorScheme) Muted(t string) string { + // Fallback to previous logic if accessible colors preview is disabled. + if !c.accessibleColors { + return c.Gray(t) + } + + // Muted text is only stylized if color is enabled. + if !c.enabled { + return t + } + + switch c.theme { + case LightTheme: + return lightThemeMuted(t) + default: + return darkThemeMuted(t) + } +} + +func (c *ColorScheme) Mutedf(t string, args ...interface{}) string { + return c.Muted(fmt.Sprintf(t, args...)) +} + func (c *ColorScheme) Red(t string) string { if !c.enabled { return t @@ -113,6 +140,7 @@ func (c *ColorScheme) GreenBold(t string) string { return greenBold(t) } +// Deprecated: Use Muted instead for thematically contrasting color. func (c *ColorScheme) Gray(t string) string { if !c.enabled { return t @@ -123,6 +151,7 @@ func (c *ColorScheme) Gray(t string) string { return gray(t) } +// Deprecated: Use Mutedf instead for thematically contrasting color. func (c *ColorScheme) Grayf(t string, args ...interface{}) string { return c.Gray(fmt.Sprintf(t, args...)) } diff --git a/pkg/iostreams/color_test.go b/pkg/iostreams/color_test.go index b35c2eb73..a399e6d58 100644 --- a/pkg/iostreams/color_test.go +++ b/pkg/iostreams/color_test.go @@ -20,28 +20,28 @@ func TestColorFromRGB(t *testing.T) { hex: "fc0303", text: "red", wants: "\033[38;2;252;3;3mred\033[0m", - cs: NewColorScheme(true, true, true, NoTheme), + cs: NewColorScheme(true, true, true, false, NoTheme), }, { name: "no truecolor", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(true, true, false, NoTheme), + cs: NewColorScheme(true, true, false, false, NoTheme), }, { name: "no color", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(false, false, false, false, NoTheme), }, { name: "invalid hex", hex: "fc0", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(false, false, false, false, NoTheme), }, } @@ -64,28 +64,28 @@ func TestHexToRGB(t *testing.T) { hex: "fc0303", text: "red", wants: "\033[38;2;252;3;3mred\033[0m", - cs: NewColorScheme(true, true, true, NoTheme), + cs: NewColorScheme(true, true, true, false, NoTheme), }, { name: "no truecolor", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(true, true, false, NoTheme), + cs: NewColorScheme(true, true, false, false, NoTheme), }, { name: "no color", hex: "fc0303", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(false, false, false, false, NoTheme), }, { name: "invalid hex", hex: "fc0", text: "red", wants: "red", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(false, false, false, false, NoTheme), }, } @@ -109,61 +109,61 @@ func TestTableHeader(t *testing.T) { }{ { name: "when color is disabled, text is not stylized", - cs: NewColorScheme(false, false, false, NoTheme), + cs: NewColorScheme(false, false, false, true, NoTheme), input: "this should not be stylized", expected: "this should not be stylized", }, { name: "when 4-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, false, false, NoTheme), + cs: NewColorScheme(true, false, false, true, NoTheme), input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { name: "when 4-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, false, false, LightTheme), + cs: NewColorScheme(true, false, false, true, LightTheme), input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { name: "when 4-bit color is enabled and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, false, false, DarkTheme), + cs: NewColorScheme(true, false, false, true, DarkTheme), input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, { name: "when 8-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, true, false, NoTheme), + cs: NewColorScheme(true, true, false, true, NoTheme), input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { name: "when 8-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, true, false, LightTheme), + cs: NewColorScheme(true, true, false, true, LightTheme), input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { name: "when 8-bit color is true and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, true, false, DarkTheme), + cs: NewColorScheme(true, true, false, true, DarkTheme), input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, { name: "when 24-bit color is enabled but no theme, 4-bit default color and underline are used", - cs: NewColorScheme(true, true, true, NoTheme), + cs: NewColorScheme(true, true, true, true, NoTheme), input: "this should have no explicit color but underlined", expected: fmt.Sprintf("%sthis should have no explicit color but underlined%s", defaultUnderline, reset), }, { name: "when 24-bit color is enabled and theme is light, 4-bit dark color and underline are used", - cs: NewColorScheme(true, true, true, LightTheme), + cs: NewColorScheme(true, true, true, true, LightTheme), input: "this should have dark foreground color and underlined", expected: fmt.Sprintf("%sthis should have dark foreground color and underlined%s", brightBlackUnderline, reset), }, { name: "when 24-bit color is true and theme is dark, 4-bit light color and underline are used", - cs: NewColorScheme(true, true, true, DarkTheme), + cs: NewColorScheme(true, true, true, true, DarkTheme), input: "this should have light foreground color and underlined", expected: fmt.Sprintf("%sthis should have light foreground color and underlined%s", dimBlackUnderline, reset), }, diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 6c12d7911..30981386b 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -70,8 +70,9 @@ type IOStreams struct { stderrTTYOverride bool stderrIsTTY bool - colorOverride bool - colorEnabled bool + colorOverride bool + colorEnabled bool + accessibleColorsEnabled bool pagerCommand string pagerProcess *os.Process @@ -366,7 +367,7 @@ func (s *IOStreams) TerminalWidth() int { } func (s *IOStreams) ColorScheme() *ColorScheme { - return NewColorScheme(s.ColorEnabled(), s.ColorSupport256(), s.HasTrueColor(), s.TerminalTheme()) + return NewColorScheme(s.ColorEnabled(), s.ColorSupport256(), s.HasTrueColor(), s.AccessibleColorsEnabled(), s.TerminalTheme()) } func (s *IOStreams) ReadUserFile(fn string) ([]byte, error) { @@ -391,6 +392,14 @@ func (s *IOStreams) TempFile(dir, pattern string) (*os.File, error) { return os.CreateTemp(dir, pattern) } +func (s *IOStreams) SetAccessibleColorsEnabled(enabled bool) { + s.accessibleColorsEnabled = enabled +} + +func (s *IOStreams) AccessibleColorsEnabled() bool { + return s.accessibleColorsEnabled +} + func System() *IOStreams { terminal := ghTerm.FromEnv() From d2cd14b4cdb32d5edf97294dac9a462672450924 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 31 Mar 2025 11:58:30 -0400 Subject: [PATCH 2/7] Remove out of scope changes, update list commands After discussing this with the team, the `gh config` changes to display `accessible_colors` have been removed from this branch being outside of acceptance criteria. This will be moved to a separate issue along with any other work needed to finalize the public preview such as `gh help` entries for `GH_ACCESSIBLE_COLORS` environment variable. List commands that use ColorScheme.Gray have been updated to use ColorScheme.Muted. --- internal/config/config.go | 14 -------------- internal/gh/gh.go | 2 -- pkg/cmd/cache/list/list.go | 9 +++++---- pkg/cmd/codespace/list.go | 4 ++-- pkg/cmd/gist/list/list.go | 2 +- pkg/cmd/gpg-key/list/list.go | 4 ++-- pkg/cmd/pr/list/list.go | 2 +- pkg/cmd/release/list/list.go | 10 +++++----- pkg/cmd/repo/deploy-key/list/list.go | 2 +- pkg/cmd/repo/list/list.go | 4 ++-- pkg/cmd/run/list/list.go | 2 +- pkg/cmd/ssh-key/list/list.go | 4 ++-- 12 files changed, 22 insertions(+), 37 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 51116df6c..a3849b9af 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -110,11 +110,6 @@ func (c *cfg) Authentication() gh.AuthConfig { return &AuthConfig{cfg: c.cfg} } -func (c *cfg) AccessibleColors(hostname string) gh.ConfigEntry { - // Intentionally panic if there is no user provided value or default value (which would be a programmer error) - return c.GetOrDefault(hostname, accessibleColorsKey).Unwrap() -} - func (c *cfg) Browser(hostname string) gh.ConfigEntry { // Intentionally panic if there is no user provided value or default value (which would be a programmer error) return c.GetOrDefault(hostname, browserKey).Unwrap() @@ -611,15 +606,6 @@ var Options = []ConfigOption{ return c.Browser(hostname).Value }, }, - { - Key: accessibleColorsKey, - Description: "toggle preference for accessible colors that can be customized", - DefaultValue: "disabled", - AllowedValues: []string{"enabled", "disabled"}, - CurrentValue: func(c gh.Config, hostname string) string { - return c.AccessibleColors(hostname).Value - }, - }, } func HomeDirPath(subdir string) (string, error) { diff --git a/internal/gh/gh.go b/internal/gh/gh.go index 4f3ece204..8e640c41a 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -35,8 +35,6 @@ type Config interface { // Set provides primitive access for setting configuration values, optionally scoped by host. Set(hostname string, key string, value string) - // AccessibleColors returns the configured accessible colors preference, optionally scoped by host. - AccessibleColors(hostname string) ConfigEntry // Browser returns the configured browser, optionally scoped by host. Browser(hostname string) ConfigEntry // Editor returns the configured editor, optionally scoped by host. diff --git a/pkg/cmd/cache/list/list.go b/pkg/cmd/cache/list/list.go index 9f39876cb..d699e2c3d 100644 --- a/pkg/cmd/cache/list/list.go +++ b/pkg/cmd/cache/list/list.go @@ -99,11 +99,12 @@ func listRun(opts *ListOptions) error { } client := api.NewClientFromHTTP(httpClient) + cs := opts.IO.ColorScheme() opts.IO.StartProgressIndicator() result, err := shared.GetCaches(client, repo, shared.GetCachesOptions{Limit: opts.Limit, Sort: opts.Sort, Order: opts.Order, Key: opts.Key, Ref: opts.Ref}) opts.IO.StopProgressIndicator() if err != nil { - return fmt.Errorf("%s Failed to get caches: %w", opts.IO.ColorScheme().FailureIcon(), err) + return fmt.Errorf("%s Failed to get caches: %w", cs.FailureIcon(), err) } if len(result.ActionsCaches) == 0 && opts.Exporter == nil { @@ -130,11 +131,11 @@ func listRun(opts *ListOptions) error { tp := tableprinter.New(opts.IO, tableprinter.WithHeader("ID", "KEY", "SIZE", "CREATED", "ACCESSED")) for _, cache := range result.ActionsCaches { - tp.AddField(opts.IO.ColorScheme().Cyan(fmt.Sprintf("%d", cache.Id))) + tp.AddField(cs.Cyanf("%d", cache.Id)) tp.AddField(cache.Key) tp.AddField(humanFileSize(cache.SizeInBytes)) - tp.AddTimeField(opts.Now, cache.CreatedAt, opts.IO.ColorScheme().Gray) - tp.AddTimeField(opts.Now, cache.LastAccessedAt, opts.IO.ColorScheme().Gray) + tp.AddTimeField(opts.Now, cache.CreatedAt, cs.Muted) + tp.AddTimeField(opts.Now, cache.LastAccessedAt, cs.Muted) tp.EndRow() } diff --git a/pkg/cmd/codespace/list.go b/pkg/cmd/codespace/list.go index c1acccabb..238c4a6a7 100644 --- a/pkg/cmd/codespace/list.go +++ b/pkg/cmd/codespace/list.go @@ -152,7 +152,7 @@ func (a *App) List(ctx context.Context, opts *listOptions, exporter cmdutil.Expo case false: nameColor = cs.Yellow case true: - nameColor = cs.Gray + nameColor = cs.Muted } tp.AddField(formattedName, tableprinter.WithColor(nameColor)) @@ -172,7 +172,7 @@ func (a *App) List(ctx context.Context, opts *listOptions, exporter cmdutil.Expo if err != nil { return fmt.Errorf("error parsing date %q: %w", c.CreatedAt, err) } - tp.AddTimeField(time.Now(), ct, cs.Gray) + tp.AddTimeField(time.Now(), ct, cs.Muted) if hasNonProdVSCSTarget { tp.AddField(c.VSCSTarget) diff --git a/pkg/cmd/gist/list/list.go b/pkg/cmd/gist/list/list.go index d46d4da02..a4fd245f6 100644 --- a/pkg/cmd/gist/list/list.go +++ b/pkg/cmd/gist/list/list.go @@ -206,7 +206,7 @@ func printTable(io *iostreams.IOStreams, gists []shared.Gist, filter *regexp.Reg tableprinter.WithColor(highlightFilesFunc(&gist)), ) tp.AddField(visibility, tableprinter.WithColor(visColor)) - tp.AddTimeField(time.Now(), gist.UpdatedAt, cs.Gray) + tp.AddTimeField(time.Now(), gist.UpdatedAt, cs.Muted) tp.EndRow() } diff --git a/pkg/cmd/gpg-key/list/list.go b/pkg/cmd/gpg-key/list/list.go index 9acf1d7b6..4244ba349 100644 --- a/pkg/cmd/gpg-key/list/list.go +++ b/pkg/cmd/gpg-key/list/list.go @@ -78,7 +78,7 @@ func listRun(opts *ListOptions) error { t.AddField(gpgKey.Emails.String()) t.AddField(gpgKey.KeyID) t.AddField(gpgKey.PublicKey, tableprinter.WithTruncate(truncateMiddle)) - t.AddTimeField(now, gpgKey.CreatedAt, cs.Gray) + t.AddTimeField(now, gpgKey.CreatedAt, cs.Muted) expiresAt := gpgKey.ExpiresAt.Format(time.RFC3339) if t.IsTTY() { if gpgKey.ExpiresAt.IsZero() { @@ -87,7 +87,7 @@ func listRun(opts *ListOptions) error { expiresAt = gpgKey.ExpiresAt.Format("2006-01-02") } } - t.AddField(expiresAt, tableprinter.WithColor(cs.Gray)) + t.AddField(expiresAt, tableprinter.WithColor(cs.Muted)) t.EndRow() } diff --git a/pkg/cmd/pr/list/list.go b/pkg/cmd/pr/list/list.go index ee708f161..0b86d5e11 100644 --- a/pkg/cmd/pr/list/list.go +++ b/pkg/cmd/pr/list/list.go @@ -224,7 +224,7 @@ func listRun(opts *ListOptions) error { if !isTTY { table.AddField(shared.PrStateWithDraft(&pr)) } - table.AddTimeField(opts.Now(), pr.CreatedAt, cs.Gray) + table.AddTimeField(opts.Now(), pr.CreatedAt, cs.Muted) table.EndRow() } err = table.Render() diff --git a/pkg/cmd/release/list/list.go b/pkg/cmd/release/list/list.go index 24ec41840..dfc3ba8c1 100644 --- a/pkg/cmd/release/list/list.go +++ b/pkg/cmd/release/list/list.go @@ -88,7 +88,7 @@ func listRun(opts *ListOptions) error { } table := tableprinter.New(opts.IO, tableprinter.WithHeader("Title", "Type", "Tag name", "Published")) - iofmt := opts.IO.ColorScheme() + cs := opts.IO.ColorScheme() for _, rel := range releases { title := text.RemoveExcessiveWhitespace(rel.Name) if title == "" { @@ -100,13 +100,13 @@ func listRun(opts *ListOptions) error { var badgeColor func(string) string if rel.IsLatest { badge = "Latest" - badgeColor = iofmt.Green + badgeColor = cs.Green } else if rel.IsDraft { badge = "Draft" - badgeColor = iofmt.Red + badgeColor = cs.Red } else if rel.IsPrerelease { badge = "Pre-release" - badgeColor = iofmt.Yellow + badgeColor = cs.Yellow } table.AddField(badge, tableprinter.WithColor(badgeColor)) @@ -116,7 +116,7 @@ func listRun(opts *ListOptions) error { if rel.PublishedAt.IsZero() { pubDate = rel.CreatedAt } - table.AddTimeField(time.Now(), pubDate, iofmt.Gray) + table.AddTimeField(time.Now(), pubDate, cs.Muted) table.EndRow() } err = table.Render() diff --git a/pkg/cmd/repo/deploy-key/list/list.go b/pkg/cmd/repo/deploy-key/list/list.go index 0cf2861c1..79d00c912 100644 --- a/pkg/cmd/repo/deploy-key/list/list.go +++ b/pkg/cmd/repo/deploy-key/list/list.go @@ -90,7 +90,7 @@ func listRun(opts *ListOptions) error { } t.AddField(sshType) t.AddField(deployKey.Key, tableprinter.WithTruncate(truncateMiddle)) - t.AddTimeField(now, deployKey.CreatedAt, cs.Gray) + t.AddTimeField(now, deployKey.CreatedAt, cs.Muted) t.EndRow() } diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index 2cee2c480..116439290 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -181,7 +181,7 @@ func listRun(opts *ListOptions) error { totalMatchCount := len(listResult.Repositories) for _, repo := range listResult.Repositories { info := repoInfo(repo) - infoColor := cs.Gray + infoColor := cs.Muted if repo.IsPrivate { infoColor = cs.Yellow @@ -195,7 +195,7 @@ func listRun(opts *ListOptions) error { tp.AddField(repo.NameWithOwner, tableprinter.WithColor(cs.Bold)) tp.AddField(text.RemoveExcessiveWhitespace(repo.Description)) tp.AddField(info, tableprinter.WithColor(infoColor)) - tp.AddTimeField(opts.Now(), *t, cs.Gray) + tp.AddTimeField(opts.Now(), *t, cs.Muted) tp.EndRow() } diff --git a/pkg/cmd/run/list/list.go b/pkg/cmd/run/list/list.go index 7b18c391d..7128ae253 100644 --- a/pkg/cmd/run/list/list.go +++ b/pkg/cmd/run/list/list.go @@ -176,7 +176,7 @@ func listRun(opts *ListOptions) error { tp.AddField(string(run.Event)) tp.AddField(fmt.Sprintf("%d", run.ID), tableprinter.WithColor(cs.Cyan)) tp.AddField(run.Duration(opts.now).String()) - tp.AddTimeField(opts.now, run.StartedTime(), cs.Gray) + tp.AddTimeField(opts.now, run.StartedTime(), cs.Muted) tp.EndRow() } diff --git a/pkg/cmd/ssh-key/list/list.go b/pkg/cmd/ssh-key/list/list.go index eebc82f90..f69e57ea3 100644 --- a/pkg/cmd/ssh-key/list/list.go +++ b/pkg/cmd/ssh-key/list/list.go @@ -89,11 +89,11 @@ func listRun(opts *ListOptions) error { t.AddField(id) t.AddField(sshKey.Key, tableprinter.WithTruncate(truncateMiddle)) t.AddField(sshKey.Type) - t.AddTimeField(now, sshKey.CreatedAt, cs.Gray) + t.AddTimeField(now, sshKey.CreatedAt, cs.Muted) } else { t.AddField(sshKey.Title) t.AddField(sshKey.Key) - t.AddTimeField(now, sshKey.CreatedAt, cs.Gray) + t.AddTimeField(now, sshKey.CreatedAt, cs.Muted) t.AddField(id) t.AddField(sshKey.Type) } From 3c38cedaf3949018cbcbab63fc7c280481e39fa0 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 31 Mar 2025 15:33:52 -0400 Subject: [PATCH 3/7] Implement tests for muted logic, standardize reset This commit covers testing around the new ColorScheme.Muted logic based on various situations to gain confidence we get the accessible colors expected when enabled. Additionally, this commit includes a small change to the existing 8-bit color logic to standardize on the same reset sequence for testing purposes. Essentially, `ESC[m` and `ESC[0m` are equivalent but this inconsistency with our other libraries makes setting up tests a little extra confusing and difficult. --- pkg/cmd/gist/list/list_test.go | 6 ++-- pkg/iostreams/color.go | 6 ++-- pkg/iostreams/color_test.go | 64 ++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/gist/list/list_test.go b/pkg/cmd/gist/list/list_test.go index 4f6c8a9f7..ad6d8cdb5 100644 --- a/pkg/cmd/gist/list/list_test.go +++ b/pkg/cmd/gist/list/list_test.go @@ -487,8 +487,8 @@ func Test_listRun(t *testing.T) { }, wantOut: heredoc.Docf(` %[1]s[0;4;39mID %[1]s[0m %[1]s[0;4;39mDESCRIPTION %[1]s[0m %[1]s[0;4;39mFILES %[1]s[0m %[1]s[0;4;39mVISIBILITY%[1]s[0m %[1]s[0;4;39mUPDATED %[1]s[0m - 1234 %[1]s[0;30;43mocto%[1]s[0m%[1]s[0;1;39m match in the description%[1]s[0m 1 file %[1]s[0;32mpublic %[1]s[0m %[1]s[38;5;242mabout 6 hours ago%[1]s[m - 2345 %[1]s[0;1;39mmatch in the file name %[1]s[0m %[1]s[0;30;43m2 files%[1]s[0m %[1]s[0;31msecret %[1]s[0m %[1]s[38;5;242mabout 6 hours ago%[1]s[m + 1234 %[1]s[0;30;43mocto%[1]s[0m%[1]s[0;1;39m match in the description%[1]s[0m 1 file %[1]s[0;32mpublic %[1]s[0m %[1]s[38;5;242mabout 6 hours ago%[1]s[0m + 2345 %[1]s[0;1;39mmatch in the file name %[1]s[0m %[1]s[0;30;43m2 files%[1]s[0m %[1]s[0;31msecret %[1]s[0m %[1]s[38;5;242mabout 6 hours ago%[1]s[0m `, "\x1b"), }, { @@ -694,7 +694,7 @@ func Test_highlightMatch(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cs := iostreams.NewColorScheme(tt.color, false, false, iostreams.NoTheme) + cs := iostreams.NewColorScheme(tt.color, false, false, false, iostreams.NoTheme) matched := false got, err := highlightMatch(tt.input, regex, &matched, cs.Blue, cs.Highlight) diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 3e2d08ac2..9e6c4f7ca 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -37,7 +37,7 @@ var ( noThemeTableHeader = ansi.ColorFunc("default+u") gray256 = func(t string) string { - return fmt.Sprintf("\x1b[%d;5;%dm%s\x1b[m", 38, 242, t) + return fmt.Sprintf("\x1b[%d;5;%dm%s\x1b[0m", 38, 242, t) } ) @@ -91,8 +91,10 @@ func (c *ColorScheme) Muted(t string) string { switch c.theme { case LightTheme: return lightThemeMuted(t) - default: + case DarkTheme: return darkThemeMuted(t) + default: + return t } } diff --git a/pkg/iostreams/color_test.go b/pkg/iostreams/color_test.go index a399e6d58..b0cf994e3 100644 --- a/pkg/iostreams/color_test.go +++ b/pkg/iostreams/color_test.go @@ -175,3 +175,67 @@ func TestTableHeader(t *testing.T) { }) } } + +func TestMuted(t *testing.T) { + reset := "\x1b[0m" + gray4bit := "\x1b[0;90m" + gray8bit := "\x1b[38;5;242m" + brightBlack4bit := "\x1b[0;90m" + dimBlack4bit := "\x1b[0;2;37m" + + tests := []struct { + name string + cs *ColorScheme + input string + expected string + }{ + { + name: "when color is disabled but accessible colors are disabled, text is not stylized", + cs: NewColorScheme(false, false, false, false, NoTheme), + input: "this should not be stylized", + expected: "this should not be stylized", + }, + { + name: "when 4-bit color is enabled but accessible colors are disabled, legacy 4-bit gray color is used", + cs: NewColorScheme(true, false, false, false, NoTheme), + input: "this should be 4-bit gray", + expected: fmt.Sprintf("%sthis should be 4-bit gray%s", gray4bit, reset), + }, + { + name: "when 8-bit color is enabled but accessible colors are disabled, legacy 8-bit gray color is used", + cs: NewColorScheme(true, true, false, false, NoTheme), + input: "this should be 8-bit gray", + expected: fmt.Sprintf("%sthis should be 8-bit gray%s", gray8bit, reset), + }, + { + name: "when 24-bit color is enabled but accessible colors are disabled, legacy 8-bit gray color is used", + cs: NewColorScheme(true, true, true, false, NoTheme), + input: "this should be 8-bit gray", + expected: fmt.Sprintf("%sthis should be 8-bit gray%s", gray8bit, reset), + }, + { + name: "when 4-bit color is enabled and theme is dark, 4-bit light color is used", + cs: NewColorScheme(true, true, true, true, DarkTheme), + input: "this should be 4-bit dim black", + expected: fmt.Sprintf("%sthis should be 4-bit dim black%s", dimBlack4bit, reset), + }, + { + name: "when 4-bit color is enabled and theme is light, 4-bit dark color is used", + cs: NewColorScheme(true, true, true, true, LightTheme), + input: "this should be 4-bit bright black", + expected: fmt.Sprintf("%sthis should be 4-bit bright black%s", brightBlack4bit, reset), + }, + { + name: "when 4-bit color is enabled but no theme, 4-bit default color is used", + cs: NewColorScheme(true, true, true, true, NoTheme), + input: "this should have no explicit color", + expected: "this should have no explicit color", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.cs.Muted(tt.input)) + }) + } +} From de86cc6c150d1ec969f935bf85b3ae8a7046f42f Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 31 Mar 2025 15:57:37 -0400 Subject: [PATCH 4/7] Remove out of scope configuration setting change --- internal/config/config.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index a3849b9af..350163cba 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -534,8 +534,6 @@ aliases: http_unix_socket: # What web browser gh should use when opening URLs. If blank, will refer to environment. browser: -# Preference for accessible colors that can be customized. This is a global config that cannot be overridden by hostname. Supported values: enabled, disabled -accessible_colors: disabled ` type ConfigOption struct { From 1bf1ad282b5bbdbe8c3ebac55e8632cfc2cf5d09 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 31 Mar 2025 15:58:37 -0400 Subject: [PATCH 5/7] Remove configuration setting vestage --- internal/config/config.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 350163cba..476154d66 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -12,11 +12,9 @@ import ( o "github.com/cli/cli/v2/pkg/option" ghauth "github.com/cli/go-gh/v2/pkg/auth" ghConfig "github.com/cli/go-gh/v2/pkg/config" - xcolor "github.com/cli/go-gh/v2/pkg/x/color" ) const ( - accessibleColorsKey = xcolor.AccessibleColorsSetting aliasesKey = "aliases" browserKey = "browser" editorKey = "editor" From e36d795629ba1e3f6a51fdb6a0718037a08b4d0b Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 31 Mar 2025 15:59:18 -0400 Subject: [PATCH 6/7] Regenerate mocks --- internal/gh/mock/config.go | 44 -------------------------------------- 1 file changed, 44 deletions(-) diff --git a/internal/gh/mock/config.go b/internal/gh/mock/config.go index ecb8814b8..569af1fac 100644 --- a/internal/gh/mock/config.go +++ b/internal/gh/mock/config.go @@ -19,9 +19,6 @@ var _ gh.Config = &ConfigMock{} // // // make and configure a mocked gh.Config // mockedConfig := &ConfigMock{ -// AccessibleColorsFunc: func(hostname string) gh.ConfigEntry { -// panic("mock out the AccessibleColors method") -// }, // AliasesFunc: func() gh.AliasConfig { // panic("mock out the Aliases method") // }, @@ -74,9 +71,6 @@ var _ gh.Config = &ConfigMock{} // // } type ConfigMock struct { - // AccessibleColorsFunc mocks the AccessibleColors method. - AccessibleColorsFunc func(hostname string) gh.ConfigEntry - // AliasesFunc mocks the Aliases method. AliasesFunc func() gh.AliasConfig @@ -124,11 +118,6 @@ type ConfigMock struct { // calls tracks calls to the methods. calls struct { - // AccessibleColors holds details about calls to the AccessibleColors method. - AccessibleColors []struct { - // Hostname is the hostname argument value. - Hostname string - } // Aliases holds details about calls to the Aliases method. Aliases []struct { } @@ -201,7 +190,6 @@ type ConfigMock struct { Write []struct { } } - lockAccessibleColors sync.RWMutex lockAliases sync.RWMutex lockAuthentication sync.RWMutex lockBrowser sync.RWMutex @@ -219,38 +207,6 @@ type ConfigMock struct { lockWrite sync.RWMutex } -// AccessibleColors calls AccessibleColorsFunc. -func (mock *ConfigMock) AccessibleColors(hostname string) gh.ConfigEntry { - if mock.AccessibleColorsFunc == nil { - panic("ConfigMock.AccessibleColorsFunc: method is nil but Config.AccessibleColors was just called") - } - callInfo := struct { - Hostname string - }{ - Hostname: hostname, - } - mock.lockAccessibleColors.Lock() - mock.calls.AccessibleColors = append(mock.calls.AccessibleColors, callInfo) - mock.lockAccessibleColors.Unlock() - return mock.AccessibleColorsFunc(hostname) -} - -// AccessibleColorsCalls gets all the calls that were made to AccessibleColors. -// Check the length with: -// -// len(mockedConfig.AccessibleColorsCalls()) -func (mock *ConfigMock) AccessibleColorsCalls() []struct { - Hostname string -} { - var calls []struct { - Hostname string - } - mock.lockAccessibleColors.RLock() - calls = mock.calls.AccessibleColors - mock.lockAccessibleColors.RUnlock() - return calls -} - // Aliases calls AliasesFunc. func (mock *ConfigMock) Aliases() gh.AliasConfig { if mock.AliasesFunc == nil { From b3c8c70cba824ea4d0ded0c47fcb88838d58a5e8 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 31 Mar 2025 17:00:40 -0400 Subject: [PATCH 7/7] Remove deprecated note on gray color functions Without fixing all ColorScheme.Gray and ColorScheme.Grayf usage in this pull request, golangci-lint throws errors for using deprecated functions. As that code should be replaced within github/cli#833, I'm removing the deprecation indicator for now to get this PR passing. --- pkg/iostreams/color.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 9e6c4f7ca..58d13e6ef 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -142,7 +142,7 @@ func (c *ColorScheme) GreenBold(t string) string { return greenBold(t) } -// Deprecated: Use Muted instead for thematically contrasting color. +// Use Muted instead for thematically contrasting color. func (c *ColorScheme) Gray(t string) string { if !c.enabled { return t @@ -153,7 +153,7 @@ func (c *ColorScheme) Gray(t string) string { return gray(t) } -// Deprecated: Use Mutedf instead for thematically contrasting color. +// Use Mutedf instead for thematically contrasting color. func (c *ColorScheme) Grayf(t string, args ...interface{}) string { return c.Gray(fmt.Sprintf(t, args...)) }