dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.23k stars 1.57k forks source link

Show type's definition and comment on hover of variable or var - make it easier to see the type comment #42684

Open atreeon opened 4 years ago

atreeon commented 4 years ago

I have the following code (and a possible feature request)

///This is an A
class Z {}

var z = Z();

it is difficult to get to the class's comment without navigating to it's definition. Would it be possible to show the type's definition when hovering over the variable z or the var keyword?

Screenshot 2020-07-14 at 09 56 51

bwilkerson commented 4 years ago

I'm not sure I'm understanding what you're asking for. Are you asking to have the documentation comment for the class Z displayed when showing the hover for the top-level variable z?

If so, then the technical answer is yes, we could provide that information to clients and they could display it. Clients would need to be careful to display it in such a way that makes it clear which comment is for the variable and which is for the type.

But if you're asking for a hover for var that exposes both the type of the variable and the comment for the type, then I think that's a much cleaner UX, and also something that would be easier to implement. We'll need to figure out what to do when there are multiple variables of different types, but I like the idea of having a hover for var.

atreeon commented 4 years ago

Ah cool! Yes hovering over var and getting some type information and comments would be great, if there are multiple variables I'd imagine they should all be able to evaluate to a specific type (or to dynamic).

Most generally I would just like some improvements to accessing the comments on variables. Hovering over z already shows Type: Z in IntelliJ but it doesn't show Z's comments. Maybe, if one clicks on the Type: Z text on the popup for variable z, could it then show the comment and some details for type Z?

bwilkerson commented 4 years ago

Hovering over z already shows Type: Z in IntelliJ but it doesn't show Z's comments.

I believe that the reason for that is because it does show zs comments, and it might be confusing to display both sets of comments unless they can be clearly differentiated.

@alexander-doroshko For ideas about IntelliJ improvements

alexander-doroshko commented 4 years ago

I like the idea to have hyperlinks in the documentation popup. In this case Z could be clickable in Type: Z. This is already done for Java in IntelliJ IDEA. To have it implemented in Dart we'd need to add the feature to the Anaysis Server protocol and handle it in the Dart plugin as well. Example for Java: image

bwilkerson commented 4 years ago

... add the feature to the Anaysis Server protocol ...

What information would need to be added?

alexander-doroshko commented 4 years ago

@bwilkerson

What information would need to be added?

HoverInformation object contains a bunch of Strings. Some strings are class names (like staticType which is equal to Z in the example above). Some may include other class names (e.g. elementDescription may be "class Foo extends Bar implements Baz"). To render Z, Bar, and Baz as links and show their docs on click, IDE needs information about the location of the corresponding classes (file path and offset - this way IDE will be able to ask for HoverInformation on click).

For elementDescription == "class Foo extends Bar implements Baz", IDE also needs to know clickable regions (I mean range of Bar and Baz in this string).

HoverInformation.dartdoc may also contain links to other classes/functions/fields/methods/whatever. It would be great to have them clickable too. This is an example where Object and hashCode could be clickable in the doc popup. image

Note that Object and hashCode are already clickable in code. IDE has navigation information for references inside doc comments. But it's quite tricky to use navigation information when rendering HoverInformation.

P.S. I guess there should be no Containing class line on the last screenshot. It doesn't add any info but eats precious space and distracts attention. Filed https://github.com/dart-lang/sdk/issues/40056.

bwilkerson commented 4 years ago

But it's quite tricky to use navigation information when rendering HoverInformation.

Do you mean that it's hard to use the hover information obtained for the enclosing file? If so, I would expect that. It wouldn't have the range information for the pieces of text in the HoverInformation.

But it sounds like we would essentially need a single list of NavigationTargets and one list of NavigationRegions for each of the fields of the HoverInformation that could contain references to documented elements.

Would we want to omit regions associated with elements for which there is no documentation (because there's no documentation to show if the user clicked on such a region)?

alexander-doroshko commented 4 years ago

Do you mean that it's hard to use the hover information obtained for the enclosing file? If so, I would expect that. It wouldn't have the range information for the pieces of text in the HoverInformation.

Yes, I mean that we have ranges in file but not in HoverInformation.dartdoc string.

But it sounds like we would essentially need a single list of NavigationTargets and one list of NavigationRegions for each of the fields of the HoverInformation that could contain references to documented elements.

Agree, that would work.

Would we want to omit regions associated with elements for which there is no documentation (because there's no documentation to show if the user clicked on such a region)?

Yes, I think we want to omit them if it's not expensive to pre-calculate. I guess we have some HoverInformation for any valid symbol, so we talk only about invalid references.

In Java there's also error highlighting in doc popup but I don't think this is a 'must have' feature. image

atreeon commented 3 years ago

Any update on this? @alexander-doroshko

Navdeep-Goyal commented 1 year ago

Any update on this if this is being worked on?