-
Notifications
You must be signed in to change notification settings - Fork 42
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
Theme name in settings #33
Conversation
Currently the `theme_index` is always set to `0` on startup, regardless of what is in the configuration file. `theme_index` is loaded on startup however so apply it when creating the editor component.
Add a new property (`theme_name`) to the configuration file to contain the name of the theme. This replaces `theme_id` which contained the numeric index of the theme. e.g. theme_name = "base16-material" versus theme_id = 15
zee/src/editor/mod.rs
Outdated
let mut theme_index = 0; | ||
for (i, (_, name)) in THEMES.iter().enumerate() { | ||
if *name == properties.settings.theme_name { | ||
theme_index = i; | ||
break; | ||
} | ||
} |
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.
This would be clearer with iterator combinators, in particular using position -- additionally, it'd avoid the use of mut
and make the default case when the theme name cannot be found more explicit (e.g. .unwrap_or(0)
)
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.
Great suggestion, I wasn't aware of that API. Updated as suggested.
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.
Thank you for the PR and apologies for being so slow. Happy with the PR, with one suggested change to use iterator combinators rather than a for
loop with break
For context though -- (I also mentioned in #32) that the settings file will soon change
The "settings file" is really half-baked, as you can see there's not a lot to configure right now. I am working on changing this in #29 and making more things configurable (e.g. what tree sitter parsers are support, modes, key bindings etc.). In the meantime, we should get this fix in.
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.
Looks good, thanks for the changes!
Add a new property (
theme_name
) to the configuration file to contain the name of the theme. This replacestheme_id
which contained the numeric index of the theme.e.g.
theme_name = "base16-material"
versus
theme_id = 15
This pull request also contains the fix from #32