[PATCH] D129579: [clangd] Remove `allCommitCharacters`

Wed Jul 13 05:51:12 PDT 2022

sammccall added a comment.

In D129579#3647834 <https://reviews.llvm.org/D129579#3647834>, @kadircet wrote:

> sorry for being late to the party, but reading the spec <https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#:~:text=accept%20it%20first%20and%20then%20type%20that%20character> it sounds like vscode behaviour is actually the correct one (despite not being the optimal UX, at least to our understanding).

Agree it's the specified behavior, does the comment suggest otherwise? (I did remember the spec being more ambiguous than that, but it's clear).

> so at least for the intermediate state (and possibly ever, if LSP folks don't change the wording or add a capability) we might consider still advertising operators as commit characters (e.g. `.`, `-`, `,` ...)

In VSCode this also yields (IMO) poor UX, because VSCode always considers the top completion to be selected and ready to be committed (vs considering nothing initially selected).
See reports to us in https://github.com/clangd/vscode-clangd/issues/356 as well as 357, and https://github.com/microsoft/vscode/issues/139825 and the various dupes it links to: basically this seems like bad behavior that the VSCode devs have dug their heels in on and makes commitchars unhelpful.

This problem is VSCode-specific so in principle we could allow `.` for other editors, but that's hard to detect (and IMO complexity vs benefit here is too high).

> but i think there's an even bigger problem in vscode at the moment around combined effect of this and snippets

I think as you described above, *that* interaction (where `(` inserts a second bracket inside the snippet's brackets) isn't vscode-specific but actually specified.

> maybe we should disable commit characters on items with snippets and have the rest use the "safe" set of defaults. WDYT?

Having behavior dynamically depend on whether a result has to be a snippet is useless because the user('s muscle memory) doesn't know whether it's a snippet.

We could have different value for editors that don't support snippets at all, but this introduces extra configurations to reason about - supporting non-overlapping sets of features is hard to explain!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129579/new/

https://reviews.llvm.org/D129579