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

Support allCommitCharacters even if the client announces commit characters per item. #673

Closed
DanTup opened this issue Sep 22, 2020 · 5 comments
Milestone

Comments

@DanTup
Copy link
Contributor

DanTup commented Sep 22, 2020

'm using the client library here for LSP support in a VS Code extension. My server is sending a dynamic registration that includes commit characters like this:

{
	"id": "3",
	"method": "textDocument/completion",
	"registerOptions": {
		"documentSelector": [
			{
				"language": "dart",
				"scheme": "file"
			}
		],
		"triggerCharacters": [
			".",
			"=",
			"(",
			"$"
		],
		"allCommitCharacters": [
			".",
			"("
		],
		"resolveProvider": true
	}
},

However pressing the commit characters doesn't seem to have any effect. It's enabled in VS Code and it works as expected for TypeScript. Is there anything else I need to do?

@dbaeumer
Copy link
Member

This can only be used for clients that don't support individual commit character on individual completion items.

See https://github.com/microsoft/vscode-languageserver-node/blob/master/protocol/src/common/protocol.ts#L1664

This field can be used if clients don't support individual commmit characters per completion item. See ClientCapabilities.textDocument.completion.completionItem.commitCharactersSupport

Since VS Code supports individual commmit characters per completion item it will not honor this during registration.

@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label Sep 23, 2020
@dbaeumer
Copy link
Member

Is this OK for you?

@DanTup
Copy link
Contributor Author

DanTup commented Sep 23, 2020

@dbaeumer I think the wording on the spec should be clearer if it will only be used by clients that don't support it. Right now it seems like clients can use both.

But - is there any reason this can't be supported? The payload size for completion is already quite a real issue and duplicating the same array on every single item is needlessly wasteful. It would be much nicer if this could be supported (either here on the registration, or on a single property for the whole completion list) IMO.

@dbaeumer
Copy link
Member

Technically there is nothing that prevents us from implementing it. It is simply not done.

@dbaeumer dbaeumer added feature request and removed info-needed Issue requires more information from poster labels Sep 24, 2020
@dbaeumer dbaeumer added this to the Backlog milestone Sep 24, 2020
@dbaeumer dbaeumer changed the title Commit characters don't seem to work Support allCommitCharacters even if the client announces commit characters per item. Sep 24, 2020
@DanTup
Copy link
Contributor Author

DanTup commented Oct 21, 2020

btw, in the spec it has this:

	 * If a server provides both `allCommitCharacters` and commit characters on an individual
	 * completion item the ones on the completion item win.

This reads like it's reasonable to provide both, and that the client would fall back to allCommitCharacters if it's not set on the completion (otherwise it wouldn't really make sense to send both).

@dbaeumer dbaeumer modified the milestones: Backlog, 3.17 Oct 22, 2021
sam-mccall added a commit to sam-mccall/vscode-clangd that referenced this issue Jul 12, 2022
The use of commit-characters in clangd had good intentions, but it has
terrible UX in practice in VSCode, as documented on the linked bug.

We didn't notice this until it was far too late: the commit characters
were added in clangd 12, but bugs in vscode-languageclient prevented
them from being noticed until languageclient-8.
(microsoft/vscode-languageserver-node#673)

At this point we can't go back and change the server behavior so we need
to override it on the client.

Fixes clangd#357
sam-mccall added a commit to llvm/llvm-project that referenced this issue Jul 12, 2022
This was added in 2a095ff, however it never worked with VSCode
due to bugs in vscode-languageclient
(microsoft/vscode-languageserver-node#673).
Now that it does work, we can tell the interactions with text editing, with
snippets, and vscode's select-first-completion behavior are bad.

The spec is vague and clients could do something reasonable with the
current values. However they could clearly do something unreasonable
too, and over time behavior+spec tends to converge on VSCode's behavior.

This addresses clangd/vscode-clangd#358
See also clangd/vscode-clangd#358 which hotfixes
this on the client-side (as we can't apply this change retroactively to
clangd 12-14).

Differential Revision: https://reviews.llvm.org/D129579
sam-mccall added a commit to clangd/vscode-clangd that referenced this issue Jul 12, 2022
The use of commit-characters in clangd had good intentions, but it has
terrible UX in practice in VSCode, as documented on the linked bug.

We didn't notice this until it was far too late: the commit characters
were added in clangd 12, but bugs in vscode-languageclient prevented
them from being noticed until languageclient-8.
(microsoft/vscode-languageserver-node#673)

At this point we can't go back and change the server behavior so we need
to override it on the client.

Fixes #357
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Jul 14, 2022
… item when enabled

Setting commit characters on the registration is now enough to apply to all items (see microsoft/vscode-languageserver-node#673).

Change-Id: I1133005773e63f563d4e303af60f6c5bb56ec826
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/251546
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
This was added in 2a095ff6f5028b76, however it never worked with VSCode
due to bugs in vscode-languageclient
(microsoft/vscode-languageserver-node#673).
Now that it does work, we can tell the interactions with text editing, with
snippets, and vscode's select-first-completion behavior are bad.

The spec is vague and clients could do something reasonable with the
current values. However they could clearly do something unreasonable
too, and over time behavior+spec tends to converge on VSCode's behavior.

This addresses clangd/vscode-clangd#358
See also clangd/vscode-clangd#358 which hotfixes
this on the client-side (as we can't apply this change retroactively to
clangd 12-14).

Differential Revision: https://reviews.llvm.org/D129579
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants