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

Fix Base16 default dark, Add Base16 default light, terminal, tty #1078

Closed
wants to merge 0 commits into from

Conversation

NNBnh
Copy link
Contributor

@NNBnh NNBnh commented Nov 11, 2021

  • Fix Base16 default dark:
    • Fix some typo.
    • Make the styling more accurate.
    • Improve contrast.
  • Add Base16 default light.
  • Add Base16 terminal & Base16 tty:
    • This is Base16 with Terminal color which could close Create an alternative default theme for 16 color terminals #791.
    • Some options with "white" foreground have a #FIXME comment at the end, that because the intent foreground color is "default" (which does not exist) not "white". If we remove that options, the color will not fall back to the default foreground terminal color, but turn into a strange blue color:

Current:

1

Remove options with #FIXME:

2

other blue UI is the picker:

2021-11-24

I'm currently running Helix 0.5.0.

@archseer
Copy link
Member

* f we remove that options, the color will not fall back to the default foreground terminal color, but turn into a strange blue color:

Hmm yeah, this was added because we normally always expect a style to be returned, this way if a theme color is missing it'll stand out.

.unwrap_or_else(|| Style::default().fg(Color::Rgb(0, 0, 255)))

Maybe we should only do that in debug builds?

@EpocSquadron
Copy link
Contributor

@archseer maybe it's better to add a "foreground" and "background" keyword similar to the ansi color names, rather than behaving differently depending on type of build.

@Omnikar
Copy link
Contributor

Omnikar commented Nov 14, 2021

This PR does not do this:

Embed it in the editor just like the default theme, and then, if the user doesn't have a theme configured, detect whether their terminal supports full color, and use the 16 color alternate default theme if it doesn't.

However, I was able to implement it here: NNBnh#1

@archseer
Copy link
Member

Oof, terminfo pulls in a lot of dependencies. term seems a bit more lightweight: https://docs.rs/term/0.7.0/term/terminfo/struct.TermInfo.html

@archseer
Copy link
Member

Both are unmaintained though. I wonder if we could just send a raw query

@EpocSquadron
Copy link
Contributor

@archseer a robust terminfo parser could be useful for further features though. I've been eyeing curly underline, double underline with separate colors from the text, like kakoune just added (I requested it there long ago). We would need to feature detect that with terminal.

@archseer
Copy link
Member

Yeah I was thinking we could copy over term::terminfo submodule and maintain it ourselves if necessary. It's still passively maintained upstream so we could rely on the crate for the time being.

@Omnikar
Copy link
Contributor

Omnikar commented Nov 14, 2021

Oof, terminfo pulls in a lot of dependencies. term seems a bit more lightweight: https://docs.rs/term/0.7.0/term/terminfo/struct.TermInfo.html

Hm, I'm not sure what to actually use to check for true color support?

@archseer
Copy link
Member

Let's check the COLORTERM env variable, as suggested by https://gist.github.com/XVilka/8346728#checking-for-colorterm

We will probably need to add a config option truecolor = true to force override on terminals that don't set this correctly.

@Omnikar
Copy link
Contributor

Omnikar commented Nov 29, 2021

OK, I've done that, and with the addition of the false negative override, I've also made it so that it will reject :theme invocations and theme = configurations that try to set an unsupported theme.

@archseer
Copy link
Member

archseer commented Dec 2, 2021

@NNBnh can you merge Omnikar's PR?

@archseer
Copy link
Member

archseer commented Dec 2, 2021

And please change

.unwrap_or_else(|| Style::default().fg(Color::Rgb(0, 0, 255)))

to be .unwrap_or_default(). That will fix the blue colors

@archseer
Copy link
Member

archseer commented Dec 2, 2021

Oh also note that base16_terminal is not a good default for 16 color terminals if the user isn't using a base16 theme. The default should use the regular ANSI palette where 0-7 are not shades of gray, but colors.

#833 (comment)

@NNBnh
Copy link
Contributor Author

NNBnh commented Dec 2, 2021

The default should use the regular ANSI palette where 0-7 are not shades of gray, but colors.

Yeah, I'm thinking or creating another theme base16_tty that display well on every terminal even Linux's tty ( Ctrl + Alt + F3 ... ).

After that i will figure it out how to merge with Omnikar's PR (I'm not pro enough with git and github).

@NNBnh NNBnh changed the title Fix Base16 default dark, Add Base16 default light and Base16 terminal Fix Base16 default dark, Add Base16 default light, terminal, tty Dec 2, 2021
@NNBnh
Copy link
Contributor Author

NNBnh commented Dec 6, 2021

Screenshot from 2021-12-06 15-19-59

All Base16 themes have problem on Helix 0.5: all line-number have an one character width gap where it just show the editor's background and not the line-number's background.

This gap it's not be created by the terminal, we can see that the statusline can still expand to that gap and the gap is also appear on split buffer.

It's this bug fixed on the current branch?

@archseer
Copy link
Member

archseer commented Dec 6, 2021

That's the diagnostics gutter.

@NNBnh
Copy link
Contributor Author

NNBnh commented Dec 7, 2021

Okay i fixed it, look like everything here is done! Please merge the branch before something get conflict.

Comment on lines 36 to 38
"info" = { fg = "base03", bg = "base01" }
"hint" = { fg = "base03", bg = "base01" }
"debug" = { fg = "base03", bg = "base01" }
Copy link
Member

Choose a reason for hiding this comment

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

Since all these info/debug/warning/error colors are used in places besides the gutter, I don't recommend setting bg. Instead a new UI scope should be added ui.gutter that will be used for all the gutters including line numbers. You can add the styling here and I'll patch it in later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay changed to:

"ui.gutter" = { bg = "black" }
"info" = "gray"
"hint" = "gray"
"debug" = "gray"
"warning" = "yellow"
"error" = "red"

Is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

Let's make info light blue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed info:

  • base16_default_dark.toml: base0D
  • base16_default_light.toml: base0D
  • base16_terminal.toml: light-blue
  • base16_theme.toml: blue (instead of light-blue because for compatibility for all terminal, the default theme should not contain light variant color)

@archseer
Copy link
Member

I incorrectly pushed this.. will merge directly into master

@archseer
Copy link
Member

Pushed! 43d17c4..730d3be

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.

Create an alternative default theme for 16 color terminals
4 participants