Allow adding options in menu and picker #3082

Philipp-M · 2022-07-15T09:08:48Z

This PR allows retaining the current selected option in either the menu or the picker, when calling score (as long, as it's still in there, after calling score).
This is useful when extending the options later (via a new function add_options) in the picker or menu, which is the second addition in this PR.

This change is a requirement for #2507. In itself it doesn't change the current behavior of the editor.
In #2507, this is needed, because multiple language-servers send their results async (e.g. completion, code-actions or symbol-picker)

helix-term/src/ui/menu.rs

helix-term/src/ui/picker.rs

pickfire · 2022-07-27T13:44:36Z

helix-term/src/ui/menu.rs

@@ -58,6 +58,8 @@ pub struct Menu<T: Item> {
    /// (index, score)
    matches: Vec<(usize, i64)>,

+    previous_pattern: String,


Why do we need a previous pattern? Why can't just take from the pattern from the prompt?

This is necessary for the add_options method, because it needs to recalculate the showed options (recalculate_score), without knowing the current pattern.
In the future, this could also help optimizing/filtering previous matches similar as in the picker.

I am still thinking if this is the right place to store this, would it be better to store this outside of Menu and have pattern updated when user open a new menu?

Well as said before, the picker already had it in there, so I just adopted this in the menu as well.
For example self.matches is also dependent on the current pattern and stored inside this Component. Actually all state necessary for rendering (or managing state in) the Menu (or Picker) is stored in the Component.

It e.g. makes sense for caching, prefiltering (as done in the picker) etc.

I think it's maybe better to completely separate data and/or state/model and the view (completion menu in particular), but this is a story(or big PR) for itself, and I'm not sure if it makes the code much better/readable/compact etc.

- ui::Menu::score() now optionally supports retaining the current selected option - Added method add_options to extend options (useful, if multiple options provider asynchronously add options)

- ui::Picker::score() now optionally supports retaining the current selected option - Added method add_options to extend options (useful, if multiple options provider asynchronously add options)

…ith multiple different item sources (async, sync, refetch) 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 `ItemSource`s, which can be async and which may be refetched (see helix-editor#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 helix-editor#5055). This PR is or will be a requirement for helix-editor#2507 (currently provided by helix-editor#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 helix-editor#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 helix-editor#5055 and how to integrate that logic into helix-editor#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 helix-editor#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 helix-editor#2507. 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 `ItemSource`s * 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 helix-editor#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 helix-editor#2608 or helix-editor#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 helix-editor#2507 and it's also the (only) dependency of helix-editor#2507 for helix-editor#2608 * The `DynamicPicker` is removed as this behavior can now be modeled with a `ItemSource::AsyncRefetchOnIdleTimeoutWithPattern` which is done for the `workspace_symbol_picker`

Philipp-M · 2023-09-16T20:25:15Z

I'm going to close this, as this was mostly a hack to get #2507 working in an async way (i.e. adding more options of a different completion source/LSP later). In #2507 we came to the consensus to do this "synchronously" (i.e. all completion sources have to finish before a menu is shown). I don't think this should be used otherwise...

pickfire reviewed Jul 18, 2022

View reviewed changes

helix-term/src/ui/menu.rs Outdated Show resolved Hide resolved

pickfire reviewed Jul 18, 2022

View reviewed changes

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved

pickfire reviewed Jul 18, 2022

View reviewed changes

helix-term/src/ui/picker.rs Show resolved Hide resolved

pickfire reviewed Jul 24, 2022

View reviewed changes

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved

pickfire reviewed Jul 24, 2022

View reviewed changes

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from 3f58108 to 1e38d3a Compare July 24, 2022 08:59

pickfire reviewed Jul 27, 2022

View reviewed changes

archseer self-requested a review August 6, 2022 16:27

Philipp-M mentioned this pull request Aug 13, 2022

Add support for multiple language servers per language #2507

Merged

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from 1e38d3a to 34368bd Compare September 7, 2022 12:45

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

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from 34368bd to dc4c167 Compare September 26, 2022 21:23

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from dc4c167 to 10e2451 Compare October 16, 2022 15:07

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from 10e2451 to 2aa633f Compare October 27, 2022 23:11

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from 2aa633f to 7704398 Compare November 5, 2022 23:48

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from 7704398 to c44f622 Compare November 24, 2022 23:54

Philipp-M added 2 commits December 8, 2022 22:38

Refactored ui::Menu (score, add_options)

cbb87e6

- ui::Menu::score() now optionally supports retaining the current selected option - Added method add_options to extend options (useful, if multiple options provider asynchronously add options)

Refactored ui::Picker (score, add_options)

96177d7

- ui::Picker::score() now optionally supports retaining the current selected option - Added method add_options to extend options (useful, if multiple options provider asynchronously add options)

Philipp-M force-pushed the allow-adding-options-in-menu-and-picker branch from c44f622 to 96177d7 Compare December 8, 2022 22:45

Philipp-M mentioned this pull request Feb 14, 2023

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

Closed

Philipp-M closed this Sep 16, 2023

Philipp-M deleted the allow-adding-options-in-menu-and-picker branch September 16, 2023 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow adding options in menu and picker #3082

Allow adding options in menu and picker #3082

Philipp-M commented Jul 15, 2022

pickfire Jul 27, 2022

Philipp-M Jul 27, 2022

pickfire Sep 27, 2022

Philipp-M Sep 27, 2022

Philipp-M commented Sep 16, 2023 •

edited

Loading

Allow adding options in menu and picker #3082

Allow adding options in menu and picker #3082

Conversation

Philipp-M commented Jul 15, 2022

pickfire Jul 27, 2022

Choose a reason for hiding this comment

Philipp-M Jul 27, 2022

Choose a reason for hiding this comment

pickfire Sep 27, 2022

Choose a reason for hiding this comment

Philipp-M Sep 27, 2022

Choose a reason for hiding this comment

Philipp-M commented Sep 16, 2023 • edited Loading

Philipp-M commented Sep 16, 2023 •

edited

Loading