implementing picker preview scrolling

[Manosmer]

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

[archseer]

Shift+arrow keys are good as a secondary binding but I think we should pick a different primary key. shift-ctrl-p/-n?

[heliostatic]

Would it be possible to make the keys mappable?

[archseer]

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.

[heliostatic]

@archseer ah, I forgot. I suppose I should buckle down and tackle remapping picker keys so I can get my vim bindings back :)

[Omnikar]

Shift+arrow keys are good as a secondary binding but I think we should pick a different primary key. shift-ctrl-p/-n?

IIRC due to terminal limitations it's not possible to bind to shift-ctrl-* right?

[Manosmer]

Shift+arrow keys are good as a secondary binding but I think we should pick a different primary key. shift-ctrl-p/-n?

IIRC due to terminal limitations it's not possible to bind to shift-ctrl-* right?

I could use 'k' and 'j' as in Ctrl+K for Up and Ctrl+J for down since they respect the native navigation patterns in Helix

[Manosmer]

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: I know I have suggested we go with the the Ctrl+k, Ctrl+j approach as they are closer to the classic navigation system. However, considering the fact that Helix systematically gives the ability of alternative navigation by swapping keys with arrows, we might not want to bind Ctrl+Up, Ctrl+Down. That is, because on MacOS they are by default predefined shortcuts for "Misson Control" and "Application Windows" (I know because I encountered that problem). The user would be forced to change those settings on their system if they wished to use preview scrolling.

For now, I will leave the keybinds as is (Shift+arrows) while waiting for an answer on @Omnikar 's comment:

IIRC due to terminal limitations it's not possible to bind to shift-ctrl-* right?

Regarding @archseer 's comment on move_preview_by and normalising the scroll offset in FilePicker::render(): In order to normalise the scroll so that Rope does not move out of bounds, it is required to get the previewed document's number of lines. The FilePicker is responsible of creating the previewd document with get_preview() by reading the picker's selection, as well as of rendering that preview with render(). The Picker does not know how long the previewed document is (does not have its &Document), thus the scroll normalisation cannot happen inside Picker::move_preview_by(). It can happen only inside FilePicker::render().

However, I think move_preview_by() should remain in Picker instead of moving it in FilePicker, since handling all key_events under Picker::handle_event() looks more organised than splitting them such that we parse just the new keybinds with FilePicker::handle_event() and the rest with Picker::handle_event(). Also, resetting scroll on Picker::move_by() is very simple.

What I can do, however, is to create a FilePicker::normalize_scroll() that takes the doc.text().len_lines() and the picker's preview_scroll_offset and computes the normalised Position or even render the preview in a separate function called by FilePicker::render() to make it look nicer.

Also, I fixed preview scroll reset on move_by.

Excuse me for the long post.

[EpocSquadron]

Regarding the keybinds: I know I have suggested we go with the the Ctrl+k, Ctrl+j approach as they are closer to the classic navigation system. However, considering the fact that Helix systematically gives the ability of alternative navigation by swapping keys with arrows, we might not want to bind Ctrl+Up, Ctrl+Down. That is, because on MacOS they are by default predefined shortcuts for "Misson Control" and "Application Windows" (I know because I encountered that problem). The user would be forced to change those settings on their system if they wished to use preview scrolling.

For now, I will leave the keybinds as is (Shift+arrows) while waiting for an answer on @Omnikar 's comment:

IIRC due to terminal limitations it's not possible to bind to shift-ctrl-* right?

I'm actually not sure about shift-ctrl, but I do know that Ctrl- with any of hijklm is ambiguous in terminal emulators (unless you use CSIu). It would be bad practice to use those for that reason alone. Normally I would advocate for Ctrl-u/Ctrl-d, but those already scroll the result list, so that's out. Ctrl-f / Ctrl-b should be for full-page instead of half page scrolling (there's an inconsistency in pickers right now see #1614), so shouldn't be used for this either unless we want to walk back bindings later.

There is a bit of a tension here -- it is natural to expect the Ctrl-f/Ctrl-b, Ctrl-d/Ctrl-u, and PageDown/PageUp keys to affect pagewise scrolling for both the preview and the result list, but we can't have them for both and splitting them among the two is dubious. We could do something modal where a ctrl combo toggles witch pane those keys (and likely arrows for single-line adjustment) affect. That seems like a bit extra, and not very discoverable which isn't in keeping with the ideal for this editor. On the other hand, we could use the same bindings but with Alt- instead to make the delineation of which pane it should apply to.

[sudormrfbin]

On the other hand, we could use the same bindings but with Alt- instead to make the delineation of which pane it should apply to.

I quite like the alt- idea since the ctrl- bindings can be replicated on it (unless any of the alt bindings are already in use).

[Manosmer]

@sudormrfbin @EpocSquadron I changed the keybindings to Alt+u, Alt+d. However, Alt+Up, Alt+Down do not work properly. Should I retain Shift+Up, Shift+Down as an alternative to the alt bindings? I am actually one of those people that prefer arrows to keys

[Manosmer]

Upon further study on the code design, I realised that a preview exists solely when a FilePicker is created, while Picker has a more generalised usage. That means that FilePicker is the sole manager of the preview. Because of that I made some design changes to the code so that Picker is completely independent of the preview:

• Moved move_preview_by() and toggle_preview() in FilePicker.
• Moved field show_preview in FilePicker
• preview_scroll_offset is now in FilePicker. The field is now (Direction, usize)
• Created field cursor_moved: bool in Picker to indicate that move_by() occured.
• Created on_cursor_moved() in FilePicker to reset preview scrolling on move_by()
• FilePicker::handle_event() now handles the keybinds for preview scrolling and toggling

Also:

• Fixed preview not scrolling upwards when first_line was > 0
• Fixed text highlighting now moves with preview scrolling
• Now text highlighting in preview happens only when the related text is inside the preview window

[EpocSquadron]

I still think the following should be the complete bindings:

• alt-j / shift-down - Move preview down scroll-lines at a time
• alt-k / shift-up - Move preview up scroll-lines at a time
• alt-d - Move preview down half height of the preview area
• alt-u - Move preview up half height of the preview area
• alt-f / alt-PageDown - Move preview down one full preview area height
• alt-b / alt-PageUp - Move preview up one full preview area height

I'm not 100% certain alt- is valid with PageUp/PageDown in terminals, but worth a try.

[Manosmer]

* `alt-j` / `shift-down` - Move preview down `scroll-lines` at a time

* `alt-k` / `shift-up` - Move preview up `scroll-lines` at a time

* `alt-d` - Move preview down half height of the preview area

* `alt-u` - Move preview up half height of the preview area

* `alt-f` / `alt-PageDown` - Move preview down one full preview area height

* `alt-b` / `alt-PageUp` - Move preview up one full preview area height

I updated the keybindings accordingly.

I'm not 100% certain alt- is valid with PageUp/PageDown in terminals, but worth a try.

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.

[EpocSquadron]

Works great! I tried using the bindings on a file that was "<File too big to preview>" 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.

[Manosmer]

Fixed linting test issues with cargo fmt --all

[LeoniePhiline]

This would be great to have in the next release! Is there anything left to do before it could be reviewed and merged?

[LeoniePhiline]

Shouldn't the keys for scrolling the <space> k LSP documentation popup be adjusted as well?

Maybe the alt and shift key combinations added, but the ctrl combinations with u and d retained. (Not replaced.)

[YeungOnion]

Shouldn't the keys for scrolling the <space> k LSP documentation popup be adjusted as well?

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.

[GNUSheep]

@archseer @the-mikedavis

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.

[the-mikedavis]

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.

[GNUSheep]

@the-mikedavis

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?