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

Allow for loading a single default theme #478

Open
CosmicHorrorDev opened this issue May 23, 2023 · 2 comments · May be fixed by #480
Open

Allow for loading a single default theme #478

CosmicHorrorDev opened this issue May 23, 2023 · 2 comments · May be fixed by #480

Comments

@CosmicHorrorDev
Copy link
Contributor

Motivation

I have a project where we allow users to pick one of the builtin syntect themes as the syntax highlighter. Only one theme will be used for the whole execution of the program, so to get that theme we have some code that's something along the lines of

use syntect::highlighting::{Theme as SyntectTheme, ThemeSet as SyntectThemeSet};

/// These represent the theme that the user selects
pub enum ThemeDefaults {
    Base16OceanDark,
    Base16EightiesDark,
    Base16MochaDark,
    Base16OceanLight,
    InspiredGithub,
    SolarizedDark,
    SolarizedLight,
}

impl ThemeDefaults {
    pub fn as_syntect_name(self) -> &'static str {
        match self {
            Self::Base16OceanDark => "base16-ocean.dark",
            Self::Base16EightiesDark => "base16-eighties.dark",
            Self::Base16MochaDark => "base16-mocha.dark",
            Self::Base16OceanLight => "base16-ocean.light",
            Self::InspiredGithub => "InspiredGitHub",
            Self::SolarizedDark => "Solarized (dark)",
            Self::SolarizedLight => "Solarized (light)",
        }
    }
}

impl From<ThemeDefaults> for SyntectTheme {
    fn from(default: ThemeDefaults) -> Self {
        let mut default_themes = SyntectThemeSet::load_defaults();
        default_themes
            .themes
            .remove(default.as_syntect_name())
            .expect("Included with defaults")
    }
}

It would be nice if syntect exposed a way to do something like the above with a Theme::load_default(ThemeDefaults) method that allowed for loading a single theme. I see that the current implementation compresses all of the themes together which makes sense for trying to reduce binary size. This means that loading a single theme will still have to decompress and deserialize everything, but this should still be more ergonomic when the user only wants to load a single theme (which I would assume is the common case)

I'd be happy to open a PR if this seems like a reasonable addition

@CosmicHorrorDev CosmicHorrorDev linked a pull request May 29, 2023 that will close this issue
@Enselic
Copy link
Collaborator

Enselic commented Jul 4, 2023

In bat we already load themes lazily, see https://github.com/sharkdp/bat/blob/8676bbf97f2832ad2231e102ca9c9b7b72267fda/src/assets/lazy_theme_set.rs. Maybe you can do something similar? The default theme set in syntect is deliberately not maintained, so I'm not sure it makes sense to make it more "usable".

@CosmicHorrorDev
Copy link
Contributor Author

I feel like the performance notes I talked about are a bit of a red herring here. I like the idea of a LazyThemeSet and would be glad if syntect adopted it officially (since I already have roughly the same implementation copied in another one of my projects), but this issue is more about API ergonomics. I mentioned performance to note that this wasn't for a performance improvement

All I really want is an enum that enumerates all of the values contained in the default theme set since right now it's up to library consumers manually checking all of the keys to figure out what themes are actually in the set

The default theme set in syntect is deliberately not maintained, so I'm not sure it makes sense to make it more "usable".

Can you elaborate on what you mean here? This addresses the main pain-point I had when dealing with the default theme set where the included themes are pretty opaque

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 a pull request may close this issue.

2 participants