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 enum autocompletion for core classes #89382

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

dcaoc03
Copy link
Contributor

@dcaoc03 dcaoc03 commented Mar 11, 2024

While writing a gdscript, autocompletion wouldn't suggest Enum names for core classes and also wouldn't suggest Enum Values even after the Enum name had been typed. This has now been fixed.

Now, while finding identifiers in a class, enumeration names are also fetched and enumeration constants are now suggested as well.

Fixes #88858.

@dcaoc03 dcaoc03 requested a review from a team as a code owner March 11, 2024 08:17
@dalexeev dalexeev added this to the 4.3 milestone Mar 11, 2024
@akien-mga akien-mga changed the title Fixed enum autocompletion for core classes (Issue #88858) Fix enum autocompletion for core classes Mar 11, 2024
@akien-mga
Copy link
Member

CC @HolonProduction

Copy link
Member

@HolonProduction HolonProduction left a comment

Choose a reason for hiding this comment

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

I can confirm, that the changes solve the issue 👍

Still the location should get set correctly as well. Otherwise we might run into sorting problems.

Besides that it would be nice to have tests to ensure this working. Would be nice if you could add them. (The whole testing system for autocompletion, is quite new and might have some rough edges. So if you don't know how, feel free to ask, or just don't do it and I'll put the issue on my todo list.)

modules/gdscript/gdscript_editor.cpp Outdated Show resolved Hide resolved
modules/gdscript/gdscript_editor.cpp Outdated Show resolved Hide resolved
@dcaoc03
Copy link
Contributor Author

dcaoc03 commented Mar 12, 2024

Sure thing! I'll be sure to add the locations and try to do some tests.

@dcaoc03
Copy link
Contributor Author

dcaoc03 commented Mar 12, 2024

@HolonProduction just to be sure I understood the autocompletion test syntax. A very simple test could look like this:

enum_values_autocompletion.gd

extends Control

func _ready():
    AStarGrid2D.Heuristic.➡

enum_values_autocompletion.cfg

[output]
include=[
    {"display": "HEURISTIC_MAX"},
    {"display": "HEURISTIC_OCTILE"},
]

Please feel free to correct me, I'll be sure to fix anything before committing any new changes.

@HolonProduction
Copy link
Member

That looks about right. I'd maybe add builtin to the file name, to separate it from GDScript enums which take a different code path.

Also nitpick but in this case it might make sense to also test the location.
For context a situation which might currently fail (without you adding location information):

  • You do completion on a type which inherits from control
  • The enum names from Control should appear after the stuff from the type which you are completing on, but currently it would appear among them

So see that the location is LOCATION_OTHER when completing on Control. And test that it is LOCATION_OTHER + 1 when completing on a type directly inheriting from Control.

Also I am currently unsure why we have to fallthrough in the ENUM case. It's totally right that you do it, since it aligns with what we did before. I'm just curious why we are doing it.

@dcaoc03
Copy link
Contributor Author

dcaoc03 commented Mar 14, 2024

@HolonProduction sorry to bother again, but I'm not sure on how I should be testing the value of location. In my test, I'm autocompleting a node of type BaseButton (which inherits from Control) and my .cfg looks like this:

[output]
include=[
    {"display": "DrawMode",
     "location": },
    {"display": "Anchor",
     "location": },
]

In this case, the location of DrawMode should be LOCATION_OTHER and the location of Achor should be LOCATION_OTHER + 1, but it seems that I can't directly define "location" as LOCATION_OTHER in the .cfg.

@HolonProduction
Copy link
Member

HolonProduction commented Mar 14, 2024

In this case, the location of DrawMode should be LOCATION_OTHER and the location of Achor should be LOCATION_OTHER + 1, but it seems that I can't directly define "location" as LOCATION_OTHER in the .cfg.

From the code LOCATION_OTHER is 1 << 10 or 1024.

Edit: It isn't LOCATION_OTHER but LOCATION_PARENT_MASK, but you seem to have figured that out yourself 👍

Minor fix consisted in the use of [[fallthrough]] macro
Copy link
Member

@HolonProduction HolonProduction left a comment

Choose a reason for hiding this comment

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

Gave it another test and seems to work well with the sorting. Code makes sense as well and the tests seem to pass. LGTM

Still needs approval from the GDScript Core team though

Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

LGTM

@akien-mga akien-mga merged commit 9a9045c into godotengine:master Apr 9, 2024
16 checks passed
@akien-mga
Copy link
Member

akien-mga commented Apr 9, 2024

Thanks! And congrats for your first merged Godot contribution 🎉

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.

Autocompletion does not suggest enum names for core classes
6 participants