Add workspace and document diagnostics picker #2013

hirschenberger · 2022-04-07T22:08:14Z

I mapped it to SPC g and SPC G but feel free to suggest better mappings. Also the style of the picker's lines is open for discussion.

I thought about adding another box on top of the code preview to display the whole diagnostic message without clipping. This adds more space to the list to display the uri.
But requires implementing a custom picker and may introduce a lot of code duplication or extending the existing picker in a smart way.

fixes helix-editor#1891

archseer · 2022-04-12T08:01:48Z

helix-term/src/application.rs

+                        self.editor
+                            .diagnostics
+                            .insert(params.uri.clone(), params.diagnostics.clone());
+


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.

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?

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.

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?

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.

helix-term/src/commands.rs

helix-term/src/commands/lsp.rs

helix-term/src/ui/mod.rs

helix-term/src/ui/picker.rs

helix-term/src/commands/lsp.rs

# Conflicts: # helix-term/src/commands/lsp.rs

helix-term/src/keymap/default.rs

helix-term/src/application.rs

photex · 2022-06-07T20:29:43Z

Ahoy! I'd really like to see this feature land. Could we perhaps make a branch for the on-going effort so that more than one of us could contribute?

I've noticed after rebasing the work that @hirschenberger has done so far on top of HEAD:

SPC-G still appears to only show diagnostics for files that have already been opened.
- If I open helix: hx ., select a file, and then SPC-G: I only see local diagnostics. If I open another file, then SPC-g is correctly limited to diagnostics in this file, while SPC-G is now showing the diagnostics from both open files. It would be ideal if SPC-G showed all of the feedback from clippy.
the function push_jump has changed and this PR needs to be updated 🤷 not a big deal, just happened to notice.

Having the diag list essentially duplicate entries in order to display messages isn't ideal (and doesn't suit the minimal and clean aesthetic of Helix). I'd personally vote that we only show the diagnostic once. Perfect is the enemy of good. I personally think that the suggestion to show extended info over the preview of each file is a good idea but could come after this feature.

helix-term/src/commands/lsp.rs

hirschenberger · 2022-06-13T23:58:11Z

Ahoy! I'd really like to see this feature land. Could we perhaps make a branch for the on-going effort so that more than one of us could contribute?

Hey, sorry for the delayed answer. I currently have not much time, but I'd also like to get this landed. I'm ok with adding more contributors to my branch to continue work on this,

I've noticed after rebasing the work that @hirschenberger has done so far on top of HEAD:

* SPC-G still appears to only show diagnostics for files that have already been opened.
  * If I open helix: `hx .`, select a file, and then `SPC-G`: I only see local diagnostics. If I open another file, then `SPC-g` is correctly limited to diagnostics in this file, while `SPC-G` is now showing the diagnostics from both open files. It would be ideal if `SPC-G` showed all of the feedback from clippy.

Strange, it works here with diagnostics for all files of the project. Which language have you tried? I tested it with rust and it works. Maybe because I enabled the clippy lints in rust-analyzer in my languages.toml

[[language]]
name="rust"

[language.config]
checkOnSave = {command = "clippy"}
cargo = {allFeatures = true}
procMacro = {enable = true }
auto-format = false

* the function `push_jump` has changed and this PR needs to be updated shrug not a big deal, just happened to notice.

I fixed it in my branch.

Having the diag list essentially duplicate entries in order to display messages isn't ideal (and doesn't suit the minimal and clean aesthetic of Helix). I'd personally vote that we only show the diagnostic once. Perfect is the enemy of good. I personally think that the suggestion to show extended info over the preview of each file is a good idea but could come after this feature.

I agree

@photex @BearOve Do you want to participate in this PR?

hirschenberger · 2022-06-17T11:30:31Z

But it would be useful to see something like:

> [rs] db::models::User: E0432 - unresolved imports ...
  [rs] db::models::User: E0432 - unresolved imports ...
  [rs] db::models::Region: E0277 - the trait bound `Li...
  [rs] db::models::Region: E0277 - the trait bound `Li...
  [rs] db::models::Region: E0277 - the trait bound `Li...

Yes this would be a nice feature, but I don't really know where I can get the module path from. The lsp diagnostic does not contain such info.

bjorn-ove · 2022-06-17T12:39:11Z

How about the last n characters if the file path?

hirschenberger · 2022-06-20T08:44:07Z

How about the last n characters if the file path?

That may lead to e.g. on rust many lib.rs entries.
A deduplicated unique path representation would be nice to save some space and have a unique name.
E.g. like helix-tu*/s*/ba*/mod.rs or even helix-tu/s/ba/mod.rs indicating the shortened path components with a different color.

Anybody familiar of a crate that does this? Or we have to do it ourselves by using a patricia-tree

the-mikedavis · 2022-06-20T14:48:53Z

I think that could be solved in-house (i.e. without adding a crate). It might be useful for #2759 as well

helix-core/src/path.rs

hirschenberger · 2022-06-24T15:58:29Z

What else do we need here to get this PR landed?

the-mikedavis · 2022-06-25T16:27:53Z

Afaik this just needs a round of review. Refactoring to use a table (#2814 (comment)) can be left for a follow-up PR.

archseer · 2022-06-26T09:00:34Z

helix-core/src/path.rs

+    let file = path.file_name().unwrap_or_default();
+    let base = path.parent().unwrap_or_else(|| Path::new(""));
+    let mut ret = PathBuf::new();
+    for d in base {


Is this loop necessary? d is an Option. I'd use if let Some()

Seems to me you actually want to iterate over ancestors()

d is an OsStr and base is a &Path

Path implements Iterator for component traversal.

archseer · 2022-06-26T09:01:45Z

helix-term/src/commands/lsp.rs

@@ -145,6 +150,97 @@ fn sym_picker(
    .truncate_start(false)
 }

+fn diag_picker(


diagnostics_picker

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

helix-term/src/commands/lsp.rs

archseer · 2022-06-26T09:08:12Z

helix-term/src/ui/picker.rs

-                true,
-                self.truncate_start,
-            );
+            spans.0.into_iter().fold(inner, |pos, span| {


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

Yes, but it does not support highlighting fuzzy search results with a style callback function.

archseer

Thanks for working on this! Sorry the review took a while

hirschenberger added 2 commits April 7, 2022 23:59

Add workspace and document diagnostics picker

7245c7f

fixes helix-editor#1891

Merge branch 'master' of https://github.com/helix-editor/helix

e8a0198

archseer reviewed Apr 12, 2022

View reviewed changes

hirschenberger added 3 commits April 14, 2022 08:14

Merge branch 'master' of https://github.com/helix-editor/helix

f3c6c40

Fix some of @archseer's annotations

b4c5fdd

Add From<&Spans> impl for String

e164409

dead10ck reviewed Apr 14, 2022

View reviewed changes

helix-term/src/commands/lsp.rs Show resolved Hide resolved

helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved

helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved

hirschenberger and others added 6 commits April 14, 2022 18:36

More descriptive parameter names.

2920808

Merge branch 'master' of https://github.com/helix-editor/helix

f0e0890

# Conflicts: # helix-term/src/commands/lsp.rs

Adding From<Cow<str>> impls for Span and Spans

6f87bab

Merge remote-tracking branch 'upstream/master'

469c4cf

Merge branch 'master' of https://github.com/helix-editor/helix

ce36e55

Merge branch 'master' of https://github.com/helix-editor/helix

7086952

hirschenberger requested a review from archseer April 23, 2022 15:03

Merge branch 'master' of https://github.com/helix-editor/helix

07854d9

pickfire reviewed Apr 26, 2022

View reviewed changes

helix-term/src/keymap/default.rs Show resolved Hide resolved

Falco Hirschenberger and others added 3 commits April 28, 2022 13:30

Merge remote-tracking branch 'upstream/master'

6ce102c

Add new keymap entries to docs

4819226

Merge branch 'master' of https://github.com/helix-editor/helix

dbd2848

archseer reviewed May 2, 2022

View reviewed changes

helix-term/src/application.rs Show resolved Hide resolved

Falco Hirschenberger and others added 3 commits May 3, 2022 16:50

Merge remote-tracking branch 'upstream/master'

60daf1c

Avoid some clones

2bfc8f9

Merge branch 'master' of https://github.com/helix-editor/helix

4c5c9df

the-mikedavis mentioned this pull request May 9, 2022

Persistent workspace file tree (similar to VSCode) #2438

Closed

the-mikedavis added the S-waiting-on-review Status: Awaiting review from a maintainer. label May 18, 2022

bjorn-ove reviewed Jun 10, 2022

View reviewed changes

helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/helix-editor/helix

e94bb27

Fix api change

19aa615

Merge branch 'master' of https://github.com/helix-editor/helix

48f1d4f

hirschenberger and others added 5 commits June 21, 2022 02:56

Merge branch 'master' of https://github.com/helix-editor/helix

6779697

Add truncated filepath

627bb09

Typo

45ff7fd

Merge branch 'master' of https://github.com/helix-editor/helix

9477627

Merge branch 'master' of https://github.com/helix-editor/helix

3bb9498

This was referenced Jun 21, 2022

Reuse menu::Item trait in picker #2814

Merged

Add more context to the symbol picker #2452

Closed

hirschenberger added 3 commits June 22, 2022 07:50

Merge branch 'master' of https://github.com/helix-editor/helix

8c5e289

Merge branch 'master' of https://github.com/hirschenberger/helix

973f4dd

Strip cwd from paths and use url-path without schema

50e67c9

sudormrfbin reviewed Jun 22, 2022

View reviewed changes

helix-core/src/path.rs Outdated Show resolved Hide resolved

Make tests a doctest

00be70b

lazytanuki mentioned this pull request Jun 23, 2022

feat: icon support #2869

Open

15 tasks

rsHalford mentioned this pull request Jun 24, 2022

Add diagnostic markers to buffer picker #2871

Closed

hirschenberger requested review from archseer, pickfire and sudormrfbin June 26, 2022 08:11

archseer reviewed Jun 26, 2022

View reviewed changes

hirschenberger added 2 commits June 26, 2022 14:56

Merge branch 'master' of https://github.com/helix-editor/helix

1b94cb7

Better variable names

21607e0

archseer approved these changes Jun 30, 2022

View reviewed changes

archseer merged commit ed89f88 into helix-editor:master Jun 30, 2022

EpocSquadron mentioned this pull request Aug 13, 2022

Add goto_next_diag_error/goto_prev_diag_error #3405

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workspace and document diagnostics picker #2013

Add workspace and document diagnostics picker #2013

hirschenberger commented Apr 7, 2022

archseer Apr 12, 2022

hirschenberger Apr 14, 2022

archseer Apr 17, 2022

hirschenberger Apr 19, 2022

archseer May 2, 2022

photex commented Jun 7, 2022

hirschenberger commented Jun 13, 2022

hirschenberger commented Jun 17, 2022 •

edited

Loading

bjorn-ove commented Jun 17, 2022

hirschenberger commented Jun 20, 2022

the-mikedavis commented Jun 20, 2022

hirschenberger commented Jun 24, 2022

the-mikedavis commented Jun 25, 2022

archseer Jun 26, 2022

archseer Jun 26, 2022

hirschenberger Jun 26, 2022

archseer Jun 26, 2022

hirschenberger Jun 26, 2022

archseer Jun 26, 2022

hirschenberger Jun 26, 2022

archseer left a comment

Add workspace and document diagnostics picker #2013

Add workspace and document diagnostics picker #2013

Conversation

hirschenberger commented Apr 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

photex commented Jun 7, 2022

hirschenberger commented Jun 13, 2022

hirschenberger commented Jun 17, 2022 • edited Loading

bjorn-ove commented Jun 17, 2022

hirschenberger commented Jun 20, 2022

the-mikedavis commented Jun 20, 2022

hirschenberger commented Jun 24, 2022

the-mikedavis commented Jun 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archseer left a comment

Choose a reason for hiding this comment

hirschenberger commented Jun 17, 2022 •

edited

Loading