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

CompletionItem missing InsertTextMode #2577

Closed
mickaelistria opened this issue Apr 5, 2023 · 12 comments · Fixed by #2613
Closed

CompletionItem missing InsertTextMode #2577

mickaelistria opened this issue Apr 5, 2023 · 12 comments · Fixed by #2613

Comments

@mickaelistria
Copy link
Contributor

Since 3.16, InsertTextMode is supposed to be added to completion item in order to hint the client about whether to adjust indentation or not. This information is not provided by JDT-LS which makes client use the default behavior (which was not clearly specified before 3.16, so clients may handle it different)

@rgrunber
Copy link
Contributor

rgrunber commented Apr 5, 2023

We can support insertTextMode in the completion item, or globally with https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionClientCapabilities (there's a capability to declare support and another field to declare what the option is). There's even a 3rd option, which is to set a default at the completion list.

I'm assuming most clients would want adjustIndentation ? Is there any case for asIs ?

@mickaelistria
Copy link
Contributor Author

I'm assuming most clients would want adjustIndentation ? Is there any case for asIs ?

For LSP4E, AdjustIndentation would be fine, it's jut the case of "unset" that is not guaranteed.

@rgrunber
Copy link
Contributor

Maybe something @JessicaJHee can look at in the future.

@JessicaJHee
Copy link
Contributor

I'm interested in working on this!

@rgrunber
Copy link
Contributor

rgrunber commented May 16, 2023

As an example, for VS Code, I see :

client completion item capabilities sent in initialize

"insertTextModeSupport": {
  "valueSet": [
    1,
    2
  ]
},

client completion capabilities sent in initialize

"insertTextMode": 2

If the client sets these in initialize, then the LS won't add the entry for (AdjustIndentation) in the completion list (or item). If the client doesn't, but does support indentation it will. We only really prefer that the client adjust the indentation, so if that's not supported, the client is free to do as it wishes. We can revise when we think of particular completion items that we might prefer have no indentation at all.

@mickaelistria
Copy link
Contributor Author

If the client sets these in initialize, then the LS won't add the entry for (AdjustIndentation) in the completion list (or item)

Isn't it a bug? The specification doesn't mention that a handshake of capabilities is enough to determine the strategythat client should use, which can even be different from one completion list/item to another. The expectation is really that the intsertTextMode is made explicit on every response.

@mickaelistria
Copy link
Contributor Author

The fix doesn't work with LSP4E. The insertTextMode is not set on the completionList nor the completionItem.
Can you please reopen?

@rgrunber
Copy link
Contributor

There's no need to make insertTextMode explicit on every completin list response (or individual completion item response) if the client is declaring AdjustIndentation as the default in the capabilities for the initialize request. This is because the LS already prefers AdjustIndentation for the items.

If I look at https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#completionClientCapabilities , it states :

/**
	 * The client's default when the completion item doesn't provide a
	 * `insertTextMode` property.
	 *
	 * @since 3.17.0
	 */
	insertTextMode?: [InsertTextMode](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#insertTextMode);

So if the client declares this, there's no need to also set it for completion items.

@rgrunber rgrunber reopened this May 16, 2023
@mickaelistria
Copy link
Contributor Author

LSP4E doesn't deine the insertTextMode completion capability, so it doesn't tell a default, so the LS shouldn't assume one.

{"jsonrpc":"2.0","id":"1","method":"initialize","params":{ ...., "4completion":{"completionItem":{"snippetSupport":true,"documentationFormat":["markdown","plaintext"],"resolveSupport":{"properties":["documentation","detail","additionalTextEdits"]},"insertTextModeSupport":{"valueSet":[1,2]}}},... }}

IMO, it's always better to be as explicit as possible, setting the insertTextMode on completion is much more universal, more backward-compatible and less error-prone.

@rgrunber
Copy link
Contributor

If lsp4e doesn't define the default insertTextMode in the capabilities sent at initialize, and probably no itemDefaults, then insertTextMode should be sent in every completion item. If it isn't then it's a bug. Maybe I can debug and figure out why it isn't being set.

@mickaelistria
Copy link
Contributor Author

My bad, I double checked and I do see that AdjustIndent is set on CompletionItems. So from LS perspective, I guess everything is correct.

@mickaelistria
Copy link
Contributor Author

and I confirm eclipseide-jdtls is happy with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants