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

feat: menu sort strategy for fuzzy match score sorting in completion … #3215

Closed

Conversation

lazytanuki
Copy link
Contributor

This adds a sorting strategy to the menu.

As of now, all completions are sorted alphabetically after passing the fuzzy match filter. With this PR they are sorted by their fuzzy match score after passing the filter.
Code actions menu's behavior remains the same.

This comes from #2705, where it is needed for LSP preselect.

The diff may seem big because of the rustmft formatting that changed the indentation of a whole closure, but the only changes are:

  • a new enum
  • a new argument to the Menu constructor
  • a match on the enum in the score() method

@ChrHorn
Copy link
Contributor

ChrHorn commented Jul 27, 2022

This would fix #2508

@the-mikedavis
Copy link
Member

I don't think this fixes #2508. I tried it against the reproduction case there and the results are the same.

@lazytanuki
Copy link
Contributor Author

Are you sure ? I just tried the reproduction case in #2508 and this seems to solve the issue for me.
image

@the-mikedavis
Copy link
Member

Whoops I was on the wrong branch 😅

Yep, looks like this does fix #2508 👍

@the-mikedavis the-mikedavis linked an issue Jul 27, 2022 that may be closed by this pull request
@lazytanuki
Copy link
Contributor Author

Just rebased this on master, could someone review this pretty please ? It would unblock some PRs, including the LSP preselect feature that I implemented a couple months ago.

As a reminder: the diff may seem big because of the rustmft formatting that changed the indentation of a whole closure, but the only changes are:

  • a new enum
  • a new argument to the Menu constructor
  • a match on the enum in the score() method

@archseer
Copy link
Member

archseer commented Sep 9, 2022

Do we need the two strategies though? Why not just change the behaviour to sort based on the score?

@lazytanuki
Copy link
Contributor Author

Some pickers such as code actions do not use a score AFAIK, so it makes sense for them to be sorted alphabetically (which they are on master).

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 13, 2022
@lukepighetti
Copy link

Looks like this is blocking #3084 (comment)

@lazytanuki
Copy link
Contributor Author

image

@archseer
Copy link
Member

To review I've used ?w=1 on the URL which ignores whitespace changes.

I still think this could be a method on the Item trait? Something that returns Ord and pattern and score would be one of the inputs. That way different menus could have their own sort strategies rather than hardcoding it to two.

Some pickers such as code actions do not use a score AFAIK, so it makes sense for them to be sorted alphabetically (which they are on master).

#4134 does change the strategy to sort by score too. So maybe #4134 is enough, then #2705 can be implemented on top of 63a54ee

@lazytanuki
Copy link
Contributor Author

lazytanuki commented Oct 26, 2022

Superseeded by #4134 .

@lazytanuki lazytanuki closed this Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sometimes wrong order in autocomplete menu
6 participants