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

Ctrl hover shows a less useful hint when fn opening brace is on a new line #39458

Closed
bmewburn opened this issue Dec 2, 2017 · 9 comments
Closed
Assignees
Labels
editor-contrib Editor collection of extras editor-hover Editor mouse hover good first issue Issues identified as good for first-time contributors
Milestone

Comments

@bmewburn
Copy link

bmewburn commented Dec 2, 2017

ctrl_hover_hints

The textDocument/definition response range is that of the entire symbol definition (header + body) in both cases.

@vscodebot vscodebot bot added editor editor-contrib Editor collection of extras labels Dec 2, 2017
@bmewburn bmewburn changed the title Ctrl hover does not show a useful hint when opening brace is on a new line Ctrl hover does not show a useful hint when fn opening brace is on a new line Dec 2, 2017
@bmewburn bmewburn changed the title Ctrl hover does not show a useful hint when fn opening brace is on a new line Ctrl hover shows a less useful hint when fn opening brace is on a new line Dec 2, 2017
@cleidigh cleidigh added the editor-hover Editor mouse hover label Dec 2, 2017
@mjbvz mjbvz added the php PHP support issues label Dec 4, 2017
@mjbvz mjbvz assigned roblourens and unassigned mjbvz Dec 4, 2017
@roblourens roblourens removed the php PHP support issues label Feb 9, 2018
@roblourens roblourens assigned mjbvz and unassigned roblourens Feb 9, 2018
@roblourens
Copy link
Member

@mjbvz Problem is not with PHP, is with the hover widget and rendering these LSP messages. Who owns that?

@mjbvz mjbvz assigned alexdima and unassigned mjbvz Feb 9, 2018
@alexdima
Copy link
Member

alexdima commented Mar 1, 2018

This is the jump to definition hover.

@alexdima alexdima assigned bpasero and unassigned alexdima Mar 1, 2018
@alexdima alexdima removed the editor label Mar 1, 2018
@bpasero
Copy link
Member

bpasero commented Mar 1, 2018

I think the current incarnation of that hover came in via 34cdf9f

@bpasero bpasero assigned jrieken and unassigned bpasero Mar 1, 2018
@jrieken jrieken added the good first issue Issues identified as good for first-time contributors label Mar 1, 2018
@faso
Copy link

faso commented Mar 1, 2018

I'd like to take this one

@jrieken
Copy link
Member

jrieken commented Mar 1, 2018

It's yours @faso. The code is here and I am slightly embarrassed that there are no tests: https://github.com/Microsoft/vscode/blob/e7bc65ad3e1d5cf5b32054d86cbba6e2fef05af3/src/vs/editor/contrib/goToDeclaration/goToDeclarationMouse.ts#L144

It's a heuristic that does the following: Once the target document is resolved (textEditorModel) we try to find "good chunk to preview". It starts at the (start) line of the reference and walks downwards until it finds a line with the same indentation. That works ok for foo() { but not for foo()\n{... I think a nicer model would be to look for balanced brackets, e.g. continue until you find the right }. Maybe a combination of both approaches because often references are to fields/constants and not bracket-blocks (like functions et al).
Hope I didn't scare you, feel free to ask questions.

@zeerorg
Copy link

zeerorg commented Apr 7, 2018

If this issue is open I would like to take this up.
I also looked around the code pointed to by @jrieken and tried to figure what was going on.
As suggested, we can improve this by

  1. Differentiating between variables and functions/classes
  2. For functions/Classes scanning the lines and using something like a simple stack to figure out where the scope ends.

@jrieken
Copy link
Member

jrieken commented Apr 9, 2018

@zeerorg it is up to @faso who put his stick into the ground first (albeit quite a while back). Still, be aware of us not knowing the semantics of sources. That makes assumptions like 'for a variable do this', 'for a method do that' very hard.

@natiiix
Copy link

natiiix commented May 31, 2018

Not sure if this is entirely related to the issue, but I couldn't find a more appropriate one. The behavior when hovering over a variable is a little confusing at first sight, as pointed out by @zeerorg.

Right now it simply reads the next non-whitespace line of code, which has usually nothing at all to do with the variable itself. See the attached image for reference.

image

The same problem seems to occur for C# inline-defined (not sure what is the official name) functions.

image

An idea for a possible fix for C-like languages (admittedly not very robust):
If the symbol name is followed by =, look for ;, otherwise look for }.
Of course, it must not appear inside of a string literal or a nested scope.
This would theoretically solve the initial problem, the problem with variable definitions and the problem with inline-defined functions. It would also work for code with no indentation.
It should be possible to come up with a similar rule for indentation-based language such as Python.

@jrieken jrieken added this to the June 2018 milestone Jun 21, 2018
jrieken added a commit that referenced this issue Jun 21, 2018
@jrieken
Copy link
Member

jrieken commented Jun 21, 2018

fixed via #52506

@jrieken jrieken closed this as completed Jun 21, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-contrib Editor collection of extras editor-hover Editor mouse hover good first issue Issues identified as good for first-time contributors
Projects
None yet
Development

No branches or pull requests

10 participants