Refactor picker/menu logic into an options manager which can be fed with multiple different item sources (async, sync, refetch)

[Philipp-M]

This PR unifies the functionality of the Picker and the Menu in the struct OptionsManager (i.e. matching/filtering, sorting, cursor management etc.), and extends the logic of score by being able to retain the cursor position (which is useful, if new entries are added async).

The main reason for this PR though is to have multiple option ItemSources, which can be async and which may be refetched (see #5055) to provide the options for the OptionsManager. It currently allows 3 different input sources: synchronous data, asynchronous data, and asynchronous functions which provide the data, which are reexecuted after an IdleTimeout occured (similar as #5055).
This PR is or will be a requirement for #2507 (currently provided by #3082), which I will rebase onto this PR soon.

Noticeable effects right now:

• The Menu now has the more sophisticated logic of the picker (mostly better caching while changing the query)
• The Menu currently doesn't reset the cursor position, if score() is called (effectively different pattern query), I'm not sure if we should keep this behavior, I guess it needs some testing and feedback. I have kept the original behavior of the Picker (everytime score() changes it resets the cursor)

This PR is an alternative to #3082, which I wasn't completely happy with (adding options async was/is kind of a hack to keep it simple) thus this PR will supersede it. It was originally motivated by #5055 and how to integrate that logic into #2507 (which is not done yet). The PR unfortunately grew a little bit more than I've anticipated, but in general I think it's the cleaner and more future proof way to be able to have async item sources, which may change the options overtime.
For example the same logic of #5055 could be simply implemented for the completion menu with different multiple completion sources, whether it makes sense or not (though also one of the motivations of this PR to have this possibility).
This should also cleanup and greatly (hopefully) simplify #2507.

Technical Overview

To make reviewing easier (I hope) I provide a technical summary of this PR:

• OptionsManager contains all logic that is necessary to manage options (d'oh), that means:
  • Filtering/matching of options with score() which takes a few more parameters now:
    • Optional pattern to be able to recalculate the score without providing a pattern (necessary for async addition of the options)
    • Boolean flag to retain the cursor, since it will likely be annoying, if an item source takes so long that the user is already selecting options of a different item source and it gets resetted because score has to be called again for matches that respect the new option source).
    • force_recalculation, kind of an optimization, if nothing external has changed (except new async options)
  • The OptionsManager is fed by ItemSources
    • Fetching of items from the item source is started by the creation of the OptionsManager (create_from_item_sources), and as soon one item source provides some items a construction callback is executed (see below)
    • Async fetching is done by looping over FuturesUnordered async requests and adding the options via an mpsc, I wanted to avoid any Mutex or something alike to keep the rest of the logic as is (and likely to be more performant as well). Thus instead the editor redraw_handle is notified when new options are available and in the render functions new options are polled (via poll_for_new_options). Although not optimal, it was IMHO the best tradeoff (I tried different solutions, and came to that conclusion).
    • Options are stored with an additional index to the item source to be able to easily change the options on the fly, it provides some iterator wrapper functions to access the options/matches outside as usual.
  • When using the OptionManager with async sources, I have decided (to mostly mirror current logic) to use a "constructor callback" in the create_from_item_sources for creating e.g. the actual Menu or Picker when at least one item is available (this callback is only then executed), otherwise an optional other callback can show e.g. editor status messages, when there was no item source provided any data.
  • The (File-)Picker and Menu now takes the OptionsManager instead of the items or editor data (this is now provided in the ItemSource)
  • For sync data it's also possible to directly create the OptionsManager without a callback using create_from_items but only with this function.
• I've imported the CompletionItem from Add support for multiple language servers per language #2507 to provide additional information where the item came from (which language server id and offset encoding) when multiple language servers are used as Item source, it's currently an enum to avoid unnecessary refactoring if other completion sources are added (such as Add support for path completion #2608 or Completion sources #1015), though it's not really relevant for this PR currently (as the doc has just one language server anyway), but I think it may be easier to review it here separated from Add support for multiple language servers per language #2507 and it's also the (only) dependency of Add support for multiple language servers per language #2507 for Add support for path completion #2608
• The DynamicPicker is removed as this behavior can now be modeled with a ItemSource::AsyncRefetchOnIdleTimeoutWithPattern which is done for the workspace_symbol_picker

[gibbz00]

Also see #5801!

[pascalkuthe]

Thank you for putting in the effort on this PR. This overlaps heavily with the work @sudormrfbin and me have been planning/doing on the picker. I am afraid this PR is not quite to direction that I was hoping to take the picker

I think the picker and the menu should remain separate, they are different UI elements that behave differently. For example: Their filtering works completely differently (and that's on purpose): The picker supports FZF like special characters like !, splitting at spaces etc and uses smart case. The completion is supposed to be forgiving of typos (ignore case) and doesn't use special character.

In particular the async support implemented here should be handled separately between completion menu and the picker. I think for the for #2507 you actually do not need to update the picker/menu asynchronously at all. You should simply join all futures of the server and only create the menu/picker once all servers have responded to the request. This actually seems like better UX to me rather than having items pop-in per picker. I think the case of one misbehaving LSP blocking the picker/menu is rare enough that we can ignore it for now. The picker/menu would just open as slowly as it would if the slowest LS was used alone. That seems ok to me.

I believe for the menu we actually don't ever want to update the menu asynchronously, that seems like bad UI to me.

The picker is a different story. I want to allow every picker to be asynchronously updated in the future to allow the file-picker and global-search picker to open instantly and stream in new results as they are discovered similar to how fzf and skim behave (try opening in the / directory). This has been something that had a lot of discussion and prior art in #1987, #1543 and #1707 and #5871.

However when streaming in a potentially arbitrary number of files performs becomes very critical (after all we are interested in this picker for cases with millions of options). The implementation in this PR is unfortunately not performant and doesn't work the way I had imagined a streaming picker to work. This requires some clever algorithms/data-structure to handle well. Probably something somewhat similar to https://github.com/lotabout/skim/blob/fbb270b772209a95b69cf7dafa9819115b65b873/src/orderedvec.rs. I think the implementation would look very different to what you implemented here and would require (yet another) rewrite of the async logic added here.

The reason is mostly that you designed the OptionsManager to occasionally update the picker/menu (when LSP completes or on workspace symbol re request) whereas a truly streaming picker has very different design concerns. Furthermore the dynamic picker logic would need to be closely integrated with this streaming interface so we can restart the stream (for #196)

So overall I think this PR sadly solves the wrong problem:

• Unifying the menu and picker logic is not desirable IMO
• Updating the picker/menu async for multiple LS (Add support for multiple language servers per language #2507) is actually not something we would want to do. We should simply wait for all LS to respond first.
• refactoring the picker to be updatable asynchronously beyond what is already possible with the dynamic picker is should allow streaming which has it's own challenges and conflicts with the approach here

That being said we very much appreciate your contributions and hope to see #2507 land sooner rather than later.
In the future when working on larger PRs it would be great to either start with a github discussion first or discussing with us on matrix. This avoids wasted effort and allows us to guide you in the right direction.

[Philipp-M]

I appreciate the honest extensive comment.

Well it's a little bit unfortunate, as I've rebased #2507 already on top of this (PR not publicly updated yet, though public branch: https://github.com/Philipp-M/helix/tree/multiple-language-servers-with-options-manager, as I wanted to test this at least myself for a few days). But as you said, that's on me:

In the future when working on larger PRs it would be great to either start with a github discussion first or discussing with us on matrix. This avoids wasted effort and allows us to guide you in the right direction.

you're right here indeed (though hyperfocus/fixation makes this difficult sometimes), I may do this in the future, as this PR indeed took a little bit of effort.

This overlaps heavily with the work @sudormrfbin and me have been planning/doing on the picker.

Is this work publicly? Is it this PR: #5801?
I have unfortunately overlooked this PR, but AFAIK it doesn't really clash with this PR, and could probably be merged together without many issues. Are there more PRs/issues I've overlooked? Maybe open a tracking/encompassing issue with all kinds of plans/ideas for this (and maybe other RFC like things, as I find it difficult to follow technical/architectural ideas on something like matrix)?

The implementation in this PR is unfortunately not performant and doesn't work the way I had imagined a streaming picker to work.

Well yeah, that's probably because it's not (yet) really designed for continuous/real streaming, but rather polling sources that don't support streaming (AFAIK workspace symbols and completions do have to be polled in case they should be updated dynamically). But I think it could be implemented/refactored on top.

The way I see the OptionsManager (in the potential future) is rather an intermediate, higher level construct/interface on top of the matcher/selection logic (which I still think is pretty similar, enough to be shared, between the menu and picker IMHO), kind of a "data-only/model" representation of this menu/picker selection/score logic. I think streaming etc. should probably be done at a lower level at some time, and likely the way the OptionsManager currently fetches ItemSources and how options/items will be merged/contained will have to be refactored as well (e.g. to be more similar as the OrderedVec of skim you sent me).

While I understand your arguments that picker logic and menu logic at some point may really diverge (I don't think that's the case currently, as it should be easy to change the Matcher for either the menu or the picker, which is AFAIK responsible for the different scoring behavior, (maybe via a builder pattern for the OptionsManager, also for other options like reset-cursor?)), or about the true streaming possibility, I at least disagree with that point:

You should simply join all futures of the server and only create the menu/picker once all servers have responded to the request. This actually seems like better UX to me rather than having items pop-in per picker.

Updating the picker/menu async for multiple LS (Add support for multiple language servers per language configuration (closes #1396) #2507) is actually not something we would want to do. We should simply wait for all LS to respond first.

I often retrigger completion (while completion menu is open) to get better results, I think it would be useful at some point to automatically refetch completion results on idle timeout (at least have an option/config for it). I also want to have completion results (or results of other sources as well) as fast as possible, and don't mind if there are some options popping in of a different completion option source (whichever it may be), it should just be there as fast as possible, I think this could indeed be an issue, if using many (maybe slow) completion sources (especially if you're a fast typer).

But I may be alone with this opinion, so I would like to gain more feedback from other users/maintainers as well.

All of the above shouldn't mean though that I'm not open for a bigger adjustment, to align the views/visions where ever this fuzzy picking/matching/item-fetching/updating logic may go either for the menu or picker (or different ui-components that behave similar), do you have already a high level/architectural idea maybe even some code sketches/PRs etc. of what your vision would be?

[pascalkuthe]

I am going to close this PR. It has extremely heavy conflicts now (that will continue to get worse). With the move to nucleo we actually get fully async streaming injection into the picker almost for free (and with #8021 its really easy to add in polled things too, @the-mikedavis is using that to implement interactive global search at the moment).

While this isn't used for completions right now that is actually rather easy now in case we ever do want that (although with the direction I went with in #8021 I believe that just a more aggressive timeout would be enough, which is also how VScode handlers this). The work on nucleo and #8021 is what I was referring to above.

Thanks for all your efforts anyway! We still got #2507 over the finish line in a good state :)

[Philipp-M]

Yeah I was going to close this PR anyway, but forgot about it.

Great work with nucleo and the new event-system btw. that's certainly cleaner and more robust.

Though regarding completion (and one of the main motivations for this PR): It would be awesome to have something like dynamically updating completion results (similarly as the global symbol picker currently (DynamicPicker)). I often see myself retriggering completion manually to get better results (or the results I'm looking for), whereas I think that it would be nicer if this would happen automatically, or there would at least be an option to enable that. (I'm aware that this is not standard behavior e.g. in vscode, but I don't see a real reason against it). But as you say this may be easy to implement now (or at least when the #8021 is merged).

[pascalkuthe]

That actually is fully standard behavior. It's called incomplete completion lists and very widely used.

Helix just doesn't support it yet.

If your language server sends different results once completions are requested again that most likely means it sent an incomplete completion list (RA almost always does that for example). The LSP spec mandates that we should rerewuest those in every keypress we just... don't.

It's one of the things I am aiming at with the event system. But I didn't want to make the PR too huge

[Philipp-M]

Oh, I guess I have to update my knowledge regarding LSP. Thanks for the information. Sounds great, looking forward for it :)