Use light variant of theme automatically when terminal background is light#1843
Use light variant of theme automatically when terminal background is light#1843rashil2000 wants to merge 6 commits intoClementTsang:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1843 +/- ##
===========================================
- Coverage 53.95% 35.84% -18.11%
===========================================
Files 115 115
Lines 16096 16121 +25
===========================================
- Hits 8684 5778 -2906
- Misses 7412 10343 +2931
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The CI seems to be failing in a file unrelated to my changes, should I fix it? |
|
Ugh, that's my bad. I think it's fine, it's because the new stable rust means I'm failing a clippy check in the main branch. I'll push a fix to main later. |
|
I see. And should I add tests for this as well? Codecov seems to be complaining. |
|
If you could add a test that might be nice, but it might be tricky to add one? Especially for CI, it might make sense to skip it there (or I guess ensure it handles it as expected if the terminal is a CI one). I'll leave that call up to you I suppose. |
|
Will review in a bit (probably sometime in the next few days, currently travelling). Just fixed |
|
Okay sure, no rush |
ClementTsang
left a comment
ClementTsang
left a comment
There was a problem hiding this comment.
Overall this looks fine, only thing to change for sure is please don't change the v0.9 schema.
My main outstanding question I guess is if we should still have a way to override the automatic detection (or skip it, same difference). I would prefer to still have a way to force a dark/light mode setting, though how that might look, I'm open for ideas.
| # - "default-light" | ||
| # - "gruvbox" | ||
| # - "gruvbox-light" | ||
| # - "nord" | ||
| # - "nord-light". |
There was a problem hiding this comment.
Hm, my gut reaction is that I'm not a huge fan of not being able to override the automatic settings, though I'm also not sure how certain interactions should behave in that case.
There was a problem hiding this comment.
The (ugly?) workarounds off the top of my head to support this are either:
- Add
-darkvariants to mirror-light, and ignore auto-detection for those. - Add a flag to explicitly set light/dark modes for existing themes, though I'm not a huge fan of having another flag for this unless necessary.
There was a problem hiding this comment.
(Open to any other suggestions/opinions)
There was a problem hiding this comment.
I had a few ideas in mind, but all of them involve some amount of breaking changes:
- we could keep the light variants as is: if the user has explicitly specified a light variant, we pick it, if not, we do detection. This would still confuse users if on the off chance they decide to launch
bottomon a light terminal background and expect the dark variant to show up. - Add corresponding separate dark variants (as you mentioned) - if variants are specified explicitly, we don't do detection, and if variants are not specified (just the name is specified), we do detection. This will cause a similar gotcha as above (although that's still a very niche edge case).
I wanted an implementation as clean as possible without confusing existing users.
There is an argument that can be made for allowing users to override the theme - which is, the auto-detection for some terminal is so bad that they have to specify the light variant. I'm not sure how probable this is though.
| "default-light", | ||
| "gruvbox", | ||
| "gruvbox-light", | ||
| "nord", | ||
| "nord-light" |
There was a problem hiding this comment.
Please don't change this file, this is the schema for an old release so this doesn't really make sense, since these changes wouldn't apply to that version.
If you're going to update the schema, run the schema updating script ./scripts/schema/generate.sh to update nightly's schema automatically. We can then cut the nightly schema for a stable release when necessary.
There was a problem hiding this comment.
Sorry, my bad. I have reverted the change.
I tried running ./scripts/schema/generate.sh - it ran successfully but didn't make any changes in the schema/nightly/bottom.json file.
| None => match config.styles.as_ref().and_then(|s| s.theme.as_ref()) { | ||
| Some(theme) => Self::from_theme(theme)?, | ||
| None => Self::default(), | ||
| None => Self::from_theme("default")?, |
There was a problem hiding this comment.
nit: Any reason to change this line?
There was a problem hiding this comment.
Self::default() here will skip automatic detection if the user has no theme specified in the config file.
2 participants
Description
This PR adds the ability to detect the background color of the terminal, using the terminal-colorsaurus library. This library has extensive support for terminals, and is pretty quick at detecting the color or failing gracefully. Popular tools such as delta and bat also use this.
Issue
Closes: #1284
Testing
If relevant, please state how this was tested. All changes must be tested to work:
If this is a code change, please also indicate which platforms were tested:
Checklist
If relevant, ensure the following have been met:
cargo fmt)README.md, help menu, doc pages, etc.)