helix icon indicating copy to clipboard operation
helix copied to clipboard

The ability to define global theme overrides

Open Veranorooz opened this issue 2 years ago • 8 comments

Currently, making changes to a theme is only possible by inheritance or editing the theme directly. Both of these options require a per-theme approach. But overrides such as "using the terminal's background" usually are meant for any theme. This makes it annoying to change themes after doing some alterations to the previous one.

~~This PR add the option to create a theme_overrides.toml file in the config directory and apply the changes to any theme currently active.~~

This PR adds the option to use a theme table in config.toml that has theme.overrides option. The keys specified here will override the active theme.

  • The changes are backwards compatible.
  • The overrides will persist while changing themes with :theme.
  • theme.overrides table is internally the same as a theme so any subkeys a theme can have, overrides can also have.

Example:

[theme]
name = "github_dark"

[theme.overrides]
"ui.background" = {}
"ui.cursor" = { fg = "dark", bg = "light" }
"ui.statusline" = { bg = {} }
"ui.statusline.normal" = { fg = "dark", bg = "light" }
"ui.statusline.insert" = { fg = "dark", bg = "light" }
"ui.statusline.select" = { fg = "dark", bg = "light" }
"ui.statusline.separator" = { fg = "light", bg = "dark" }

[theme.overrides.palette]
dark = "#000000"
light = "#E9E0C9"

A somewhat related issue

Veranorooz avatar Feb 05 '23 16:02 Veranorooz

Instead of a seperate config file could this not be subkeys in config.toml, for example:

[theme.overwrite]
# Theme overwrites here

pascalkuthe avatar Feb 05 '23 16:02 pascalkuthe

Exactly my thoughts as well, but theme would then be a toml map and I think we lose the ability to do theme = "onedark".

sudormrfbin avatar Feb 05 '23 16:02 sudormrfbin

Instead of a seperate config file could this not be subkeys in config.toml, for example:

[theme.overwrite]
# Theme overwrites here

@pascalkuthe wouldn't it add unnecessary overhead to the configuration file if a user wants to make too many overrides? That was my thinking.

ghost avatar Feb 05 '23 16:02 ghost

Exactly my thoughts as well, but theme would then be a toml map and I think we lose the ability to do theme = "onedark".

Ad damm I always forget about that. I hate that aspect of toml even if that makes it a lot easier to deal with. You could do soo many cool things like this. Maybe we could just allow a hybrid. Either allow the current theme = "onedark" or make it a map:

[theme]
name = "onedark"
[theme.overwrite]
# Theme overwrites here

Might be confusing tough so perhaps just adding a separate theme_overwrite map might be better.

[theme_overwrite]
# Theme overwrites here

Not sure which of those two options are better.

Instead of a seperate config file could this not be subkeys in config.toml, for example:

[theme.overwrite]
# Theme overwrites here

@pascalkuthe wouldn't it add unnecessary overhead to the configuration file if a user wants to make too many overrides? That was my thinking.

I think a global overwrite like this is only useful for a few themekeys like the background and maybe the cursor. Most other overwrites would be theme specifc (it would look if all themes used the same color for certain syntax highlights) so I don't think that would be a problem.

I would be very cautious about adding new config files.

pascalkuthe avatar Feb 05 '23 16:02 pascalkuthe

I think a global overwrite like this is only useful for a few themekeys like the background and maybe the cursor. Most other overwrites would be theme specifc (it would look if all themes used the same color for certain syntax highlights) so I don't think that would be a problem.

That is correct and my use case as well.

I would be very cautious about adding new config files.

I think including this in the config is the better choice anyway but for my reference, what is the potential issue with adding new config files?

Regarding how to define in the config, first option seems more extendable and structured. However, wouldn't it break existing configs?

ghost avatar Feb 05 '23 16:02 ghost

I think including this in the config is the better choice anyway but for my reference, what is the potential issue with adding new config files?

Primarily, If we keep adding new config files, the config becomes more spread-out and harder to find what you are looking for. There is also an effort to add per-workspace configs (#5748) and each new file needs to be discovered with multiple fallback paths etc.

Regarding how to define in the config, first option seems more extendable and structured. However, wouldn't it break existing configs?

As I said we would allow both options for backwards compatibility. We check if theme is a string and treat it as we do now or we check if theme is a table and interpret as I showed above. We have avoided breaking changes to the config multiple times with schemes like this.

It's a bit confusing that you would need to change the way the theme name is specified if you want to overwrite something that's why I also offered the second alternative but we do stuff like this already for other settings too so it's probably fine.

pascalkuthe avatar Feb 05 '23 17:02 pascalkuthe

As I said we would allow both options for backwards compatibility. We check if theme is a string and treat it as we do now or we check if theme is a table and interpret as I showed above. We have avoided breaking changes to the config multiple times with schemes like this.

This is quite common, and serde even has a doc page on the subject: https://serde.rs/string-or-struct.html.

sudormrfbin avatar Feb 05 '23 17:02 sudormrfbin

@pascalkuthe these changes capture what I meant to do

ghost avatar Feb 07 '23 12:02 ghost