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

The ability to define global theme overrides #5839

Closed
wants to merge 9 commits into from
Closed

The ability to define global theme overrides #5839

wants to merge 9 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 5, 2023

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

@ghost ghost changed the title The ability to define a global theme override The ability to define global theme overrides Feb 5, 2023
@pascalkuthe
Copy link
Member

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

[theme.overwrite]
# Theme overwrites here

@pascalkuthe pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much A-theme Area: Theme and appearence related S-waiting-on-review Status: Awaiting review from a maintainer. C-enhancement Category: Improvements labels Feb 5, 2023
@sudormrfbin
Copy link
Member

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

@ghost
Copy link
Author

ghost commented Feb 5, 2023

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.

@pascalkuthe
Copy link
Member

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.

@ghost
Copy link
Author

ghost commented Feb 5, 2023

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?

@pascalkuthe
Copy link
Member

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.

@sudormrfbin
Copy link
Member

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.

@ghost ghost closed this Feb 5, 2023
@ghost ghost reopened this Feb 5, 2023
@ghost
Copy link
Author

ghost commented Feb 7, 2023

@pascalkuthe these changes capture what I meant to do

@ghost ghost closed this May 25, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Theme and appearence related C-enhancement Category: Improvements E-easy Call for participation: Experience needed to fix: Easy / not much S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants