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

Allow peek editor to be disabled #53164

Closed
ghost opened this issue Jun 27, 2018 · 50 comments
Closed

Allow peek editor to be disabled #53164

ghost opened this issue Jun 27, 2018 · 50 comments
Assignees
Labels
editor-symbols definitions, declarations, references feature-request Request for new features or functionality on-testplan
Milestone

Comments

@ghost
Copy link

ghost commented Jun 27, 2018

I'd like to be able to disable the peek editor. I don't want to see it or use it ever.

Thanks for a great product.

@vscodebot
Copy link

vscodebot bot commented Jun 27, 2018

@vscodebot vscodebot bot added the editor label Jun 27, 2018
@roblourens
Copy link
Member

Can you just avoid using "find all references", "peek definition", etc? We don't have another UI option for "find all references" currently. Can you explain some more about what you want?

@roblourens roblourens added the info-needed Issue requires more information from poster label Jun 27, 2018
@ghost
Copy link
Author

ghost commented Jun 27, 2018

I don’t want it to pop up when I use “go to definition“, even if there are multiple definitions.

@mjbvz
Copy link
Collaborator

mjbvz commented Jun 27, 2018

What would be the expected behavior when there are multiple definitions? There would be no way to reach anything but the first one

@ghost
Copy link
Author

ghost commented Jun 28, 2018

So, multiple definitions seem to always happen in cases like this:

function foo() { /* ... */ } // <-- definition 2

export default {
  foo, // <-- definition 1
};

and the only time the peek editor shows up is when it navigates to a different file. Once I'm in that file, at definition 1 in my example above where I always land, activating "go to definition" again brings me to definition 2.

So, going to definitions seems to be a completely independent of the peek editor.

What about multiple definitions requires a visual UI? I'd like to just navigate them as if I were navigating through a chain of bookmarked spots, by continually pressing F12.

@JesterXL
Copy link

+1 for:

  1. giving us a way to disable this completely
  2. default to "finding first"; we trust y'alls ability to make a reasonably good guess as to what's first

Mine's completely broken with curried functions in the new version of Code, old worked. Example, I have a function called createUserUnsafe:

.then( ({salt, encryptedPassword}) =>
    createUserUnsafe(
        dbClient,
        newUsername,
        encryptedPassword,
        salt,
        newEmail
    )
)

I COMMAND+Click, and it shows that it has 2 entries, which is not true:

screen shot 2018-06-28 at 9 53 02 am

  1. I'm not using TypeScript
  2. I only defined this function once, in the same file.

Rather than fix it to support curried functions, just give us the ability to turn this off, and guess if you find multiple.

@jrieken
Copy link
Member

jrieken commented Jul 9, 2018

at definition 1 in my example above where I always land, activating "go to definition" again brings me to definition 2.

Ok, you describe a mode in which F12 walks you to all definition of that symbol (function foo) but isn't the question then how to you leave that mode?

@ghost
Copy link
Author

ghost commented Jul 9, 2018

@jrieken There is no mode to leave. If I'm in a file and I hit F12 on a function call and that function is defined in the same file...there is no visual that pops up. The cursor simply jumps to the locally defined function.

@jrieken
Copy link
Member

jrieken commented Jul 10, 2018

There is no mode to leave. If I'm in a file and I hit F12 on a function call and that function is defined in the same file...there is no visual that pops up. The cursor simply jumps to the locally defined function.

Yeah, but that's how it works today. uUnless there are multiple definitions in which case don't pick on randomly but show the peek.

@ghost
Copy link
Author

ghost commented Jul 10, 2018

Yes and I'm asking them to pick one and not show the peek editor.

They are already picking one randomly (although, it's not that random) and navigating to that spot right before showing the peek editor.

So, the simple request here is to just not show the peek editor. If other users want the peek editor - it can be enabled by default for those people.

@ghost
Copy link
Author

ghost commented Jul 10, 2018

I'm going to work on an AutoHotKey script to do what I want, which is basically this:

If VSCode Window is Active,
When User Hits F12;
Wait 50ms
Send the ESC key;

@jrieken
Copy link
Member

jrieken commented Jul 10, 2018

Adding a setting that reduces many definition to just one is easy if that's what people want. Instead of fiddling with AutoHotKey, how about a PR? Code pointer is this: https://github.com/Microsoft/vscode/blob/763dea187d39c04b68281049814d3b494a408a63/src/vs/editor/contrib/goToDefinition/goToDefinitionCommands.ts#L99

@jrieken jrieken added help wanted Issues identified as good community contribution opportunities editor-symbols definitions, declarations, references good first issue Issues identified as good for first-time contributors and removed info-needed Issue requires more information from poster labels Jul 10, 2018
@ghost
Copy link
Author

ghost commented Jul 10, 2018

OK, I was looking through the code surrounding the peek editor earlier and I couldn't find anything obvious.

For some reason, that link doesn't work for me - I get a 404 - however, there's enough information there to find the file. Thanks!

@ghost
Copy link
Author

ghost commented Jul 10, 2018

I think this is where the decision is made - https://github.com/Microsoft/vscode/blob/987160c70adb3fdab30da30fef7d1c05a74ff858/src/vs/editor/contrib/goToDefinition/goToDefinitionCommands.ts#L138-L143

I have no clue how to get the setting of a global value from within that module, so I'm just leaving this note here in case I can't get it done.

@jrieken
Copy link
Member

jrieken commented Jul 11, 2018

This is a sample for how to add a new setting to the system (https://github.com/Microsoft/vscode/blob/0c27d9b38e1ae2572e32475abde13d29f2eb71bf/src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts#L494-L506) and then use the IConfigurationService to read the value

@mjbvz mjbvz removed their assignment Jul 12, 2018
@ghost
Copy link
Author

ghost commented Jul 13, 2018

So, if I actually put some work into doing this - how long can I expect to wait before my pull request is merged?

I might just use my 15-minute AutoHotKey solution for now because it's very simple compared to possibly waiting weeks or months for a merge and/or maintaining my own branch of VSCode...

@ghost
Copy link
Author

ghost commented Jul 13, 2018

The reason I ask is that I've seen other good pull requests go un-merged and I don't want to waste my time if that's going to happen.

@jrieken
Copy link
Member

jrieken commented Jul 16, 2018

wait before my pull request is merged?

Hard to make a prediction... The first hurdle (do we want this) is already taken, it's mostly about code quality then.

@10hendersonm
Copy link

The peek editor is pretty annoying in my workflow too.. Here's an example:

import React, {Component} from 'react'

class MainComponent extends Component {
  render () {
    return <SubComponent text="Hello World" />
  }
}

const SubComponent = ({text}) => {
  return (
    <div>{text}</div>
  )
}

export default MainComponent

If I F12 into SubComponent, it will pull up 2 definitions in the peek editor. One for the word SubComponent in the constant declaration, and the second for...

({text}) => {
  return (
    <div>{text}</div>
  )
}

Why would I ever need multiple definitions in this scenario? It's literally calling the function and the function's own declaration 2 different definitions. I have never used the peek editor, only ever dismissed it.

Please, give me an option to let it die.

For a stretch goal, it could be made less intrusive. Take me to the first definition, but put a small tooltip or intellisense context menu thing telling me there are additional definitions, and allowing me to go to the next one.

@the-noob-101
Copy link

the-noob-101 commented Dec 27, 2018

Just saw the PR, no reviews since September. Anything I can do to speed up the process / help on that PR ?
I would really fancy that feature to be honest

@hhu94
Copy link
Contributor

hhu94 commented Jan 30, 2019

I'm revisiting this issue now and I can't find any situation in which Go to Definition would produce more than one definition (at least not in Typescript).

If someone can point me to a way to reproduce the issue I'll resolve the conflicts on my PR and push to get it merged. Otherwise I'll close it since it would mean the issue is not an issue anymore.

@DaanDeMeyer
Copy link

In a C++ source file, if I Go To definition of a function with its implementation in that source file and its declaration in a header file, Go To Definition will jump to the implementation in the source file but the peek editor will still pop up with the declaration in the header file.

Ideally VSCode would just jump to the implementation and the peek editor wouldn't pop up.

@hhu94
Copy link
Contributor

hhu94 commented Jan 30, 2019

@DaanDeMeyer I can't seem to reproduce this. Which C++ extension are you using? Using this one , Go to Definition always jumps to the declaration in the header file, it doesn't show the peek editor.

In fact even using Peek Definition shows just the declaration in the header file, not the implementation.

// my_class.h
namespace N
{
    class my_class
    {
    public:
        void do_something();
    };

}
// my_class.cpp
#include "my_class.h" // header in local directory

using namespace N;

void my_class::do_something()
{
}
// my_program.cpp
#include "my_class.h"

using namespace N;

int main()
{
    my_class mc;
    mc.do_something();
    return 0;
}

@DaanDeMeyer
Copy link

DaanDeMeyer commented Jan 30, 2019

I'm using clangd (from LLVM) with the corresponding extension vscode-clangd.

To reproduce the problem I have to Go To Definition of an implementation from the same source file where the implementation is defined (in your example, do_something would have to be called inside another function or method in my_class.cpp

@jrieken
Copy link
Member

jrieken commented Jan 30, 2019

To reproduce the problem I have to Go To Definition of an implementation from the same source file where the implementation is defined (in your example, do_something would have to be called inside another function or method in my_class.cpp

If you think "go to definition" shouldn't return multiple definitions (in some cases) then I would strongly encourage you to file an issue against the extension. Assume we implement "disablement" of peek by picking just one of many elements, how high is the change that the element we pick is the one you want? The "disablement" solution might always pick the wrong one, wrecking this feature for you.

@DaanDeMeyer
Copy link

I'm mostly looking to replicate the behavior from CLion (C++ IDE from Jetbrains). ctrl+b would move to the declaration/definition when used at a call site. When positioned over a definition/declaration, ctrl+b's semantics would change to switch between the declaration/definition. So to go to the declaration I'd just have to invoke ctrl+b a maximum of two times.

With VSCode, the peek editor pops up and I'm forced to use the arrow keys to get to the declaration/definition I need. As long as the amount of declarations/definitions isn't high (I'm thinking there are few cases where it would exceed 2), pressing ctrl+b again to get to the other declaration is a single keystroke whereas with the peek editor I have to navigate with the arrow keys to the right declaration which in my case with C++ is 5 keystrokes (4 to navigate + enter).

If the correct definition/declaration is chosen, the peek editor still pops up and I have to press a second keystroke (esc) to get it to disappear.

In short, I'd prefer to replace the peek editor with the Go To definition shortcut that navigates through the different declarations/definitions one by one. The order of declarations/definitions should be determined by the LSP extension used.

@jrieken
Copy link
Member

jrieken commented Jan 30, 2019

When positioned over a definition/declaration, ctrl+b's semantics would change to switch between the declaration/definition. So to go to the declaration I'd just have to invoke ctrl+b a maximum of two times.

Oh, I like that. We could be smarter there, esp when having two and being on one of them.

In short, I'd prefer to replace the peek editor with the Go To definition shortcut that navigates through the different declarations/definitions one by one.

Yes, that's what I have in mind and that's better than picking a single element. But also that isn't stateless and at least some status bar like 'At Definition 1 of 17' would be needed and a way to get out of this.

@DaanDeMeyer
Copy link

and a way to get out of this.

Can this be accomplished by just moving the cursor out of the declaration/definition name?

But also that isn't stateless

Depending on the exact definition of stateless, it seems stateless to me. The way I see it, given a list of definitions/declarations and the current definition where we're at, Go To Definition would simply move us to the next definition/declaration in the list. If we're at a call site, Go To Definition moves us to the first definition/declaration in the list.

The counter can then simply be the index of the current definition/declaration in the list.

at least some status bar like 'At Definition 1 of 17'

I'm not sure how many definitions/declarations we can realistically expect to have in a single list. I'm not sure if this would be used to cycle between virtual method implementations as well. If that's the case, the number could potentially be high and in that case a visual element telling at what definition we're at would definitely be useful.

Another thing that might be useful if the number of declarations/definitions is high is an option to open the peek editor since that's a scenario where it becomes really useful.

@hhu94
Copy link
Contributor

hhu94 commented Jan 31, 2019

I feel like allowing disabling of the peek editor would be more of a workaround the problem instead of a real solution to the problem. The problem being that the UX around Go to Definition shouldn't use the peek editor, but a different mechanism.

Because of this I feel like my PR doesn't add much value and so I should abandon it. What do you think @jrieken ?

@Asgator
Copy link

Asgator commented Feb 5, 2019

@hhu94 if i have component with enhancer export default withDeviceInfo(FilterVacanciesMain); and jumping on him, this window will seem to me

@jrieken
Copy link
Member

jrieken commented Mar 11, 2019

With #70032 I have added a new setting editor.gotoLocation.many with options peek, revealAndPeek, and reveal. The last option is very much related to this issue because it effectively disables peek and reveals the "first" result only.

@leodutra
Copy link

leodutra commented Apr 5, 2019

I suggest "definition hierarchy" peek in another shortcut, as on Eclipse and some others.
This would prevent users from having to config to find a comfortable usage.

@dutzi
Copy link

dutzi commented Apr 16, 2019

It's "editor.gotoLocation.multiple": "goto" now

@gucoi
Copy link

gucoi commented May 1, 2019

It's "editor.gotoLocation.multiple": "goto" now

i use this but no reply

@jrieken jrieken removed good first issue Issues identified as good for first-time contributors help wanted Issues identified as good community contribution opportunities labels May 21, 2019
@jrieken jrieken added this to the May 2019 milestone May 21, 2019
@jrieken
Copy link
Member

jrieken commented May 21, 2019

Remaining work for May, make F12 go to the next declaration/definition when invoked from within one, see #53164 (comment)

@jrieken
Copy link
Member

jrieken commented May 23, 2019

This is how it will look like

  • use 'goto' setting
  • trigger go to def, go to decl, go to implementation on a symbol with multiple results
  • the first is revealed, status message shows, hitting F12 cycles through them, hitting ESC or selection changes end this

May-23-2019 19-54-36

@jrieken jrieken closed this as completed May 23, 2019
@gucoi
Copy link

gucoi commented May 24, 2019

thank you for this

@AndreVanKammen
Copy link

Thanks for the option, the peek window is a really annoying one which breaks my workflow because escape doesn't make it go away. I have to grab my mouse to get it out of the way which is not very user friendly. The other option is pressing enter, but only if you don't press escape 1st. Also the focus (choices) are shown on the right side of the preview, not the point where is was looking.

A simple popup with the filenames+line numbers as choices is better IMHO.

@jrieken
Copy link
Member

jrieken commented Jun 24, 2019

because escape doesn't make it go away

Well, the default is to hide on Esc. Do you use the editor.stablePeek setting by any chance?

@vscodebot vscodebot bot locked and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
editor-symbols definitions, declarations, references feature-request Request for new features or functionality on-testplan
Projects
None yet