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

Fix gdscript language server auto completion compatibility with external text editors. #49391

Closed
wants to merge 1 commit into from

Conversation

igordreher
Copy link

@igordreher igordreher commented Jun 7, 2021

The gdscript language server changes the content of insertText during resolve.
But, by the lsp specs, that should not happen, as pointed out godotengine/godot-vscode-plugin#142 (comment) (his emphasis):

The request can only delay the computation of the detail and documentation properties. Other properties like sortText, filterText, insertText, textEdit and additionalTextEdits must be provided in the textDocument/completion response and must not be changed during resolve.

Because of this, bugs in external text editors regarding auto completion happen.
By removing the changes to insertText in the language server, this commit ensures better compatibility with external editors.

This commit removes the parenthesis addition to functions in auto complete. This feature should exist in the text editor and not in the language server.

Bugsquad edit:

The gdscript language server changes the content of insertText during resolve.
But, by the lsp specs, that should not happen. Because of this, bugs in external text editors regarding autocompletion happen.
By not following the lsp specs, this commit ensures better compatibility with external editors.

Fix for #48436
@igordreher igordreher requested a review from a team as a code owner June 7, 2021 16:07
@AaronRecord
Copy link
Contributor

Does this happen to fix #48116, or is that a different issue?

@igordreher
Copy link
Author

Does this happen to fix #48116, or is that a different issue?

I'm not sure, but I think that is a different issue.

@igordreher igordreher changed the title Fix gdscript language server auto completion Fix gdscript language server auto completion compatibility with external text editors. Jun 7, 2021
@YuriSizov YuriSizov added this to the 4.0 milestone Jul 18, 2021
@akien-mga
Copy link
Member

CC @Razoric480

@akien-mga akien-mga added cherrypick:3.4 cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 25, 2022
@kurtlachmann
Copy link
Contributor

I disagree with the statement that this feature should be moved into the language client.
Adding parentheses to function calls is something that should definitely be the responsibility of the language server.
That is the whole point of a language server. The server should be the only implementation for autocompletion (towards which I'd count adding function parentheses since that is also the default behavior in the internal Godot editor).
A language client should only be responsible for inserting the text which it receives from the language server into its respective editor. If we say that this feature shouldn't be part of the language server then each client would be required to implement the logic independently which is exactly the problem the LSP was supposed to solve in the first place.

Also @igordreher said that "The gdscript language server changes the content of insertText during resolve." That's not really true. Or at least that's not the cause of the bugs. The insertText field is initialized in resolve. We could just as well move that block into the completion method which would make us compliant to the LSP specs. However that would not fix the bugs.

The problem is not that we're "changing the content of insertText". The problem is that the autocompletion in the language server is not behaving as expected. It's true that deleting these lines would fix the mentioned bugs. But I think in the long run they should instead be replaced by a better implementation.

@kurtlachmann
Copy link
Contributor

I believe I found a better solution. See #59482

Note that neither this nor my own pull request would fix #48436 and godotengine/godot-vscode-plugin#142
Fixing the unwanted quotes is a more complex issue.

@igordreher
Copy link
Author

If we say that this feature shouldn't be part of the language server then each client would be required to implement the logic independently which is exactly the problem the LSP was supposed to solve in the first place.

Good point.

As @kurtlachmann pointed out #49391 (comment),
this PR would only be a work around and not the correct approach.
And so, I'm closing this PR in favor of #59482

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