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

Send range with textDocument/hover when possible #1900

Merged
merged 19 commits into from
Nov 27, 2021
Merged

Send range with textDocument/hover when possible #1900

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
66b0fe0
Send range with textDocument/hover when possible
ayoub-benali Nov 18, 2021
38f51c9
Delay capability check
ayoub-benali Nov 18, 2021
ffef3bc
Address PR comments
ayoub-benali Nov 18, 2021
21169c9
Send range when hovering selection as well
ayoub-benali Nov 18, 2021
8bdc9bf
fix typo
ayoub-benali Nov 18, 2021
6df1166
Fix condition
ayoub-benali Nov 18, 2021
7fad7cf
Revert hide_lsp_popup
ayoub-benali Nov 18, 2021
8107120
Fix test case
ayoub-benali Nov 19, 2021
eb66975
Do range check in request_symbol_hover_async
ayoub-benali Nov 19, 2021
7376b2c
make flake8 happy
ayoub-benali Nov 19, 2021
265e422
remove duplicated code
ayoub-benali Nov 19, 2021
7972cd1
remove comment
ayoub-benali Nov 19, 2021
5b3182a
put space back
ayoub-benali Nov 19, 2021
5efa539
Simplify case
ayoub-benali Nov 19, 2021
6a21f05
Set range field directly
ayoub-benali Nov 19, 2021
7e5e045
create document position inside the loop
ayoub-benali Nov 19, 2021
d95f7e5
Extract range logic
ayoub-benali Nov 19, 2021
e47c1f4
change functions order
ayoub-benali Nov 19, 2021
2c66b1a
Send position along range
ayoub-benali Nov 23, 2021
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
4 changes: 4 additions & 0 deletions plugin/core/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ def versioned_text_document_identifier(view: sublime.View, version: int) -> Dict
def text_document_position_params(view: sublime.View, location: int) -> TextDocumentPositionParams:
return {"textDocument": text_document_identifier(view), "position": position(view, location)}

def text_document_range_params(view: sublime.View, region: sublime.Region) -> Dict[str, Any]:
return {"textDocument": text_document_identifier(view), "range": region_to_range(view, region).to_lsp()}
rchl marked this conversation as resolved.
Show resolved Hide resolved

def did_open_text_document_params(view: sublime.View, language_id: str) -> Dict[str, Any]:
return {"textDocument": text_document_item(view, language_id)}
Expand Down Expand Up @@ -404,6 +406,8 @@ def text_document_code_action_params(
# Workaround for a limited margin-collapsing capabilities of the minihtml.
LSP_POPUP_SPACER_HTML = '<div class="lsp_popup--spacer"></div>'

def hide_lsp_popup(view: sublime.View) -> None:
mdpopups.hide_popup(view)
rchl marked this conversation as resolved.
Show resolved Hide resolved

rchl marked this conversation as resolved.
Show resolved Hide resolved
def show_lsp_popup(view: sublime.View, contents: str, location: int = -1, md: bool = False, flags: int = 0,
css: Optional[str] = None, wrapper_class: Optional[str] = None,
Expand Down
67 changes: 42 additions & 25 deletions plugin/hover.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
from .core.views import is_location_href
from .core.views import make_command_link
from .core.views import make_link
from .core.views import hide_lsp_popup
from .core.views import show_lsp_popup
from .core.views import text_document_position_params
from .core.views import text_document_range_params
from .core.views import unpack_href_location
from .core.views import update_lsp_popup
from .core.windows import AbstractViewListener
Expand All @@ -35,7 +37,7 @@
SUBLIME_WORD_MASK = 515
SessionName = str
ResolvedHover = Union[Hover, Error]

PointOrRegion = Union[int, sublime.Region]

_test_contents = [] # type: List[str]

Expand Down Expand Up @@ -82,17 +84,16 @@ def run(
point: Optional[int] = None,
event: Optional[dict] = None
) -> None:
temp_point = point
if temp_point is None:
point_or_region = point # type: Optional[PointOrRegion]
if point_or_region is None:
region = first_selection_region(self.view)
if region is not None:
temp_point = region.begin()
if temp_point is None:
point_or_region = region
if point_or_region is None:
return
window = self.view.window()
if not window:
return
hover_point = temp_point
rchl marked this conversation as resolved.
Show resolved Hide resolved
wm = windows.lookup(window)
self._base_dir = wm.get_project_path(self.view.file_name() or "")
self._hover_responses = [] # type: List[Hover]
Expand All @@ -106,22 +107,38 @@ def run_async() -> None:
if not listener:
return
if not only_diagnostics:
self.request_symbol_hover_async(listener, hover_point)
self._diagnostics_by_config, covering = listener.diagnostics_touching_point_async(
hover_point, userprefs().show_diagnostics_severity_level)
if self._diagnostics_by_config:
self.show_hover(listener, hover_point, only_diagnostics)
if not only_diagnostics and userprefs().show_code_actions_in_hover:
actions_manager.request_for_region_async(
self.view, covering, self._diagnostics_by_config,
functools.partial(self.handle_code_actions, listener, hover_point))
self.request_symbol_hover_async(listener, point_or_region)
if isinstance(point_or_region, int):
self._diagnostics_by_config, covering = listener.diagnostics_touching_point_async(
point_or_region, userprefs().show_diagnostics_severity_level)
else:
self._diagnostics_by_config, covering = listener.diagnostics_intersecting_region_async(
point_or_region)
if isinstance(point_or_region, int):
hover_point = point_or_region
if self._diagnostics_by_config:
self.show_hover(listener, hover_point, only_diagnostics)
if not only_diagnostics and userprefs().show_code_actions_in_hover:
actions_manager.request_for_region_async(
self.view, covering, self._diagnostics_by_config,
functools.partial(self.handle_code_actions, listener, hover_point))

sublime.set_timeout_async(run_async)

def request_symbol_hover_async(self, listener: AbstractViewListener, point: int) -> None:
def request_symbol_hover_async(self, listener: AbstractViewListener, point_or_region: PointOrRegion) -> None:
hover_promises = [] # type: List[Promise[ResolvedHover]]
document_position = text_document_position_params(self.view, point)
point = point_or_region if isinstance(point_or_region, int) else point_or_region.begin()
for session in listener.sessions_async('hoverProvider'):
range_hover_provider = session.get_capability('experimental.rangeHoverProvider')
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, but I still think this is way too complicated for its own good?

I was imagining a server capability path hoverProvider.rangeSupport, which is a boolean, which is true when the server has support for a range in the request.

Client side request:

  • If the server has hoverProvider.rangeSupport, and
  • If the position is contained in the current selection, and
  • If the current selection is not empty,
  • then add params["range"] = region_to_range(first_selection_region(view), view)

(I'm unsure if all these additional checks other than the hoverProvider.rangeSupport are needed. I could imagine other servers wanting to have the information about the current position in any case, whether it's contained in the range or not)

Server side:

  • Check whether the request contains a "range" and do something clever with that

Client side response:

  • Show whatever the server returns

This whole scheme requires no more than 8 lines of diff, no? We're in a position to cook up an additional server capability and enhancement to the textDocument/hover request and I think the above scheme is the least intrusive for the spec.

Copy link
Member

Choose a reason for hiding this comment

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

By the way, whether the server capability is considered experimental or not (experimental.rangeHoverProvider) is an implementation detail for LSP-metals I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Client side request:

If the server has hoverProvider.rangeSupport, and
If the position is contained in the current selection, and
If the current selection is not empty,
then add params["range"] = region_to_range(first_selection_region(view), view)
(I'm unsure if all these additional checks other than the hoverProvider.rangeSupport are needed. I could imagine other servers wanting to have the information about the current position in any case, whether it's contained in the range or not)

Two scenarios are missing:

  • lsp_hover is invoked via keyboard and the mouse isn't hovering. textDocument/hover should be sent with selection
  • there is a selection but mouse is hovering an other location. textDocument/hover should be sent with mouse position

This is why there are four cases

Copy link
Member

Choose a reason for hiding this comment

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

I mean, maybe I want to see both the expression type of the range as well as type info of the position outside of the range using the mouse? Why must that be excluded?

Copy link
Contributor Author

@ayoub-benali ayoub-benali Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

yes and it is possible with the current implementation. We just can't send a request with both fields set: https://scalameta.org/metals/docs/integrations/new-editor#textdocumenthover

Why must that be excluded?

it is not excluded but as I said there are multiple combinations and these four checks are there for that.
Are you saying you can handle all those cases with fewer checks ?

Copy link
Member

Choose a reason for hiding this comment

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

lsp_hover is invoked via keyboard and the mouse isn't hovering. textDocument/hover should be sent with selection

This still feels wrong to me and I would argue to not send the selection if it is entirely unrelated to mouse hover. Shouldn't it be a separate command which can be used to assign a key binding then? For example lsp_selection_info or so.

Copy link
Member

Choose a reason for hiding this comment

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

Well, actually the lsp_hover already takes the optional point argument, and as implemented here it is just extending this concept. So feel free to ignore my concern if it's useful to combine them all into lsp_hover, even though not really related to "hover".

Copy link
Member

Choose a reason for hiding this comment

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

lsp_hover is invoked via keyboard and the mouse isn't hovering. textDocument/hover should be sent with selection

This still feels wrong to me and I would argue to not send the selection if it is entirely unrelated to mouse hover. Shouldn't it be a separate command which can be used to assign a key binding then? For example lsp_selection_info or so.

When using the keyboard the "point" has to follow the caret. It wouldn't make sense to follow the mouse pointer when using keyboard.

Yes, the naming of textDcoument/hover request makes it confusing to think about in terms of keyboard usage but if we want to allow triggering the hover info from the keyboard (which we are and should) then it only makes sense to follow the caret position. It would be unusable otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I think I had this confusion because I don't have this keybinding set up, and because lsp_hover does different things depending on how it was invoked. Should be fine then as it is!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwols I simplified it now and all cases work. Sorry for missing your point last time.

if range_hover_provider and isinstance(point_or_region, int):
for region in self.view.sel():
if region.contains(point):
# when hovering selection send it as range
document_position = text_document_range_params(self.view, region)
rchl marked this conversation as resolved.
Show resolved Hide resolved
else:
document_position = text_document_position_params(self.view, point_or_region)
else:
document_position = text_document_range_params(self.view, PointOrRegion)
rchl marked this conversation as resolved.
Show resolved Hide resolved
hover_promises.append(session.send_request_task(
Request("textDocument/hover", document_position, self.view)
))
Expand Down Expand Up @@ -209,15 +226,15 @@ def _show_hover(self, listener: AbstractViewListener, point: int, only_diagnosti
_test_contents.append(contents) # for testing only

if contents:
# The previous popup could be in a different location from the next one
rchl marked this conversation as resolved.
Show resolved Hide resolved
if self.view.is_popup_visible():
update_lsp_popup(self.view, contents)
else:
show_lsp_popup(
self.view,
contents,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
on_navigate=lambda href: self._on_navigate(href, point))
hide_lsp_popup(self.view)
rchl marked this conversation as resolved.
Show resolved Hide resolved
show_lsp_popup(
self.view,
contents,
flags=sublime.HIDE_ON_MOUSE_MOVE_AWAY,
location=point,
on_navigate=lambda href: self._on_navigate(href, point))

def _on_navigate(self, href: str, point: int) -> None:
if href.startswith("subl:"):
Expand Down