From ce1fb9e64c189fb7476b4c72c6774a5bf6cbfd0f Mon Sep 17 00:00:00 2001 From: paul-scott Date: Fri, 10 Mar 2023 01:50:43 +1100 Subject: [PATCH 01/23] Generalised to multiple runtime directories with priorities (#5411) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Generalised to multiple runtime directories with priorities This is an implementation for #3346. Previously, one of the following runtime directories were used: 1. `$HELIX_RUNTIME` 2. sibling directory to `$CARGO_MANIFEST_DIR` 3. subdirectory of user config directory 4. subdirectory of path to helix executable The first directory provided / found to exist in this order was used as a root for all runtime file searches (grammars, themes, queries). This change lowers the priority of `$HELIX_RUNTIME` so that the user config runtime has higher priority. More significantly, all of these directories are now searched for runtime files, enabling a user to override default or system-level runtime files. If the same file name appears in multiple runtime directories, the following priority is now used: 1. sibling directory to `$CARGO_MANIFEST_DIR` 2. subdirectory of user config directory 3. `$HELIX_RUNTIME` 4. subdirectory of path to helix executable One exception to this rule is that a user can have a `themes` directory directly in the user config directory that has higher piority to `themes` directories in runtime directories. That behaviour has been preserved. As part of implementing this feature `theme::Loader` was simplified and the cycle detection logic of the theme inheritance was improved to cover more cases and to be more explicit. * Removed AsRef usage to avoid binary growth * Health displaying ;-separated runtime dirs * Changed HELIX_RUNTIME build from src instructions * Updated doc for more detail on runtime directories * Improved health symlink printing and theme cycle errors The health display of runtime symlinks now prints both ends of the link. Separate errors are given when theme file is not found and when the only theme file found would form an inheritence cycle. * Satisfied clippy on passing Path * Clarified highest priority runtime directory purpose * Further clarified multiple runtime details in book Also gave markdown headings to subsections. Fixed a error with table indentation not building table that also appears present on master. --------- Co-authored-by: Paul Scott Co-authored-by: Blaž Hrastnik --- book/src/install.md | 38 +++++++++---- helix-loader/src/grammar.rs | 23 +++++--- helix-loader/src/lib.rs | 81 ++++++++++++++++++++++----- helix-term/src/application.rs | 10 ++-- helix-term/src/commands/typed.rs | 2 +- helix-term/src/health.rs | 38 +++++++++---- helix-term/src/ui/mod.rs | 8 +-- helix-view/src/theme.rs | 95 ++++++++++++++++++-------------- 8 files changed, 197 insertions(+), 98 deletions(-) diff --git a/book/src/install.md b/book/src/install.md index f9cf9a3b..bd3f502b 100644 --- a/book/src/install.md +++ b/book/src/install.md @@ -137,8 +137,8 @@ cargo install --path helix-term --locked ``` This command will create the `hx` executable and construct the tree-sitter -grammars either in the `runtime` folder, or in the folder specified in `HELIX_RUNTIME` -(as described below). To build the tree-sitter grammars requires a c++ compiler to be installed, for example `gcc-c++`. +grammars in the local `runtime` folder. To build the tree-sitter grammars requires +a c++ compiler to be installed, for example `gcc-c++`. > 💡 If you are using the musl-libc instead of glibc the following environment variable must be set during the build > to ensure tree-sitter grammars can be loaded correctly: @@ -149,11 +149,13 @@ grammars either in the `runtime` folder, or in the folder specified in `HELIX_RU > 💡 Tree-sitter grammars can be fetched and compiled if not pre-packaged. Fetch > grammars with `hx --grammar fetch` (requires `git`) and compile them with -> `hx --grammar build` (requires a C++ compiler). +> `hx --grammar build` (requires a C++ compiler). This will install them in +> the `runtime` directory within the user's helix config directory (more +> [details below](#multiple-runtime-directories)). ### Configuring Helix's runtime files -- **Linux and macOS** +#### Linux and macOS Either set the `HELIX_RUNTIME` environment variable to point to the runtime files and add it to your `~/.bashrc` or equivalent: @@ -167,7 +169,7 @@ Or, create a symlink in `~/.config/helix` that links to the source code director ln -s $PWD/runtime ~/.config/helix/runtime ``` -- **Windows** +#### Windows Either set the `HELIX_RUNTIME` environment variable to point to the runtime files using the Windows setting (search for `Edit environment variables for your account`) or use the `setx` command in @@ -182,13 +184,27 @@ setx HELIX_RUNTIME "%userprofile%\source\repos\helix\runtime" Or, create a symlink in `%appdata%\helix\` that links to the source code directory: - | Method | Command | - | ---------- | -------------------------------------------------------------------------------------- | - | PowerShell | `New-Item -ItemType Junction -Target "runtime" -Path "$Env:AppData\helix\runtime"` | - | Cmd | `cd %appdata%\helix`
`mklink /D runtime "%userprofile%\src\helix\runtime"` | +| Method | Command | +| ---------- | -------------------------------------------------------------------------------------- | +| PowerShell | `New-Item -ItemType Junction -Target "runtime" -Path "$Env:AppData\helix\runtime"` | +| Cmd | `cd %appdata%\helix`
`mklink /D runtime "%userprofile%\src\helix\runtime"` | - > 💡 On Windows, creating a symbolic link may require running PowerShell or - > Cmd as an administrator. +> 💡 On Windows, creating a symbolic link may require running PowerShell or +> Cmd as an administrator. + +#### Multiple runtime directories + +When Helix finds multiple runtime directories it will search through them for files in the +following order: + +1. `runtime/` sibling directory to `$CARGO_MANIFEST_DIR` directory (this is intended for + developing and testing helix only). +2. `runtime/` subdirectory of OS-dependent helix user config directory. +3. `$HELIX_RUNTIME`. +4. `runtime/` subdirectory of path to Helix executable. + +This order also sets the priority for selecting which file will be used if multiple runtime +directories have files with the same name. ### Validating the installation diff --git a/helix-loader/src/grammar.rs b/helix-loader/src/grammar.rs index 01c966c8..a85cb274 100644 --- a/helix-loader/src/grammar.rs +++ b/helix-loader/src/grammar.rs @@ -67,8 +67,9 @@ pub fn get_language(name: &str) -> Result { #[cfg(not(target_arch = "wasm32"))] pub fn get_language(name: &str) -> Result { use libloading::{Library, Symbol}; - let mut library_path = crate::runtime_dir().join("grammars").join(name); - library_path.set_extension(DYLIB_EXTENSION); + let mut rel_library_path = PathBuf::new().join("grammars").join(name); + rel_library_path.set_extension(DYLIB_EXTENSION); + let library_path = crate::runtime_file(&rel_library_path); let library = unsafe { Library::new(&library_path) } .with_context(|| format!("Error opening dynamic library {:?}", library_path))?; @@ -252,7 +253,9 @@ fn fetch_grammar(grammar: GrammarConfiguration) -> Result { remote, revision, .. } = grammar.source { - let grammar_dir = crate::runtime_dir() + let grammar_dir = crate::runtime_dirs() + .first() + .expect("No runtime directories provided") // guaranteed by post-condition .join("grammars") .join("sources") .join(&grammar.grammar_id); @@ -350,7 +353,9 @@ fn build_grammar(grammar: GrammarConfiguration, target: Option<&str>) -> Result< let grammar_dir = if let GrammarSource::Local { path } = &grammar.source { PathBuf::from(&path) } else { - crate::runtime_dir() + crate::runtime_dirs() + .first() + .expect("No runtime directories provided") // guaranteed by post-condition .join("grammars") .join("sources") .join(&grammar.grammar_id) @@ -401,7 +406,10 @@ fn build_tree_sitter_library( None } }; - let parser_lib_path = crate::runtime_dir().join("grammars"); + let parser_lib_path = crate::runtime_dirs() + .first() + .expect("No runtime directories provided") // guaranteed by post-condition + .join("grammars"); let mut library_path = parser_lib_path.join(&grammar.grammar_id); library_path.set_extension(DYLIB_EXTENSION); @@ -511,9 +519,6 @@ fn mtime(path: &Path) -> Result { /// Gives the contents of a file from a language's `runtime/queries/` /// directory pub fn load_runtime_file(language: &str, filename: &str) -> Result { - let path = crate::RUNTIME_DIR - .join("queries") - .join(language) - .join(filename); + let path = crate::runtime_file(&PathBuf::new().join("queries").join(language).join(filename)); std::fs::read_to_string(path) } diff --git a/helix-loader/src/lib.rs b/helix-loader/src/lib.rs index 8dc2928a..04b44b5a 100644 --- a/helix-loader/src/lib.rs +++ b/helix-loader/src/lib.rs @@ -2,11 +2,12 @@ pub mod config; pub mod grammar; use etcetera::base_strategy::{choose_base_strategy, BaseStrategy}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; pub const VERSION_AND_GIT_HASH: &str = env!("VERSION_AND_GIT_HASH"); -pub static RUNTIME_DIR: once_cell::sync::Lazy = once_cell::sync::Lazy::new(runtime_dir); +static RUNTIME_DIRS: once_cell::sync::Lazy> = + once_cell::sync::Lazy::new(prioritize_runtime_dirs); static CONFIG_FILE: once_cell::sync::OnceCell = once_cell::sync::OnceCell::new(); @@ -25,31 +26,83 @@ pub fn initialize_config_file(specified_file: Option) { CONFIG_FILE.set(config_file).ok(); } -pub fn runtime_dir() -> PathBuf { - if let Ok(dir) = std::env::var("HELIX_RUNTIME") { - return dir.into(); - } - +/// A list of runtime directories from highest to lowest priority +/// +/// The priority is: +/// +/// 1. sibling directory to `CARGO_MANIFEST_DIR` (if environment variable is set) +/// 2. subdirectory of user config directory (always included) +/// 3. `HELIX_RUNTIME` (if environment variable is set) +/// 4. subdirectory of path to helix executable (always included) +/// +/// Postcondition: returns at least two paths (they might not exist). +fn prioritize_runtime_dirs() -> Vec { + const RT_DIR: &str = "runtime"; + // Adding higher priority first + let mut rt_dirs = Vec::new(); if let Ok(dir) = std::env::var("CARGO_MANIFEST_DIR") { // this is the directory of the crate being run by cargo, we need the workspace path so we take the parent let path = std::path::PathBuf::from(dir).parent().unwrap().join(RT_DIR); log::debug!("runtime dir: {}", path.to_string_lossy()); - return path; + rt_dirs.push(path); } - const RT_DIR: &str = "runtime"; - let conf_dir = config_dir().join(RT_DIR); - if conf_dir.exists() { - return conf_dir; + let conf_rt_dir = config_dir().join(RT_DIR); + rt_dirs.push(conf_rt_dir); + + if let Ok(dir) = std::env::var("HELIX_RUNTIME") { + rt_dirs.push(dir.into()); } // fallback to location of the executable being run // canonicalize the path in case the executable is symlinked - std::env::current_exe() + let exe_rt_dir = std::env::current_exe() .ok() .and_then(|path| std::fs::canonicalize(path).ok()) .and_then(|path| path.parent().map(|path| path.to_path_buf().join(RT_DIR))) - .unwrap() + .unwrap(); + rt_dirs.push(exe_rt_dir); + rt_dirs +} + +/// Runtime directories ordered from highest to lowest priority +/// +/// All directories should be checked when looking for files. +/// +/// Postcondition: returns at least one path (it might not exist). +pub fn runtime_dirs() -> &'static [PathBuf] { + &RUNTIME_DIRS +} + +/// Find file with path relative to runtime directory +/// +/// `rel_path` should be the relative path from within the `runtime/` directory. +/// The valid runtime directories are searched in priority order and the first +/// file found to exist is returned, otherwise None. +fn find_runtime_file(rel_path: &Path) -> Option { + RUNTIME_DIRS.iter().find_map(|rt_dir| { + let path = rt_dir.join(rel_path); + if path.exists() { + Some(path) + } else { + None + } + }) +} + +/// Find file with path relative to runtime directory +/// +/// `rel_path` should be the relative path from within the `runtime/` directory. +/// The valid runtime directories are searched in priority order and the first +/// file found to exist is returned, otherwise the path to the final attempt +/// that failed. +pub fn runtime_file(rel_path: &Path) -> PathBuf { + find_runtime_file(rel_path).unwrap_or_else(|| { + RUNTIME_DIRS + .last() + .map(|dir| dir.join(rel_path)) + .unwrap_or_default() + }) } pub fn config_dir() -> PathBuf { diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index d56e7c88..c7e93995 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -31,6 +31,7 @@ use crate::{ use log::{debug, error, warn}; use std::{ io::{stdin, stdout}, + path::Path, sync::Arc, time::{Duration, Instant}, }; @@ -113,10 +114,9 @@ impl Application { use helix_view::editor::Action; - let theme_loader = std::sync::Arc::new(theme::Loader::new( - &helix_loader::config_dir(), - &helix_loader::runtime_dir(), - )); + let mut theme_parent_dirs = vec![helix_loader::config_dir()]; + theme_parent_dirs.extend(helix_loader::runtime_dirs().iter().cloned()); + let theme_loader = std::sync::Arc::new(theme::Loader::new(&theme_parent_dirs)); let true_color = config.editor.true_color || crate::true_color(); let theme = config @@ -162,7 +162,7 @@ impl Application { compositor.push(editor_view); if args.load_tutor { - let path = helix_loader::runtime_dir().join("tutor"); + let path = helix_loader::runtime_file(Path::new("tutor")); editor.open(&path, Action::VerticalSplit)?; // Unset path to prevent accidentally saving to the original tutor file. doc_mut!(editor).set_path(None)?; diff --git a/helix-term/src/commands/typed.rs b/helix-term/src/commands/typed.rs index 5ea61108..e9a72225 100644 --- a/helix-term/src/commands/typed.rs +++ b/helix-term/src/commands/typed.rs @@ -1565,7 +1565,7 @@ fn tutor( return Ok(()); } - let path = helix_loader::runtime_dir().join("tutor"); + let path = helix_loader::runtime_file(Path::new("tutor")); cx.editor.open(&path, Action::Replace)?; // Unset path to prevent accidentally saving to the original tutor file. doc_mut!(cx.editor).set_path(None)?; diff --git a/helix-term/src/health.rs b/helix-term/src/health.rs index 6558fe19..480c2c67 100644 --- a/helix-term/src/health.rs +++ b/helix-term/src/health.rs @@ -52,7 +52,7 @@ pub fn general() -> std::io::Result<()> { let config_file = helix_loader::config_file(); let lang_file = helix_loader::lang_config_file(); let log_file = helix_loader::log_file(); - let rt_dir = helix_loader::runtime_dir(); + let rt_dirs = helix_loader::runtime_dirs(); let clipboard_provider = get_clipboard_provider(); if config_file.exists() { @@ -66,17 +66,31 @@ pub fn general() -> std::io::Result<()> { writeln!(stdout, "Language file: default")?; } writeln!(stdout, "Log file: {}", log_file.display())?; - writeln!(stdout, "Runtime directory: {}", rt_dir.display())?; - - if let Ok(path) = std::fs::read_link(&rt_dir) { - let msg = format!("Runtime directory is symlinked to {}", path.display()); - writeln!(stdout, "{}", msg.yellow())?; - } - if !rt_dir.exists() { - writeln!(stdout, "{}", "Runtime directory does not exist.".red())?; - } - if rt_dir.read_dir().ok().map(|it| it.count()) == Some(0) { - writeln!(stdout, "{}", "Runtime directory is empty.".red())?; + writeln!( + stdout, + "Runtime directories: {}", + rt_dirs + .iter() + .map(|d| d.to_string_lossy()) + .collect::>() + .join(";") + )?; + for rt_dir in rt_dirs.iter() { + if let Ok(path) = std::fs::read_link(rt_dir) { + let msg = format!( + "Runtime directory {} is symlinked to: {}", + rt_dir.display(), + path.display() + ); + writeln!(stdout, "{}", msg.yellow())?; + } + if !rt_dir.exists() { + let msg = format!("Runtime directory does not exist: {}", rt_dir.display()); + writeln!(stdout, "{}", msg.yellow())?; + } else if rt_dir.read_dir().ok().map(|it| it.count()) == Some(0) { + let msg = format!("Runtime directory is empty: {}", rt_dir.display()); + writeln!(stdout, "{}", msg.yellow())?; + } } writeln!(stdout, "Clipboard provider: {}", clipboard_provider.name())?; diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index d7717f8c..3e9a14b0 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -280,10 +280,10 @@ pub mod completers { } pub fn theme(_editor: &Editor, input: &str) -> Vec { - 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() { + names.extend(theme::Loader::read_names(&rt_dir.join("themes"))); + } names.push("default".into()); names.push("base16_default".into()); names.sort(); diff --git a/helix-view/src/theme.rs b/helix-view/src/theme.rs index ce061bab..5d79ff26 100644 --- a/helix-view/src/theme.rs +++ b/helix-view/src/theme.rs @@ -1,5 +1,5 @@ use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, path::{Path, PathBuf}, str, }; @@ -37,19 +37,21 @@ pub static BASE16_DEFAULT_THEME: Lazy = Lazy::new(|| Theme { #[derive(Clone, Debug)] pub struct Loader { - user_dir: PathBuf, - default_dir: PathBuf, + /// Theme directories to search from highest to lowest priority + theme_dirs: Vec, } impl Loader { - /// Creates a new loader that can load themes from two directories. - pub fn new>(user_dir: P, default_dir: P) -> Self { + /// Creates a new loader that can load themes from multiple directories. + /// + /// The provided directories should be ordered from highest to lowest priority. + /// The directories will have their "themes" subdirectory searched. + pub fn new(dirs: &[PathBuf]) -> Self { Self { - user_dir: user_dir.as_ref().join("themes"), - default_dir: default_dir.as_ref().join("themes"), + theme_dirs: dirs.iter().map(|p| p.join("themes")).collect(), } } - /// Loads a theme first looking in the `user_dir` then in `default_dir` + /// Loads a theme searching directories in priority order. pub fn load(&self, name: &str) -> Result { if name == "default" { return Ok(self.default()); @@ -58,7 +60,8 @@ impl Loader { return Ok(self.base16_default()); } - let theme = self.load_theme(name, name, false).map(Theme::from)?; + let mut visited_paths = HashSet::new(); + let theme = self.load_theme(name, &mut visited_paths).map(Theme::from)?; Ok(Theme { name: name.into(), @@ -66,16 +69,18 @@ impl Loader { }) } - // load the theme and its parent recursively and merge them - // `base_theme_name` is the theme from the config.toml, - // used to prevent some circular loading scenarios - fn load_theme( - &self, - name: &str, - base_theme_name: &str, - only_default_dir: bool, - ) -> Result { - let path = self.path(name, only_default_dir); + /// Recursively load a theme, merging with any inherited parent themes. + /// + /// The paths that have been visited in the inheritance hierarchy are tracked + /// to detect and avoid cycling. + /// + /// It is possible for one file to inherit from another file with the same name + /// so long as the second file is in a themes directory with lower priority. + /// However, it is not recommended that users do this as it will make tracing + /// errors more difficult. + fn load_theme(&self, name: &str, visited_paths: &mut HashSet) -> Result { + let path = self.path(name, visited_paths)?; + let theme_toml = self.load_toml(path)?; let inherits = theme_toml.get("inherits"); @@ -92,11 +97,7 @@ impl Loader { // load default themes's toml from const. "default" => DEFAULT_THEME_DATA.clone(), "base16_default" => BASE16_DEFAULT_THEME_DATA.clone(), - _ => self.load_theme( - parent_theme_name, - base_theme_name, - base_theme_name == parent_theme_name, - )?, + _ => self.load_theme(parent_theme_name, visited_paths)?, }; self.merge_themes(parent_theme_toml, theme_toml) @@ -148,7 +149,7 @@ impl Loader { merge_toml_values(theme, palette.into(), 1) } - // Loads the theme data as `toml::Value` first from the user_dir then in default_dir + // Loads the theme data as `toml::Value` fn load_toml(&self, path: PathBuf) -> Result { let data = std::fs::read_to_string(path)?; let value = toml::from_str(&data)?; @@ -156,25 +157,35 @@ impl Loader { Ok(value) } - // Returns the path to the theme with the name - // With `only_default_dir` as false the path will first search for the user path - // disabled it ignores the user path and returns only the default path - fn path(&self, name: &str, only_default_dir: bool) -> PathBuf { + /// Returns the path to the theme with the given name + /// + /// Ignores paths already visited and follows directory priority order. + fn path(&self, name: &str, visited_paths: &mut HashSet) -> Result { let filename = format!("{}.toml", name); - let user_path = self.user_dir.join(&filename); - if !only_default_dir && user_path.exists() { - user_path - } else { - self.default_dir.join(filename) - } - } - - /// Lists all theme names available in default and user directory - pub fn names(&self) -> Vec { - let mut names = Self::read_names(&self.user_dir); - names.extend(Self::read_names(&self.default_dir)); - names + let mut cycle_found = false; // track if there was a path, but it was in a cycle + self.theme_dirs + .iter() + .find_map(|dir| { + let path = dir.join(&filename); + if !path.exists() { + None + } else if visited_paths.contains(&path) { + // Avoiding cycle, continuing to look in lower priority directories + cycle_found = true; + None + } else { + visited_paths.insert(path.clone()); + Some(path) + } + }) + .ok_or_else(|| { + if cycle_found { + anyhow!("Theme: cycle found in inheriting: {}", name) + } else { + anyhow!("Theme: file not found for: {}", name) + } + }) } pub fn default_theme(&self, true_color: bool) -> Theme { From 9b4326b18b24fa8ec8ffd0cb261a8c82ecad32d6 Mon Sep 17 00:00:00 2001 From: "Taylor C. Richberger" Date: Tue, 10 Jan 2023 12:21:44 -0700 Subject: [PATCH 02/23] allow LSP insert text to replace non-matching prefixes (#5469) Most LSPs will complete case-insensitive matches, particularly from lowercase to uppercase. In some cases, notably Pyright, this is given as a simple insert text instead of TextEdit. When this happens, the prefix text was left unedited. --- helix-term/src/ui/completion.rs | 42 +++++++++++++++++---------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index ef88938f..336b75cb 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -124,6 +124,8 @@ impl Completion { ) -> Transaction { use helix_lsp::snippet; let selection = doc.selection(view_id); + let text = doc.text().slice(..); + let primary_cursor = selection.primary().cursor(text); let (start_offset, end_offset, new_text) = if let Some(edit) = &item.text_edit { let edit = match edit { @@ -133,8 +135,6 @@ impl Completion { lsp::TextEdit::new(item.replace, item.new_text.clone()) } }; - let text = doc.text().slice(..); - let primary_cursor = selection.primary().cursor(text); let start_offset = match util::lsp_pos_to_pos(doc.text(), edit.range.start, offset_encoding) { @@ -149,24 +149,26 @@ impl Completion { (start_offset, end_offset, edit.new_text) } else { - let new_text = item.insert_text.as_ref().unwrap_or(&item.label); - // Some LSPs just give you an insertText with no offset ¯\_(ツ)_/¯ - // in these cases we need to check for a common prefix and remove it - let prefix = Cow::from(doc.text().slice(start_offset..trigger_offset)); - let new_text = new_text.trim_start_matches::<&str>(&prefix); - - // TODO: this needs to be true for the numbers to work out correctly - // in the closure below. It's passed in to a callback as this same - // formula, but can the value change between the LSP request and - // response? If it does, can we recover? - debug_assert!( - doc.selection(view_id) - .primary() - .cursor(doc.text().slice(..)) - == trigger_offset - ); - - (0, 0, new_text.into()) + let new_text = item + .insert_text + .clone() + .unwrap_or_else(|| item.label.clone()); + + // check that we are still at the correct savepoint + // we can still generate a transaction regardless but if the + // document changed (and not just the selection) then we will + // likely delete the wrong text (same if we applied an edit sent by the LS) + debug_assert!(primary_cursor == trigger_offset); + + // TODO: Respect editor.completion_replace? + // Would require detecting the end of the word boundary for every cursor individually. + // We don't do the same for normal `edits, to be consistent we would have to do it for those too + + ( + start_offset as i128 - primary_cursor as i128, + trigger_offset as i128 - primary_cursor as i128, + new_text, + ) }; if matches!(item.kind, Some(lsp::CompletionItemKind::SNIPPET)) From e91289fda1c81bc3f3cc32735a1b6841e6df55a5 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 9 Mar 2023 22:09:12 +0100 Subject: [PATCH 03/23] Add IntoIterator implementation for Selection --- helix-core/src/selection.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/helix-core/src/selection.rs b/helix-core/src/selection.rs index 0eb2b755..8e93c633 100644 --- a/helix-core/src/selection.rs +++ b/helix-core/src/selection.rs @@ -661,6 +661,15 @@ impl<'a> IntoIterator for &'a Selection { } } +impl IntoIterator for Selection { + type Item = Range; + type IntoIter = smallvec::IntoIter<[Range; 1]>; + + fn into_iter(self) -> smallvec::IntoIter<[Range; 1]> { + self.ranges.into_iter() + } +} + // TODO: checkSelection -> check if valid for doc length && sorted pub fn keep_or_remove_matches( From cdec933523560f71c665469adc409d7ac0e06171 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 9 Mar 2023 22:17:12 +0100 Subject: [PATCH 04/23] avoid allocations during snippet rendering --- helix-lsp/src/snippet.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/helix-lsp/src/snippet.rs b/helix-lsp/src/snippet.rs index b27077e7..4713ad8b 100644 --- a/helix-lsp/src/snippet.rs +++ b/helix-lsp/src/snippet.rs @@ -1,7 +1,7 @@ use std::borrow::Cow; use anyhow::{anyhow, Result}; -use helix_core::{smallvec, SmallVec}; +use helix_core::{smallvec, SmallVec, Tendril}; #[derive(Debug, PartialEq, Eq)] pub enum CaseChange { @@ -57,10 +57,10 @@ pub fn parse(s: &str) -> Result> { fn render_elements( snippet_elements: &[SnippetElement<'_>], - insert: &mut String, + insert: &mut Tendril, offset: &mut usize, tabstops: &mut Vec<(usize, (usize, usize))>, - newline_with_offset: &String, + newline_with_offset: &str, include_placeholer: bool, ) { use SnippetElement::*; @@ -121,10 +121,10 @@ fn render_elements( #[allow(clippy::type_complexity)] // only used one time pub fn render( snippet: &Snippet<'_>, - newline_with_offset: String, + newline_with_offset: &str, include_placeholer: bool, -) -> (String, Vec>) { - let mut insert = String::new(); +) -> (Tendril, Vec>) { + let mut insert = Tendril::new(); let mut tabstops = Vec::new(); let mut offset = 0; @@ -133,7 +133,7 @@ pub fn render( &mut insert, &mut offset, &mut tabstops, - &newline_with_offset, + newline_with_offset, include_placeholer, ); From 2b64a64d7ea43e22ad82f97f2c118891b74c3199 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 9 Mar 2023 22:19:14 +0100 Subject: [PATCH 05/23] Add API to create a Transaction from potentially overlapping changes This commit adds new functions to `Transaction` that allow creating edits that might potentially overlap. Any change that overlaps previous changes is ignored. Furthermore, a utility method is added that also drops selections associated with dropped changes (for transactions that are created from a selection). This is needed to avoid crashes when applying multicursor autocompletions, as the edit from a previous cursor may overlap with the next cursor/edit. --- helix-core/src/transaction.rs | 67 +++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/helix-core/src/transaction.rs b/helix-core/src/transaction.rs index d2f4de07..d8e581aa 100644 --- a/helix-core/src/transaction.rs +++ b/helix-core/src/transaction.rs @@ -1,3 +1,5 @@ +use smallvec::SmallVec; + use crate::{Range, Rope, Selection, Tendril}; use std::borrow::Cow; @@ -466,6 +468,33 @@ impl Transaction { self } + /// Generate a transaction from a set of potentially overlapping changes. The `change_ranges` + /// iterator yield the range (of removed text) in the old document for each edit. If any change + /// overlaps with a range overlaps with a previous range then that range is ignored. + /// + /// The `process_change` callback is called for each edit that is not ignored (in the order + /// yielded by `changes`) and should return the new text that the associated range will be + /// replaced with. + /// + /// To make this function more flexible the iterator can yield additional data for each change + /// that is passed to `process_change` + pub fn change_ignore_overlapping( + doc: &Rope, + change_ranges: impl Iterator, + mut process_change: impl FnMut(usize, usize, T) -> Option, + ) -> Self { + let mut last = 0; + let changes = change_ranges.filter_map(|(from, to, data)| { + if from < last { + return None; + } + let tendril = process_change(from, to, data); + last = to; + Some((from, to, tendril)) + }); + Self::change(doc, changes) + } + /// Generate a transaction from a set of changes. pub fn change(doc: &Rope, changes: I) -> Self where @@ -513,6 +542,44 @@ impl Transaction { Self::change(doc, selection.iter().map(f)) } + pub fn change_by_selection_ignore_overlapping( + doc: &Rope, + selection: &Selection, + mut change_range: impl FnMut(&Range) -> (usize, usize), + mut create_tendril: impl FnMut(usize, usize) -> Option, + ) -> (Transaction, Selection) { + let mut last_selection_idx = None; + let mut new_primary_idx = None; + let mut ranges: SmallVec<[Range; 1]> = SmallVec::new(); + let process_change = |change_start, change_end, (idx, range): (usize, &Range)| { + // update the primary idx + if idx == selection.primary_index() { + new_primary_idx = Some(idx); + } else if new_primary_idx.is_none() { + if idx > selection.primary_index() { + new_primary_idx = last_selection_idx; + } else { + last_selection_idx = Some(idx); + } + } + ranges.push(*range); + create_tendril(change_start, change_end) + }; + let transaction = Self::change_ignore_overlapping( + doc, + selection.iter().enumerate().map(|range| { + let (change_start, change_end) = change_range(range.1); + (change_start, change_end, range) + }), + process_change, + ); + + ( + transaction, + Selection::new(ranges, new_primary_idx.unwrap_or(0)), + ) + } + /// Insert text at each selection head. pub fn insert(doc: &Rope, selection: &Selection, text: Tendril) -> Self { Self::change_by_selection(doc, selection, |range| { From b1f75280909884c9621b553b43030ac39cfa47ce Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 9 Mar 2023 23:08:55 +0100 Subject: [PATCH 06/23] fix snippet bugs and multicursor completion edgecases Multicursor completions may overlap and therefore overlapping completions must be dropped to avoid crashes. Furthermore, multicursor edits might simply be out of range if the word before/after the cursor is shorter. This currently leads to crashes, instead these selections are now also removed for completions. This commit also significantly refactors snippet transaction generation so that tabstops behave correctly with the above rules. Furthermore, snippet tabstops need to be carefully mapped to ensure their position is correct and consistent with our selection semantics. Finally, we now keep a partially updated Rope while creating snippet transactions so that we can fill information into snippets that depends on the position in the document. --- helix-lsp/src/lib.rs | 241 +++++++++++++++++++++++--------- helix-term/src/ui/completion.rs | 18 +-- 2 files changed, 180 insertions(+), 79 deletions(-) diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index 147b381c..58e8d83d 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -59,8 +59,8 @@ pub enum OffsetEncoding { pub mod util { use super::*; use helix_core::line_ending::{line_end_byte_index, line_end_char_index}; + use helix_core::{chars, RopeSlice, SmallVec}; use helix_core::{diagnostic::NumberOrString, Range, Rope, Selection, Tendril, Transaction}; - use helix_core::{smallvec, SmallVec}; /// Converts a diagnostic in the document to [`lsp::Diagnostic`]. /// @@ -247,13 +247,46 @@ pub mod util { Some(Range::new(start, end)) } + /// If the LS did not provide a range for the completion or the range of the + /// primary cursor can not be used for the secondary cursor, this function + /// can be used to find the completion range for a cursor + fn find_completion_range(text: RopeSlice, cursor: usize) -> (usize, usize) { + let start = cursor + - text + .chars_at(cursor) + .reversed() + .take_while(|ch| chars::char_is_word(*ch)) + .count(); + (start, cursor) + } + fn completion_range( + text: RopeSlice, + edit_offset: Option<(i128, i128)>, + cursor: usize, + ) -> Option<(usize, usize)> { + let res = match edit_offset { + Some((start_offset, end_offset)) => { + let start_offset = cursor as i128 + start_offset; + if start_offset < 0 { + return None; + } + let end_offset = cursor as i128 + end_offset; + if end_offset > text.len_chars() as i128 { + return None; + } + (start_offset as usize, end_offset as usize) + } + None => find_completion_range(text, cursor), + }; + Some(res) + } + /// Creates a [Transaction] from the [lsp::TextEdit] in a completion response. /// The transaction applies the edit to all cursors. pub fn generate_transaction_from_completion_edit( doc: &Rope, selection: &Selection, - start_offset: i128, - end_offset: i128, + edit_offset: Option<(i128, i128)>, new_text: String, ) -> Transaction { let replacement: Option = if new_text.is_empty() { @@ -263,83 +296,163 @@ pub mod util { }; let text = doc.slice(..); + let (removed_start, removed_end) = + completion_range(text, edit_offset, selection.primary().cursor(text)) + .expect("transaction must be valid for primary selection"); + let removed_text = text.slice(removed_start..removed_end); - Transaction::change_by_selection(doc, selection, |range| { - let cursor = range.cursor(text); - ( - (cursor as i128 + start_offset) as usize, - (cursor as i128 + end_offset) as usize, - replacement.clone(), - ) - }) + let (transaction, mut selection) = Transaction::change_by_selection_ignore_overlapping( + doc, + selection, + |range| { + let cursor = range.cursor(text); + completion_range(text, edit_offset, cursor) + .filter(|(start, end)| text.slice(start..end) == removed_text) + .unwrap_or_else(|| find_completion_range(text, cursor)) + }, + |_, _| replacement.clone(), + ); + if transaction.changes().is_empty() { + return transaction; + } + selection = selection.map(transaction.changes()); + transaction.with_selection(selection) } /// Creates a [Transaction] from the [snippet::Snippet] in a completion response. /// The transaction applies the edit to all cursors. + #[allow(clippy::too_many_arguments)] pub fn generate_transaction_from_snippet( doc: &Rope, selection: &Selection, - start_offset: i128, - end_offset: i128, + edit_offset: Option<(i128, i128)>, snippet: snippet::Snippet, line_ending: &str, include_placeholder: bool, + tab_width: usize, ) -> Transaction { let text = doc.slice(..); - // For each cursor store offsets for the first tabstop - let mut cursor_tabstop_offsets = Vec::>::new(); - let transaction = Transaction::change_by_selection(doc, selection, |range| { - let cursor = range.cursor(text); - let replacement_start = (cursor as i128 + start_offset) as usize; - let replacement_end = (cursor as i128 + end_offset) as usize; - let newline_with_offset = format!( - "{line_ending}{blank:width$}", - line_ending = line_ending, - width = replacement_start - doc.line_to_char(doc.char_to_line(replacement_start)), - blank = "" - ); - - let (replacement, tabstops) = - snippet::render(&snippet, newline_with_offset, include_placeholder); - - let replacement_len = replacement.chars().count(); - cursor_tabstop_offsets.push( - tabstops - .first() - .unwrap_or(&smallvec![(replacement_len, replacement_len)]) - .iter() - .map(|(from, to)| -> (i128, i128) { - ( - *from as i128 - replacement_len as i128, - *to as i128 - replacement_len as i128, - ) - }) - .collect(), - ); - - (replacement_start, replacement_end, Some(replacement.into())) - }); + let mut off = 0i128; + let mut mapped_doc = doc.clone(); + let mut selection_tabstops: SmallVec<[_; 1]> = SmallVec::new(); + let (removed_start, removed_end) = + completion_range(text, edit_offset, selection.primary().cursor(text)) + .expect("transaction must be valid for primary selection"); + let removed_text = text.slice(removed_start..removed_end); - // Create new selection based on the cursor tabstop from above - let mut cursor_tabstop_offsets_iter = cursor_tabstop_offsets.iter(); - let selection = selection - .clone() - .map(transaction.changes()) - .transform_iter(|range| { - cursor_tabstop_offsets_iter - .next() - .unwrap() - .iter() - .map(move |(from, to)| { - Range::new( - (range.anchor as i128 + *from) as usize, - (range.anchor as i128 + *to) as usize, - ) - }) - }); + let (transaction, selection) = Transaction::change_by_selection_ignore_overlapping( + doc, + selection, + |range| { + let cursor = range.cursor(text); + completion_range(text, edit_offset, cursor) + .filter(|(start, end)| text.slice(start..end) == removed_text) + .unwrap_or_else(|| find_completion_range(text, cursor)) + }, + |replacement_start, replacement_end| { + let mapped_replacement_start = (replacement_start as i128 + off) as usize; + let mapped_replacement_end = (replacement_end as i128 + off) as usize; + + let line_idx = mapped_doc.char_to_line(mapped_replacement_start); + let pos_on_line = mapped_replacement_start - mapped_doc.line_to_char(line_idx); + + // we only care about the actual offset here (not virtual text/softwrap) + // so it's ok to use the deprecated function here + #[allow(deprecated)] + let width = helix_core::visual_coords_at_pos( + mapped_doc.line(line_idx), + pos_on_line, + tab_width, + ) + .col; + let newline_with_offset = format!( + "{line_ending}{blank:width$}", + line_ending = line_ending, + blank = "" + ); + + let (replacement, tabstops) = + snippet::render(&snippet, &newline_with_offset, include_placeholder); + selection_tabstops.push((mapped_replacement_start, tabstops)); + mapped_doc.remove(mapped_replacement_start..mapped_replacement_end); + mapped_doc.insert(mapped_replacement_start, &replacement); + off += + replacement_start as i128 - replacement_end as i128 + replacement.len() as i128; + + Some(replacement) + }, + ); - transaction.with_selection(selection) + let changes = transaction.changes(); + if changes.is_empty() { + return transaction; + } + + let mut mapped_selection = SmallVec::with_capacity(selection.len()); + let mut mapped_primary_idx = 0; + let primary_range = selection.primary(); + for (range, (tabstop_anchor, tabstops)) in selection.into_iter().zip(selection_tabstops) { + if range == primary_range { + mapped_primary_idx = mapped_selection.len() + } + + let range = range.map(changes); + let tabstops = tabstops.first().filter(|tabstops| !tabstops.is_empty()); + let Some(tabstops) = tabstops else{ + // no tabstop normal mapping + mapped_selection.push(range); + continue; + }; + + // expand the selection to cover the tabstop to retain the helix selection semantic + // the tabstop closest to the range simply replaces `head` while anchor remains in place + // the remaining tabstops receive their own single-width cursor + if range.head < range.anchor { + let first_tabstop = tabstop_anchor + tabstops[0].1; + + // if selection is forward but was moved to the right it is + // contained entirely in the replacement text, just do a point + // selection (fallback below) + if range.anchor >= first_tabstop { + let range = Range::new(range.anchor, first_tabstop); + mapped_selection.push(range); + let rem_tabstops = tabstops[1..] + .iter() + .map(|tabstop| Range::point(tabstop_anchor + tabstop.1)); + mapped_selection.extend(rem_tabstops); + continue; + } + } else { + let last_idx = tabstops.len() - 1; + let last_tabstop = tabstop_anchor + tabstops[last_idx].1; + + // if selection is forward but was moved to the right it is + // contained entirely in the replacement text, just do a point + // selection (fallback below) + if range.anchor <= last_tabstop { + // we can't properly compute the the next grapheme + // here because the transaction hasn't been applied yet + // that is not a problem because the range gets grapheme aligned anyway + // tough so just adding one will always cause head to be grapheme + // aligned correctly when applied to the document + let range = Range::new(range.anchor, last_tabstop + 1); + mapped_selection.push(range); + let rem_tabstops = tabstops[..last_idx] + .iter() + .map(|tabstop| Range::point(tabstop_anchor + tabstop.0)); + mapped_selection.extend(rem_tabstops); + continue; + } + }; + + let tabstops = tabstops + .iter() + .map(|tabstop| Range::point(tabstop_anchor + tabstop.0)); + mapped_selection.extend(tabstops); + } + + transaction.with_selection(Selection::new(mapped_selection, mapped_primary_idx)) } pub fn generate_transaction_from_edits( diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 336b75cb..99c33781 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -118,7 +118,6 @@ impl Completion { view_id: ViewId, item: &CompletionItem, offset_encoding: helix_lsp::OffsetEncoding, - start_offset: usize, trigger_offset: usize, include_placeholder: bool, ) -> Transaction { @@ -147,28 +146,18 @@ impl Completion { None => return Transaction::new(doc.text()), }; - (start_offset, end_offset, edit.new_text) + (Some((start_offset, end_offset)), edit.new_text) } else { let new_text = item .insert_text .clone() .unwrap_or_else(|| item.label.clone()); - // check that we are still at the correct savepoint // we can still generate a transaction regardless but if the // document changed (and not just the selection) then we will // likely delete the wrong text (same if we applied an edit sent by the LS) debug_assert!(primary_cursor == trigger_offset); - - // TODO: Respect editor.completion_replace? - // Would require detecting the end of the word boundary for every cursor individually. - // We don't do the same for normal `edits, to be consistent we would have to do it for those too - - ( - start_offset as i128 - primary_cursor as i128, - trigger_offset as i128 - primary_cursor as i128, - new_text, - ) + (None, Some(0), new_text) }; if matches!(item.kind, Some(lsp::CompletionItemKind::SNIPPET)) @@ -186,6 +175,7 @@ impl Completion { snippet, doc.line_ending.as_str(), include_placeholder, + doc.tab_width(), ), Err(err) => { log::error!( @@ -232,7 +222,6 @@ impl Completion { view.id, item, offset_encoding, - start_offset, trigger_offset, true, ); @@ -254,7 +243,6 @@ impl Completion { view.id, item, offset_encoding, - start_offset, trigger_offset, false, ); From d63e570e0a4013f5ad703a9b1ce2d19a06630a82 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 9 Mar 2023 23:21:02 +0100 Subject: [PATCH 07/23] treat replace/insertmode consistently, default to insert --- book/src/configuration.md | 1 + helix-lsp/src/lib.rs | 45 +++++++++++++++++++++++---------- helix-term/src/ui/completion.rs | 24 ++++++++++++------ helix-view/src/editor.rs | 4 +++ 4 files changed, 53 insertions(+), 21 deletions(-) diff --git a/book/src/configuration.md b/book/src/configuration.md index aebf5ff0..4b62ca52 100644 --- a/book/src/configuration.md +++ b/book/src/configuration.md @@ -50,6 +50,7 @@ signal to the Helix process on Unix operating systems, such as by using the comm | `auto-save` | Enable automatic saving on the focus moving away from Helix. Requires [focus event support](https://github.com/helix-editor/helix/wiki/Terminal-Support) from your terminal | `false` | | `idle-timeout` | Time in milliseconds since last keypress before idle timers trigger. Used for autocompletion, set to 0 for instant | `400` | | `completion-trigger-len` | The min-length of word under cursor to trigger autocompletion | `2` | +| `completion-replace` | Set to `true` to make completions always replace the entire word and not just the part before the cursor | `false` | | `auto-info` | Whether to display info boxes | `true` | | `true-color` | Set to `true` to override automatic detection of terminal truecolor support in the event of a false negative | `false` | | `rulers` | List of column positions at which to display the rulers. Can be overridden by language specific `rulers` in `languages.toml` file | `[]` | diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index 58e8d83d..1463ccb3 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -250,18 +250,27 @@ pub mod util { /// If the LS did not provide a range for the completion or the range of the /// primary cursor can not be used for the secondary cursor, this function /// can be used to find the completion range for a cursor - fn find_completion_range(text: RopeSlice, cursor: usize) -> (usize, usize) { + fn find_completion_range(text: RopeSlice, replace_mode: bool, cursor: usize) -> (usize, usize) { let start = cursor - text .chars_at(cursor) .reversed() .take_while(|ch| chars::char_is_word(*ch)) .count(); - (start, cursor) + let mut end = cursor; + if replace_mode { + end += text + .chars_at(cursor) + .skip(1) + .take_while(|ch| chars::char_is_word(*ch)) + .count(); + } + (start, end) } fn completion_range( text: RopeSlice, edit_offset: Option<(i128, i128)>, + replace_mode: bool, cursor: usize, ) -> Option<(usize, usize)> { let res = match edit_offset { @@ -276,7 +285,7 @@ pub mod util { } (start_offset as usize, end_offset as usize) } - None => find_completion_range(text, cursor), + None => find_completion_range(text, replace_mode, cursor), }; Some(res) } @@ -287,6 +296,7 @@ pub mod util { doc: &Rope, selection: &Selection, edit_offset: Option<(i128, i128)>, + replace_mode: bool, new_text: String, ) -> Transaction { let replacement: Option = if new_text.is_empty() { @@ -296,9 +306,13 @@ pub mod util { }; let text = doc.slice(..); - let (removed_start, removed_end) = - completion_range(text, edit_offset, selection.primary().cursor(text)) - .expect("transaction must be valid for primary selection"); + let (removed_start, removed_end) = completion_range( + text, + edit_offset, + replace_mode, + selection.primary().cursor(text), + ) + .expect("transaction must be valid for primary selection"); let removed_text = text.slice(removed_start..removed_end); let (transaction, mut selection) = Transaction::change_by_selection_ignore_overlapping( @@ -306,9 +320,9 @@ pub mod util { selection, |range| { let cursor = range.cursor(text); - completion_range(text, edit_offset, cursor) + completion_range(text, edit_offset, replace_mode, cursor) .filter(|(start, end)| text.slice(start..end) == removed_text) - .unwrap_or_else(|| find_completion_range(text, cursor)) + .unwrap_or_else(|| find_completion_range(text, replace_mode, cursor)) }, |_, _| replacement.clone(), ); @@ -326,6 +340,7 @@ pub mod util { doc: &Rope, selection: &Selection, edit_offset: Option<(i128, i128)>, + replace_mode: bool, snippet: snippet::Snippet, line_ending: &str, include_placeholder: bool, @@ -336,9 +351,13 @@ pub mod util { let mut off = 0i128; let mut mapped_doc = doc.clone(); let mut selection_tabstops: SmallVec<[_; 1]> = SmallVec::new(); - let (removed_start, removed_end) = - completion_range(text, edit_offset, selection.primary().cursor(text)) - .expect("transaction must be valid for primary selection"); + let (removed_start, removed_end) = completion_range( + text, + edit_offset, + replace_mode, + selection.primary().cursor(text), + ) + .expect("transaction must be valid for primary selection"); let removed_text = text.slice(removed_start..removed_end); let (transaction, selection) = Transaction::change_by_selection_ignore_overlapping( @@ -346,9 +365,9 @@ pub mod util { selection, |range| { let cursor = range.cursor(text); - completion_range(text, edit_offset, cursor) + completion_range(text, edit_offset, replace_mode, cursor) .filter(|(start, end)| text.slice(start..end) == removed_text) - .unwrap_or_else(|| find_completion_range(text, cursor)) + .unwrap_or_else(|| find_completion_range(text, replace_mode, cursor)) }, |replacement_start, replacement_end| { let mapped_replacement_start = (replacement_start as i128 + off) as usize; diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 99c33781..6303793b 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -108,6 +108,7 @@ impl Completion { start_offset: usize, trigger_offset: usize, ) -> Self { + let replace_mode = editor.config().completion_replace; // Sort completion items according to their preselect status (given by the LSP server) items.sort_by_key(|item| !item.preselect.unwrap_or(false)); @@ -120,18 +121,23 @@ impl Completion { offset_encoding: helix_lsp::OffsetEncoding, trigger_offset: usize, include_placeholder: bool, + replace_mode: bool, ) -> Transaction { use helix_lsp::snippet; let selection = doc.selection(view_id); let text = doc.text().slice(..); let primary_cursor = selection.primary().cursor(text); - let (start_offset, end_offset, new_text) = if let Some(edit) = &item.text_edit { + let (edit_offset, new_text) = if let Some(edit) = &item.text_edit { let edit = match edit { lsp::CompletionTextEdit::Edit(edit) => edit.clone(), lsp::CompletionTextEdit::InsertAndReplace(item) => { - // TODO: support using "insert" instead of "replace" via user config - lsp::TextEdit::new(item.replace, item.new_text.clone()) + let range = if replace_mode { + item.replace + } else { + item.insert + }; + lsp::TextEdit::new(range, item.new_text.clone()) } }; @@ -157,7 +163,7 @@ impl Completion { // document changed (and not just the selection) then we will // likely delete the wrong text (same if we applied an edit sent by the LS) debug_assert!(primary_cursor == trigger_offset); - (None, Some(0), new_text) + (None, new_text) }; if matches!(item.kind, Some(lsp::CompletionItemKind::SNIPPET)) @@ -170,8 +176,8 @@ impl Completion { Ok(snippet) => util::generate_transaction_from_snippet( doc.text(), selection, - start_offset, - end_offset, + edit_offset, + replace_mode, snippet, doc.line_ending.as_str(), include_placeholder, @@ -190,8 +196,8 @@ impl Completion { util::generate_transaction_from_completion_edit( doc.text(), selection, - start_offset, - end_offset, + edit_offset, + replace_mode, new_text, ) } @@ -224,6 +230,7 @@ impl Completion { offset_encoding, trigger_offset, true, + replace_mode, ); // initialize a savepoint @@ -245,6 +252,7 @@ impl Completion { offset_encoding, trigger_offset, false, + replace_mode, ); doc.apply(&transaction, view.id); diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index c6541105..1b4664ff 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -251,6 +251,9 @@ pub struct Config { )] pub idle_timeout: Duration, pub completion_trigger_len: u8, + /// Whether to instruct the LSP to replace the entire word when applying a completion + /// or to only insert new text + pub completion_replace: bool, /// Whether to display infoboxes. Defaults to true. pub auto_info: bool, pub file_picker: FilePickerConfig, @@ -738,6 +741,7 @@ impl Default for Config { color_modes: false, soft_wrap: SoftWrap::default(), text_width: 80, + completion_replace: false, } } } From 98415f288ffa043520b0c85bc4464dc44b85f948 Mon Sep 17 00:00:00 2001 From: Philipp Mildenberger Date: Fri, 10 Mar 2023 17:32:45 +0100 Subject: [PATCH 08/23] Improved yuck highlighting (and parser), and introduced a tag.builtin scope (#6242) --- book/src/themes.md | 1 + languages.toml | 2 +- runtime/queries/yuck/highlights.scm | 135 ++++++++++++++++++---------- runtime/queries/yuck/injections.scm | 2 +- 4 files changed, 91 insertions(+), 49 deletions(-) diff --git a/book/src/themes.md b/book/src/themes.md index af238e94..5ddd4f2c 100644 --- a/book/src/themes.md +++ b/book/src/themes.md @@ -215,6 +215,7 @@ We use a similar set of scopes as - `special` (preprocessor in C) - `tag` - Tags (e.g. `` in HTML) + - `builtin` - `namespace` diff --git a/languages.toml b/languages.toml index 8697f9fc..86f4a64d 100644 --- a/languages.toml +++ b/languages.toml @@ -2206,7 +2206,7 @@ indent = { tab-width = 2, unit = " " } [[grammar]] name = "yuck" -source = { git = "https://github.com/Philipp-M/tree-sitter-yuck", rev = "9e97da5773f82123a8c8cccf8f7e795d140ed7d1" } +source = { git = "https://github.com/Philipp-M/tree-sitter-yuck", rev = "e3d91a3c65decdea467adebe4127b8366fa47919" } [[language]] name = "prql" diff --git a/runtime/queries/yuck/highlights.scm b/runtime/queries/yuck/highlights.scm index 483348a8..9f116f15 100644 --- a/runtime/queries/yuck/highlights.scm +++ b/runtime/queries/yuck/highlights.scm @@ -1,66 +1,107 @@ +; Errors + (ERROR) @error -(line_comment) @comment +; Comments -; keywords and symbols +(comment) @comment -(keyword) @keyword -(symbol) @tag +; Operators -; literals +[ + "+" + "-" + "*" + "/" + "%" + "||" + "&&" + "==" + "!=" + "=~" + ">" + "<" + ">=" + "<=" + "!" + "?." + "?:" +] @operator -(bool_literal) @constant.builtin.boolean -(num_literal) @constant.numeric +(ternary_expression + ["?" ":"] @operator) -; strings -(string_interpolation - (string_interpolation_start) @punctuation.special - (string_interpolation_end) @punctuation.special) +; Punctuation + +[ ":" "." "," ] @punctuation.delimiter + +[ "{" "}" "[" "]" "(" ")" ] @punctuation.bracket + +; Literals + +(number (float)) @constant.numeric.float + +(number (integer)) @constant.numeric.integer + +(boolean) @constant.builtin.boolean + +; Strings (escape_sequence) @constant.character.escape -(string - [ - (unescaped_single_quote_string_fragment) - (unescaped_double_quote_string_fragment) - (unescaped_backtick_string_fragment) - "\"" - "'" - "`" - ]) @string +(string_interpolation + "${" @punctuation.special + "}" @punctuation.special) -; operators and general punctuation +[ (string_fragment) "\"" "'" "`" ] @string -(unary_expression - operator: _ @operator) +; Attributes & Fields -(binary_expression - operator: _ @operator) +(keyword) @attribute -(ternary_expression - operator: _ @operator) +; Functions -[ - ":" - "." - "," -] @punctuation.delimiter +(function_call + name: (ident) @function) -[ - "(" - ")" - "[" - "]" - "{" - "}" -] @punctuation.bracket -[ - ":" - "." - "," -] @punctuation.delimiter +; Variables -; Rest (general identifiers that are not yet catched) +(ident) @variable + +(array + (symbol) @variable) + +; Builtin widgets + +(list . + ((symbol) @tag.builtin + (#match? @tag.builtin "^(box|button|calendar|centerbox|checkbox|circular-progress|color-button|color-chooser|combo-box-text|eventbox|expander|graph|image|input|label|literal|overlay|progress|revealer|scale|scroll|transform)$"))) + +; Keywords + +; I think there's a bug in tree-sitter the anchor doesn't seem to be working, see +; https://github.com/tree-sitter/tree-sitter/pull/2107 +(list . + ((symbol) @keyword + (#match? @keyword "^(defwindow|defwidget|defvar|defpoll|deflisten|geometry|children|struts)$"))) + +(list . + ((symbol) @keyword.control.import + (#eq? @keyword.control.import "include"))) + +; Loop + +(loop_widget . "for" @keyword.control.repeat . (symbol) @variable . "in" @keyword.operator . (symbol) @variable) + +(loop_widget . "for" @keyword.control.repeat . (symbol) @variable . "in" @keyword.operator) + +; Tags + +; TODO apply to every symbol in list? I think it should probably only be applied to the first child of the list +(list + (symbol) @tag) + +; Other stuff that has not been catched by the previous queries yet -(index) @variable (ident) @variable +(index) @variable diff --git a/runtime/queries/yuck/injections.scm b/runtime/queries/yuck/injections.scm index d3fdb0ca..321c90ad 100644 --- a/runtime/queries/yuck/injections.scm +++ b/runtime/queries/yuck/injections.scm @@ -1,2 +1,2 @@ -((line_comment) @injection.content +((comment) @injection.content (#set! injection.language "comment")) From 1661e4b5e1d8ebfef28f798fcb86ba2656373ba0 Mon Sep 17 00:00:00 2001 From: Dimitar Gyurov Date: Fri, 10 Mar 2023 23:42:42 +0100 Subject: [PATCH 09/23] Add a version-control statusline element (#5682) --- Cargo.lock | 1 + book/src/configuration.md | 1 + helix-term/src/ui/statusline.rs | 14 ++++++++++++++ helix-vcs/Cargo.toml | 1 + helix-vcs/src/git.rs | 17 +++++++++++++++++ helix-vcs/src/lib.rs | 14 +++++++++++++- helix-view/src/document.rs | 16 ++++++++++++++++ helix-view/src/editor.rs | 4 ++++ 8 files changed, 67 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a1a9eae4..de985bca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1212,6 +1212,7 @@ dependencies = [ name = "helix-vcs" version = "0.6.0" dependencies = [ + "arc-swap", "gix", "helix-core", "imara-diff", diff --git a/book/src/configuration.md b/book/src/configuration.md index 4b62ca52..e698646b 100644 --- a/book/src/configuration.md +++ b/book/src/configuration.md @@ -111,6 +111,7 @@ The following statusline elements can be configured: | `position-percentage` | The cursor position as a percentage of the total number of lines | | `separator` | The string defined in `editor.statusline.separator` (defaults to `"│"`) | | `spacer` | Inserts a space between elements (multiple/contiguous spacers may be specified) | +| `version-control` | The current branch name or detached commit hash of the opened workspace | ### `[editor.lsp]` Section diff --git a/helix-term/src/ui/statusline.rs b/helix-term/src/ui/statusline.rs index 3e7065b8..88786351 100644 --- a/helix-term/src/ui/statusline.rs +++ b/helix-term/src/ui/statusline.rs @@ -159,6 +159,7 @@ where helix_view::editor::StatusLineElement::TotalLineNumbers => render_total_line_numbers, helix_view::editor::StatusLineElement::Separator => render_separator, helix_view::editor::StatusLineElement::Spacer => render_spacer, + helix_view::editor::StatusLineElement::VersionControl => render_version_control, } } @@ -476,3 +477,16 @@ where { write(context, String::from(" "), None); } + +fn render_version_control(context: &mut RenderContext, write: F) +where + F: Fn(&mut RenderContext, String, Option