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

Unnecessary parameter info for commas and opening parenthesis within strings #1150

Closed
NoelAbrahams opened this issue Nov 13, 2014 · 29 comments
Closed
Assignees
Labels
Bug A bug in TypeScript Domain: Signature Help Information in editor tooltips when invoking a function call Fixed A PR has been merged for this issue Visual Studio Integration with Visual Studio VS Code Tracked There is a VS Code equivalent to this issue

Comments

@NoelAbrahams
Copy link

Hi,

VS: 2013 Update 4 RC
TS: 1.3

commarepro

Also occurs with ( and <

@johnnyreilly
Copy link

Complete aside @NoelAbrahams but what did you use to record and create the animated GIF? This could be useful to me!

Like the Throught for the Day BTW - I expect to hear you on Radio 4 soon. 😄

@NoelAbrahams
Copy link
Author

@johnnyreilly, I think all credit to @heroboy for introducing this. See #895

@johnnyreilly
Copy link

Oooh that's amazing! I'm off to download now....

@mhegazy mhegazy added the Bug A bug in TypeScript label Nov 13, 2014
@DanielRosenwasser
Copy link
Member

I've actually been discussing this with @JsonFreeman because typing those characters in tagged template strings also trigger this sort of behavior. I think the desired behavior can get a little complex, but it's doable.

@DanielRosenwasser
Copy link
Member

@CyrusNajmabadi and @mhegazy, is there any way that we could introduce the concept of"hard", "soft", and "continuation" triggers?

Specifically:

  • A "hard" trigger means that the user specifically requested signature help (Ctrl+Shift+Space).
  • A "soft" trigger means that we are hinting to the language service to provide signature help (These would be the keys for (, ,, ```, }).
  • A "continuation" trigger means that we were already showing signature help. A continuation trigger has higher priority than a "soft" trigger.

@JsonFreeman
Copy link
Contributor

We already have two kinds of triggers (called Trigger and ReTrigger). I think what you are asking for is for the signature help query to also provide the trigger reason. In that case, if the language service encounters a comma inside a string literal, it would only show signature help on a ReTrigger, not a Trigger.

@mhegazy mhegazy modified the milestones: TypeScript 1.5, TypeScript 1.4 Dec 1, 2014
@mhegazy mhegazy added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label May 5, 2015
@mhegazy
Copy link
Contributor

mhegazy commented May 5, 2015

Looking at this again, i do not think the complexity of any solution here matches the value. i would be open to new suggestions to get this done without having to significantly change the interface between the host and the language service.

@mhegazy mhegazy closed this as completed May 5, 2015
@DanielRosenwasser DanielRosenwasser removed the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Jul 20, 2015
@DanielRosenwasser
Copy link
Member

I don't think this should be marked as by design.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 20, 2015

Do you have a proposal?

@JsonFreeman
Copy link
Contributor

I think what should happen is the managed side should ask the script side if the comma is a trigger character. The script side can figure out that this is not a trigger character.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 6, 2015

closing again as by design. please reactivate with a proposal if there is one, and outline what is the work needed.

@mhegazy mhegazy closed this as completed Oct 6, 2015
@CyrusNajmabadi
Copy link
Contributor

Going back to this:

The entirety of the signature help MVC could be made asynchronous.

So looking at the managed side, this remains true. however. I'm a little wary of going down that path. The reason for this is that this will essentially mean that in the internal machinery of sighelp we'll always be kicking off this async chain of events on every keystroke (since we don't know if the character will be a trigger character or not).

That's a lot of churning that can happen that i'd like to avoid.

However, from looking at this, the solution seems much easier and would go like this:

  1. TS would still return "true" for IsTriggerChracter for the comma character.
  2. Roslyn then calls into TypeScript through "ISignatureHelpProvider.GetItemsAsync(Document, int, SignatureHelpTriggerInfo, CancellationToken);"
  3. That SignatureHelpTriggerInfo has a TriggerReason of "TypeCharCommand".

Because of this, you now know you were triggered because something was typed. In that case, you're now on the background, and you're async. You can just check then "was this trigger character really something we needed to trigger for". You'll send that over to the non-blocking syntactic thread, it will look at the tree, realize it's in a string, and say "no". You can then just return no items for GetItemsAsync.

This is much simpler, and fits into the model we have today.

@JsonFreeman
Copy link
Contributor

Oh, good idea! The TriggerReason would actually contain the information that was missing before.

@DanielRosenwasser
Copy link
Member

Yeah, I think that's what I originally meant by "soft"/"hard" trigger.

@JsonFreeman
Copy link
Contributor

I have an idea to make this simpler. Instead of tracking a TriggerReason, only trigger signature help if you are not in some nested comma-separated list. This includes object literals, arrays, parameter lists, etc. Even if the user explicitly requested signature help! I am basing this on the assumption that the majority of signature help requests come from typed commas.

@marcoms
Copy link

marcoms commented Feb 28, 2018

This isn't limited to strings for me, this also happens within object and array literals after the comma - it makes calling functions that accept objects instead of repeated arguments very tedious. See #22106.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 16, 2018

Hey guys what's the difference between a Trigger and a ReTrigger?

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jun 28, 2018

Here's my understanding:

  • User-requested trigger: User hit ctrl+shift+space, specifically asked for signature help. The language service should always try to find signature help.
  • Trigger: The editor was not previously showing signature help. The user hit ( or ,; the user presumably wants to get signature if available. The language service should respond if the trigger character appropriately corresponds to an argument list.
  • ReTrigger: The editor is currently showing signature help, and the user hit ). The editor is now checking whether there is any outer signature help available at the cursor. The language service should always try to find a signature to keep the experience consistent.

Does that seem right? @amcasey @mjbvz @armanio123

@DanielRosenwasser
Copy link
Member

The above seems to be validated. Here's what Roslyn calls them: https://github.com/dotnet/roslyn/blob/0472794308b0abbc6a9ab3bb30e0303410aae3df/src/Features/Core/Portable/SignatureHelp/SignatureHelpTriggerReason.cs

I'll add that from investigating, cursor movements cause retriggers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Signature Help Information in editor tooltips when invoking a function call Fixed A PR has been merged for this issue Visual Studio Integration with Visual Studio VS Code Tracked There is a VS Code equivalent to this issue
Projects
None yet
Development

No branches or pull requests