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

Go to Definition shouldn't open file when there are multiple definitions #31046

Closed
Timmmm opened this issue Jul 19, 2017 · 17 comments
Closed

Go to Definition shouldn't open file when there are multiple definitions #31046

Timmmm opened this issue Jul 19, 2017 · 17 comments
Assignees
Labels
editor-contrib Editor collection of extras under-discussion Issue is under discussion for relevance, priority, approach

Comments

@Timmmm
Copy link
Contributor

Timmmm commented Jul 19, 2017

  • VSCode Version: 1.14.1
  • OS Version: OSX 10.12.5

Steps to Reproduce:

  1. Create two files:
// foo.cpp
#include "foo.h"
int main() {
    return FOO;
}

// foo.h
#ifdef DEBUG
#define FOO 1
#else
#define FOO 2
#endif

  1. Ctrl-click or command-click on FOO in foo.cpp

VSC realises that FOO has multiple definitions and opens an interface to let you choose which one to go to. The problem is it does that after opening foo.h and jumping to a random #define FOO. It's really confusing. Look at this screenshot! I barely understand what is going on in this UI, and this is a simple example! Imagine actual code...

image

Wouldn't this make much more sense (excuse my poor image editing)?

image

This happens in all languages as far as I can tell.

@jrieken jrieken added the *question Issue represents a question, should be posted to StackOverflow (VS Code) label Jul 20, 2017
@jrieken
Copy link
Member

jrieken commented Jul 20, 2017

There are two separate features: "Peek Definition" and "Go to Definition". Ctrl+click is bound to the latter, not former. "Peek Definition" doesn't change the current location but shows all definitions of a symbol in-place. "Go to Definition" does change the current location, 'go to' vs 'peek'. In case of multiple definitions it jumps to the nearest definition, showing all definitions of that symbol in a peek view.

@jrieken jrieken closed this as completed Jul 20, 2017
@Timmmm
Copy link
Contributor Author

Timmmm commented Jul 20, 2017

Ah I see, sorry I didn't know the terminology. So to more succinctly describe the issue: "Go to Definition" should act exactly like "Peek Definition" does when there is more than one definition.

Can you reopen this? By the way if someone gives me a pointer to the code that controls this I might have a go at a patch.

@Timmmm Timmmm changed the title Peek shouldn't open file on ctrl-click Go to Definition shouldn't open file when there are multiple definitions Jul 20, 2017
@jrieken jrieken added under-discussion Issue is under discussion for relevance, priority, approach and removed new release *question Issue represents a question, should be posted to StackOverflow (VS Code) labels Jul 20, 2017
@jrieken
Copy link
Member

jrieken commented Jul 20, 2017

The design was a conscious decision and I am not ready to change the behaviour because of a single feedback item. That means there are no plans to change the behaviour unless a large enough group of people vote for this.

@jrieken jrieken reopened this Jul 20, 2017
@Timmmm
Copy link
Contributor Author

Timmmm commented Jul 20, 2017

Ah ok. To be honest I think it's obvious that that should be the behaviour. I asked on StackOverflow a while ago how to entirely disable it instead of improving it but I thought that would go down less well here. Seems like at least 9 other people find it confusing so far.

Also there are a couple of other very highly upvoted questions asking how to disable peek in Visual Studio proper, so I think it is not a particularly loved UI. You can turn it off in Visual Studio but I'd be happy if it just did the logical thing at least in VSCode.

@jrieken jrieken added editor-symbols definitions, declarations, references editor-contrib Editor collection of extras and removed editor-symbols definitions, declarations, references labels Jul 24, 2017
@chrisneal
Copy link

I agree with @Timmmm. If there are multiple definitions, it makes no sense for it to jump to a single definition, without giving the user the option of selecting which one. Sublime Text opens the navigator with the files containing the definition, so it's easy to select which one you want to open.

@CobusKruger
Copy link

This seems to have been fixed in a newer version. If I now hover over FOO in foo.cpp, I see the normal tooltip #define FOO 2. If I press Ctrl, the message expands to add the text "Click to show 2 definitions" and if I click while still holding Ctrl, I get the peek window, as requested.

@jaynadeau
Copy link

This does not seemed to be fixed in Version 1.24.1. When I ctrl+click on a method I am taken to the method with a list of all other methods with the same name. I would also prefer to stay in place and be presented with a list of options to choose. Furthermore, this feature also shows a list of all methods with the same name in all classes. This seems incorrect to me since the method is scoped to the class that contains it and it is not the same as any other methods in other classes. Yet I am presented with hundreds of options because they have the same name in other classes. There should be no peek in this situation.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Jun 22, 2018

The C/C++ extension would prefer Go to Definition not auto-goto the 1st definition found, since it typically wouldn't be the one desired so remaining at the same location is preferred -- users generally don't know beforehand if multiple definitions will be found or not. Some sort of opt-in setting would be fine (not sure what other languages have this problem). When our extension implements an improved go to definition which can actually find the correct (single) definition, then this won't be as much of an issue unless it falls back to the old behavior due to setup issues.

@johanforssell
Copy link

johanforssell commented Jul 3, 2018

I "my" TypeScript project I'm getting two definitions for this single function, which is annoying as f.

image

(Sorry for the close crop, there where some things I didn't want to show in the screenshot)

Any idea on what to do about this? Should I create an issue somewhere else?

@sean-mcmanus
Copy link
Contributor

@johanforssell If one of the Go to Definition targets is invalid, I believe you should create a new bug...otherwise, I think it's "by design". This issue is just about the fact that it auto-moves the context to one of the definitions.

@toh-ableton
Copy link

The design was a conscious decision and I am not ready to change the behaviour because of a single feedback item. That means there are no plans to change the behaviour unless a large enough group of people vote for this.

I also find the current behavior annoying. How do I vote, @jrieken?

@jrieken
Copy link
Member

jrieken commented Jul 6, 2018

Add a thumbs up there screen shot 2018-07-06 at 16 14 20

@graph
Copy link

graph commented Jul 11, 2018

I would like to add, right now for multiple definitions it goes to the first one in alphabetical order based on file names. It would be nice to be smarter than that. For example prefer the actual definition instead of a forward declaration. Maybe be even smarter and combine all the forward declarations into one tree unit.

And for less than a sane number of results like 20, just expand everything automatically. No reason to have 5 results in 5 different files all collapsed. For more than 20 expand good looking results automatically.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Jul 11, 2018

@graph The C++ extension is supposed to return definitions and exclude declarations unless no definitions are found (unless we have some bug), or maybe you're using a different definition provider.

+1 on auto-expanding the results under each file.

@graph
Copy link

graph commented Jul 11, 2018

Sorry I meant declarations everywhere I said definitions (I think, I always get these 2 terms mixed up). E.g. in opencv if you cmd+click some opencv structs it jumps to a forward declaration of the struct first which doesn't tell me what the fields it has, I have to click a few times to find the real declaration. I'm using the default plugins/providers I have no minimal custom setup with vs code.

@fazaldegui
Copy link

If I click "Peek Declaration (Ctrl+Alt+F12)" from the context menu it open the peek view without changing file. But using Ctrl+Click it does change the file, which is a bit annoying.
OS: Windows 10 64-bits
VSCode: 1.28.2
C++ extension: 0.19.0

@Timmmm
Copy link
Contributor Author

Timmmm commented Feb 9, 2019

This has hopefully been fixed by #68023!

@Timmmm Timmmm closed this as completed Feb 9, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 26, 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 under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants