-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add PageUp
, PageDown
, Ctrl-u
, Ctrl-d
, Home
, End
keyboard shortcuts to file picker
#1612
Conversation
…shortcuts to file picker
If we're following the usual convention, (There is a possible exception to this in that popups support |
@EpocSquadron I changed it, thank you for the feedback. |
@Aloso I see you changed the existing bindings, but didn't add the half height implementations and bindings. Do you want to leave this for a follow-up or do you have time to add those too? |
@EpocSquadron I can implement that, EDIT: I'd rather not add this, because it makes the scrolling logic much more complex. Currently, the completion list always scrolls to It would be possible to make the completion list behave more like the actual editor in this regard, but I think it would be better to do that in a separate PR. |
@Aloso thanks for giving it a go! Totally worth separating out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. It would be easier to review if the other stuff is separated but it's fine I guess.
What is the overlay component commit doing? I think this is what pickfire is referring to by "other stuff". If it's just clean up, and not helping implement the scrolling bindings in the picker, then I agree it should go on it's own PR. |
|
||
/// Contains a component placed in the center of the parent component | ||
#[derive(Debug)] | ||
pub struct Overlay<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please separate this out, Popup
with some calculation should be enough to replace this type altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(definitely avoid floats and use a percentage value between 0 and 100)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if a type that can only guarantee the value range but I am not sure if we have something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@archseer I refactored the code again, it is now much simpler. Also, I'm not sure I understand what you mean with your first comment.
b2f0635
to
7cbd215
Compare
7cbd215
to
915cfb1
Compare
helix-term/src/ui/picker.rs
Outdated
) -> Overlay<Self> { | ||
Overlay { | ||
content: Self { | ||
picker: Picker::new(options, format_fn, callback_fn), | ||
truncate_start: true, | ||
preview_cache: HashMap::new(), | ||
read_buffer: Vec::with_capacity(1024), | ||
file_fn: Box::new(preview_fn), | ||
}, | ||
calc_child_size: Box::new(|rect: Rect| clip_rect_relative(rect.clip_bottom(2), 90, 90)), | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's return FilePicker
here and have it wrapped in an overlay inside the ui::file_picker
method. That makes it reusable in cases where we don't want to render it centered but instead nest it in a Popup for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@archseer done. There were a few other places though where it needed to be wrapped in an Overlay
. The FilePicker
struct is also used for selecting a buffer, for global search, for symbol search, and for Go to implementation/references.
Fixes #1467.