refactor: Merge Picker and FilePicker #5801

sudormrfbin · 2023-02-03T06:27:44Z

Merges the code for the Picker and FilePicker into a single Picker that can show a file preview if a preview callback is provided. This change was mainly made to facilitate refactoring out a simple skeleton of a picker that does not do any filtering to be reused in a normal Picker and a DynamicPicker (see #5714; in particular @the-mikedavis's comment and @sudormrfbin's comment).

The crux of the issue is that a picker maintains a list of predefined options (eg. list of files in the directory) and (re-)filters them every time the picker prompt changes, while a dynamic picker (eg. interactive global search, #4687) recalculates the full list of options on every prompt change. Using a filtering picker to drive a dynamic picker hence does duplicate work of filtering thousands of matches for no reason. It could also cause problems like interfering with the regex pattern in the global search.

I tried to directly extract a PickerBase to be reused in Picker, FilePicker and DynamicPicker, but the problem is that DynamicPicker is actually a DynamicFilePicker (i.e. it can preview file contents) which means we would need PickerBase, Picker, FilePicker, DynamicPicker and DynamicFilePicker and then another way of sharing the previewing code between a FilePicker and a DynamicFilePicker. By merging Picker and FilePicker into Picker, we only need PickerBase, Picker and DynamicPicker.

The PR has been organized into commits that can be easily reviewed. Each commit moves a set of related functionality from Picker into FilePicker and keeps a placeholder unimplemented!() call in the original region. When all methods have been moved into FilePicker, the original Picker struct and impl are deleted and FilePicker is renamed to Picker. There's close to no new code, save for some new methods like Picker::with_preview and Picker::without_preview.

Merges the code for the Picker and FilePicker into a single Picker that can show a file preview if a preview callback is provided. This change was mainly made to facilitate refactoring out a simple skeleton of a picker that does not do any filtering to be reused in a normal Picker and a DynamicPicker (see helix-editor#5714; in particular [mikes-comment] and [gokuls-comment]). The crux of the issue is that a picker maintains a list of predefined options (eg. list of files in the directory) and (re-)filters them every time the picker prompt changes, while a dynamic picker (eg. interactive global search, helix-editor#4687) recalculates the full list of options on every prompt change. Using a filtering picker to drive a dynamic picker hence does duplicate work of filtering thousands of matches for no reason. It could also cause problems like interfering with the regex pattern in the global search. I tried to directly extract a PickerBase to be reused in Picker and FilePicker and DynamicPicker, but the problem is that DynamicPicker is actually a DynamicFilePicker (i.e. it can preview file contents) which means we would need PickerBase, Picker, FilePicker, DynamicPicker and DynamicFilePicker and then another way of sharing the previewing code between a FilePicker and a DynamicFilePicker. By merging Picker and FilePicker into Picker, we only need PickerBase, Picker and DynamicPicker. [gokuls-comment]: helix-editor#5714 (comment) [mikes-comment]: helix-editor#5714 (comment)

When Picker and FilePicker are merged, not all Pickers will be able to show a preview

the-mikedavis

The commit layout is awesome, it feels like I just watched you make this change live 😀

This refactor makes sense to me - I think I see where you're going for making filtering/sorting optional 👍

the-mikedavis · 2023-02-10T22:13:49Z

helix-term/src/ui/picker.rs

@@ -883,15 +863,15 @@ pub type DynQueryCallback<T> =
 /// A picker that updates its contents via a callback whenever the
 /// query string changes. Useful for live grep, workspace symbols, etc.
 pub struct DynamicPicker<T: ui::menu::Item + Send> {
-    file_picker: FilePicker<T>,
+    file_picker: Picker<T>,


Should this be renamed to just "picker" now?

Proful · 2023-02-10T23:33:45Z

The commit layout is awesome, it feels like I just watched you make this change live 😀

any video link?

Proful · 2023-02-10T23:38:48Z

helix-term/src/ui/picker.rs

        options: Vec<T>,
        editor_data: T::Data,
-        callback_fn: impl Fn(&mut Context, &T, Action) + 'static,
+        callback_fn: Box<dyn Fn(&mut Context, &T, Action) + 'static>,


Reason behind changing from static dispatch to dynamic dispatch?

impl Fn -> Box<dyn Fn

Proful · 2023-02-10T23:39:48Z

helix-term/src/ui/picker.rs

        options: Vec<T>,
        editor_data: T::Data,
-        callback_fn: impl Fn(&mut Context, &T, Action) + 'static,
+        callback_fn: Box<dyn Fn(&mut Context, &T, Action) + 'static>,
+        preview_fn: Option<FilePreviewFn<T>>,


As Picker is generic now File prefix can be renamed from FilePreviewFn?

Currently the preview only works for files so I think this name fits. This type also didn't exist before this change

Proful · 2023-02-10T23:41:32Z

helix-term/src/ui/picker.rs

+        options: Vec<T>,
+        editor_data: T::Data,
+        callback_fn: impl Fn(&mut Context, &T, Action) + 'static,
+        preview_fn: impl Fn(&Editor, &T) -> Option<FileLocation> + 'static,


It would be better Preview can be any content not only File content.
FileLocation?

Changing what the preview can show would be out of scope for this refactor PR I think - https://github.com/helix-editor/helix/pull/5492/files#r1070336613 is looking to change that anyways

Maybe just calling it PreviewFn now would be easy enough so we don't have to rename in the future?

I don't think the capability itself should be added in this PR but it would be a small change to future proof here to avoid renames (and larger diffs/merge conflicts) in the future

archseer · 2023-02-11T07:01:03Z

helix-term/src/commands.rs

@@ -2616,7 +2616,7 @@ pub fn command_palette(cx: &mut Context) {
                }
            }));

-            let picker = Picker::new(commands, keymap, move |cx, command, _action| {
+            let picker = Picker::without_preview(commands, keymap, move |cx, command, _action| {


I'd still name this new() we could also make the with_preview() a builder function that takes `self‘ so you'd chain these: new(..).with_preview(..)

Yes, I think beside this, everything else looks okay.

sudormrfbin changed the title ~~Move FilePicker::render from Component impl to normal impl~~ refactor: Merge Picker and FilePicker Feb 3, 2023

sudormrfbin added 12 commits February 3, 2023 12:06

Copy struct fields and new() from Picker to FilePicker

3fe4a31

Move score methods from Picker to FilePicker

55930a4

Move navigation methods from Picker to FilePicker

f01844a

Move handle_event methods from Picker to FilePicker

d0b781b

Move Component methods except render() to FilePicker

062d936

Move Picker::render into FilePicker::render

c082da0

Render the preview in FilePicker

60c51b1

Move FilePicker struct def closer to impl block

be76a1e

Make file preview callback optional

6dc42b5

When Picker and FilePicker are merged, not all Pickers will be able to show a preview

Completely remove old Picker and rename FilePicker to Picker

ef0528a

Fix clippy lints

792c8f9

sudormrfbin force-pushed the merge-picker-and-filepicker branch from 15b8c3e to 792c8f9 Compare February 3, 2023 06:55

pascalkuthe added A-helix-term Area: Helix term improvements E-medium Call for participation: Experience needed to fix: Medium / intermediate S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 3, 2023

pascalkuthe added this to the next milestone Feb 9, 2023

the-mikedavis approved these changes Feb 10, 2023

View reviewed changes

Proful reviewed Feb 10, 2023

View reviewed changes

archseer reviewed Feb 11, 2023

View reviewed changes

gibbz00 mentioned this pull request Feb 16, 2023

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

Closed

archseer modified the milestones: 23.03, next Mar 29, 2023

the-mikedavis mentioned this pull request Jun 6, 2023

Merge FilePicker into Picker #7264

Merged

the-mikedavis removed this from the next milestone Jun 16, 2023

pascalkuthe closed this in #7264 Jun 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Merge Picker and FilePicker #5801

refactor: Merge Picker and FilePicker #5801

sudormrfbin commented Feb 3, 2023 •

edited

Loading

the-mikedavis left a comment

the-mikedavis Feb 10, 2023

Proful commented Feb 10, 2023 •

edited

Loading

Proful Feb 10, 2023

Proful Feb 10, 2023

the-mikedavis Feb 11, 2023

Proful Feb 10, 2023

the-mikedavis Feb 11, 2023

pascalkuthe Feb 11, 2023

archseer Feb 11, 2023

pickfire Mar 13, 2023

refactor: Merge Picker and FilePicker #5801

refactor: Merge Picker and FilePicker #5801

Conversation

sudormrfbin commented Feb 3, 2023 • edited Loading

the-mikedavis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Proful commented Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudormrfbin commented Feb 3, 2023 •

edited

Loading

Proful commented Feb 10, 2023 •

edited

Loading