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

LSP: Fix autocomplete quote handling #81833

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

0x4448
Copy link
Contributor

@0x4448 0x4448 commented Sep 17, 2023

Fixes #48436

Updates to language server:

  • Add missing context to CompletionParams so context can be used in the resolve function.
  • Omit quotes in auto-complete suggestion if triggered with single or double quote.

I'm new to Godot and C++, but after poking around the code base a bit, I was able to get it working by making two changes:

  1. CompletionParams has a new context member. It provided an implementation for load() to include that data, but not one for to_json(). So context was missing from the JSON and there's already code in GDScriptTextDocument::resolve() that depends on that. I added context to the JSON so it can be used in resolve().
  2. Now that resolve() has the context, the current bug can be fixed. If the completion was triggered with a single or double quote, then there's already a quote (or a pair of quotes) in the external editor. The auto-complete suggestion should be returned without quotes.

Both changes are in language server classes, so this change should not break the built-in editor. I did a quick test to confirm that still returns quotes.

Again, I'm new to Godot and C++, so if I'm overlooking something obvious, please let me know and I'll try to fix it.

@0x4448 0x4448 requested a review from a team as a code owner September 17, 2023 22:19
@Calinou Calinou added bug topic:gdscript topic:editor cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Sep 17, 2023
@Calinou Calinou added this to the 4.2 milestone Sep 17, 2023
@Ury18

This comment was marked as off-topic.

Copy link
Member

@adamscott adamscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems fine to me! I tested it and it works.

And thank you for this first contribution. It's clean and does the job. Good job!

Don't hesitate to join the developer chat, especially if you want to continue contributing to Godot! Happy to see you there!

@AThousandShips
Copy link
Member

Please squash your commits into one, see here

@0x4448 0x4448 force-pushed the fix-autocomplete-quotes branch 2 times, most recently from 27d3146 to 2db11c1 Compare September 21, 2023 13:48
@0x4448
Copy link
Contributor Author

0x4448 commented Sep 21, 2023

Please squash your commits into one, see here

I've squashed the revisions to resolve() into one commit. I can also squash the to_json() change, but since that also fixes other existing code, I thought that might be better in a separate commit.

@AThousandShips
Copy link
Member

No need for two different commits here, the changes aren't significant enough

Typing a single or double quote in an external editor triggers
auto-completion. The returned CompletionItem should not include
quotes since they're already in the editor.

CompletionParams was missing context in to_json() and this is
required to detect whether a quote was typed.
@0x4448 0x4448 force-pushed the fix-autocomplete-quotes branch from 2db11c1 to 7ea4247 Compare September 22, 2023 00:42
@akien-mga akien-mga merged commit 3e15c8f into godotengine:master Sep 25, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Andy608
Copy link

Andy608 commented Oct 1, 2023

Hello, I'm a noob when it comes to this kind of stuff, but I was wondering if this fix is going to be merged into Godot 3.5 LTS? Just started working with Godot and I've noticed this bug present in the current 3.5.3 LTS version. Thanks!

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Oct 2, 2023
@akien-mga akien-mga changed the title Fix autocomplete quotes LSP: Fix autocomplete quote handling Oct 3, 2023
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:editor topic:gdscript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Language server string autocomplete place undesired quotes
8 participants