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

Show previous/next diagnostic with phantom #783

Merged
merged 23 commits into from
Nov 27, 2019

Conversation

tomv564
Copy link
Contributor

@tomv564 tomv564 commented Nov 19, 2019

Press F8 to open/next, shift+F8 for previous, esc to close. (see suggested keybindings)

Screenshot 2019-11-22 at 10 34 08

Screenshot 2019-11-26 at 15 19 08

Essentially like VS Code's "Next Problem" feature.

Suggested keybindings

{ "keys": ["f8"], "command": "lsp_next_diagnostic"},
{ "keys": ["shift+f8"], "command": "lsp_previous_diagnostic"},
{ "keys": ["escape"], "command": "lsp_hide_diagnostic", "context":
	[
		{ "key": "setting.lsp_diagnostic_phantom"}
	]
}

Closes #746:

  • Allows navigation for those who don't like to use the panel for it (SublimeLinter style)
  • Allows a faster and more readable way to see a diagnostic + related info than hovering.

Implementation details:

  • The DiagnosticsPresenter passes user actions (forward/back) and server diagnostic updates into the DiagnosticsCursor
  • The cursor selects a different walking logic based on an already-selected diagnostic or a cursor position in a file.
  • The Escape key is "stolen" if the view has a lsp_diagnostic_phantom setting applied. There is some risk with stealing a so often-used key.
  • Phantoms are not hidden if a diagnostic disappears.

To do:

  • Implement UI + wire up Esc for dismissing phantoms
  • Implement UI for related info
  • Update UI for code actions (if keeping at all)
  • Review behaviour when diagnostics removed/moved/added.
  • Only include warnings and errors (See Don't show hint diagnostics in problems view? microsoft/vscode#45436)
  • UI polish (add prev/next, related info spacing and color, top arrow, rounded corners?)
  • Organize diagnostics code and add unit tests

@coveralls
Copy link

coveralls commented Nov 21, 2019

Coverage Status

Coverage increased (+1.9%) to 37.886% when pulling 6a8757c on select-and-phantom-diagnostic into 75fe01c on master.

@tomv564 tomv564 force-pushed the select-and-phantom-diagnostic branch from e920e3a to 53026d0 Compare November 22, 2019 07:16
@tomv564 tomv564 changed the title WIP: Show previous/next diagnostic with phantom Show previous/next diagnostic with phantom Nov 26, 2019
@tomv564 tomv564 requested a review from rwols November 26, 2019 14:40
@rwols
Copy link
Member

rwols commented Nov 26, 2019

I haven't really looked at all the code yet, but I noticed these two things:

  1. phantomdiags
    The whiteish/bluish box around the related diags is not present. Is this intentional? It feels to me as if this part should share the same css code as the hover popup.

  2. The command lsp_next_diagnostic does not cycle back to the beginning of the phantoms list, but lsp_previous_diagnostic does cycle forward to the end of the phantoms list if invoking it on the first diag. I think they should both have the cycle behavior.

Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

see above comment

@tomv564
Copy link
Contributor Author

tomv564 commented Nov 26, 2019

Good catch, even had a TODO for testing the missed wrap-forward from diagnostic logic

The color was doable in this manner:

Screenshot 2019-11-27 at 00 10 57

But I feel the extra markup/css is a bit unfortunate.
The amount of styling on hovers gets a little distracting, like this example:

Screenshot 2019-11-27 at 00 11 11

For me the additional info is part of the error and could just be red.

@rwols
Copy link
Member

rwols commented Nov 27, 2019

For me the additional info is part of the error and could just be red.

I'm okay with having it red, but IMO there should be some sort of separation between the main diag and the related diags. Indentation for the related diags perhaps? Or a horizontal line? It's otherwise too vague (for me, probably also for other users) what the actual problem is, and what "additional info" is.

@tomv564
Copy link
Contributor Author

tomv564 commented Nov 27, 2019

I like your suggestion of the horizontal line a lot (I tried indent too but this is more space-efficient)

Screenshot 2019-11-27 at 13 42 54

If you also think this looks good, then I will apply the same change to hovers

@rwols
Copy link
Member

rwols commented Nov 27, 2019

Looks good!

@tomv564
Copy link
Contributor Author

tomv564 commented Nov 27, 2019

I will merge this now and document it when we get to a release soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to jump to next/previous diagnostic?
3 participants