-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Generalised to multiple runtime directories with priorities #5411
Changes from all commits
294d1a1
b642e90
c4620ff
5a0a1db
d157b74
97bbec3
8cc8b2a
4e7529a
349a4d5
93c0969
22c3cc7
0c6a824
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -280,10 +280,10 @@ pub mod completers { | |
} | ||
|
||
pub fn theme(_editor: &Editor, input: &str) -> Vec<Completion> { | ||
let mut names = theme::Loader::read_names(&helix_loader::runtime_dir().join("themes")); | ||
names.extend(theme::Loader::read_names( | ||
&helix_loader::config_dir().join("themes"), | ||
)); | ||
let mut names = theme::Loader::read_names(&helix_loader::config_dir().join("themes")); | ||
for rt_dir in helix_loader::runtime_dirs() { | ||
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. I think the function I commented in earlier has been inlined here (maybe by the merge from master?). This function has the same issue where we might ensup with the same theme name multiple times. I think an (unstable) sort and deadup would be a good solution 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. Look down a couple of lines to 289 and 290 and it looks like this being done already. 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. Ah you are right, sorry about that 😅 |
||
names.extend(theme::Loader::read_names(&rt_dir.join("themes"))); | ||
} | ||
names.push("default".into()); | ||
names.push("base16_default".into()); | ||
names.sort(); | ||
|
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.
Hmm, shouldn't this be ~/.local/state/helix/runtime (
XDG_STATE_HOME
)?This is clearly not config, but state, and we don't want backup programs to back up compiled tree-sitter .so grammars…
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.
That's #584
This PR kept it to
~/.config
for backwards compatibilityThere 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.
#5636 🤷