search

Gert-dev / php-ide-serenata

Atom IDE package that integrates the Serenata server to provide PHP code assistance
https://serenata.gitlab.io/
Other
275 stars 22 forks source link

Fix autocompletion leading to duplicated symbols and tokens #527

Closed machitgarha closed 2 years ago

machitgarha commented 2 years ago

This should close #487.

machitgarha commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

machitgarha commented 2 years ago

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

machitgarha commented 2 years ago

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 commented 2 years ago

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

Gert-dev commented 2 years ago

LGTM, thanks!

Gert-dev commented 2 years ago

@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 :slightly_smiling_face: .

machitgarha commented 2 years ago

@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 commented 2 years ago

@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 :slightly_smiling_face:. However, if you are uncertain about something, or want to make an MR or get input regardless, I'll still happily do so :smile: .

nelson6e65 commented 2 years ago

Thanks for this bugfix! 🥟