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.

Sign up for GitHub

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

Reload language config with :config-reload #5239

Merged
merged 5 commits into from
Dec 29, 2022
Merged

Reload language config with :config-reload #5239

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4451435
language config reloading added
willful759 Dec 21, 2022
b323f5a
function `refresh_language` renamed to `refresh_language_config`
willful759 Dec 22, 2022
b0254d2
`refresh_language_config` factored out of `refresh_theme`
willful759 Dec 22, 2022
4b16a7b
theme and language config reloading logic updated to use `Result`
willful759 Dec 27, 2022
523b830
truecolor support error message refactored to use `anyhow::bail`
willful759 Dec 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 34 additions & 23 deletions helix-term/src/application.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,38 +393,49 @@ impl Application {
self.editor.refresh_config();
}

/// refresh language config after config change
fn refresh_language_config(&mut self) -> Result<(), Error> {
let syntax_config = helix_core::config::user_syntax_loader()
.map_err(|err| anyhow::anyhow!("Failed to load language config: {}", err))?;

self.syn_loader = std::sync::Arc::new(syntax::Loader::new(syntax_config));
for document in self.editor.documents.values_mut() {
document.detect_language(self.syn_loader.clone());
}

Ok(())
}

/// Refresh theme after config change
fn refresh_theme(&mut self, config: &Config) {
fn refresh_theme(&mut self, config: &Config) -> Result<(), Error> {
if let Some(theme) = config.theme.clone() {
let true_color = self.true_color();
match self.theme_loader.load(&theme) {
Ok(theme) => {
if true_color || theme.is_16_color() {
self.editor.set_theme(theme);
} else {
self.editor
.set_error("theme requires truecolor support, which is not available");
}
}
Err(err) => {
let err_string = format!("failed to load theme `{}` - {}", theme, err);
self.editor.set_error(err_string);
}
let theme = self
.theme_loader
.load(&theme)
.map_err(|err| anyhow::anyhow!("Failed to load theme `{}`: {}", theme, err))?;

if true_color || theme.is_16_color() {
self.editor.set_theme(theme);
} else {
anyhow::bail!("theme requires truecolor support, which is not available")
}
}

Ok(())
}

fn refresh_config(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the refresh_config, refresh_language_config and refresh_theme commands could use some refactoring to improve the error handling. With this change, if you have an error in your language config and a theme error, you only end up seeing the theme error since it overwrites the editor.set_error from refresh_language_config

refresh_language_config and refresh_theme can return Result<(), Error>s instead and then use the anyhow helpers like anyhow::anyhow! or anyhow::bail! to format the error messages and return early in cases of errors. Then refresh_config can set the editor error if there are any failures:

fn refresh_config(&mut self) {
    let mut refresh_config = || -> Result<(), Error> {
        let config = Config::load_default()
            .map_err(|err| anyhow::anyhow!("failed to load config: {}", err))?;
        self.refresh_language_config()?;
        self.refresh_theme(&config)?;

        // Store new config
        self.config.store(Arc::new(config));

        Ok(())
    };
    if let Err(err) = refresh_config() {
        self.editor.set_error(err.to_string());
    }
}

And the other functions can use ? to return errors early and remove the duplicated error handling code:

fn refresh_language_config(&mut self) -> Result<(), Error> {
    let syntax_config = helix_core::config::user_syntax_loader()?;
    self.syn_loader = // ...
    // ...
    Ok(())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Liked the suggestion, code looks a lot cleaner now.

I'm new to rust so recommendations like this are always welcomed

match Config::load_default() {
Ok(config) => {
self.refresh_theme(&config);
let mut refresh_config = || -> Result<(), Error> {
let default_config = Config::load_default()
.map_err(|err| anyhow::anyhow!("Failed to load config: {}", err))?;
self.refresh_language_config()?;
self.refresh_theme(&default_config)?;
Ok(())
};

// Store new config
self.config.store(Arc::new(config));
}
Err(err) => {
self.editor.set_error(err.to_string());
}
if let Err(err) = refresh_config() {
self.editor.set_error(err.to_string());
}
}

Expand Down