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

Make tty_indexed.go respond to None like tty_truecolour.go #869

Merged
merged 3 commits into from Oct 15, 2023

Conversation

walles
Copy link
Contributor

@walles walles commented Oct 12, 2023

Before this change, asking tty_indexed.go for the None type ("No highlighting") returned an empty string.

With this change in place, asking tty_indexed.go for the None type returns (the closest it can get to) the same colour as tty_truecolor.go.

Relates to walles/moar#161 (comment).

@walles walles changed the title johan/indexed none Make tty_indexed.go respond to None like tty_truecolour.go Oct 12, 2023
@walles walles marked this pull request as ready for review October 12, 2023 08:31
Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thanks.

)

func TestClosestColour(t *testing.T) {
actual := findClosest(ttyTables[256], chroma.MustParseColour("#e06c75"))
assert.Equal(t, chroma.MustParseColour("#d75f87"), actual)
}

func TestNoneColour(t *testing.T) {
style := styles.Registry["gruvbox"]
Copy link
Owner

Choose a reason for hiding this comment

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

Let's not rely on an existing style for the test. Instead, create a style specifically for this test using the StyleBuilder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if !ok {
clr, ok = theme[token.Type.SubCategory()]
if !ok {
clr = theme[token.Type.Category()]
clr, ok = theme[token.Type.Category()]
Copy link
Owner

Choose a reason for hiding this comment

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

Seems like a reasonable change to me, thanks 👍

Use a custom style for the test.

Color value cred goes here:
https://www.litscape.com/word_tools/contains_only.php
tokenType := chroma.None

style, err := chroma.NewStyle("test", chroma.StyleEntries{
chroma.Background: "#D0ab1e",
Copy link
Owner

Choose a reason for hiding this comment

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

😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

walles added a commit to walles/moar that referenced this pull request Oct 14, 2023
Before this change, it only worked on 16M color terminals.

After there's a Chroma release with
alecthomas/chroma#869 in it we should revert
back to "None" here and use the new Chroma release instead.
@alecthomas alecthomas merged commit b127e35 into alecthomas:master Oct 15, 2023
2 checks passed
@walles walles deleted the johan/indexed-none branch November 14, 2023 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants