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

Improve sorting of Code Completion options. #59633

Merged
merged 1 commit into from
Apr 3, 2022

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented Mar 28, 2022

Continuation of #58931.

Updated after rebasing on top of #59553 as the two had some compatibility issues (and that PR took precedence). Has now been updated to be compatible, and also incorporated the comments @reduz left on #58931. Most notably, the sorting logic is now in the editor code where it belongs.

Closes godotengine/godot-proposals#4189
Closes godotengine/godot-proposals#99

Please read above proposal for more info and some example of what code completion currently looks like.
After changes in this PR:
image

Code Overview:

  • Add location property to ScriptCodeCompletionOption. It is an int, but has a corresponding enum, and more importantly one of the enum values is a mask, LOCATION_PARENT_MASK = (1 << 8),. This allows you, for example, to sort closer ancestors differently from more distant ancestors. The class itself will be value 256, while the class it inherits will have its location at 257, and then its parent at 258, etc.
  • When creating options, pass a location. This can be easily calculated if the options are created in a loop, where the loop is looping over parent classes and getting the class info for them. const int location = classes_processed | ScriptCodeCompletionOption::LOCATION_PARENT_MASK;
  • When you have all the options, simply sort on location. The lowest values will be highest priority. If two locations are the same, sort alphanumerically. The order in which different ScriptCodeCompletionOption::Kind's are sorted has been added too.

@EricEzaM EricEzaM requested a review from reduz March 28, 2022 13:34
@EricEzaM EricEzaM requested review from a team as code owners March 28, 2022 13:34
@EricEzaM EricEzaM force-pushed the better-code-complete-update branch from 8b1b386 to 41c2699 Compare March 28, 2022 13:36
@EricEzaM EricEzaM added this to the 4.0 milestone Mar 28, 2022
Comment on lines +390 to +391
ERR_CONTINUE(!op.has("location"));
option.location = op["location"];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was not sure if here you would prefer location to be optional:

if (op.has("location)){
    option.location = op["location'];
}

So I just followed what was already there. Using ERR_CONTINUE makes it mandatory to provide.

Copy link
Member

Choose a reason for hiding this comment

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

I think its ok to enforce it

@novaplusplus
Copy link
Contributor

novaplusplus commented Mar 28, 2022

I'm assuming/praying that this PR will fix things like this...?
2022-03-28_15-56

Drives me crazy 😵‍💫

@EricEzaM
Copy link
Contributor Author

Yes, it should. That item will still be suggested but it will be much further down the list.

@EricEzaM
Copy link
Contributor Author

@novaplusplus Actually I just tested, doesnt look like it will help with that specific scenario (assigning a built-in type to a variable). Interestingly "float" does not come up as an option at all. That appears to be an issue which is outside of the scope of this PR though.

Comment on lines +390 to +391
ERR_CONTINUE(!op.has("location"));
option.location = op["location"];
Copy link
Member

Choose a reason for hiding this comment

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

I think its ok to enforce it

@@ -312,20 +312,29 @@ class ScriptLanguage : public Object {
};

struct CodeCompletionOption {
enum Location {
Copy link
Member

Choose a reason for hiding this comment

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

I would probably be put as an enum of ScriptLanguage and bind it in ScriptLanguageExtension like the other enums, so the extensions can have access to it. Otherwise you dont know what they are from it.

@EricEzaM EricEzaM force-pushed the better-code-complete-update branch from 41c2699 to f969a73 Compare March 31, 2022 12:18
@novaplusplus
Copy link
Contributor

@novaplusplus Actually I just tested, doesnt look like it will help with that specific scenario (assigning a built-in type to a variable). Interestingly "float" does not come up as an option at all. That appears to be an issue which is outside of the scope of this PR though.

That's a shame... maybe there's another issue or PR already made about this. It's quite frustrating - if I were to hit "enter" at the time of that screenshot, it would have automatically replaced "float" with "AudioEffectLowPassFilter", which is just beyond absurd...

@Calinou
Copy link
Member

Calinou commented Mar 31, 2022

That's a shame... maybe there's another issue or PR already made about this. It's quite frustrating - if I were to hit "enter" at the time of that screenshot, it would have automatically replaced "float" with "AudioEffectLowPassFilter", which is just beyond absurd...

Remember that you can change the completion accept shortcuts in master to only complete suggestions when pressing Tab, not Enter. This makes autocompletion significantly less intrusive in my experience. I'd like to make this the default in 4.0, but a lot of people are used to only using Enter for autocompletion nowadays (since this is the default in VS Code and other IDEs).

@EricEzaM EricEzaM force-pushed the better-code-complete-update branch from f969a73 to 4ab605d Compare April 1, 2022 10:39
@EricEzaM EricEzaM requested a review from a team as a code owner April 1, 2022 10:39
Done by ordering options by their location in the code - e.g. local, parent class, global, etc.
@akien-mga akien-mga requested a review from reduz April 1, 2022 11:34
@akien-mga akien-mga merged commit c630c20 into godotengine:master Apr 3, 2022
@akien-mga
Copy link
Member

Thanks!

@dmaz
Copy link
Contributor

dmaz commented Apr 6, 2022

there is a pr for the built-in types... was out of sync and should now be ready
#59594

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