Skip to content
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

Merged
merged 6 commits into from
Feb 15, 2022

Conversation

Aloso
Copy link
Contributor

@Aloso Aloso commented Feb 1, 2022

Fixes #1467.

helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
@EpocSquadron
Copy link
Contributor

If we're following the usual convention, page up should correspond to ctrl-b and page down to ctrl-f. ctrl-d and ctrl-u (just d and u in view mode) move by half page everywhere else in helix, making it consistent with the vim bindings.

(There is a possible exception to this in that popups support ctrl-d and ctrl-u, but in the docs it doesn't specify if it goes by half or full. We're also missing ctrl-b and ctrl-f in normal mode and popup mode if the docs are right. I'll open a separate issue for that )

@Aloso
Copy link
Contributor Author

Aloso commented Feb 3, 2022

@EpocSquadron I changed it, thank you for the feedback.

@EpocSquadron
Copy link
Contributor

@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?

@Aloso
Copy link
Contributor Author

Aloso commented Feb 3, 2022

@EpocSquadron I can implement that, it's very easy to add.

EDIT: I'd rather not add this, because it makes the scrolling logic much more complex. Currently, the completion list always scrolls to (cursor % completion_height) in the render function, so it's not possible to scroll by half a page. If I just increment the cursor by completion_height / 2, then the cursor moves to the appropriate line, but the view isn't scrolled. This means that Ctrl-d isn't very useful, since you always have to press it twice to see new entries.

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.

@EpocSquadron
Copy link
Contributor

@Aloso thanks for giving it a go! Totally worth separating out.

Copy link
Contributor

@pickfire pickfire left a 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.

@EpocSquadron
Copy link
Contributor

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.

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

/// Contains a component placed in the center of the parent component
#[derive(Debug)]
pub struct Overlay<T> {
Copy link
Member

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.

Copy link
Member

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

helix-term/src/ui/picker.rs Show resolved Hide resolved
helix-term/src/ui/picker.rs Outdated Show resolved Hide resolved
helix-term/src/ui/overlay.rs Outdated Show resolved Hide resolved
helix-term/src/ui/overlay.rs Outdated Show resolved Hide resolved
@Aloso Aloso force-pushed the navigate-picker branch 3 times, most recently from b2f0635 to 7cbd215 Compare February 3, 2022 17:10
Comment on lines 93 to 104
) -> 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)),
}
}
Copy link
Member

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make page down/page up work in file picker
5 participants