Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve sorting for inline menu (codeaction + completion) #4134

Merged
merged 6 commits into from
Oct 21, 2022
Merged

Improve sorting for inline menu (codeaction + completion) #4134

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6b16b9d
sort codeaction by their kind instead of alphabetically
pascalkuthe Oct 6, 2022
63a54ee
sort autocompletins by fuzzy match
pascalkuthe Oct 7, 2022
a36bb1d
use stable sort instead of allocating new vectors
pascalkuthe Oct 8, 2022
f813ccc
use permalink to vscode repo
pascalkuthe Oct 11, 2022
3f4296b
never sort menu items when no fuzzy matching is possible
pascalkuthe Oct 13, 2022
2feba44
Sort by fixed diagnostics/is_preffered within codeaction categories
pascalkuthe Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 52 additions & 22 deletions helix-term/src/commands/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,43 +452,73 @@ pub fn code_action(cx: &mut Context) {
cx.callback(
future,
move |editor, compositor, response: Option<lsp::CodeActionResponse>| {
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 (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), ..
}) => {
the-mikedavis marked this conversation as resolved.
Show resolved Hide resolved
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,
});

// 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());
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);
Expand Down
2 changes: 1 addition & 1 deletion helix-term/src/ui/completion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 8 additions & 6 deletions helix-term/src/ui/menu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ impl<T: Item> Menu<T> {
// rendering)
pub fn new(
options: Vec<T>,
sort: bool,
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
editor_data: <T as Item>::Data,
callback_fn: impl Fn(&mut Editor, Option<&T>, MenuEvent) + 'static,
) -> Self {
Expand All @@ -91,8 +92,12 @@ impl<T: Item> Menu<T> {
recalculate: true,
};

// TODO: scoring on empty input should just use a fastpath
menu.score("");
archseer marked this conversation as resolved.
Show resolved Hide resolved
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
}
Expand All @@ -112,10 +117,7 @@ impl<T: Item> Menu<T> {
.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)
});
Comment on lines -115 to -118
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this change impact other menus now?

Copy link
Member Author

@pascalkuthe pascalkuthe Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The inline menu is only used for codeactions and autocompletions as far as I found.
For code-actions this function is never called.

For auto-completions sorting by fuzzy match is more desirable and infarct exactly #3215.
I have opted to include this change in this PR because almost all other changes from #3215 become unnecessary and sorting by fuzzy match only requires changing this single line. I can spin this change out into a seperate followup PR if you want to review it seperatly. I only included it because it was such a small change and just made sense at this point (I think it was only disabled previously because fuzzy matching sorted codeactions badly).

I have also updated the PR description and PR title a while ago to reflect that this changes the behaviour of all inline menus:

Edit: This PR was originally just about code-actions, however I found #3215 and noticed that the changes there are sort of the same as in this PR: both PRs needed to handle code-actions separately from auto-completion sorting, #3215 sorted the code-actions alphabetically while this PR sorts them by category. Enabling fuzzy sorting for auto completions on top this PR is a single-line change and I have therefore opted to include it in this PR. I can drop the commit again and spin that out into a separate PR if this is controversial. The change is trivial and it made sense to me to include it here. fixes #2508

self.matches.sort_unstable_by_key(|(_, score)| -score);

// reset cursor position
self.cursor = None;
Expand Down