-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
implementing picker preview scrolling #4189
Conversation
Shift+arrow keys are good as a secondary binding but I think we should pick a different primary key. |
helix-term/src/ui/picker.rs
Outdated
@@ -327,6 +334,7 @@ impl<T: Item> Picker<T> { | |||
cursor: 0, | |||
prompt, | |||
previous_pattern: String::new(), | |||
preview_scroll_offset: 0, |
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.
Offset should reset on move_by
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.
You are absolutely right, my bad
helix-term/src/ui/picker.rs
Outdated
@@ -226,7 +228,8 @@ impl<T: Item + 'static> Component for FilePicker<T> { | |||
}) | |||
.unwrap_or(0); | |||
|
|||
let offset = Position::new(first_line, 0); | |||
preview_scroll_offset = preview_scroll_offset.min(doc.text().len_lines()); |
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.
Rather than in the render loop, shouldn't we do this in move_preview_by
?
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.
move_preview_by()
is implemented in Picker and I needed the len_lines()
of the current file in order to limit the scroll. I could transfer that functionality to a FilePicker::move_preview_by()
function and handle the preview scroll events in FilePicker::handle_event()
, instead of doing all that in Picker
Would it be possible to make the keys mappable? |
I can't find the issue about it but it's not configurable right now and would require a bunch of changes. Since picker keybinds aren't "editor commands" you can map. |
@archseer ah, I forgot. I suppose I should buckle down and tackle remapping picker keys so I can get my vim bindings back :) |
IIRC due to terminal limitations it's not possible to bind to |
I could use 'k' and 'j' as in |
To elaborate a little more on the recent push, I considered all of your comments and I would like to request some opinions. Regarding the keybinds: For now, I will leave the keybinds as is (
Regarding @archseer 's comment on move_preview_by and normalising the scroll offset in FilePicker::render(): However, I think What I can do, however, is to create a Also, I fixed preview scroll reset on move_by. Excuse me for the long post. |
I'm actually not sure about shift-ctrl, but I do know that There is a bit of a tension here -- it is natural to expect the |
I quite like the |
@sudormrfbin @EpocSquadron I changed the keybindings to |
…ero first line, fixed highlights move on scroll
Upon further study on the code design, I realised that a preview exists solely when a
Also:
|
I still think the following should be the complete bindings:
I'm not 100% certain alt- is valid with PageUp/PageDown in terminals, but worth a try. |
I updated the keybindings accordingly.
I tried the above on iTerm2 without success. Therefore, I did not include those keybindings. It might be a limitation just on my platform though. |
Works great! I tried using the bindings on a file that was "" and no panics, so that's an edge case handled. It would be good to double check about binary files, but I assume it would work the same. Also working great with buffer picker and highlighted and unhighlighted previews. I think the file list regular bindings could use a touch up, but that's probably better for a separate PR as it would be breaking changes. In other words, LGTM. |
Fixed linting test issues with |
Updating branch with new commits
This would be great to have in the next release! Is there anything left to do before it could be reviewed and merged? |
Shouldn't the keys for scrolling the Maybe the |
Since the LSP doc is a popup and closes on any actions in the buffer (except for mouse input), it feels like a "primary" window. I think that the ctrl-u and ctrl-d mappings and scroll wheel are for a scrolling on a "primary" view and alt/shift is for "secondary" view - it's not been referred to that way, but that's how I process the context of ctrl and alt. I also think changing the mouse behavior is probably a different aspect and should be handled elsewhere. |
Hi! I am wondering if this feature is still desired , and if any changes are needed for this contribution to be merged (probably some key combinations). If so, I want to continue work on this feature. |
Yeah I'd like to see this feature land, it's been on a backlog of PRs I'd like to look at for a while and I think the status here was that it has been waiting for review for quite some time. In the meantime the picker has seen very significant changes and this PR will probably need to be mostly redone on top of the latest master. |
Hi again. I redone it, it's look pretty solid, I used as much of original code as I could. But I am encountering problem, while scrolling down, at some point (it is a fixed position, I can return to it by using alt+k) the preview stops rendering. It's looks like offset is breaking something. It's hard for me to determine where the problem is. Do you have any ideas? |
… with previewer
Replaced by #11441 |
Implements #4102. Currently, I have selected Shift+Up and Shift+Down to scroll up and down the preview respectively, since those keybindings were available and seemed intuitive to me.
Closes #4102