From 6b16b9dfc86234de601fef0b03cafc25220c4bb0 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Fri, 7 Oct 2022 01:44:53 +0200 Subject: [PATCH 1/6] sort codeaction by their kind instead of alphabetically --- helix-term/src/commands/lsp.rs | 84 ++++++++++++++++++++++++--------- helix-term/src/ui/completion.rs | 2 +- helix-term/src/ui/menu.rs | 9 +++- 3 files changed, 70 insertions(+), 25 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 1113b44ecdf7..17d87eb95033 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -452,43 +452,83 @@ pub fn code_action(cx: &mut Context) { cx.callback( future, move |editor, compositor, response: Option| { - let actions = match response { + let mut actions = match response { Some(a) => a, None => return, }; + if actions.is_empty() { editor.set_status("No code actions available"); return; } - let mut picker = ui::Menu::new(actions, (), move |editor, code_action, event| { - if event != PromptEvent::Validate { - return; - } + // sort by CodeActionKind + // this ensures that the most relevant codeactions (quickfix) show up first + // while more situational commands (like refactors) show up later + // this behaviour is modeled after the behaviour of vscode (editor/contrib/codeAction/browser/codeActionWidget.ts) + + let mut categories = vec![Vec::new(); 8]; + for action in actions.drain(..) { + let category = match &action { + lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + kind: Some(kind), + .. + }) => { + let mut components = kind.as_str().split('.'); + + match components.next() { + Some("quickfix") => 0, + Some("refactor") => match components.next() { + Some("extract") => 1, + Some("inline") => 2, + Some("rewrite") => 3, + Some("move") => 4, + Some("surround") => 5, + _ => 7, + }, + Some("source") => 6, + _ => 7, + } + } + _ => 7, + }; + + categories[category].push(action); + } - // always present here - let code_action = code_action.unwrap(); + for category in categories { + actions.extend(category.into_iter()) + } - match code_action { - lsp::CodeActionOrCommand::Command(command) => { - log::debug!("code action command: {:?}", command); - execute_lsp_command(editor, command.clone()); + let mut picker = + ui::Menu::new(actions, false, (), move |editor, code_action, event| { + if event != PromptEvent::Validate { + return; } - lsp::CodeActionOrCommand::CodeAction(code_action) => { - log::debug!("code action: {:?}", code_action); - if let Some(ref workspace_edit) = code_action.edit { - log::debug!("edit: {:?}", workspace_edit); - apply_workspace_edit(editor, offset_encoding, workspace_edit); - } - // if code action provides both edit and command first the edit - // should be applied and then the command - if let Some(command) = &code_action.command { + // always present here + let code_action = code_action.unwrap(); + + match code_action { + lsp::CodeActionOrCommand::Command(command) => { + log::debug!("code action command: {:?}", command); execute_lsp_command(editor, command.clone()); } + lsp::CodeActionOrCommand::CodeAction(code_action) => { + log::debug!("code action: {:?}", code_action); + if let Some(ref workspace_edit) = code_action.edit { + log::debug!("edit: {:?}", workspace_edit); + apply_workspace_edit(editor, offset_encoding, workspace_edit); + } + + // if code action provides both edit and command first the edit + // should be applied and then the command + if let Some(command) = &code_action.command { + execute_lsp_command(editor, command.clone()); + } + } } - } - }); + }); picker.move_down(); // pre-select the first item let popup = Popup::new("code-action", picker); diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 2d7d4f925695..7817f7db0b70 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -97,7 +97,7 @@ impl Completion { start_offset: usize, trigger_offset: usize, ) -> Self { - let menu = Menu::new(items, (), move |editor: &mut Editor, item, event| { + let menu = Menu::new(items, true, (), move |editor: &mut Editor, item, event| { fn item_to_transaction( doc: &Document, item: &CompletionItem, diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index 1d247b1adc82..4a9670bd7c60 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -74,6 +74,7 @@ impl Menu { // rendering) pub fn new( options: Vec, + sort: bool, editor_data: ::Data, callback_fn: impl Fn(&mut Editor, Option<&T>, MenuEvent) + 'static, ) -> Self { @@ -91,8 +92,12 @@ impl Menu { recalculate: true, }; - // TODO: scoring on empty input should just use a fastpath - menu.score(""); + if sort { + // TODO: scoring on empty input should just use a fastpath + menu.score(""); + } else { + menu.matches = (0..menu.options.len()).map(|i| (i, 0)).collect(); + } menu } From 63a54ee19b522cb0b085f1da0f3dfe07174179d5 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Fri, 7 Oct 2022 12:15:52 +0200 Subject: [PATCH 2/6] sort autocompletins by fuzzy match --- helix-term/src/ui/menu.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index 4a9670bd7c60..ee11bfbcfb50 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -117,10 +117,7 @@ impl Menu { .map(|score| (index, score)) }), ); - // matches.sort_unstable_by_key(|(_, score)| -score); - self.matches.sort_unstable_by_key(|(index, _score)| { - self.options[*index].sort_text(&self.editor_data) - }); + self.matches.sort_unstable_by_key(|(_, score)| -score); // reset cursor position self.cursor = None; From a36bb1d27cf02c8766f5e95b07916b1eadc6af1c Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Sat, 8 Oct 2022 14:18:53 +0200 Subject: [PATCH 3/6] use stable sort instead of allocating new vectors --- helix-term/src/commands/lsp.rs | 50 ++++++++++++++-------------------- 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 17d87eb95033..3bd9ac9aa883 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -467,38 +467,28 @@ pub fn code_action(cx: &mut Context) { // while more situational commands (like refactors) show up later // this behaviour is modeled after the behaviour of vscode (editor/contrib/codeAction/browser/codeActionWidget.ts) - let mut categories = vec![Vec::new(); 8]; - for action in actions.drain(..) { - let category = match &action { - lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { - kind: Some(kind), - .. - }) => { - let mut components = kind.as_str().split('.'); - - match components.next() { - Some("quickfix") => 0, - Some("refactor") => match components.next() { - Some("extract") => 1, - Some("inline") => 2, - Some("rewrite") => 3, - Some("move") => 4, - Some("surround") => 5, - _ => 7, - }, - Some("source") => 6, + actions.sort_by_key(|action| match &action { + lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { + kind: Some(kind), .. + }) => { + let mut components = kind.as_str().split('.'); + + match components.next() { + Some("quickfix") => 0, + Some("refactor") => match components.next() { + Some("extract") => 1, + Some("inline") => 2, + Some("rewrite") => 3, + Some("move") => 4, + Some("surround") => 5, _ => 7, - } + }, + Some("source") => 6, + _ => 7, } - _ => 7, - }; - - categories[category].push(action); - } - - for category in categories { - actions.extend(category.into_iter()) - } + } + _ => 7, + }); let mut picker = ui::Menu::new(actions, false, (), move |editor, code_action, event| { From f813ccc1fc91602e67750b617f3f5262b02f70a0 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Tue, 11 Oct 2022 19:37:22 +0200 Subject: [PATCH 4/6] use permalink to vscode repo --- helix-term/src/commands/lsp.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 3bd9ac9aa883..39be3069f7c0 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -465,7 +465,7 @@ pub fn code_action(cx: &mut Context) { // sort by CodeActionKind // this ensures that the most relevant codeactions (quickfix) show up first // while more situational commands (like refactors) show up later - // this behaviour is modeled after the behaviour of vscode (editor/contrib/codeAction/browser/codeActionWidget.ts) + // this behaviour is modeled after the behaviour of vscode (https://github.com/microsoft/vscode/blob/eaec601dd69aeb4abb63b9601a6f44308c8d8c6e/src/vs/editor/contrib/codeAction/browser/codeActionWidget.ts) actions.sort_by_key(|action| match &action { lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { From 3f4296b844a2e55e440b0504cfa466ec90425933 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Thu, 13 Oct 2022 20:10:10 +0200 Subject: [PATCH 5/6] never sort menu items when no fuzzy matching is possible --- helix-term/src/commands/lsp.rs | 45 ++++++++++++++++----------------- helix-term/src/ui/completion.rs | 2 +- helix-term/src/ui/menu.rs | 15 +++-------- 3 files changed, 26 insertions(+), 36 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 39be3069f7c0..0d23e8942c4c 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -490,35 +490,34 @@ pub fn code_action(cx: &mut Context) { _ => 7, }); - let mut picker = - ui::Menu::new(actions, false, (), move |editor, code_action, event| { - if event != PromptEvent::Validate { - return; - } + let mut picker = ui::Menu::new(actions, (), move |editor, code_action, event| { + if event != PromptEvent::Validate { + return; + } - // always present here - let code_action = code_action.unwrap(); + // always present here + let code_action = code_action.unwrap(); - match code_action { - lsp::CodeActionOrCommand::Command(command) => { - log::debug!("code action command: {:?}", command); - execute_lsp_command(editor, command.clone()); + match code_action { + lsp::CodeActionOrCommand::Command(command) => { + log::debug!("code action command: {:?}", command); + execute_lsp_command(editor, command.clone()); + } + lsp::CodeActionOrCommand::CodeAction(code_action) => { + log::debug!("code action: {:?}", code_action); + if let Some(ref workspace_edit) = code_action.edit { + log::debug!("edit: {:?}", workspace_edit); + apply_workspace_edit(editor, offset_encoding, workspace_edit); } - lsp::CodeActionOrCommand::CodeAction(code_action) => { - log::debug!("code action: {:?}", code_action); - if let Some(ref workspace_edit) = code_action.edit { - log::debug!("edit: {:?}", workspace_edit); - apply_workspace_edit(editor, offset_encoding, workspace_edit); - } - // if code action provides both edit and command first the edit - // should be applied and then the command - if let Some(command) = &code_action.command { - execute_lsp_command(editor, command.clone()); - } + // if code action provides both edit and command first the edit + // should be applied and then the command + if let Some(command) = &code_action.command { + execute_lsp_command(editor, command.clone()); } } - }); + } + }); picker.move_down(); // pre-select the first item let popup = Popup::new("code-action", picker); diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 7817f7db0b70..2d7d4f925695 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -97,7 +97,7 @@ impl Completion { start_offset: usize, trigger_offset: usize, ) -> Self { - let menu = Menu::new(items, true, (), move |editor: &mut Editor, item, event| { + let menu = Menu::new(items, (), move |editor: &mut Editor, item, event| { fn item_to_transaction( doc: &Document, item: &CompletionItem, diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index ee11bfbcfb50..f20561ee2c3b 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -74,15 +74,15 @@ impl Menu { // rendering) pub fn new( options: Vec, - sort: bool, editor_data: ::Data, callback_fn: impl Fn(&mut Editor, Option<&T>, MenuEvent) + 'static, ) -> Self { - let mut menu = Self { + let matches = (0..options.len()).map(|i| (i, 0)).collect(); + Self { options, editor_data, matcher: Box::new(Matcher::default()), - matches: Vec::new(), + matches, cursor: None, widths: Vec::new(), callback_fn: Box::new(callback_fn), @@ -90,16 +90,7 @@ impl Menu { size: (0, 0), viewport: (0, 0), recalculate: true, - }; - - if sort { - // TODO: scoring on empty input should just use a fastpath - menu.score(""); - } else { - menu.matches = (0..menu.options.len()).map(|i| (i, 0)).collect(); } - - menu } pub fn score(&mut self, pattern: &str) { From 2feba449e00fbf992408e22cf35ce7fd03d66144 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Fri, 14 Oct 2022 11:50:09 +0200 Subject: [PATCH 6/6] Sort by fixed diagnostics/is_preffered within codeaction categories --- helix-term/src/commands/lsp.rs | 128 +++++++++++++++++++++++++-------- 1 file changed, 100 insertions(+), 28 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 0d23e8942c4c..db9214e7404c 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -1,6 +1,6 @@ use helix_lsp::{ block_on, - lsp::{self, DiagnosticSeverity, NumberOrString}, + lsp::{self, CodeAction, CodeActionOrCommand, DiagnosticSeverity, NumberOrString}, util::{diagnostic_to_lsp_diagnostic, lsp_pos_to_pos, lsp_range_to_range, range_to_lsp_range}, OffsetEncoding, }; @@ -18,7 +18,7 @@ use crate::{ }, }; -use std::{borrow::Cow, collections::BTreeMap, path::PathBuf, sync::Arc}; +use std::{borrow::Cow, cmp::Ordering, collections::BTreeMap, path::PathBuf, sync::Arc}; /// Gets the language server that is attached to a document, and /// if it's not active displays a status message. Using this macro @@ -211,7 +211,6 @@ fn sym_picker( Ok(path) => path, Err(_) => { let err = format!("unable to convert URI to filepath: {}", uri); - log::error!("{}", err); cx.editor.set_error(err); return; } @@ -421,6 +420,63 @@ impl ui::menu::Item for lsp::CodeActionOrCommand { } } +/// Determines the category of the `CodeAction` using the `CodeAction::kind` field. +/// Returns a number that represent these categories. +/// Categories with a lower number should be displayed first. +/// +/// +/// While the `kind` field is defined as open ended in the LSP spec (any value may be used) +/// in practice a closed set of common values (mostly suggested in the LSP spec) are used. +/// VSCode displays each of these categories seperatly (seperated by a heading in the codeactions picker) +/// to make them easier to navigate. Helix does not display these headings to the user. +/// However it does sort code actions by their categories to achieve the same order as the VScode picker, +/// just without the headings. +/// +/// The order used here is modeled after the [vscode sourcecode](https://github.com/microsoft/vscode/blob/eaec601dd69aeb4abb63b9601a6f44308c8d8c6e/src/vs/editor/contrib/codeAction/browser/codeActionWidget.ts>) +fn action_category(action: &CodeActionOrCommand) -> u32 { + if let CodeActionOrCommand::CodeAction(CodeAction { + kind: Some(kind), .. + }) = action + { + let mut components = kind.as_str().split('.'); + match components.next() { + Some("quickfix") => 0, + Some("refactor") => match components.next() { + Some("extract") => 1, + Some("inline") => 2, + Some("rewrite") => 3, + Some("move") => 4, + Some("surround") => 5, + _ => 7, + }, + Some("source") => 6, + _ => 7, + } + } else { + 7 + } +} + +fn action_prefered(action: &CodeActionOrCommand) -> bool { + matches!( + action, + CodeActionOrCommand::CodeAction(CodeAction { + is_preferred: Some(true), + .. + }) + ) +} + +fn action_fixes_diagnostics(action: &CodeActionOrCommand) -> bool { + matches!( + action, + CodeActionOrCommand::CodeAction(CodeAction { + diagnostics: Some(diagnostics), + .. + }) if !diagnostics.is_empty() + ) +} + pub fn code_action(cx: &mut Context) { let (view, doc) = current!(cx.editor); @@ -457,37 +513,53 @@ pub fn code_action(cx: &mut Context) { None => return, }; + // remove disabled code actions + actions.retain(|action| { + matches!( + action, + CodeActionOrCommand::Command(_) + | CodeActionOrCommand::CodeAction(CodeAction { disabled: None, .. }) + ) + }); + if actions.is_empty() { editor.set_status("No code actions available"); return; } - // sort by CodeActionKind - // this ensures that the most relevant codeactions (quickfix) show up first - // while more situational commands (like refactors) show up later - // this behaviour is modeled after the behaviour of vscode (https://github.com/microsoft/vscode/blob/eaec601dd69aeb4abb63b9601a6f44308c8d8c6e/src/vs/editor/contrib/codeAction/browser/codeActionWidget.ts) - - actions.sort_by_key(|action| match &action { - lsp::CodeActionOrCommand::CodeAction(lsp::CodeAction { - kind: Some(kind), .. - }) => { - let mut components = kind.as_str().split('.'); - - match components.next() { - Some("quickfix") => 0, - Some("refactor") => match components.next() { - Some("extract") => 1, - Some("inline") => 2, - Some("rewrite") => 3, - Some("move") => 4, - Some("surround") => 5, - _ => 7, - }, - Some("source") => 6, - _ => 7, - } + // Sort codeactions into a useful order. This behaviour is only partially described in the LSP spec. + // Many details are modeled after vscode because langauge servers are usually tested against it. + // VScode sorts the codeaction two times: + // + // First the codeactions that fix some diagnostics are moved to the front. + // If both codeactions fix some diagnostics (or both fix none) the codeaction + // that is marked with `is_preffered` is shown first. The codeactions are then shown in seperate + // submenus that only contain a certain category (see `action_category`) of actions. + // + // Below this done in in a single sorting step + actions.sort_by(|action1, action2| { + // sort actions by category + let order = action_category(action1).cmp(&action_category(action2)); + if order != Ordering::Equal { + return order; } - _ => 7, + // within the categories sort by relevancy. + // Modeled after the `codeActionsComparator` function in vscode: + // https://github.com/microsoft/vscode/blob/eaec601dd69aeb4abb63b9601a6f44308c8d8c6e/src/vs/editor/contrib/codeAction/browser/codeAction.ts + + // if one code action fixes a diagnostic but the other one doesn't show it first + let order = action_fixes_diagnostics(action1) + .cmp(&action_fixes_diagnostics(action2)) + .reverse(); + if order != Ordering::Equal { + return order; + } + + // if one of the codeactions is marked as prefered show it first + // otherwise keep the original LSP sorting + action_prefered(action1) + .cmp(&action_prefered(action2)) + .reverse() }); let mut picker = ui::Menu::new(actions, (), move |editor, code_action, event| {