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.

Sign up for GitHub

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

Jump to bottom

Show diagnostics popup when hovering over gutter icons #2349

Merged
merged 4 commits into from
Nov 9, 2023
Merged

Show diagnostics popup when hovering over gutter icons #2349

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58ca97e
Show diagnostics popup when hovering over gutter icons
jwortmann Oct 31, 2023
c38f287
Lint
jwortmann Oct 31, 2023
143d6f6
Show diagnostics with code actions even when icon disabled
jwortmann Nov 1, 2023
295f0ab
Rename variables
jwortmann Nov 4, 2023
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
43 changes: 32 additions & 11 deletions plugin/documents.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from .completion import QueryCompletionsTask
from .core.constants import HOVER_ENABLED_KEY
from .core.logging import debug
from .core.open import open_in_browser
from .core.panels import PanelName
from .core.protocol import Diagnostic
from .core.protocol import DiagnosticSeverity
Expand All @@ -27,6 +28,7 @@
from .core.settings import userprefs
from .core.signature_help import SigHelp
from .core.types import basescope2languageid
from .core.types import ClientConfig
from .core.types import debounced
from .core.types import DebouncerNonThreadSafe
from .core.types import FEATURES_TIMEOUT
Expand All @@ -40,6 +42,7 @@
from .core.views import DOCUMENT_HIGHLIGHT_KINDS
from .core.views import first_selection_region
from .core.views import format_code_actions_for_quick_panel
from .core.views import format_diagnostic_for_html
from .core.views import make_link
from .core.views import MarkdownLangMap
from .core.views import range_to_region
Expand Down Expand Up @@ -471,17 +474,33 @@ def on_hover(self, point: int, hover_zone: int) -> None:
if hover_zone == sublime.HOVER_TEXT and window and window.settings().get(HOVER_ENABLED_KEY, True):
self.view.run_command("lsp_hover", {"point": point})
elif hover_zone == sublime.HOVER_GUTTER:
# Lightbulb must be visible and at the same line
if self._lightbulb_line != self.view.rowcol(point)[0]:
return
content = code_actions_content(self._actions_by_config)
if content:
show_lsp_popup(
self.view,
content,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
on_navigate=lambda href: self._on_navigate(href, point))
sublime.set_timeout_async(partial(self._on_hover_gutter_async, point))

def _on_hover_gutter_async(self, point: int) -> None:
content = ''
if self._lightbulb_line == self.view.rowcol(point)[0]:
content += code_actions_content(self._actions_by_config)
if userprefs().diagnostics_gutter_marker and userprefs().show_diagnostics_severity_level:
Copy link

Choose a reason for hiding this comment

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

Is this the expected behavior?

1
"diagnostics_gutter_marker": "circle",
Screenshot 2023-10-31 at 22 59 16

If there are 2 diagnostics,
and diagnostics_gutter_marker is set to "circle"
display the diagnostics on gutter hover.

2
"diagnostics_gutter_marker": "",
Screenshot 2023-10-31 at 22 58 53

If there are 2 diagnostics,
and diagnostics_gutter_marker is set to ""
don't display the diagnostics on gutter hover.

Copy link
Member Author

Choose a reason for hiding this comment

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

In case 2 it doesn't show the diagnostics in addition to the code actions, because in general they can be unrelated to each other. But I'm not sure, maybe would be better to also see the diagnostics, what do you think? In that case it should probably only filter for the diagnostics which are overlapping the current caret position or selection.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I did it correctly, then the popup in your example 2 should show only the first diagnostic with small "d" now (where the caret is).

Copy link

Choose a reason for hiding this comment

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

If I did it correctly, then the popup in your example 2 should show only the first diagnostic with small "d" now (where the caret is).

That is correct.
I will see the diagnotics where the cursor is if I hover the gutter.

Screenshot 2023-11-04 at 22 37 35

But I am not sure I think that behavior is the best choice,
because I can already hover with the cursor over the diagnostics and see the diagnostic.
So it is kind of redudant.

it should probably only filter for the diagnostics which are overlapping the current caret position or selection.

For me it would be more intuitive
if hovering the gutter should show all diagnostics that just exist on the line. (not based on cursor or selection)

So my expected behavior would be the following:
if "diagnostics_gutter_marker": ""
If there are 2 diagnostics,
display the 2 diagnostics on gutter hover.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I am not sure I think that behavior is the best choice,
because I can already hover with the cursor over the diagnostics and see the diagnostic.
So it is kind of redudant.

In this case with the diagnostic icons disabled it will show only the diagnostic(s) which is/are sent as part of the code action request (CodeActionContext). So there is a good chance that the code actions are related to the diagnostics at the cursor position, especially because we only show Quick Fix actions here. And the code actions request is sent for a particular range, so the actions should also be only applicable for the current cursor position.

You could consider the diagnostic from this screenshot to be part of the lightbulb hover, and the other diagnostics on the line or on different lines are not shown because the diagnostics gutter icons are disabled. I think it makes sense.

And I would not say that it is redundant, it's rather just a different way to see the diagnostic message. For example I have the code actions in the regular hover popup disabled, so I will never see them if I hover over the error underline.

diagnostics_with_config = [] # type: List[Tuple[ClientConfig, Diagnostic]]
max_severity_level = min(userprefs().show_diagnostics_severity_level, DiagnosticSeverity.Information)
for sb, diagnostics in self.diagnostics_intersecting_async(self.view.line(point))[0]:
diagnostics_with_config.extend(
(sb.session.config, diagnostic) for diagnostic in diagnostics
if diagnostic_severity(diagnostic) <= max_severity_level
)
if diagnostics_with_config:
diagnostics_with_config.sort(key=lambda d: diagnostic_severity(d[1]))
content += '<div class="diagnostics">'
for config, diagnostic in diagnostics_with_config:
content += format_diagnostic_for_html(config, diagnostic)
content += '</div>'
if content:
show_lsp_popup(
self.view,
content,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
on_navigate=lambda href: self._on_navigate(href, point))

def on_text_command(self, command_name: str, args: Optional[dict]) -> Optional[Tuple[str, dict]]:
if command_name == "auto_complete":
Expand Down Expand Up @@ -700,6 +719,8 @@ def _on_navigate(self, href: str, point: int) -> None:
placeholder="Code actions")
else:
self.handle_code_action_select(config_name, actions, 0)
else:
open_in_browser(href)

def handle_code_action_select(self, config_name: str, actions: List[CodeActionOrCommand], index: int) -> None:
if index == -1:
Expand Down