Avoid resetting the code completion popup excessively #81633
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
YuriSizov
reviewed
Sep 14, 2023
YuriSizov
reviewed
Sep 14, 2023
YuriSizov
reviewed
Sep 14, 2023
YuriSizov
changed the title
Yeah, that's fine. But when you do, please make sure to use a commit message that follows the project's style. The current title of this PR is a good option. |
matorin57
force-pushed
the
code-completion-popup
branch
from
0235362 to
2735aec
Compare
|
Squashed commits and addressed feedback |
Paulb23
requested changes
Sep 17, 2023
matorin57
force-pushed
the
code-completion-popup
branch
from
2735aec to
303d099
Compare
|
Addressed Feedback |
|
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #57027. The issue was that
_filter_code_completion_candidates_implalways resets the selected item. While up/down won't cause an update, there is a race condition where updating the list and then quickly changing selection will cause the selection to move and then reset. This can also happen even when the update doesn't actually change the list of options.The fix here was to have
_filter_code_completion_candidates_implcheck to see if the new options list is the same up to the currently selected item. Then we only reset the selection if the list differs before the selection. That way if the list only changes after the selection we won't reset. If the list changes before our selection, then we still want to reset as the selected item may no longer be meaningful.This is done by building a new list and then comparing. There is an increased memory footprint in that we must hold two lists while doing the update, but once we are done we discard the old list. There is also a time increase in that we need to perform the list comparison (copying over the new list should be the same as clearing the old list and building the new one like we did originally). Considering that this lists will typically be small the time and memory hit should be unnoticeable from a UX standpoint.
I also implemented it for the overridden version but I wasn't sure how to test that directly. But the tests for the non-overridden version should check the base logic. I am not 100% sure how this override system works so any notes and explanation would be appreciated.
I haven't squashed yet, but figured I was going to have to make some updates in the review anyway so will squash after the notes.