-
Notifications
You must be signed in to change notification settings - Fork 332
Use light variant of theme automatically when terminal background is light #1843
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
d864efe
af085d1
1a81043
a9355d5
414b9a5
0e151f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -175,11 +175,8 @@ | |
| "default": "default", | ||
| "enum": [ | ||
| "default", | ||
| "default-light", | ||
| "gruvbox", | ||
| "gruvbox-light", | ||
| "nord", | ||
| "nord-light" | ||
|
Comment on lines
-178
to
-182
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, my bad. I have reverted the change. I tried running |
||
| "nord" | ||
| ], | ||
| "description": "Built-in themes", | ||
| "type": "string" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ use memory::MemoryStyle; | |
| use network::NetworkStyle; | ||
| use serde::{Deserialize, Serialize}; | ||
| use tables::TableStyle; | ||
| use terminal_colorsaurus::{QueryOptions, ThemeMode, theme_mode}; | ||
| use tui::{style::Style, widgets::BorderType}; | ||
| use utils::{opt, set_colour, set_colour_list, set_style}; | ||
| use widgets::WidgetStyle; | ||
|
|
@@ -141,7 +142,7 @@ impl Styles { | |
| Some(theme) => Self::from_theme(theme)?, | ||
| 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")?, | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Any reason to change this line?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }, | ||
| }; | ||
|
|
||
|
|
@@ -155,13 +156,29 @@ impl Styles { | |
|
|
||
| fn from_theme(theme: &str) -> anyhow::Result<Self> { | ||
| let lower_case = theme.to_lowercase(); | ||
|
|
||
| // Detect terminal theme mode. | ||
| // Prefer dark palette if detection fails. | ||
| let is_dark = theme_mode(QueryOptions::default()) | ||
| .map(|mode| mode == ThemeMode::Dark) | ||
| .unwrap_or(true); | ||
|
|
||
| match lower_case.as_str() { | ||
| "default" => Ok(Self::default_style()), | ||
| "default-light" => Ok(Self::default_light_mode()), | ||
| "gruvbox" => Ok(Self::gruvbox_palette()), | ||
| "gruvbox-light" => Ok(Self::gruvbox_light_palette()), | ||
| "nord" => Ok(Self::nord_palette()), | ||
| "nord-light" => Ok(Self::nord_light_palette()), | ||
| "default" => Ok(if is_dark { | ||
| Self::default_style() | ||
| } else { | ||
| Self::default_light_mode() | ||
| }), | ||
| "gruvbox" => Ok(if is_dark { | ||
| Self::gruvbox_palette() | ||
| } else { | ||
| Self::gruvbox_light_palette() | ||
| }), | ||
| "nord" => Ok(if is_dark { | ||
| Self::nord_palette() | ||
| } else { | ||
| Self::nord_light_palette() | ||
| }), | ||
| _ => Err( | ||
| OptionError::other(format!("'{theme}' is an invalid built-in color scheme.")) | ||
| .into(), | ||
|
|
@@ -270,10 +287,41 @@ mod test { | |
| #[test] | ||
| fn built_in_colour_schemes_work() { | ||
| Styles::from_theme("default").unwrap(); | ||
| Styles::from_theme("default-light").unwrap(); | ||
| Styles::from_theme("gruvbox").unwrap(); | ||
| Styles::from_theme("gruvbox-light").unwrap(); | ||
| Styles::from_theme("nord").unwrap(); | ||
| Styles::from_theme("nord-light").unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn invalid_theme_returns_error() { | ||
| let result = Styles::from_theme("nonexistent-theme"); | ||
| assert!(result.is_err()); | ||
| assert!( | ||
| result | ||
| .unwrap_err() | ||
| .to_string() | ||
| .contains("invalid built-in color scheme") | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn theme_case_insensitive() { | ||
| // Theme names should work regardless of case | ||
| Styles::from_theme("DEFAULT").unwrap(); | ||
| Styles::from_theme("GruvBox").unwrap(); | ||
| Styles::from_theme("NORD").unwrap(); | ||
| } | ||
|
|
||
| #[test] | ||
| fn theme_detection_does_not_panic() { | ||
| // Ensure theme detection doesn't panic in non-interactive environments (CI) | ||
| // This verifies the .unwrap_or(true) fallback works correctly | ||
| let default_style = Styles::from_theme("default").unwrap(); | ||
| let gruvbox_style = Styles::from_theme("gruvbox").unwrap(); | ||
| let nord_style = Styles::from_theme("nord").unwrap(); | ||
|
|
||
| // Verify styles have expected color properties set (not all default) | ||
| assert_ne!(default_style.ram_style, Style::default()); | ||
| assert_ne!(gruvbox_style.ram_style, Style::default()); | ||
| assert_ne!(nord_style.ram_style, Style::default()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The (ugly?) workarounds off the top of my head to support this are either:
-darkvariants to mirror-light, and ignore auto-detection for those.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Open to any other suggestions/opinions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few ideas in mind, but all of them involve some amount of breaking changes:
bottomon a light terminal background and expect the dark variant to show up.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.