-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add workspace and document diagnostics picker #2013
Changes from 4 commits
7245c7f
e8a0198
f3c6c40
b4c5fdd
e164409
2920808
f0e0890
6f87bab
469c4cf
ce36e55
7086952
07854d9
6ce102c
4819226
dbd2848
60daf1c
2bfc8f9
4c5c9df
e94bb27
19aa615
4db6955
9d9aeaf
7740547
38ccf7e
7301d05
6014ec8
154e985
3fdc7d2
5a3378f
48f1d4f
6779697
627bb09
45ff7fd
9477627
3bb9498
8c5e289
973f4dd
50e67c9
00be70b
1b94cb7
21607e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,25 @@ | ||
use helix_lsp::{ | ||
block_on, lsp, | ||
block_on, | ||
lsp::{self, DiagnosticSeverity, Location, NumberOrString}, | ||
util::{lsp_pos_to_pos, lsp_range_to_range, range_to_lsp_range}, | ||
OffsetEncoding, | ||
}; | ||
use tui::text::{Span, Spans}; | ||
|
||
use super::{align_view, push_jump, Align, Context, Editor}; | ||
|
||
use helix_core::Selection; | ||
use helix_view::editor::Action; | ||
use helix_view::{ | ||
editor::Action, | ||
theme::{Modifier, Style}, | ||
}; | ||
|
||
use crate::{ | ||
compositor::{self, Compositor}, | ||
ui::{self, overlay::overlayed, FileLocation, FilePicker, Popup, PromptEvent}, | ||
}; | ||
|
||
use std::borrow::Cow; | ||
use std::{borrow::Cow, collections::BTreeMap}; | ||
|
||
#[macro_export] | ||
macro_rules! language_server { | ||
|
@@ -108,6 +113,90 @@ fn sym_picker( | |
.truncate_start(false) | ||
} | ||
|
||
fn diag_picker( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It follows the same naming schema as the There's a function |
||
cx: &Context, | ||
diagnostics: BTreeMap<lsp::Url, Vec<lsp::Diagnostic>>, | ||
current_path: Option<lsp::Url>, | ||
offset_encoding: OffsetEncoding, | ||
) -> FilePicker<(lsp::Url, lsp::Diagnostic)> { | ||
// TODO: drop current_path comparison and instead use workspace: bool flag? | ||
|
||
// flatten the map to a vec of (uri, diag) pairs | ||
let mut flat_diag = Vec::new(); | ||
for (u, diags) in diagnostics { | ||
hirschenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
flat_diag.reserve(diags.len()); | ||
for d in diags { | ||
flat_diag.push((u.clone(), d)); | ||
} | ||
} | ||
hirschenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let hint = cx.editor.theme.get("hint"); | ||
let info = cx.editor.theme.get("info"); | ||
let warning = cx.editor.theme.get("warning"); | ||
let error = cx.editor.theme.get("error"); | ||
|
||
FilePicker::new( | ||
flat_diag, | ||
move |(_url, d)| { | ||
hirschenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let mut style = d | ||
.severity | ||
.map(|s| match s { | ||
DiagnosticSeverity::HINT => hint, | ||
DiagnosticSeverity::INFORMATION => info, | ||
DiagnosticSeverity::WARNING => warning, | ||
DiagnosticSeverity::ERROR => error, | ||
_ => Style::default(), | ||
}) | ||
.unwrap_or_default(); | ||
|
||
// remove background as it is distracting in the picker list | ||
style.bg = None; | ||
hirschenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
let code = d | ||
.code | ||
.as_ref() | ||
.map(|c| match c { | ||
NumberOrString::Number(n) => n.to_string(), | ||
NumberOrString::String(s) => s.to_string(), | ||
}) | ||
.unwrap_or_default(); | ||
|
||
Spans::from(vec![ | ||
Span::styled( | ||
d.source.clone().unwrap_or_default(), | ||
style.add_modifier(Modifier::BOLD), | ||
), | ||
Span::raw(": "), | ||
Span::styled(code, style), | ||
Span::raw(" - "), | ||
Span::styled(&d.message, style), | ||
]) | ||
}, | ||
move |cx, (url, d), action| { | ||
if current_path.as_ref() == Some(url) { | ||
push_jump(cx.editor); | ||
hirschenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
let path = url.to_file_path().unwrap(); | ||
cx.editor.open(path, action).expect("editor.open failed"); | ||
hirschenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
let (view, doc) = current!(cx.editor); | ||
|
||
if let Some(range) = lsp_range_to_range(doc.text(), d.range, offset_encoding) { | ||
// we flip the range so that the cursor sits on the start of the symbol | ||
// (for example start of the function). | ||
doc.set_selection(view.id, Selection::single(range.head, range.anchor)); | ||
align_view(doc, view, Align::Center); | ||
} | ||
}, | ||
move |_editor, (url, diag)| { | ||
let location = Location::new(url.clone(), diag.range); | ||
Some(location_to_file_location(&location)) | ||
}, | ||
) | ||
.truncate_start(false) | ||
} | ||
|
||
pub fn symbol_picker(cx: &mut Context) { | ||
fn nested_to_flat( | ||
list: &mut Vec<lsp::SymbolInformation>, | ||
|
@@ -178,6 +267,37 @@ pub fn workspace_symbol_picker(cx: &mut Context) { | |
) | ||
} | ||
|
||
pub fn diagnostics_picker(cx: &mut Context) { | ||
let doc = doc!(cx.editor); | ||
let language_server = language_server!(cx.editor, doc); | ||
if let Some(current_url) = doc.url() { | ||
let offset_encoding = language_server.offset_encoding(); | ||
let diagnostics = cx | ||
.editor | ||
.diagnostics | ||
.get(¤t_url) | ||
.cloned() | ||
.unwrap_or_default(); | ||
let picker = diag_picker( | ||
cx, | ||
[(current_url.clone(), diagnostics)].into(), | ||
Some(current_url), | ||
offset_encoding, | ||
); | ||
cx.push_layer(Box::new(overlayed(picker))); | ||
} | ||
} | ||
|
||
pub fn workspace_diagnostics_picker(cx: &mut Context) { | ||
let doc = doc!(cx.editor); | ||
let language_server = language_server!(cx.editor, doc); | ||
let current_url = doc.url(); | ||
let offset_encoding = language_server.offset_encoding(); | ||
let diagnostics = cx.editor.diagnostics.clone(); | ||
let picker = diag_picker(cx, diagnostics, current_url, offset_encoding); | ||
cx.push_layer(Box::new(overlayed(picker))); | ||
} | ||
|
||
impl ui::menu::Item for lsp::CodeActionOrCommand { | ||
fn label(&self) -> &str { | ||
match self { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ use crate::{ | |
use crossterm::event::Event; | ||
use tui::{ | ||
buffer::Buffer as Surface, | ||
text::Spans, | ||
widgets::{Block, BorderType, Borders}, | ||
}; | ||
|
||
|
@@ -15,7 +16,6 @@ use tui::widgets::Widget; | |
|
||
use std::time::Instant; | ||
use std::{ | ||
borrow::Cow, | ||
cmp::Reverse, | ||
collections::HashMap, | ||
io::Read, | ||
|
@@ -87,7 +87,7 @@ impl Preview<'_, '_> { | |
impl<T> FilePicker<T> { | ||
pub fn new( | ||
options: Vec<T>, | ||
format_fn: impl Fn(&T) -> Cow<str> + 'static, | ||
format_fn: impl Fn(&T) -> Spans + 'static, | ||
callback_fn: impl Fn(&mut Context, &T, Action) + 'static, | ||
preview_fn: impl Fn(&Editor, &T) -> Option<FileLocation> + 'static, | ||
) -> Self { | ||
|
@@ -301,14 +301,14 @@ pub struct Picker<T> { | |
/// Whether to truncate the start (default true) | ||
pub truncate_start: bool, | ||
|
||
format_fn: Box<dyn Fn(&T) -> Cow<str>>, | ||
format_fn: Box<dyn Fn(&T) -> Spans>, | ||
callback_fn: Box<dyn Fn(&mut Context, &T, Action)>, | ||
} | ||
|
||
impl<T> Picker<T> { | ||
pub fn new( | ||
options: Vec<T>, | ||
format_fn: impl Fn(&T) -> Cow<str> + 'static, | ||
format_fn: impl Fn(&T) -> Spans + 'static, | ||
callback_fn: impl Fn(&mut Context, &T, Action) + 'static, | ||
) -> Self { | ||
let prompt = Prompt::new( | ||
|
@@ -373,9 +373,8 @@ impl<T> Picker<T> { | |
self.matches.retain_mut(|(index, score)| { | ||
let option = &self.options[*index]; | ||
// TODO: maybe using format_fn isn't the best idea here | ||
let text = (self.format_fn)(option); | ||
|
||
match self.matcher.fuzzy_match(&text, pattern) { | ||
let line: String = (self.format_fn)(option).into(); | ||
match self.matcher.fuzzy_match(&line, pattern) { | ||
Some(s) => { | ||
// Update the score | ||
*score = s; | ||
|
@@ -402,10 +401,10 @@ impl<T> Picker<T> { | |
} | ||
|
||
// TODO: maybe using format_fn isn't the best idea here | ||
let text = (self.format_fn)(option); | ||
let line: String = (self.format_fn)(option).into(); | ||
|
||
self.matcher | ||
.fuzzy_match(&text, pattern) | ||
.fuzzy_match(&line, pattern) | ||
.map(|score| (index, score)) | ||
}), | ||
); | ||
|
@@ -612,30 +611,34 @@ impl<T: 'static> Component for Picker<T> { | |
surface.set_string(inner.x.saturating_sub(2), inner.y + i as u16, ">", selected); | ||
} | ||
|
||
let formatted = (self.format_fn)(option); | ||
|
||
let spans = (self.format_fn)(option); | ||
let (_score, highlights) = self | ||
.matcher | ||
.fuzzy_indices(&formatted, self.prompt.line()) | ||
.fuzzy_indices(&String::from(spans.clone()), self.prompt.line()) | ||
hirschenberger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.unwrap_or_default(); | ||
|
||
surface.set_string_truncated( | ||
inner.x, | ||
inner.y + i as u16, | ||
&formatted, | ||
inner.width as usize, | ||
|idx| { | ||
if highlights.contains(&idx) { | ||
highlighted | ||
} else if is_active { | ||
selected | ||
} else { | ||
text_style | ||
} | ||
}, | ||
true, | ||
self.truncate_start, | ||
); | ||
spans.0.into_iter().fold(inner, |pos, span| { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but it does not support highlighting fuzzy search results with a style callback function. |
||
let new_x = surface | ||
.set_string_truncated( | ||
pos.x, | ||
pos.y + i as u16, | ||
&span.content, | ||
pos.width as usize, | ||
|idx| { | ||
if highlights.contains(&idx) { | ||
highlighted.patch(span.style) | ||
} else if is_active { | ||
selected.patch(span.style) | ||
} else { | ||
text_style.patch(span.style) | ||
} | ||
}, | ||
true, | ||
self.truncate_start, | ||
) | ||
.0; | ||
pos.clip_left(new_x - pos.x) | ||
}); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a whole bunch of duplication. Can we just store diagnostics on
editor
instead ofdocument
? I guess one painpoint is that raw lsp diagnostics don't have the offsets translated which makes this hard for files that haven't been opened yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean changing the local, already location-converted diagnostics to use the
self.editor.diagnostics
list and convert the locations on demand like it is done in the diags-picker?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's expensive to do that on the fly. How about using a map for diagnostics of buffers that aren't currently open? Then if the document gets opened you remove the entry from the map, convert the values and store them in the document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using the
editor.diagnostics
map as single source of truth storing both like this? So we can cache already converted diags in a central place.BTW: Another issue is the missing highlightling of the previews that have no open document. Are you planning to solve this? What would be a good way of solving this without huge performance impact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a smart way of doing it, with diagnostics being converted from
LanguageServer
toLocal
when possible.Yeah there's an issue open, I think we'll want to trigger highlighting after an idle timeout. That way it doesn't highlight hundreds of files if you're scrolling, but it will highlight the file you stop on.