Better sorting in picker in case of ties

[matklad]

Before:

After:

Wouldn't be opposed to writing a test here, but, given that this doesn't fail any existing test, also happy to let it pass :P

[pascalkuthe]

Previous work for context: #4698

Thanks for taking the time to contribute! I like the general idea of sorting file picker entries alphabetically.
However there are many pickers where the initial sorting makes much more sense compared to alphabetical sorting. For example the references in the goto_reference LSP picker items are usually sorted by their occurrence in the file (you know that better then me tough :D) .
Furthermore this particular implementation is not that efficient as filter_text actually allocates twice internally (and would be called for each picker item, on each keypress). The overhead is not huge but it would be nice to avoid nonetheless as this particular action blocks the main UI thread.

I think the same result can be better achieved as follows:
Change the Ord implementation of PickerItem to also consider index in case of a tie (right now it compares by fuzzy score and then by char length). Maybe a stable sort also works but I am not sure about that since a previous query could have changed the sorting due to a difference in fuzzy score. It might be alright depending on the particulars of how the fuzzy score works but I would rather not rely on that (and I believe an unstable sort by index is actually faster anyway).

When creating the picker we could then sort all items before passing them to Picker::new.
This has the advantage of allowing a per-picker tie breaker that is simply based on how the original picker items are sorted.

Furthermore this keeps the picker order a bit more consistent.
When a picker has not query it's items are simply displayed in original order.
When you then filter that picker with a query items would not be shuffled around randomly if they have the same matching score and instead keep their original order.

[matklad]

makes makes sense, ptal!

[matklad]

Interestingly, this also fixes the Eq impl -- previously the derived impl would disagree with the Ord

[pascalkuthe]

I suppose that was a small oversight in the previous implementation. We probably should have returned None in case of equality to be consistent with PartialEq so this is definitly more consistent 👍

[pascalkuthe]

Suggested change

	(cmp::Reverse(self.score), self.index, self.len)
	(cmp::Reverse(self.score), self.len, self.index)

I believe len and index must be swapped here.
Otherwise the length tiebreaker would have no effect (as the index is always unique)

[matklad]

🤔 do we need a len at all then? as in, I'd expect

aaaaac
bc

order perhaps?

Or maybe not -- I can see the appeal of starting with shorter entitiees... Let's go for a minimal diff for the start

[matklad]

Fixed (good thing that rust-analyzer has "flip comma" code action, hehe)

[pascalkuthe]

Sorting by length is useful because it usually shows you what you want first.
For example foo/bar matches foo/bar.rs before foo/bar/baz.rs this way.
See #4698 and #4688 for context. Fzf shares the same behavior by default and I tested this a bit and it's quite useful in practice.

Note that we only apply this sorting once something is typed into the query.
I think the following example illustrates the intended behaviour.

Consider the initial list of files (sorted alphabetically):

aaaaac
ac
bc

Once you type ac the following is shown:

ac
aaaaac

[matklad]

Should be good now! I briefly entertained thee idea of adding Picker::new where T: Ord and Picker::new_presorted, but think it's better to say that all call-sites are responsible for comming up with a reasonable order.

[pascalkuthe]

LGTM, I think this is pretty uncontroversial because it only acts as a tiebreaker (for what was previously an unstable sort).

[the-mikedavis]

Thanks!

TIL about "flip comma", that's handy 😄

[matklad]

TIL about "flip comma", that's handy smile

users not knowing what assists are available is a hard problem. I myself only remeber things I've added personally :D Some thoughts on how to address that: #2011 (comment)