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

autocomplete [nfc]: Give subclasses control of computeResults #896

Merged
merged 7 commits into from
Aug 20, 2024

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Aug 19, 2024

This gives AutocompleteView subclasses the flexibility to have an implementation that's more than a single loop through candidates.

That will be useful for #234 / #889, /cc @sm-sayedi.

To keep the tricky aspects of the main search loop encapsulated, we move them into a helper function filterCandidates which those computeResults implementations can call.

Neither existing implementation of this method uses this parameter;
it seems like the use of it should be the job of testItem anyway,
not of getSortedItemsToTest.
This method, and its caller _startSearch, already assume that the
query they're given is the current `_query`.  Remove the parameter,
to simplify away redundancy and avoid possible confusion on how the
two query values relate.
I think I probably didn't know Dart had this feature when we
originally wrote this loop.
These two methods were in between compareByRecency and compareByDms,
which are closely-related siblings to each other.  So move them
outside that group.
This gives AutocompleteView subclasses the flexibility to have an
implementation that's more than a single loop through candidates.
@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Aug 19, 2024
@chrisbobbe
Copy link
Collaborator

Thanks, LGTM! Merging.

@chrisbobbe chrisbobbe merged commit cfce389 into zulip:main Aug 20, 2024
1 check passed
@gnprice gnprice deleted the pr-autocomplete-loop branch August 21, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants