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

Show auto-completion for user-defined methods when overriding in GDScript #5896

Open
poohcom1 opened this issue Dec 4, 2022 · 9 comments · May be fixed by godotengine/godot#69798
Open

Show auto-completion for user-defined methods when overriding in GDScript #5896

poohcom1 opened this issue Dec 4, 2022 · 9 comments · May be fixed by godotengine/godot#69798

Comments

@poohcom1
Copy link

poohcom1 commented Dec 4, 2022

Describe the project you are working on

A 2D action game with a lot of interface-like classes

Describe the problem or limitation you are having in your project

Overriding user-defined methods are cumbersome as there are no auto-completion.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

In the script editor, show auto-completion for user-defined methods when declaring/overriding functions with the func keyword.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

In parent class:
image

In child class:
image

Specifics:

  • Suggestions will include all methods in all user-defined super classes. Unless we have virtual methods (Add GDScript support for defining virtual/prototype/must-override methods in base classes #1631) or some form of access modifier, all methods are technically override-able, therefore it is logical to autocomplete all methods.
  • On selection, inserted method should behave like it does currently with virtual methods, i.e. include parameters (and type-hints if text_editor/completion/add_type_hints is on).

If this enhancement will not be used often, can it be worked around with a few lines of script?

This involves core GDScript.

Is there a reason why this should be core and not an add-on in the asset library?

This involves core GDScript.

@poohcom1
Copy link
Author

poohcom1 commented Dec 4, 2022

I have a working implementation of this, but it's blocked by a few issues:

@KoBeWi
Copy link
Member

KoBeWi commented Dec 4, 2022

Technically a duplicate of #393, but you have better mock-ups and even have an implementation, so I'll close my proposal.

IMO we shouldn't show all methods for override, but there should be a way to explicitly mark them as virtual. Either by _ convention or via some new annotation.

@poohcom1
Copy link
Author

poohcom1 commented Dec 4, 2022

IMO we shouldn't show all methods for override, but there should be a way to explicitly mark them as virtual. Either by _ convention or via some new annotation.

Personally, I think keywords/annotations can wait until we have actual virtual functions. Given how previous discussions weren't really going anywhere (and people couldn't really agree on the keyword), I wanted this proposal to just be a small QoL improvement that can be easily added to Godot. Because of that, showing all methods seem to be the most 'natural' behavior.

As for _, I'm kinda on the fence. I feel like it isn't a good idea to lock features behind naming conventions unless it has an actual effect on the interpreter (like the Python double underscore). If the concern is that suggestions will get filled with unnecessary methods, I think users can come up with their own convention, and rely on the filtering to get the method they need. That said, I'm still okay with _ since it does follow the existing conventions and wouldn't be so hard to discover. Let me know what you think.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 4, 2022

Actually, maybe showing everything wouldn't be that bad. Built-in virtual methods always start with _, so we could just sort the list in a way that makes them always listed on top. Users could also take advantage of this and use the _ convention to have their "virtual" methods easily available.

To me the most important part is that autocompleting a method automatically adds all necessary arguments. I use overriding a lot and it's a pain, because I always have to lookup the declarations and copy them. I wouldn't really mind the method list suddenly getting long.

btw if you have implementation ready, you can open a PR even if it depends on something else.

@poohcom1
Copy link
Author

poohcom1 commented Dec 5, 2022

@KoBeWi I've been cleaning up the implementation a bit so I can raise a PR, but one main issue I'm stuck on is with default parameters. Since GDScript is allows any expression as default parameters, there are some edge cases where you can't really reduce the expression (like function calls). I've been using a modified gdscript_editor::_make_arguments_hint to generate the arguments and types, but it prints <unknown> for default parameters if the value couldn't be reduced.

For Example:
In parent:
image
In child:
image

This also applies for enums and arrays/dictionaries (possibly a bug).

Because of this, I thought it might just be better to just copy over the exact default value expression? Given that child classes should share all members of their parents, expressions that are correct in parents should in theory also be correct in child classes.

Any thought?

@KoBeWi
Copy link
Member

KoBeWi commented Dec 5, 2022

Yeah there is no reason to reduce them.

@OrganicPepper
Copy link

This would be super lovely to have, also perhaps options in GDScript project settings to warn / error when overriding a parent function incorrectly.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 29, 2023

warn / error when overriding a parent function incorrectly.

This already exists.

@OrganicPepper
Copy link

Ah, so it does, my mistake! It wasn't working earlier, but I had a compilation error elsewhere which was preventing the overriding error from showing!

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

Successfully merging a pull request may close this issue.

3 participants