From 7c0cc64bd77315723c2178b691ef9a6428240f2b Mon Sep 17 00:00:00 2001 From: Gokul Soumya Date: Tue, 19 Jul 2022 21:28:14 +0530 Subject: [PATCH 1/6] Add force_score() for scoring picker items without optimizations --- helix-term/src/ui/picker.rs | 45 +++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/helix-term/src/ui/picker.rs b/helix-term/src/ui/picker.rs index 5e9ca3d887a5..8b5d20eca6b8 100644 --- a/helix-term/src/ui/picker.rs +++ b/helix-term/src/ui/picker.rs @@ -469,34 +469,41 @@ impl Picker { self.matches.sort_unstable(); } else { - let query = FuzzyQuery::new(pattern); - self.matches.clear(); - self.matches.extend( - self.options - .iter() - .enumerate() - .filter_map(|(index, option)| { - let text = option.filter_text(&self.editor_data); - - query - .fuzzy_match(&text, &self.matcher) - .map(|score| PickerMatch { - index, - score, - len: text.chars().count(), - }) - }), - ); - self.matches.sort_unstable(); + self.force_score(); } log::debug!("picker score {:?}", Instant::now().duration_since(now)); // reset cursor position self.cursor = 0; + let pattern = self.prompt.line(); self.previous_pattern.clone_from(pattern); } + pub fn force_score(&mut self) { + let pattern = self.prompt.line(); + + let query = FuzzyQuery::new(pattern); + self.matches.clear(); + self.matches.extend( + self.options + .iter() + .enumerate() + .filter_map(|(index, option)| { + let text = option.filter_text(&self.editor_data); + + query + .fuzzy_match(&text, &self.matcher) + .map(|score| PickerMatch { + index, + score, + len: text.chars().count(), + }) + }), + ); + self.matches.sort_unstable(); + } + /// Move the cursor by a number of lines, either down (`Forward`) or up (`Backward`) pub fn move_by(&mut self, amount: usize, direction: Direction) { let len = self.matches.len(); From d45d2eae14e26135cdf66dee567d71c99e622cda Mon Sep 17 00:00:00 2001 From: Gokul Soumya Date: Tue, 19 Jul 2022 21:49:02 +0530 Subject: [PATCH 2/6] Add DynamicPicker for updating options on every key --- helix-term/src/ui/mod.rs | 2 +- helix-term/src/ui/overlay.rs | 4 ++ helix-term/src/ui/picker.rs | 74 +++++++++++++++++++++++++++++++++++- 3 files changed, 78 insertions(+), 2 deletions(-) diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index f61c4c450c7d..17b06638ca5a 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -19,7 +19,7 @@ pub use completion::Completion; pub use editor::EditorView; pub use markdown::Markdown; pub use menu::Menu; -pub use picker::{FileLocation, FilePicker, Picker}; +pub use picker::{DynamicPicker, FileLocation, FilePicker, Picker}; pub use popup::Popup; pub use prompt::{Prompt, PromptEvent}; pub use spinner::{ProgressSpinners, Spinner}; diff --git a/helix-term/src/ui/overlay.rs b/helix-term/src/ui/overlay.rs index 0b8a93ae833d..5b2bc806093d 100644 --- a/helix-term/src/ui/overlay.rs +++ b/helix-term/src/ui/overlay.rs @@ -69,4 +69,8 @@ impl Component for Overlay { let dimensions = (self.calc_child_size)(area); self.content.cursor(dimensions, ctx) } + + fn id(&self) -> Option<&'static str> { + self.content.id() + } } diff --git a/helix-term/src/ui/picker.rs b/helix-term/src/ui/picker.rs index 8b5d20eca6b8..821a282cf61f 100644 --- a/helix-term/src/ui/picker.rs +++ b/helix-term/src/ui/picker.rs @@ -3,6 +3,7 @@ use crate::{ ctrl, key, shift, ui::{self, fuzzy_match::FuzzyQuery, EditorView}, }; +use futures_util::future::BoxFuture; use tui::{ buffer::Buffer as Surface, widgets::{Block, BorderType, Borders}, @@ -22,7 +23,7 @@ use helix_view::{ Document, DocumentId, Editor, }; -use super::menu::Item; +use super::{menu::Item, overlay::Overlay}; pub const MIN_AREA_WIDTH_FOR_PREVIEW: u16 = 72; /// Biggest file size to preview in bytes @@ -751,3 +752,74 @@ impl Component for Picker { self.prompt.cursor(area, editor) } } + +/// Returns a new list of options to replace the contents of the picker +/// when called with the current picker query, +pub type DynQueryCallback = + Box BoxFuture<'static, anyhow::Result>>>; + +/// A picker that updates its contents via a callback whenever the +/// query string changes. Useful for live grep, workspace symbols, etc. +pub struct DynamicPicker { + file_picker: FilePicker, + query_callback: DynQueryCallback, +} + +impl DynamicPicker { + pub const ID: &'static str = "dynamic-picker"; + + pub fn new(file_picker: FilePicker, query_callback: DynQueryCallback) -> Self { + Self { + file_picker, + query_callback, + } + } +} + +impl Component for DynamicPicker { + fn render(&mut self, area: Rect, surface: &mut Surface, cx: &mut Context) { + self.file_picker.render(area, surface, cx); + } + + fn handle_event(&mut self, event: &Event, cx: &mut Context) -> EventResult { + let prev_query = self.file_picker.picker.prompt.line().to_owned(); + let event_result = self.file_picker.handle_event(event, cx); + let current_query = self.file_picker.picker.prompt.line(); + + if *current_query == prev_query || matches!(event_result, EventResult::Ignored(_)) { + return event_result; + } + + let new_options = (self.query_callback)(current_query.to_owned(), cx.editor); + + cx.jobs.callback(async move { + let new_options = new_options.await?; + let callback = + crate::job::Callback::EditorCompositor(Box::new(move |_editor, compositor| { + // Wrapping of pickers in overlay is done outside the picker code, + // so this is fragile and will break if wrapped in some other widget. + let picker = match compositor.find_id::>>(Self::ID) { + Some(overlay) => &mut overlay.content.file_picker.picker, + None => return, + }; + picker.options = new_options; + picker.cursor = 0; + picker.force_score(); + })); + anyhow::Ok(callback) + }); + EventResult::Consumed(None) + } + + fn cursor(&self, area: Rect, ctx: &Editor) -> (Option, CursorKind) { + self.file_picker.cursor(area, ctx) + } + + fn required_size(&mut self, viewport: (u16, u16)) -> Option<(u16, u16)> { + self.file_picker.required_size(viewport) + } + + fn id(&self) -> Option<&'static str> { + Some(Self::ID) + } +} From 968697a09ed768aef815a2e77346c2267b4c2b92 Mon Sep 17 00:00:00 2001 From: Gokul Soumya Date: Tue, 19 Jul 2022 22:15:03 +0530 Subject: [PATCH 3/6] Re-request workspace symbols on keypress in picker Most language servers limit the number of workspace symbols that are returned with an empty query even though all symbols are supposed to be returned, according to the spec (for perfomance reasons). This patch adds a workspace symbol picker based on a dynamic picker that allows re-requesting the symbols on every keypress (i.e. when the picker query text changes). The old behavior has been completely replaced, and I have only tested with rust-analyzer so far. --- helix-term/src/commands/lsp.rs | 50 ++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 810e3adf1115..8052dcac2f48 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -1,3 +1,4 @@ +use futures_util::FutureExt; use helix_lsp::{ block_on, lsp::{self, CodeAction, CodeActionOrCommand, DiagnosticSeverity, NumberOrString}, @@ -14,7 +15,8 @@ use helix_view::{apply_transaction, document::Mode, editor::Action, theme::Style use crate::{ compositor::{self, Compositor}, ui::{ - self, lsp::SignatureHelp, overlay::overlayed, FileLocation, FilePicker, Popup, PromptEvent, + self, lsp::SignatureHelp, overlay::overlayed, DynamicPicker, FileLocation, FilePicker, + Popup, PromptEvent, }, }; @@ -384,10 +386,48 @@ pub fn workspace_symbol_picker(cx: &mut Context) { cx.callback( future, move |_editor, compositor, response: Option>| { - if let Some(symbols) = response { - let picker = sym_picker(symbols, current_url, offset_encoding); - compositor.push(Box::new(overlayed(picker))) - } + let symbols = match response { + Some(s) => s, + None => return, + }; + let picker = sym_picker(symbols, current_url, offset_encoding); + let get_symbols = |query: String, editor: &mut Editor| { + let doc = doc!(editor); + let language_server = match doc.language_server() { + Some(s) => s, + None => { + // This should not generally happen since the picker will not + // even open in the first place if there is no server. + return async move { Err(anyhow::anyhow!("LSP not active")) }.boxed(); + } + }; + let symbol_request = match language_server.workspace_symbols(query) { + Some(future) => future, + None => { + // This should also not happen since the language server must have + // supported workspace symbols before to reach this block. + return async move { + Err(anyhow::anyhow!( + "Language server does not support workspace symbols" + )) + } + .boxed(); + } + }; + + let future = async move { + let json = symbol_request.await?; + let response: Option> = + serde_json::from_value(json)?; + + response.ok_or_else(|| { + anyhow::anyhow!("No response for workspace symbols from language server") + }) + }; + future.boxed() + }; + let dyn_picker = DynamicPicker::new(picker, Box::new(get_symbols)); + compositor.push(Box::new(overlayed(dyn_picker))) }, ) } From 108d3d93e4e66057eca944992b5bf0077a25e6d0 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Wed, 7 Dec 2022 16:27:31 -0600 Subject: [PATCH 4/6] DynamicPicker: Use idle-timeout as debounce This change uses the idle-timeout event to trigger fetching new results in the DynamicPicker, so idle-timeout becomes a sort of debounce. This prevents querying the language server overly aggressively. --- helix-term/src/ui/picker.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/helix-term/src/ui/picker.rs b/helix-term/src/ui/picker.rs index 821a282cf61f..35597843bbf3 100644 --- a/helix-term/src/ui/picker.rs +++ b/helix-term/src/ui/picker.rs @@ -763,6 +763,7 @@ pub type DynQueryCallback = pub struct DynamicPicker { file_picker: FilePicker, query_callback: DynQueryCallback, + query: String, } impl DynamicPicker { @@ -772,6 +773,7 @@ impl DynamicPicker { Self { file_picker, query_callback, + query: String::new(), } } } @@ -782,14 +784,15 @@ impl Component for DynamicPicker { } fn handle_event(&mut self, event: &Event, cx: &mut Context) -> EventResult { - let prev_query = self.file_picker.picker.prompt.line().to_owned(); let event_result = self.file_picker.handle_event(event, cx); let current_query = self.file_picker.picker.prompt.line(); - if *current_query == prev_query || matches!(event_result, EventResult::Ignored(_)) { + if !matches!(event, Event::IdleTimeout) || self.query == *current_query { return event_result; } + self.query.clone_from(current_query); + let new_options = (self.query_callback)(current_query.to_owned(), cx.editor); cx.jobs.callback(async move { From b1f081fa74bd76c29205a3952b4b4c18c889960e Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Wed, 7 Dec 2022 16:24:32 -0600 Subject: [PATCH 5/6] DynamicPicker: Reset idle timeout on refresh If the new results shown by the picker select a file that hasn't been previewed before, the idle timeout would not trigger highlighting on that file. With this change, we reset the idle timeout and allow that file to be highlighted on the next idle timeout event. --- helix-term/src/ui/picker.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/helix-term/src/ui/picker.rs b/helix-term/src/ui/picker.rs index 35597843bbf3..2d471aae6e8e 100644 --- a/helix-term/src/ui/picker.rs +++ b/helix-term/src/ui/picker.rs @@ -798,7 +798,7 @@ impl Component for DynamicPicker { cx.jobs.callback(async move { let new_options = new_options.await?; let callback = - crate::job::Callback::EditorCompositor(Box::new(move |_editor, compositor| { + crate::job::Callback::EditorCompositor(Box::new(move |editor, compositor| { // Wrapping of pickers in overlay is done outside the picker code, // so this is fragile and will break if wrapped in some other widget. let picker = match compositor.find_id::>>(Self::ID) { @@ -808,6 +808,7 @@ impl Component for DynamicPicker { picker.options = new_options; picker.cursor = 0; picker.force_score(); + editor.reset_idle_timer(); })); anyhow::Ok(callback) }); From 8a91acb08e2b1fd254cc0f0e1bf76089bc4e81c4 Mon Sep 17 00:00:00 2001 From: Michael Davis Date: Thu, 8 Dec 2022 19:54:15 -0600 Subject: [PATCH 6/6] workspace symbols: Default to empty Vec on None A language server might send None as the response to workspace symbols. We should treat this as the empty Vec rather than the server sending an error status. This fixes the interaction with gopls which uses None to mean no matching symbols. --- helix-term/src/commands/lsp.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 8052dcac2f48..86b0c5fa7417 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -386,10 +386,7 @@ pub fn workspace_symbol_picker(cx: &mut Context) { cx.callback( future, move |_editor, compositor, response: Option>| { - let symbols = match response { - Some(s) => s, - None => return, - }; + let symbols = response.unwrap_or_default(); let picker = sym_picker(symbols, current_url, offset_encoding); let get_symbols = |query: String, editor: &mut Editor| { let doc = doc!(editor); @@ -420,9 +417,7 @@ pub fn workspace_symbol_picker(cx: &mut Context) { let response: Option> = serde_json::from_value(json)?; - response.ok_or_else(|| { - anyhow::anyhow!("No response for workspace symbols from language server") - }) + Ok(response.unwrap_or_default()) }; future.boxed() };