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

Feature Request: open Peek References when Ctrl-click is on the symbol in its declaration #73081

Closed
inliquid opened this issue Apr 30, 2019 · 15 comments
Assignees
Labels
editor-contrib Editor collection of extras editor-symbols definitions, declarations, references feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@inliquid
Copy link

Initially this issue was opened in vscode-go extension repo: microsoft/vscode-go#2451

Maintainer (@ramya-rao-a) asked to reopen it here.

By default, with Ctrl-click you go to symbol definition, f.e. to a function declaration. When you make Ctrl-click on a declared name, VS Code in fact does nothing, which seems counter-intuitive. In most of the cases I need to Shift-F12 to see the usages of that symbol or if I want to go back from that place after some navigation happened around that symbol.

I think it would be very useful, if when you Ctrl-click on a function, variable, or a type name in a place where the symbol is defined, VS Code will open a Peek References of that symbol. This would be close to what Goland does by default and what I personally found extremely helpful.

@vscodebot vscodebot bot added the editor-contrib Editor collection of extras label Apr 30, 2019
@jrieken jrieken added editor-symbols definitions, declarations, references feature-request Request for new features or functionality labels Apr 30, 2019
@ramya-rao-a
Copy link
Contributor

Translating the above in terms of providers:

When a definition provider returns the same location as the word on which the cursor is, trigger Peek References command

@jrieken jrieken self-assigned this Oct 24, 2019
@jrieken jrieken added this to the November 2019 milestone Oct 24, 2019
@jrieken jrieken closed this as completed in 1711f6e Nov 5, 2019
@bobbrow
Copy link
Member

bobbrow commented Nov 8, 2019

We find this feature very confusing. Can we have a conversation about it before it ships in stable?

C++ find all references is (potentially) very expensive to compute and it's counter-intuitive to invoke feature B when you ask for feature A. Can you expose the ability to invoke find all references via goto definition some other way? I think a lot of people are going to be confused by this behavior.

@jrieken
Copy link
Member

jrieken commented Nov 9, 2019

Well, users have asked for it and it's when other editors, like IntelliJ, do. For instance, this is a nice comparison from the perspective of a power user: https://gist.github.com/joeytwiddle/49ecf347bf97c5a67a8d713dd437efcf#go-to-defintionreferences

We find this feature very confusing. Can we have a conversation about it before it ships in stable?

I am unsure if you say this as a user or as the owner of a slow references provider? For C++ or for slow providers in general I feel it needs some way to allow them to express that they are slow. TBH even today peek references doesn't work well for slow providers and either per provider or per request a way to say "this will take a while" might be needed. In that case the UX can adapt and for instance not offer such rapid code navigation features.

@bobbrow
Copy link
Member

bobbrow commented Nov 9, 2019

This was reported to me as counter intuitive by my team members and I agreed with them. The potential slowness of find all references for C++ is just an additional disadvantage that we are sensitive about and I probably shouldn't have mentioned it.

Just to be clear, I'm not saying you shouldn't ever have this feature because clearly there are a few people who want it. But the number of upvotes here doesn't seem to make the case that this is a universally desired feature so perhaps a setting should exist to control the behavior or a new key binding should be added. Users are accustomed to CTL+click and F12 doing a goto definition operation. Having that spuriously change to find all references depending on the context creates uncertainty for the user.

Another idea I just had while typing this... What if hover (or CTL+hover) was improved to add a hyperlink that links to the command to find all references? The fixit experience for problems provides a UI paradigm that could be reused for this:
image

Instead of "Peek Problem" or "Quick Fix", links could be added for "Find References" or "Peek References". In this case the developer knows exactly which language feature will execute. And the cost in clicks to invoke this feature can equal that of WebStorm.

@jrieken
Copy link
Member

jrieken commented Nov 11, 2019

desired feature so perhaps a setting should exist to control the behavior or a new key binding should be added.

Sure, we can always add a way to enable/disable this and in fact insiders is the vehicle to collect such feedback. Yours and your team's feedback has now been noted and see what else comes in. Note that we have also received positive feedback. Still, adding a new feature and disabling it by default is almost like not adding a new feature and assuming we have a setting to control, it will likely be on by default.

@bobbrow
Copy link
Member

bobbrow commented Nov 11, 2019

Note that we have also received positive feedback

Again, I'm not trying to contest that this feature is bad or confusing for everyone. I don't think we have enough data at this point to make a call about the majority opinion though (and maybe we won't until after it ships in stable).

I suggested an alternate implementation that has zero chance of confusion but I didn't hear your thoughts on that proposal. If the goal is quick access to additional language server commands, then making those commands easily available in the hover UI would address that need without introducing confusion for users who have been using F12 for years.

@jrieken
Copy link
Member

jrieken commented Nov 11, 2019

I suggested an alternate implementation that has zero chance of confusion but I didn't hear your thoughts on that proposal.

I did ping you on a partner issue where this was already suggested. In fact Java is doing something very similar already today and we could make it a thing. Having said that, I don't think they are mutually exclusive - I actually want both

@yume-chan
Copy link
Contributor

yume-chan commented Nov 14, 2019

Seems that in JavaScript files, imports with a proper definition (from source file or .d.ts) are considered as "references", but ones without are "declarations".

So now when I press F12 on those imports, some of them will jump to their declarations but others will peek all references. It's super confusing and super annoying and I consider this as a bug.

@mdzahidh
Copy link

is there a way to turn off this feature please ? this gives pretty horrible experience with C/C++

@sean-mcmanus
Copy link
Contributor

Yeah, we would like some way we could disable it for our C/C++ extension.

@inliquid
Copy link
Author

I'm wondering, why do you guys ever Ctrl-click (or press F12) when you are at definition of smth, if you don't want IDE to find anything about this definition?

@sean-mcmanus
Copy link
Contributor

@inliquid Users might not notice they're at the definition already (i.e. they may mistake the code for a C++ declaration or vice versa) or they might expect to go to the declaration instead. Maybe users will stop doing that once they realize it invokes Find All References?

@jrieken
Copy link
Member

jrieken commented Nov 19, 2019

@inliquid Users might not notice they're at the definition already (i.e. they may mistake the code for a C++ declaration or vice versa)

We can think about adding a setting which defines the fallback command, e.g. goto declaration for cpp from definitions or simply nothing

@jrieken
Copy link
Member

jrieken commented Nov 22, 2019

fyi - I have pushed a commit that adds settings like editor.gotoLocation.alternativeDefinitionCommand etc. They allow to define what happens when the corresponding go-to-command navigates to the current location, e.g @sean-mcmanus could make definition and declaration be "counter parts" so that go to definition on a definition goes to the declaration and the other way around

@bobbrow
Copy link
Member

bobbrow commented Nov 22, 2019

Thanks @jrieken!

@jrieken jrieken added the verification-needed Verification of issue is requested label Dec 2, 2019
@roblourens roblourens added the verified Verification succeeded label Dec 4, 2019
@jrieken jrieken added the on-release-notes Issue/pull request mentioned in release notes label Dec 5, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 20, 2019
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-symbols definitions, declarations, references feature-request Request for new features or functionality on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

8 participants