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 + Click on @override should take you to the method being overriden #55932

Open
yjbanov opened this issue Jun 4, 2024 · 8 comments
Open
Labels
analyzer-server analyzer-ux area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@yjbanov
Copy link

yjbanov commented Jun 4, 2024

I think this is a feature request for the Dart analyzer, but maybe VSCode (I'd love this to work in all editors though).

When I Ctrl + Click on the @override annotation, it take me to the definition of the @override annotation. By now I know what @override means. The vastly more important use-case is to be able to go from the overriding method to the overridden method. So I think control-clicking on this annotation to take you to the overridden method.

To reproduce, use this code:

class A {
  void foo() {}
}

class B extends A {
  @override
  void foo() {}
}

Ctrl + Click on @override, observe.

@bwilkerson bwilkerson added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jun 4, 2024
@bwilkerson
Copy link
Member

Yes, this is a request for the analysis server, and yes, if we made this change it would work in all of the IDEs that support navigation through the analysis server.

I agree that it would be good to have better support for navigating to the overridden method.

The one downside I can think of to using ctrl-click navigation for that purpose is that it makes the feature inconsistent. Everywhere else (including other annotations) it will take you to the declaration of the identifier, but here it wouldn't. I don't know how jarring that behavior would be for newer users.

We've been discussing how to provide good navigation when augmentations are enabled and one of the ideas that was proposed was to use code lenses to navigate to important targets, including to overridden methods. That solution leaves ctrl-click navigation consistent while providing more options for navigating to where users might want to go (including being able to navigate both directions in the override chain).

As noted in that comment, there are already commands to support some of this navigation, but they aren't very discoverable.

@yjbanov
Copy link
Author

yjbanov commented Jun 5, 2024

TIL what "CodeLens" is, so this is all based on very early impression. I'm open to alternatives to ctrl-click, but I have concerns about CodeLens specifically:

  • Is this a proprietary VSCode feature? If so, how can it cover all editors?
  • It injects copious amounts of UI into the editor with no user action. That seems excessive.
  • It seems all CodeLenses need to be precomputed across the entire file to generate these extra links. This makes me worried about performance.

@ykmnkmi
Copy link
Contributor

ykmnkmi commented Jun 5, 2024

Why not click on the member name? Currently, it does nothing (VS Code). If it is overridden, go to the original member.

@keertip keertip added analyzer-ux P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug labels Jun 5, 2024
@bwilkerson
Copy link
Member

Is this a proprietary VSCode feature? If so, how can it cover all editors?

No, it's supported by LSP, so any editor using LSP can support this feature. (Not all LSP-based editors choose to support it, but there's not much we can do about that.)

IntelliJ and Android Studio don't use LSP, but they both have different support for this feature (icons in the editor's left gutter) that the analysis server has been supporting for many years. VS Code is actually behind IntelliJ in this case.

It injects copious amounts of UI into the editor with no user action. That seems excessive.

That's a fair concern. I believe that the current plan is to have a user-setting to enable or disable those code lenses, and to have commands on the pop-up menu for all of these options. Menu items aren't as convenient, but they are less invasive.

It might be worth opening an issue with the VS Code team asking for a cleaner UI.

It seems all CodeLenses need to be precomputed across the entire file to generate these extra links. This makes me worried about performance.

That's also a fair concern, and we'll be keeping a close eye on the performance characteristics. My expectation, partially based on the experiments that @DanTup wrote up, is that it's not likely to be an issue. Hopefully those aren't "famous last words".

Why not click on the member name? Currently, it does nothing (VS Code). If it is overridden, go to the original member.

That would also be a viable option, but it only handles one direction of navigation, and it's useful to be able to navigate both ways (up and down the override chain).

We will also need to support navigation across the 'augmentation' chain if we decide to support augmentations and macros. Having a consistent UX for navigation might trump many other considerations.

@DanTup
Copy link
Collaborator

DanTup commented Jun 5, 2024

FWIW, we do have a "Go to Super" command + context menu entry that will take you where you want in the example above:

Screenshot 2024-06-05 at 17 42 07

Why not click on the member name? Currently, it does nothing (VS Code). If it is overridden, go to the original member.

Ctrl+Click on a declaration actually does do something in VS Code. By default it will try to "go to definition" and when we return a definition that matches where you invoked it, it will fall back to "go to references". So in your example code above, if there are calls to foo() it should show those results. This behaviour can be configured in VS Code (although the options are limited to other similar commands):

Screenshot 2024-06-05 at 17 51 49

Unfortuantely we can't really change the behaviour here much because VS Code doesn't ask us to "handle a Ctrl+Click here", it just asks us "Where is the definition of the reference at this location", which is a generic request it uses in many places. Returning the overridden member instead could cause other functionality that uses "definition" to behave incorrectly. While it's more limiting, this does also ensure more consistency between different languages because the kind of navigation is configured once centrally.

I did file an issue to about making "Super" a first-class feature in VS Code (so it might have better UI), but unfortunately it was rejected (microsoft/vscode#47126).

@yjbanov
Copy link
Author

yjbanov commented Jun 5, 2024

After playing around with it a bit, I think the current state of things is sufficient. I see that the "Go to Super Class/Member" is also available from the keyboard-invocable menu (Ctrl + Shift + P in VSCode), so this action can be performed without much extra UI, or having to do much mouse/trackpad work.

That said, for all intents and purposes @override is part of the language, even if it's technically not according to the language team. Perhaps it should formally become a modifier keyword, like @required did. There are several places in Dart where keywords could be excellent navigation anchors, such as:

override void foo() {}
var bar = getBar();
final baz = getBaz();

override could navigate to the super method, and var and final could navigate to the inferred type declaration.

As for this issue, I'm OK with closing it given the existing functionality.

@bwilkerson
Copy link
Member

There's a proposal for override becoming a language feature: https://github.com/dart-lang/language/blob/main/working/1610%20-%20override/proposal.md.

... var and final could navigate to the inferred type declaration.

I like that idea.

@DanTup
Copy link
Collaborator

DanTup commented Jun 5, 2024

FWIW, in LSP we already support "Go to Type Definition" which you can invoke on a variable (not only where it's declared) to jump directly to the type :-)

Screenshot 2024-06-05 at 19 55 48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-server analyzer-ux area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants