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.

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

Merged
merged 41 commits into from
Jun 30, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
7245c7f
Add workspace and document diagnostics picker
hirschenberger Apr 7, 2022
e8a0198
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Apr 8, 2022
f3c6c40
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Apr 14, 2022
b4c5fdd
Fix some of @archseer's annotations
hirschenberger Apr 14, 2022
e164409
Add From<&Spans> impl for String
hirschenberger Apr 14, 2022
2920808
More descriptive parameter names.
hirschenberger Apr 14, 2022
f0e0890
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Apr 19, 2022
6f87bab
Adding From<Cow<str>> impls for Span and Spans
hirschenberger Apr 19, 2022
469c4cf
Merge remote-tracking branch 'upstream/master'
Apr 20, 2022
ce36e55
Merge branch 'master' of https://github.com/helix-editor/helix
Apr 20, 2022
7086952
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Apr 22, 2022
07854d9
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Apr 25, 2022
6ce102c
Merge remote-tracking branch 'upstream/master'
Apr 28, 2022
4819226
Add new keymap entries to docs
Apr 28, 2022
dbd2848
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Apr 28, 2022
60daf1c
Merge remote-tracking branch 'upstream/master'
May 3, 2022
2bfc8f9
Avoid some clones
May 3, 2022
4c5c9df
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger May 7, 2022
e94bb27
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Jun 13, 2022
19aa615
Fix api change
hirschenberger Jun 13, 2022
4db6955
Merge branch 'master' of https://github.com/hirschenberger/helix
hirschenberger Jun 14, 2022
9d9aeaf
Update helix-term/src/application.rs
hirschenberger Jun 14, 2022
7740547
Fix a clippy hint
Jun 14, 2022
38ccf7e
Sort diagnostics first by URL and then by severity.
Jun 14, 2022
7301d05
Sort diagnostics first by URL and then by severity.
Jun 14, 2022
6014ec8
Merge branch 'master' of https://github.com/hirschenberger/helix
Jun 14, 2022
154e985
Ignore missing lsp severity entries
Jun 15, 2022
3fdc7d2
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Jun 15, 2022
5a3378f
Merge branch 'master' of https://github.com/hirschenberger/helix
hirschenberger Jun 15, 2022
48f1d4f
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Jun 17, 2022
6779697
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Jun 21, 2022
627bb09
Add truncated filepath
Jun 21, 2022
45ff7fd
Typo
hirschenberger Jun 21, 2022
9477627
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Jun 21, 2022
3bb9498
Merge branch 'master' of https://github.com/helix-editor/helix
Jun 21, 2022
8c5e289
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Jun 22, 2022
973f4dd
Merge branch 'master' of https://github.com/hirschenberger/helix
hirschenberger Jun 22, 2022
50e67c9
Strip cwd from paths and use url-path without schema
hirschenberger Jun 22, 2022
00be70b
Make tests a doctest
hirschenberger Jun 22, 2022
1b94cb7
Merge branch 'master' of https://github.com/helix-editor/helix
hirschenberger Jun 26, 2022
21607e0
Better variable names
hirschenberger Jun 26, 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
12 changes: 8 additions & 4 deletions helix-term/src/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,13 @@ impl Application {
}
}
Notification::PublishDiagnostics(params) => {
// Insert the original lsp::Diagnostics here because we may have no open document
// for diagnosic message and so we can't calculate the exact position.
// When using them later in the diagnostics picker, we calculate them on-demand.
self.editor
.diagnostics
.insert(params.uri.clone(), params.diagnostics.clone());

Copy link
Member

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 of document? 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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

enum DiagnosticType {
    Hx(Diagnostic),
    Lsp(LspDiagnostic)
}

BTreeMap<lsp::Url, Vec<DiagnosticType>>

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?

Copy link
Member

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.

I think that's a smart way of doing it, with diagnostics being converted from LanguageServer to Local when possible.

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?

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.

let path = params.uri.to_file_path().unwrap();
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved
let doc = self.editor.document_by_path_mut(&path);

Expand All @@ -431,10 +438,7 @@ impl Application {
.diagnostics
.into_iter()
.filter_map(|diagnostic| {
use helix_core::{
diagnostic::{Range, Severity::*},
Diagnostic,
};
use helix_core::diagnostic::{Diagnostic, Range, Severity::*};
use lsp::DiagnosticSeverity;

let language_server = doc.language_server().unwrap();
Expand Down
9 changes: 6 additions & 3 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub(crate) mod typed;

pub use dap::*;
pub use lsp::*;
use tui::text::Spans;
pub use typed::*;

use helix_core::{
Expand Down Expand Up @@ -263,6 +264,8 @@ impl MappableCommand {
buffer_picker, "Open buffer picker",
symbol_picker, "Open symbol picker",
workspace_symbol_picker, "Open workspace symbol picker",
diagnostics_picker, "Open diagnostic picker",
workspace_diagnostics_picker, "Open workspace diagnostic picker",
last_picker, "Open last picker",
prepend_to_line, "Insert at start of line",
append_to_line, "Insert at end of line",
Expand Down Expand Up @@ -2058,7 +2061,7 @@ fn buffer_picker(cx: &mut Context) {
}

impl BufferMeta {
fn format(&self) -> Cow<str> {
fn format(&self) -> Spans {
let path = self
.path
.as_deref()
Expand All @@ -2081,7 +2084,7 @@ fn buffer_picker(cx: &mut Context) {
} else {
format!(" ({})", flags.join(""))
};
Cow::Owned(format!("{} {}{}", self.id, path, flag))
format!("{} {}{}", self.id, path, flag).into()
}
}

Expand Down Expand Up @@ -2152,7 +2155,7 @@ pub fn command_palette(cx: &mut Context) {
MappableCommand::Typable { doc, name, .. } => match keymap.get(name as &String)
{
Some(bindings) => format!("{} ({})", doc, fmt_binding(bindings)).into(),
None => doc.into(),
None => doc.clone().into(),
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved
},
MappableCommand::Static { doc, name, .. } => match keymap.get(*name) {
Some(bindings) => format!("{} ({})", doc, fmt_binding(bindings)).into(),
Expand Down
115 changes: 112 additions & 3 deletions helix-term/src/commands/lsp.rs
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 {
Expand Down Expand Up @@ -108,6 +113,83 @@ fn sym_picker(
.truncate_start(false)
}

fn diag_picker(
Copy link
Member

Choose a reason for hiding this comment

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

diagnostics_picker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It follows the same naming schema as the sym_picker:

There's a function symbol_picker that calls a function sym_picker, the same as the function diagnostics_picker is calling diag_picker

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
for d in diags {
flat_diag.push((u.clone(), d));
}
}

let theme = cx.editor.theme.clone();
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved

FilePicker::new(
flat_diag,
move |(_url, d)| {
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved
let mut style = d.severity.map_or(Style::default(), |s| match s {
DiagnosticSeverity::HINT => theme.get("hint"),
DiagnosticSeverity::INFORMATION => theme.get("info"),
DiagnosticSeverity::WARNING => theme.get("warning"),
DiagnosticSeverity::ERROR => theme.get("error"),
_ => Style::default(),
});
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved

// 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>,
Expand Down Expand Up @@ -178,6 +260,33 @@ 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
.clone()
.into_iter()
.filter(|(u, _)| *u == current_url)
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved
.collect();
let picker = diag_picker(cx, diagnostics, 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 {
Expand Down
2 changes: 2 additions & 0 deletions helix-term/src/keymap/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,8 @@ pub fn default() -> HashMap<Mode, Keymap> {
"b" => buffer_picker,
"s" => symbol_picker,
"S" => workspace_symbol_picker,
"g" => diagnostics_picker,
"G" => workspace_diagnostics_picker,
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved
"a" => code_action,
"'" => last_picker,
"d" => { "Debug (experimental)" sticky=true
Expand Down
9 changes: 8 additions & 1 deletion helix-term/src/ui/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ pub use text::Text;
use helix_core::regex::Regex;
use helix_core::regex::RegexBuilder;
use helix_view::{Document, Editor, View};
use tui;
use tui::text::Spans;

use std::path::PathBuf;

Expand Down Expand Up @@ -177,7 +179,12 @@ pub fn file_picker(root: PathBuf, config: &helix_view::editor::Config) -> FilePi
files,
move |path: &PathBuf| {
// format_fn
path.strip_prefix(&root).unwrap_or(path).to_string_lossy()
Spans::from(
path.strip_prefix(&root)
.unwrap_or(path)
.to_string_lossy()
.into_owned(),
hirschenberger marked this conversation as resolved.
Show resolved Hide resolved
)
},
move |cx, path: &PathBuf, action| {
cx.editor
Expand Down
61 changes: 32 additions & 29 deletions helix-term/src/ui/picker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::{
use crossterm::event::Event;
use tui::{
buffer::Buffer as Surface,
text::Spans,
widgets::{Block, BorderType, Borders},
};

Expand All @@ -15,7 +16,6 @@ use tui::widgets::Widget;

use std::time::Instant;
use std::{
borrow::Cow,
cmp::Reverse,
collections::HashMap,
io::Read,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand All @@ -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))
}),
);
Expand Down Expand Up @@ -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| {
Copy link
Member

Choose a reason for hiding this comment

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

I think you want set_spans? It has a specifiable max width

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)
});
}
}

Expand Down
5 changes: 1 addition & 4 deletions helix-tui/src/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,7 @@ impl<'a> From<Span<'a>> for Spans<'a> {

impl<'a> From<Spans<'a>> for String {
fn from(line: Spans<'a>) -> String {
line.0.iter().fold(String::new(), |mut acc, s| {
acc.push_str(s.content.as_ref());
acc
})
line.0.iter().map(|s| &*s.content).collect()
}
}

Expand Down
Loading