Fix autocompletion leading to duplicated symbols and tokens

[machitgarha]

This should close #487.

[machitgarha]

To fix variables producing two dollars in autocompletion, we should change Serenata instead. atom-languageclient excludes trigger characters from being replaced (i.e. $ is not included in replacementPrefix in this case). So, we should send a TextEdit.newText from Serenata not including the dollar sign itself.

@Gert-dev, what's your opinion?

[Gert-dev]

tl;dr: IIRC the textEdit property and its range should take care of this so the client does not need to make assumptions. The old way of inserting using a special separator character list is only still present for clients not supporting that later revision of the protocol.

IIRC, though it's been a while, the Serenata server should already be sending an actual text edit with a range that includes the dollar sign when it needs to be replaced in the textEdit property, and the client should be adhering to that range.

From what I remember the list of special characters to treat differently during completion were the initial way for servers to say "insert this text at the current position", and clients needed to be smart and decide what parts of the word needed to be replaced when inserting the completion provided by the server. This resulted in problems with special characters such as dollar signs as in most languages these are not part of identifiers like PHP, and making a good single decision for all programming languages and their different servers isn't easy.

In a later revision of the protocol, they introduced the textEdit property on completions with a range to fix this, so the server can say exactly what characters need to be replaced.

[machitgarha]

So you think we should report an issue in atom-languageclient?

[machitgarha]

So this is it; this PR is ready. It contains a workaround for variables, until the upstream PR is merged.

[machitgarha]

It is not ready yet. The update to 1.16 of atom-languageclient caused a regression: when trying to use a class, and the auto-insertion of the use statements happens on top of the file, it places two duplicated ones, which is annoying and must be fixed.

[machitgarha]

The regression is fixed. We should be ready, finally. :)

[Gert-dev]

LGTM, thanks!

[Gert-dev]

@machitgarha I've also invited you as collaborator since I already accepted your request on GitLab. I'm still willing to review these changes and answer question as I'm doing now, but I haven't used Atom in a long time, so if you still do, you are probably in a better position to decide what works and what doesn't for Atom specifically 🙂 .

[machitgarha]

@Gert-dev, wow, thank you very much!

You mean, when I create PRs, I should ask you to review them before merging? Or you mean you continue to have a look on this project?

[Gert-dev]

@machitgarha I meant you don't (even) have to ask me for permission before merging, since I think you are in a better position to judge what works and what doesn't for Atom 🙂. However, if you are uncertain about something, or want to make an MR or get input regardless, I'll still happily do so 😄 .

[nelson6e65]

Thanks for this bugfix! 🥟