atom-ide-ui icon indicating copy to clipboard operation
atom-ide-ui copied to clipboard

Label of active parameter is missing in signature help

Open Gert-dev opened this issue 8 years ago • 4 comments

Description

It seems the label in signature help parameter information objects is not displayed in the signature help pop-up, thus providing the user no information as to what parameter he is currently specifying the argument to the call for. Only the documentation is listed.

I realize signature help is experimental, but as I'm integrating with it early I thought I might offer some feedback in order to help it stabilize.

Expected Behavior

Both the label and the documentation of the active parameter are displayed and are updated as the user moves between the arguments of the call.

Actual Behavior

Only the documentation of the parameter is displayed.

Versions

  • Atom: 1.23.3
  • Client OS: Linux 64-bit
  • atom-ide-ui: 0.7.2

Gert-dev avatar Jan 25 '18 20:01 Gert-dev

It's subtle, but the label for the active parameter is bolded (but only if the label is included in the overall signature label). it's not the most obvious behavior but it matches what VScode also does - but maybe we can do something else if it doesn't match :)

On Jan 25, 2018 12:10 PM, "Gert-dev" [email protected] wrote:

Description

It seems the label in signature help parameter information objects https://microsoft.github.io/language-server-protocol/specification#signature-help-request-leftwards_arrow_with_hook is not displayed in the signature help pop-up, thus providing the user no information as to what parameter he is currently specifying the argument to the call for. Only the documentation is listed.

I realize signature help is experimental, but as I'm integrating with it early I thought I might offer some feedback in order to help it stabilize. Expected Behavior

Both the label and the documentation of the active parameter are displayed and are updated as the user moves between the arguments of the call. Actual Behavior

Only the documentation of the parameter is displayed. Versions

  • Atom: 1.23.3
  • Client OS: Linux 64-bit
  • atom-ide-ui: 0.7.2

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/facebook-atom/atom-ide-ui/issues/155, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjPYno-6GG_nCd4lr5mN549fM0Hmc_4ks5tON-_gaJpZM4Rta0h .

hansonw avatar Jan 25 '18 20:01 hansonw

Oh, that explains why I'm running in to it; it wasn't forgotten, I'm just hitting an edge case :-).

If I understood correctly, this works if the label for each ParameterInformation is included in the label of the SignatureInformation?

It seems possible to do this in my server, though I'm wondering a bit if it is the right thing to do. The specification doesn't really say either way and leaves the server to decide what to send in the labels. Perhaps the easiest would be to keep the current behavior intact, but if the label of the parameter is not found in the overall signature label, it is then shown separately?

Gert-dev avatar Jan 26 '18 19:01 Gert-dev

It makes sense to me. The logic is here: https://github.com/facebook-atom/atom-ide-ui/blob/master/modules/atom-ide-ui/pkg/atom-ide-signature-help/lib/getSignatureDatatip.js#L48

If you want to try it out and submit a pull request I'd be happy to take a look :)

hansonw avatar Jan 26 '18 23:01 hansonw

I tinkered with it a bit and came up with this, which replaces the if-statement you linked:

// Find the label inside the signature label, and bolden it.
if (activeParameter.label !== '') {
  const idx = markedStrings[0].value.indexOf(activeParameter.label);
  if (idx === -1) {
    markedStrings[0].value += '(' + activeSignature.parameters.map((parameter) => parameter.label).join(', ') + ')';
  }
  const newIndex = markedStrings[0].value.indexOf(activeParameter.label);
  if (newIndex !== -1) {
    markedStrings[0].value =
      markedStrings[0].value.substr(0, newIndex) +
      '**' + activeParameter.label + '**' +
      markedStrings[0].value.substr(newIndex + activeParameter.label.length);
  }
}

Basically this checks if the parameter is in the label to highlight it, as before. But if it isn't, it assumes that the signature label doesn't contain the parameter labels itself and then proceeds to add them first, finally repeating the same searching.

This makes both approaches - including the parameter labels in the signature label or not - have the same behavior. A possible issue is that display of the parameter list here is using parantheses and commas, which may not be consistent with languages that don't have those and that also ship their parameter labels in the signature label itself.

On another note, I've given this some more thought, and I came to the conclusion that the approach of including the parameter label inside the signature label server-side is not undesirable, even though it is already included in the parameter labels as well, for the reason that it should probably identify the "entire signature". However, I do bellieve the signature label probably wasn't meant to be scanned for the parameter label for the following reasons:

  • It may not work in all cases if the parameter label is for some reason different than the parameter label inside the signature label returned by the server, i.e. the parameter label may just contain the parameter name, the signature label its types as well, and the parameter label may occur multiple times as substring in the signature label, breaking the scanning - using word boundaries here is dangerous as the UI has no notion of what the language identified by the server identifies as boundaries in the signature or not
  • There is an activeParameter property to explicitly identify the active parameter
  • The LSP states Will be shown in the UI. for both the signature label and the parameter label separately; that seems to imply both should be shown separately, but that's debatable I guess

The parameter label could be displayed separately instead of bolded, i.e. above or next to the description, but I think just prepending it to the current description section won't look that well visually - but, I'm in no way an expert on design. What do you think?

Gert-dev avatar Jan 27 '18 11:01 Gert-dev