Skip to content

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

Merged
alecthomas merged 3 commits intoalecthomas:masterfrom
walles:johan/indexed-none
Oct 15, 2023
Merged

Make tty_indexed.go respond to None like tty_truecolour.go#869
alecthomas merged 3 commits intoalecthomas:masterfrom
walles:johan/indexed-none

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/moor#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 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.

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/moor 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
@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.

2 participants