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

control-click on 'var' should navigate directly to the definition #2535

Closed
bsutton opened this issue Jun 9, 2020 · 6 comments
Closed

control-click on 'var' should navigate directly to the definition #2535

bsutton opened this issue Jun 9, 2020 · 6 comments
Labels
fixed in lsp in editor Relates to code editing or language features is bug
Milestone

Comments

@bsutton
Copy link

bsutton commented Jun 9, 2020

If I perform a 'ctrl-click' on a type such as:

Widget aWigglyWidget;

Then dart-code takes me to the library for the Widget type.

If I perform a 'ctrl-click' on a var such as:

var aWigglyWidget = getAWidget();

Then dart-code opens a code lense.
I both cases I want to be taken to the dart library for Widget.

@DanTup
Copy link
Member

DanTup commented Jun 11, 2020

@bwilkerson interested in your thoughts on this (it would need to be done in the server). If it seems sensible, does the server have easy access to get the type? (I'm happy to take a look if it's within my ability!).

@DanTup DanTup added awaiting info Requires more information from the customer to progress in editor Relates to code editing or language features is enhancement labels Jun 11, 2020
@bwilkerson
Copy link

Yes. It should be fairly straightforward to extend the _DartNavigationComputerVisitor to add regions for the keyword var.

If we do that, should we also provide navigation from const and final? If yes, should we always provide it or only when there is no explicit type?

(And out of curiosity, what's a "code lense"?)

@DanTup
Copy link
Member

DanTup commented Jun 11, 2020

If we do that, should we also provide navigation from const and final? If yes, should we always provide it or only when there is no explicit type?

I think if it was supported on them without types, it should also work with types - if a user has it in their mind that it works on const and their cursor is already on const, they might (reasonably) assume it'll work there and press the keybind for it.

out of curiosity, what's a "code lense"?

In VS Code (and I think VS), there's a feature called CodeLens that allows injecting links into the editor. We use it in the VS Code extensions to provide easy links for things like running/debugging tests:

Screenshot 2020-06-11 at 16 17 39

(unfortunately it doesn't work in the analysis server repo because it uses the outline data to know where tests are, and things work a bit differently there - something that bugs me and I'd love to find a fix for!).

However, I think in this context Brett was referring to the "peek"-style window that opens when Ctrl+Clicking var. It turns out we do actually find class, but we also include the location of the variable, and that results in VS Code this opening this to pick between the two definitions:

Screenshot 2020-06-11 at 16 20 54

So it turns out, we don't need to add the location of the class, rather we need to not include the variable. For non-LSP, this could be done in the VS Code extension code (assuming we can tell which one is which - perhaps when there are multiple we should ignore the one that's on the same line that we searched?), and for LSP this would be done in the server. WDYT?

@bwilkerson
Copy link

For non-LSP, this could be done in the VS Code extension code (assuming we can tell which one is which - perhaps when there are multiple we should ignore the one that's on the same line that we searched?), ...

Using the line number seems reasonable. It's highly unlikely that anyone will declare the type on the same line as a variable of that type.

... and for LSP this would be done in the server.

Yep, that would be the appropriate place. Then it will work the same for all LSP-based clients.

@DanTup DanTup closed this as completed in 2a952c1 Jun 11, 2020
@DanTup
Copy link
Member

DanTup commented Jun 11, 2020

Great - I pushed a fix here for non-LSP and a CL at https://dart-review.googlesource.com/c/sdk/+/150980/ for LSP.

@DanTup DanTup added is bug and removed awaiting info Requires more information from the customer to progress is enhancement labels Jun 11, 2020
@DanTup DanTup changed the title control-click on 'var' should navigate to the type's libraries. control-click on 'var' should navigate directly to the definition Jun 11, 2020
@bsutton
Copy link
Author

bsutton commented Jun 11, 2020 via email

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Jun 12, 2020
Fixes Dart-Code/Dart-Code#2535.

Change-Id: I2e64d0cd53abcde4ac3ecae1682b6b054b16407a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/150980
Commit-Queue: Danny Tuppeny <danny@tuppeny.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
@DanTup DanTup added this to the v3.12.0 milestone Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in lsp in editor Relates to code editing or language features is bug
Projects
None yet
Development

No branches or pull requests

3 participants