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 completion of parameters in function call #64717

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Aug 22, 2022

Fix #58561

Demo:

gds_bugfix

@Chaosus Chaosus requested a review from a team as a code owner August 22, 2022 09:04
@Chaosus Chaosus added this to the 4.0 milestone Aug 22, 2022
@Chaosus Chaosus changed the title Fix completion of parameters in GDScript's functions Fix completion of parameters in function call Aug 22, 2022
@Chaosus Chaosus force-pushed the gds_fix_param_completion branch from a8b9715 to ec51ebd Compare August 22, 2022 09:38
Comment on lines +2952 to +2943
if (argument->type == Node::IDENTIFIER && current.cursor_place == GDScriptTokenizer::CURSOR_BEGINNING) {
completion_context.type = COMPLETION_IDENTIFIER;
}
Copy link
Member

Choose a reason for hiding this comment

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

Just commenting what we discussed on chat:

I'm not really familiar with the GDScript parser and completion but this looks a bit like a hack here, as if it's not the right place to do this check. No other code in this function accesses completion_context directly as far as I can see.

This looks more like the main loop that calls various other functions to update the completion context so my feeling is that the fix should be done in one of those functions, but again I don't really know much about it, it's just a gut feeling.

It feels like this should maybe set ct (and prevent it from being overridden) instead of editing the created completion context?

Would be great if @vnen could confirm, but otherwise it might be fine to merge anyway and it can be refactored later.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is the argument type and cursor place should be checked, which happened after the creation of context in the make_completion_context function and parse_expression. Otherwise, the completion would be incorrect.

@Chaosus Chaosus force-pushed the gds_fix_param_completion branch from ec51ebd to ca91320 Compare September 30, 2022 11:51
@Chaosus Chaosus force-pushed the gds_fix_param_completion branch from ca91320 to 5d4853f Compare September 30, 2022 12:42
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Let's go with this fix for now and if it turns out that it could be done a better way, this can be revisit later.

@Chaosus Chaosus merged commit 6f75b0d into godotengine:master Sep 30, 2022
@Chaosus Chaosus deleted the gds_fix_param_completion branch September 30, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GDScript 2.0 Methods' argument autocomplete for variables doesn't popup
2 participants