Dart-Code / Dart-Code

Dart and Flutter support for VS Code
https://dartcode.org/
MIT License
1.47k stars 300 forks source link

Hide redundant inlay hints #4890

Open dcharkes opened 9 months ago

dcharkes commented 9 months ago

Edit: Repurposed issue to hide redundant inlay hints https://github.com/Dart-Code/Dart-Code/issues/4890#issuecomment-1847326547

Is your feature request related to a problem? Please describe. When calling functions with positional parameters, it might not be obvious at the call site what the parameters mean.

void main() {
  eatBananas(true, false, 123);
}

Describe the solution you'd like

DartCode displays <param-name>: in light gray before the parameter, unless the expression passed in is a variable name with exactly the parameter name.

void main() {
  // these are _not_ named parameters, but VSCode displays them as is with light gray!
  eatBananas(peelFirst: true, hidePeels: false, numberOfBananas: 123);
}

Example from the cpp editor in VSCode, see name::

image

Describe alternatives you've considered

DanTup commented 9 months ago

@dcharkes I think this is handled by inlay hints (see https://github.com/dart-lang/language/issues/3502#issuecomment-1847310443 / https://github.com/Dart-Code/Dart-Code/issues/3609 ?

dcharkes commented 9 months ago

Way ahead of me! (Christmas came early! 🎁 )

Apparently I didn't know what to search for! https://dartcode.org/releases/v3-58/

Thanks @DanTup keep up the great work! 🚀

dcharkes commented 9 months ago

Can we hide the inlay hints if the param name is exactly the same as the argument?

image
DanTup commented 9 months ago

We could, but I'm not sure if it would be obvious to users what's happening there. Since we don't display these by default, we've so far always been very explicit with them (often showing redundant information for consistency).

dcharkes commented 9 months ago

It is the behavior in the C++ editor, and it I have it always on there. If it's less clutter, that's more an argument for having it on by default. (Just we like have the closing brackets comments on by default.)

Not sure who would be the right person to make a call here. My personal vote would be always on but hide the redundancy that doesn't add any information. My experience in the C++ editor which switched last year to show inlay hints has been great.

dcharkes commented 9 months ago

I tried to dig up the heuristics for the cpp editor, but I couldn't find a spec.

dcharkes commented 9 months ago

Type hints are redundant if a constructor is called immediately:

image
DanTup commented 8 months ago

My concern here is that it might not always be obvious to a user why a hint is missing and that could be confusing. For example a static method might look the same as a constructor, but only one would get hints. A mix of parameters where some of the names match and some don't would also look inconsistent.

I feel like these hints are always going to be too noisy to reasonably have them enabled all the time (which is why we default to only showing them while the key is held down), and so if they're only shown temporarily then being consistent may be better than being terse.

That said - I also don't use these that often, so perhaps my opinion shouldn't hold much weight. Perhaps we should leave this issue to collect some opinions (or 👍 👎 votes) before deciding.

dcharkes commented 8 months ago

My concern here is that it might not always be obvious to a user why a hint is missing and that could be confusing.

It seems there's a trade-off between not being noisy and being consistent. My experience in the C++ editor where 'obvious' duplicate information is omitted, is not confusing (to me).

For example a static method might look the same as a constructor, but only one would get hints.

Maybe static methods that return the same type as the surrounding class should maybe also not get a hint. Maybe they should. Not sure what is the right answer here.

A mix of parameters where some of the names match and some don't would also look inconsistent.

This doesn't seem to bother me in the C++ editor.

I feel like these hints are always going to be too noisy to reasonably have them enabled all the time (which is why we default to only showing them while the key is held down), and so if they're only shown temporarily then being consistent may be better than being terse.

I feel like aiming for them not being so noisy that they can always be on is a better design goal. But that is my subjective opinion.

Would it technically be possible to make what inlay hints are available configurable?

clangd makes them configurable in the .clangd file:

InlayHints:
  Enabled: Yes
  ParameterNames: Yes
  DeducedTypes: No

We could do something similar in the DartCode extension settings in vscode possibly?

        "[dart]": {
            // 'off' - never show
            // 'on' - always show
            // 'offUnlessPressed' - show only while shortcut keys are held down (default)
            // 'onUnlessPressed' - show, but hide while shortcut keys are held down
            "editor.inlayHints.enabled": "on",
            "editor.inlayHints.parameterNames": "on", // or "off", or "terse"
        },

That said - I also don't use these that often, so perhaps my opinion shouldn't hold much weight. Perhaps we should leave this issue to collect some opinions (or 👍 👎 votes) before deciding.

I guess I'm going to have to upvote my own issue? 😆

If we want to get some insight on this, we could request a user study. @mit-mit Who is the right person to ask about something like this? (For a user study it might be nice to already have the configurable options already.)

DanTup commented 8 months ago

I guess I'm going to have to upvote my own issue? 😆

heh, I often upvote my own in the VS Code tracker - it makes it less clicks for others to add theirs 😄

We could certainly add settings to control this (although we can't put them in editor.inlayHints, they'd have to be dart.*), although it would ofcourse be simpler if we just had one set of rules.

Perhaps it would be useful to collect a specific list of suggestions so it's clearer how it might look? I just looked at a few files with them one one thing that struck me is how different things are depending on whether you already use explicitly types (like Flutter). If you already write things like:

String s = getString();

Which is common in the Flutter rep, then things are a lot less noisy than if you have:

var s = getString();

Because with the latter, you end up with var String (which is more verbose than writing it verbosely). It's possible that having it always on would be much more reasonable if you'r ein the former camp than the latter (I'm in the latter :) ).