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.

Sign up for GitHub

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

Jump to bottom

Sort code autocompletion with rules #75746

Merged
merged 1 commit into from Jun 8, 2023
Merged

Sort code autocompletion with rules #75746

merged 1 commit into from Jun 8, 2023

Conversation

ajreckof
Copy link
Contributor

@ajreckof ajreckof commented Apr 6, 2023 •
edited by akien-mga

supersedes #65655

This PR sort results based on the rules following :

  • if we are completing an empty string it is sort alphabetically (might be a good idea to modify that)
  • it ranks higher, in this order of priority, matches :
    1. with lesser fragment
    2. starting from the start of the string to complete
    3. closer in terms of scope
    4. with lesser mismatched case
    5. starting the closest from the start
  • if it fails it will try sorting them based on all the position of matches ( going through all positions the first match matching at this position while the other doesn't will be ranked higher)
    -if it still fails they are sorted alphabetically

The sort on option will first get the best match for each option based on the rules above. After that it will rank every option based on the best match found.

If you have found a case that is wrongly sorted I would gladly improve the rules. If in addition you have an idea on how to correct the rules for your use case feel free to propose a new rule.

Bugsquad edit:

@ajreckof ajreckof requested review from a team as code owners April 6, 2023 11:39
@Mickeon
Copy link
Contributor

Mickeon commented Apr 6, 2023

Thank you for crediting and the inspiration from the very original PR's algorithm!

@Chaosus Chaosus added this to the 4.1 milestone Apr 6, 2023
adamscott
adamscott reviewed Apr 14, 2023
View reviewed changes
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.

The GDScript team reviewed the PR. The idea is nice, @vnen will review the code later. Maybe myself too.

Will be watching this PR closely. We'll ask the production team if it's worth it to merge it sooner rather than later in order to have feedback.

Please squash your commits in order to be able to merge it later.

Thank you!

@ajreckof ajreckof force-pushed the order_autocomplete branch 3 times, most recently from c33cc4c to c89f84b Compare April 14, 2023 20:31
@YuriSizov
Copy link
Contributor

We'll ask the production team if it's worth it to merge it sooner rather than later in order to have feedback.

There is no hurry per se, but given the current state of the feature it's something we should fix for 4.1. I think we should still give it reasonable testing before merging, so we are not only dependent on arbitrary and sporadic user feedback.

We should consider adding tests for this, and then if we get more feedback for cases we didn't consider, then we add more tests. Testing this can be tricky. I think we might want to expose some more internal methods to get a list of ranked suggestions and their values for a certain context and input.

@ajreckof
Copy link
Contributor Author

I'll look into it. For the moment I'll populate with all examples that were given on Mickeon's PR.

@vnen
Copy link
Member

vnen commented Apr 20, 2023

We should consider adding tests for this, and then if we get more feedback for cases we didn't consider, then we add more tests. Testing this can be tricky. I think we might want to expose some more internal methods to get a list of ranked suggestions and their values for a certain context and input.

I have been thinking on how to test completion since I started the test runner, so far I could not find a solution. The main problem is that completion is a list of things in the engine, which may change at any point. I wouldn't want that a change in Node (like adding a new function) breaks the GDScript test just because the completions list changed.

So for now I wouldn't fret about testing completion for this PR in particular. If at some point we're able to figure out testing we can include this code as one thing to test. It would be nice if more people tested the PR before merging but I think it's unlikely to happen. I believe this one is already better than the current state (which isn't great) and we can tweak it later if needed.

@ajreckof
Copy link
Contributor Author

I do agree that it should not break just because someone added a new method somewhere. So that's why I was looking into giving him a set of options a string to complete and checking the order on this small set of options. It would only check the ordering and not really the autocompletion in itself. But as this PR is about ordering and not extracting options from environnement, it feels like that is what i should add test about here.

Paulb23
Paulb23 requested changes Apr 23, 2023
View reviewed changes
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

We should probably update the GDVIRTUAL_CALL(_filter_code_completion_candidates return and arg dictionarys to support the new property, to stop it from diverging. As it's a dictionary shouldn't break ABI compact.

doc/classes/CodeEdit.xml Outdated Show resolved Hide resolved
editor/plugins/script_text_editor.cpp Outdated Show resolved Hide resolved
@ajreckof ajreckof requested a review from a team as a code owner May 11, 2023 14:22
This was referenced May 11, 2023
Open
Closed
@ajreckof ajreckof force-pushed the order_autocomplete branch 2 times, most recently from 729f19a to 8b8ed5e Compare May 13, 2023 17:30
Paulb23
Paulb23 reviewed May 14, 2023
View reviewed changes
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the order_autocomplete branch 3 times, most recently from f4ca07d to 501268f Compare May 18, 2023 04:12
vnen
vnen reviewed May 18, 2023
View reviewed changes
core/object/script_language.h Outdated Show resolved Hide resolved
@ajreckof ajreckof force-pushed the order_autocomplete branch 2 times, most recently from ba6b54a to 5b8d053 Compare May 21, 2023 05:16
Paulb23
Paulb23 approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Can't speak to the new sorting.

But the CodeEdit and ScriptTextEditor changes look good to me.

@akien-mga akien-mga changed the title sort code autocompletion with rules Sort code autocompletion with rules May 22, 2023
akien-mga
akien-mga reviewed May 22, 2023
View reviewed changes
doc/classes/ScriptLanguageExtension.xml Outdated
Comment on lines 361 to 370
The option is local to the location of the code completion query - e.g. a local variable.
</constant>
<constant name="LOCATION_PARENT_MASK" value="256" enum="CodeCompletionLocation">
The option is from the containing class or a parent class, relative to the location of the code completion query. Perform a bitwise OR with the class depth (e.g. 0 for the local class, 1 for the parent, 2 for the grandparent, etc) to store the depth of an option in the class or a parent class.
</constant>
<constant name="LOCATION_OTHER_USER_CODE" value="512" enum="CodeCompletionLocation">
The option is from user code which is not local and not in a derived class (e.g. Autoload Singletons).
</constant>
<constant name="LOCATION_OTHER" value="1024" enum="CodeCompletionLocation">
The option is from other engine code, not covered by the other enum constants - e.g. built-in classes.
Copy link
Member

@akien-mga akien-mga May 22, 2023 •
edited

Choose a reason for hiding this comment

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

This were likely lost by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed and put in code_edit.xml it felt better as this was what was done for the other shared enum

Copy link
Member

Choose a reason for hiding this comment

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

Well this class still needs documentation too, so we shouldn't remove docs. It's not uncommon to have duplicate descriptions for duplicated APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought this was intended as this class does not have any documentation. Then should I also duplicate the doc for the other enum that is shared?

akien-mga
akien-mga reviewed May 22, 2023
View reviewed changes
scene/gui/code_edit.h Outdated Show resolved Hide resolved
@ajreckof @Mickeon @EricEzaM
sort code completions with rules
006e899 
Fixups

Add levenshtein distance for comparisons, remove kind sort order, try to improve as many different use cases as possible

Trying again to improve code completion

Sort code autocompletion options by similarity based on input

To make it really brief, uses a combination `String.similiary`, the category system introduced in a previous PR, and some filtering to yield more predictable results, instead of scattering every completion option at seemingly random.

It also gives much higher priority to strings that contain the base in full, closer to the beginning or are perfect matches.

Also moves CodeCompletionOptionCompare to code_edit.cpp

Co-Authored-By: Micky <66727710+Mickeon@users.noreply.github.com>
Co-Authored-By: Eric M <41730826+EricEzaM@users.noreply.github.com>
@akien-mga akien-mga added the bug label Jun 8, 2023
@akien-mga akien-mga mentioned this pull request Jun 8, 2023
Closed
akien-mga
akien-mga approved these changes Jun 8, 2023
View reviewed changes
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.

Looks good to me. Let's get this tested more widely, but it should be a significant improvement compared to the Godot 4.0 autocompletion.

@akien-mga akien-mga merged commit 577ab3c into godotengine:master Jun 8, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga mentioned this pull request Jun 16, 2023
Closed
@rsubtil rsubtil mentioned this pull request Aug 8, 2023
Closed
@dalexeev dalexeev mentioned this pull request Aug 29, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vnen vnen vnen left review comments

@Paulb23 Paulb23 Paulb23 approved these changes

@akien-mga akien-mga akien-mga approved these changes

@adamscott adamscott Awaiting requested review from adamscott

Assignees
No one assigned
Labels
bug enhancement topic:editor topic:gdscript topic:gui
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.

Autocomplete matching should be case insensitive, otherwise case mismatch leads to confusing results
8 participants
@ajreckof @Mickeon @YuriSizov @vnen @akien-mga @adamscott @Paulb23 @Chaosus