Add ability to switch themes #267

vv9k · 2021-06-14T19:09:21Z

I know the scope of this PR might be a bit too wide so I'm very much open to agressively adjusting the goal. In the process of adding the ability to switch themes using a command I went into a pretty deep hole of static variables accessed from a few places where it's not as obvious how to pass the necessary values to the function. In the process I got rid of the static syntax::LOADER in helix-core and added the loader to main and most of the function signatures that used it. The highlight_config field of Document got removed as well, now calling highlight_config recomputes the configuration on each call and this is the part that I don't have figured out yet.

A global configuration file in ~/.config/helix/config.toml is added where users can specify the theme they want to use by default. Themes are now placed in ~/.config/helix/themes. During runtime one can set the theme with :theme <name> or list themes with :list-themes. For now the listing is pretty naive and just displays the names in status bar.

I'm looking for general comments on this approach, perhaps it's undesired to get rid of static variables I might be uninformed here.

archseer · 2021-06-15T01:19:56Z

helix-core/src/syntax.rs

@@ -144,34 +142,30 @@ fn read_query(language: &str, filename: &str) -> String {

 impl LanguageConfiguration {
    pub fn highlight_config(&self, scopes: &[String]) -> Option<Arc<HighlightConfiguration>> {
-        self.highlight_config


I think the idea here was to only recalculate highlight_config if absolutely necessary (we only need to do this if the theme changed and we have different scopes now). HighlightConfiguration isn't cheap to construct and it also takes several fs reads so ideally we want to reuse them across files if they're the same language.

archseer · 2021-06-15T01:22:07Z

helix-term/src/main.rs

+    use helix_core::syntax;
+
+    // load $HOME/.config/helix/languages.toml, fallback to default config
+    let lang_config = std::fs::read(conf_dir.join("languages.toml"));


I think all this was recently moved into Application::new, can we just do it there?

archseer · 2021-06-15T01:25:02Z

helix-term/src/main.rs

@@ -91,8 +92,39 @@ FLAGS:

    setup_logging(logpath, args.verbosity).context("failed to initialize logging")?;

+    let themes = std::sync::Arc::new(Themes::load());


This should really be delayed, we don't want to load all available themes into memory. list-themes should read the directory entries (I think we should move contrib/themes under runtime/themes), then when setting the theme we actually read the file.

archseer · 2021-06-15T01:26:10Z

helix-term/src/main.rs

+    let app_config = application::Configuration { theme: None };
+    let app_config = std::fs::read(conf_dir.join("config.toml"))
+        .map(|v| toml::from_slice(v.as_slice()))
+        .unwrap_or_else(|_| Ok(app_config.clone()))


I don't quite follow the double unwrap. I think the .map on the previous line should be a and_then

archseer · 2021-06-15T01:27:20Z

helix-term/src/main.rs

+        .unwrap_or_else(|_| Ok(app_config.clone()))
+        .unwrap_or_else(|_| app_config);
+
+    let theme = if let Some(theme) = &app_config.theme {


let theme = app_config.theme.as_ref().map(|theme| themes.get(theme)).unwrap_or_else(|_| themes.default())

archseer · 2021-06-15T01:27:57Z

helix-term/src/application.rs

        use helix_view::editor::Action;
        let mut compositor = Compositor::new()?;
        let size = compositor.size();
-        let mut editor = Editor::new(size);
+        let theme = if let Some(theme) = &config.theme {
+            if let Some(theme) = themes.get(theme) {


Ditto here, also the to_owned branch is followed by a clone which is not ideal (it's a double clone)

archseer · 2021-06-15T01:28:32Z

helix-term/src/ui/completion.rs

+                            option.detail.as_deref().unwrap_or_default(),
+                            contents.clone()
+                        ),
+                        cx.editor.config_loader.clone(),


Yeah so this is why I switched to a global loader, it was annoying to pass it around everywhere

archseer · 2021-06-15T01:32:11Z

helix-view/src/editor.rs

+    pub fn set_theme(&mut self, theme: Theme) {
+        let config_loader = self.config_loader.clone();
+        let (_, doc) = self.current();
+        doc.detect_language(Some(&theme), config_loader);


This really needs to be done in all documents, since the list of scopes might have changed. We also don't need to trash the whole syntax tree, simply update the highlight configuration on it I think.

archseer · 2021-06-15T01:33:45Z

helix-view/src/editor.rs

@@ -161,7 +174,7 @@ impl Editor {
        let id = if let Some(id) = id {
            id
        } else {
-            let mut doc = Document::load(path)?;
+            let mut doc = Document::load(path, Some(self.config_loader.clone()))?;


There's no need to clone an arc around if you're just passing it as a reference (cloning then dropping results in extra operations to modify the refcount inside the Arc). &self.config_loader and then using &Loader on the type would work.

archseer · 2021-06-15T01:34:28Z

helix-term/src/application.rs

@@ -32,19 +36,41 @@ pub type LspCallback =
 pub type LspCallbacks = FuturesUnordered<LspCallback>;
 pub type LspCallbackWrapper = Box<dyn FnOnce(&mut Editor, &mut Compositor) + Send>;

+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub struct Configuration {
+    pub theme: Option<String>,


I think this is good and can also hold the keymap from #268 later, \cc @PabloMansanet

cessen · 2021-06-15T15:47:25Z

Just want to comment that I'm definitely in favor of the general approach, from a user perspective. Haven't had a chance to look at the code itself yet.

archseer · 2021-06-17T16:25:27Z

#268 landed so this can now share the config type :)

vv9k · 2021-06-18T08:26:15Z

Not sure what to do about the LanguageConfiguration. Currently it is loaded once when asking for highlight_config and later it is impossible to configure HighlightConfiguration with scopes from a different theme. I was thinking of just cloning the HighlightConfiguration when already initialized and configuring it with different scopes but it doesn't implement Clone because Query from tree-sitter doesn't implement it.

helix/helix-core/src/syntax.rs

Lines 145 to 175 in 103bd14

    
           impl LanguageConfiguration { 
        
               pub fn highlight_config(&self, scopes: &[String]) -> Option<Arc<HighlightConfiguration>> { 
        
                   self.highlight_config 
        
                       .get_or_init(|| { 
        
                           let language = get_language_name(self.language_id).to_ascii_lowercase(); 
        
                           let highlights_query = read_query(&language, "highlights.scm"); 
        
                           // always highlight syntax errors 
        
                           // highlights_query += "\n(ERROR) @error"; 
        
                           let injections_query = read_query(&language, "injections.scm"); 
        
                           let locals_query = ""; 
        
                           if highlights_query.is_empty() { 
        
                               None 
        
                           } else { 
        
                               let language = get_language(self.language_id); 
        
                               let mut config = HighlightConfiguration::new( 
        
                                   language, 
        
                                   &highlights_query, 
        
                                   &injections_query, 
        
                                   locals_query, 
        
                               ) 
        
                               .unwrap(); // TODO: no unwrap 
        
                               config.configure(scopes); 
        
                               Some(Arc::new(config)) 
        
                           } 
        
                       }) 
        
                       .clone() 
        
               }

vv9k · 2021-06-19T06:53:26Z

Not sure what to do about the LanguageConfiguration...

I managed to come up with a solution that doesn't require mutating the HighlightConfiguration. Now all themes have all scopes defined on creation and the scopes that are missing from .toml file are replaced with the ones from the default theme.

Here's a demo of the commands:

archseer · 2021-06-19T07:40:06Z

I think we'll need to do the fix in a different way. We can't have a fixed set of scopes, because it's up to the theme to pick a range of scopes to support. For example, one can define a function.builtin in one theme to distinguish from function, but only use function in another theme. Tree sitter is smart and it will simply report function.builtin as function in those. If we always pass in a list of default scopes and fallback, then now function.builtin would be incorrectly highlighted.

archseer · 2021-06-19T07:40:55Z

(see part 12.4: https://macromates.com/manual/en/language_grammars)

archseer · 2021-06-19T07:43:33Z

Maybe we could use arc-swap here? Initialize a new highlight_config and swap it into the cell.

archseer · 2021-06-19T07:45:16Z

If you replace the Arc contained inside syntax instead:

helix/helix-core/src/syntax.rs

Line 279 in ca806d4

config: Arc<HighlightConfiguration>,

then we could potentially have a theme per document even.

vv9k · 2021-06-19T07:48:32Z

But initializing highlight_config is exactly what takes up the longest, I've measured and even the file system reads are almost not noticable next to the operations that are then made on the .scm files when doing HighlightConfiguration::new. It takes about ~105ms in debug mode on my pc to initialize and the file reads happen in <1ms.

archseer · 2021-06-19T07:48:48Z

Note: if you add a custom completer then we can also offer themes as completions on :theme

vv9k · 2021-06-19T07:50:00Z

Note: if you add a custom completer then we can also offer themes as completions on :theme

I do plan to add a completer once we figure out a good way to have multiple highlight configs.

archseer · 2021-06-19T07:50:04Z

But initializing highlight_config is exactly what takes up the longest, I've measured and even the file system reads are almost not noticable next to the operations that are then made on the .scm files when doing HighlightConfiguration::new. It takes about ~105ms in debug mode on my pc to initialize and the file reads happen in <1ms.

Yeah it's precisely because the highlight query needs to be reloaded and re-analyzed to support the new scopes. It's shared across all files of the same language though. Should be much faster in release mode.

vv9k · 2021-06-19T07:52:26Z

Yeah it's precisely because the highlight query needs to be reloaded and re-analyzed to support the new scopes. It's shared across all files of the same language though. Should be much faster in release mode.

Doing just configure with new scopes on the HighlightConfiguration is actually quiet fast, it's the initial query construction that takes a lot of time.

One thing that I'm also wondering is if we were to have multiple HighlightConfigurations we'd have to reload the highlight files for each theme anyway as there is no Clone impl for it.

archseer · 2021-06-19T07:53:34Z

Also, it's possible you're iterating over all languages over config.language? This will load up highlight configs for languages that aren't open in the editor. You probably need to use https://docs.rs/once_cell/1.8.0/once_cell/sync/struct.OnceCell.html#method.get to check if a cell is initialized first.

archseer · 2021-06-19T07:55:17Z

Yeah all the files of the same type share a single highlight config inside an Arc, so iterating only over config.language, checking if cell has value via get().is_some(), then swapping the Arc contents via arc-swap should be enough to update all documents.

vv9k · 2021-06-19T07:55:30Z

Do you mean here: https://github.com/helix-editor/helix/pull/267/files#diff-e78218ec4fe8cd27d4141d6d1077b4e929f734f03e97dcadfcc82f03d9ed661cL211 ?

pickfire · 2021-06-19T08:13:37Z

There is one thing I always want in switching theme, like previewing themes dynamically. I have the same issue in vim, neovim, kakoune, doom emacs. I need to manually type in the command to switch the theme one by one, I wish I can just tab and the whole thing is being previewed to that theme, of course we need some extra handler around the prompt if we want to do this. Usually these require addons and if I do it by hand it is going to be tedious. Only vscode get this right.

archseer

Verified on local. Thanks!

archseer requested changes Jun 15, 2021

View reviewed changes

vv9k force-pushed the config branch 2 times, most recently from d7aae56 to b3b6601 Compare June 15, 2021 07:41

vv9k marked this pull request as draft June 16, 2021 06:35

vv9k force-pushed the config branch from 4d5b117 to 103bd14 Compare June 17, 2021 19:18

vv9k changed the title ~~Move configuration to main, add ability to switch themes~~ Add ability to switch themes Jun 18, 2021

vv9k force-pushed the config branch 2 times, most recently from 7170494 to 136286a Compare June 19, 2021 06:53

vv9k marked this pull request as ready for review June 19, 2021 06:59

vv9k force-pushed the config branch from 136286a to 3772862 Compare June 19, 2021 07:18

vv9k force-pushed the config branch 2 times, most recently from 6685f4b to 16126e8 Compare June 19, 2021 09:45

vv9k marked this pull request as draft June 19, 2021 09:46

vv9k force-pushed the config branch from 16126e8 to cc4728f Compare June 19, 2021 11:01

vv9k marked this pull request as ready for review June 19, 2021 11:02

vv9k added 7 commits June 19, 2021 13:19

Update docs

706cc72

Move themes to runtime/themes, add link from contrib/themes

2c65e57

Add themes loader

176ca4d

Add ability to change theme on editor

e86fc33

Add theme completer

06ce71d

Add theme to global configuration

24b557f

Add :theme <name> command

c65a12a

vv9k force-pushed the config branch from 50df4b9 to c65a12a Compare June 19, 2021 11:27

Only reconfiure highlights when setting theme

0aadb16

vv9k force-pushed the config branch from 25ad6c0 to 0aadb16 Compare June 19, 2021 11:56

archseer approved these changes Jun 19, 2021

View reviewed changes

archseer merged commit 6825e19 into helix-editor:master Jun 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to switch themes #267

Add ability to switch themes #267

vv9k commented Jun 14, 2021 •

edited

Loading

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

archseer Jun 15, 2021

cessen commented Jun 15, 2021

archseer commented Jun 17, 2021

vv9k commented Jun 18, 2021

vv9k commented Jun 19, 2021 •

edited

Loading

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

pickfire commented Jun 19, 2021 •

edited

Loading

archseer left a comment

		@@ -91,8 +92,39 @@ FLAGS:

		setup_logging(logpath, args.verbosity).context("failed to initialize logging")?;

		let themes = std::sync::Arc::new(Themes::load());

Add ability to switch themes #267

Add ability to switch themes #267

Conversation

vv9k commented Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cessen commented Jun 15, 2021

archseer commented Jun 17, 2021

vv9k commented Jun 18, 2021

vv9k commented Jun 19, 2021 • edited Loading

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

archseer commented Jun 19, 2021

archseer commented Jun 19, 2021

vv9k commented Jun 19, 2021

pickfire commented Jun 19, 2021 • edited Loading

archseer left a comment

Choose a reason for hiding this comment

vv9k commented Jun 14, 2021 •

edited

Loading

vv9k commented Jun 19, 2021 •

edited

Loading

pickfire commented Jun 19, 2021 •

edited

Loading