implementing picker preview scrolling #4189

Manosmer · 2022-10-11T00:29:27Z

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 · 2022-10-11T00:31:38Z

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

archseer · 2022-10-11T00:32:34Z

helix-term/src/ui/picker.rs

@@ -327,6 +334,7 @@ impl<T: Item> Picker<T> {
 cursor: 0,
 prompt,
 previous_pattern: String::new(),
+ preview_scroll_offset: 0,


Offset should reset on move_by

You are absolutely right, my bad

archseer · 2022-10-11T00:33:32Z

helix-term/src/ui/picker.rs

@@ -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());


Rather than in the render loop, shouldn't we do this in move_preview_by?

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

heliostatic · 2022-10-11T00:59:07Z

Would it be possible to make the keys mappable?

archseer · 2022-10-11T01:00:46Z

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 · 2022-10-11T01:07:03Z

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

Omnikar · 2022-10-11T01:24:07Z

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 · 2022-10-11T01:40:08Z

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 · 2022-10-11T11:55:08Z

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 · 2022-10-11T14:11:39Z

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 · 2022-10-11T16:30:47Z

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 · 2022-10-11T18:46:12Z

@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

…ero first line, fixed highlights move on scroll

Manosmer · 2022-10-13T12:01:49Z

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 · 2022-10-16T14:49:59Z

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 · 2022-10-16T15:58:59Z

* `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 · 2022-10-16T16:06:12Z

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.

Manosmer · 2022-10-17T14:21:09Z

Fixed linting test issues with cargo fmt --all

Updating branch with new commits

LeoniePhiline · 2023-01-26T10:21:26Z

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

LeoniePhiline · 2023-02-01T21:23:25Z

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 · 2023-08-23T16:18:28Z

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 · 2024-06-13T15:17:23Z

@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 · 2024-06-14T15:23:50Z

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 · 2024-06-16T11:03:46Z

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

… with previewer

archseer · 2024-08-09T15:43:51Z

Replaced by #11441

implementing picker preview scrolling

e55a754

archseer reviewed Oct 11, 2022

View reviewed changes

fixed reset scroll on move_by, reorganized key_event matching

68f8716

Manosmer added 2 commits October 12, 2022 17:32

changed keybindings to alt+u alt+d

05d14b3

better code design for preview management, fixed scrolling up on nonz…

6f75057

…ero first line, fixed highlights move on scroll

Manosmer marked this pull request as draft October 13, 2022 13:43

kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Oct 15, 2022

Manosmer added 2 commits October 15, 2022 18:42

fixed preview scroll resets only on file change

3d08447

preview scrolls only if show_preview = true

a0101d6

Manosmer marked this pull request as ready for review October 16, 2022 14:47

updated keybindings

12d4539

Manosmer added 3 commits October 17, 2022 16:07

rearrange the order of imports

77d7f37

merge upstream changes and resolving conflicts

377d131

fix lint test with "cargo fmt --all"

b28ac5f

Manosmer requested a review from archseer October 18, 2022 19:57

David-Else mentioned this pull request Oct 19, 2022

Ctrl-d/Ctrl-u to scroll the option's documentation on auto-complete #4364

Closed

Manosmer added 8 commits October 23, 2022 13:19

Merge branch 'master' into preview-scroll

831575f

Updating branch with new commits

Merge branch 'master' into preview-scroll

d2a5b28

added scrollbar for preview scrolling

dc054ec

Merge branch 'master' into preview-scroll

d0fde28

cargo fmt

2501166

fix: scroll reset on diagnostic picker

03e2970

Merge branch 'master' into preview-scroll

616c919

removed unnecessary log::info

23189b9

GNUSheep added a commit to GNUSheep/helix that referenced this pull request Jun 16, 2024

Redoing helix-editor#4189 to be up-to-date with master; TODO: fix bug…

8a0645b

… with previewer

GNUSheep added a commit to GNUSheep/helix that referenced this pull request Aug 7, 2024

Redone helix-editor#4189 on top of master, now everyting works

8065161

archseer closed this Aug 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implementing picker preview scrolling #4189

implementing picker preview scrolling #4189

Manosmer commented Oct 11, 2022 •

edited

Loading

archseer commented Oct 11, 2022

archseer Oct 11, 2022

Manosmer Oct 11, 2022

archseer Oct 11, 2022

Manosmer Oct 11, 2022 •

edited

Loading

heliostatic commented Oct 11, 2022

archseer commented Oct 11, 2022

heliostatic commented Oct 11, 2022

Omnikar commented Oct 11, 2022

Manosmer commented Oct 11, 2022

Manosmer commented Oct 11, 2022

EpocSquadron commented Oct 11, 2022

sudormrfbin commented Oct 11, 2022

Manosmer commented Oct 11, 2022

Manosmer commented Oct 13, 2022 •

edited

Loading

EpocSquadron commented Oct 16, 2022

Manosmer commented Oct 16, 2022 •

edited

Loading

EpocSquadron commented Oct 16, 2022

Manosmer commented Oct 17, 2022

LeoniePhiline commented Jan 26, 2023

LeoniePhiline commented Feb 1, 2023 •

edited

Loading

YeungOnion commented Aug 23, 2023

GNUSheep commented Jun 13, 2024

the-mikedavis commented Jun 14, 2024

GNUSheep commented Jun 16, 2024

archseer commented Aug 9, 2024

implementing picker preview scrolling #4189

implementing picker preview scrolling #4189

Conversation

Manosmer commented Oct 11, 2022 • edited Loading

archseer commented Oct 11, 2022

archseer Oct 11, 2022

Choose a reason for hiding this comment

Manosmer Oct 11, 2022

Choose a reason for hiding this comment

archseer Oct 11, 2022

Choose a reason for hiding this comment

Manosmer Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

heliostatic commented Oct 11, 2022

archseer commented Oct 11, 2022

heliostatic commented Oct 11, 2022

Omnikar commented Oct 11, 2022

Manosmer commented Oct 11, 2022

Manosmer commented Oct 11, 2022

EpocSquadron commented Oct 11, 2022

sudormrfbin commented Oct 11, 2022

Manosmer commented Oct 11, 2022

Manosmer commented Oct 13, 2022 • edited Loading

EpocSquadron commented Oct 16, 2022

Manosmer commented Oct 16, 2022 • edited Loading

EpocSquadron commented Oct 16, 2022

Manosmer commented Oct 17, 2022

LeoniePhiline commented Jan 26, 2023

LeoniePhiline commented Feb 1, 2023 • edited Loading

YeungOnion commented Aug 23, 2023

GNUSheep commented Jun 13, 2024

the-mikedavis commented Jun 14, 2024

GNUSheep commented Jun 16, 2024

archseer commented Aug 9, 2024

Manosmer commented Oct 11, 2022 •

edited

Loading

Manosmer Oct 11, 2022 •

edited

Loading

Manosmer commented Oct 13, 2022 •

edited

Loading

Manosmer commented Oct 16, 2022 •

edited

Loading

LeoniePhiline commented Feb 1, 2023 •

edited

Loading