Redone #4189, working preview scrolling #11441

GNUSheep · 2024-08-07T13:58:19Z

Finally redone preview scrolling on current master. Works pretty fine.

I used code from: @Manosmer, and make it work with current version of helix.

Connects #1614

thomasaarholt · 2024-08-18T18:14:04Z

Current keybindings:

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

Originally posted by @EpocSquadron in #4189 (comment)

thomasaarholt · 2024-08-18T18:17:09Z

I do observe that when scrolling, the syntax highlighting does not update. Only the text first shown in the preview pane is syntax highlighted.

GNUSheep · 2024-08-19T14:50:05Z

@thomasaarholt

Hi! Thank you for your help. I fixed my mistakes, and now all should work well.

My only concern is about keybinding alt-PageUp, because as mentioned before:

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.

In my case alt-PageDown works pretty fine, but alt-PageUp doesn't.
I tested on alattracity, and gnome terminal and got same result.

But as mentioned above it might be just a limitation on my platform.

GNUSheep · 2024-09-10T17:44:46Z

@archseer
@the-mikedavis

EmrysMyrddin · 2024-09-18T18:45:42Z

I've tried it, works very well, very welcomed addition ! I hope it will be merged soon :-)

EmrysMyrddin · 2024-09-18T19:07:06Z

I'm using Helix based on your PR, and I came across another issue:

Here the diagnostic is miss-rendered. It should not underline the whole comment.

Here how it looks in the main buffer:

Note: the diagnostic is rendered in the preview only if the file have been opened at least once since Helix started.

David-Else · 2024-09-18T19:09:26Z

@thomasaarholt

My only concern is about keybinding alt-PageUp, because as mentioned before:

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.

In my case alt-PageDown works pretty fine, but alt-PageUp doesn't. I tested on alattracity, and gnome terminal and got same result.

But as mentioned above it might be just a limitation on my platform.

I am not a fan of using the Alt key, particularly with the Alt + PageUp/PageDown combinations. I have noticed that currently, PageUp/PageDown seems to jump up and down the file or item list by a seemingly random amount in different pickers. I really can't find a good use for the current functionality in the picker. It would be better to use PageUp/PageDown for preview scrolling instead, similar to how LazyGit does. Now that we have Pickers v2, it makes more sense to refine the search rather than scrolling through the list of items in such large increments.

GNUSheep · 2024-09-20T17:38:15Z

@EmrysMyrddin

Hi! Thank u for using helix based on my PR.
I fixed that bug,
(I just forgot to change offset.anchor in doc_diagnostics_highlights)

If u find more bugs I will be happy to fix them :)

GNUSheep · 2024-09-20T18:51:26Z

@David-Else
Hello! Looking at the code PageUp and PageDown, moves up or down by a exactly one page. But I agree with you that it is not so useful, I think that many people just search by using search bar rather than scrolling up.

Using just PageUp or PageDown seems like nice idea.

GNUSheep · 2024-10-24T11:42:45Z

@archseer @the-mikedavis @pascalkuthe

qiu-x · 2024-12-30T17:56:18Z

Are there any blockers before this can be merged?
@the-mikedavis

rockboynton · 2025-01-12T03:32:16Z

I can consistently reproduce a panic when using this. (disclaimer, I do have other branches too merged into my fork too, but I can only reproduce this issue when this PR is included):

Open a repo with lsp (rust-analyzer in my case)
Open global symbol picker (<space>S)
Typy-type
The panic occurs sometimes while typing, sometimes when I hit <esc> to close, sometimes if I close, re-open the global symbol picker and type more.

Can you try to reproduce? It may be possible it is just my unique setup, but like I said, I've only seen this when using this PR.

󱞪 RUST_BACKTRACE=full hx .
thread 'main' panicked at /nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-vendor-cargo-deps/c19b7c6f923b580ac259164a89f2577984ad5ab09ee9d583b888f934adbbe8d0/ropey-1.6.1/src/slice.rs:453:41:
called `Result::unwrap()` on an `Err` value: Line index out of bounds: line index 6457, Rope/RopeSlice line count 436
stack backtrace:
   0:     0x55555661be0b - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::h1b9dad2a88e955ff
   1:     0x55555577884b - core::fmt::write::h4b5a1270214bc4a7
   2:     0x555556617b73 - std::io::Write::write_fmt::hd04af345a50c312d
   3:     0x55555661de2a - std::panicking::default_hook::{{closure}}::h96ab15e9936be7ed
   4:     0x55555661da77 - std::panicking::default_hook::h3cacb9c27561ad33
   5:     0x55555661e6a8 - std::panicking::rust_panic_with_hook::hfe205f6954b2c97b
   6:     0x55555661e397 - std::panicking::begin_panic_handler::{{closure}}::h6cb44b3a50f28c44
   7:     0x55555661c2d9 - std::sys::backtrace::__rust_end_short_backtrace::hf1c1f2a92799bb0e
   8:     0x55555661e024 - rust_begin_unwind
   9:     0x555555680593 - core::panicking::panic_fmt::h3d8fc78294164da7
  10:     0x555555680a26 - core::result::unwrap_failed::hfa79a499befff387
  11:     0x555556106c47 - <helix_term::ui::picker::Picker<I,D> as helix_term::compositor::Component>::render::h33dd85836441c517
  12:     0x555556377c03 - helix_term::application::Application::render::{{closure}}::h108963a17ac2b9f0
  13:     0x55555638c7b0 - hx::main_impl::{{closure}}::h1105d798dbc7e566
  14:     0x5555563898c8 - tokio::runtime::park::CachedParkThread::block_on::h390deff2c402d9ac
  15:     0x55555642a4de - tokio::runtime::context::runtime::enter_runtime::h6b594ce14328c083
  16:     0x555556439f46 - tokio::runtime::runtime::Runtime::block_on::h03ec0308555e9d53
  17:     0x5555563cdb73 - hx::main::h3debb9f5c1109bd7
  18:     0x5555563f4143 - std::sys::backtrace::__rust_begin_short_backtrace::ha704d94dc880694d
  19:     0x5555563fbb3d - std::rt::lang_start::{{closure}}::hf9a24b6ef71d3115
  20:     0x55555660e685 - std::rt::lang_start_internal::h5e7c81cecd7f0954
  21:     0x5555563cdc65 - main
  22:     0x7ffff7c2a1fc - __libc_start_call_main
  23:     0x7ffff7c2a2b9 - __libc_start_main_alias_2
  24:     0x5555556f3535 - _start
  25:                0x0 - <unknown>

GNUSheep · 2025-01-15T14:35:14Z

@rockboynton

Hi! Thank you for pointing out the error. This panic was indeed related to this PR. I missed it during testing - perhaps I didn't run enough tests - so I am sorry for that oversight.

Together with @qiu-x, we have fixed the issue. We switched to using offset_anchor instead of offset_vertical_offset to move the preview, and this resolved the problem. We ran several tests and did not encounter any more panics.

I hope everything works fine now. I look forward to your feedback! Please let me know if everything is working as expected
I really hope it does. :)

Thank you once again!

nik-rev · 2025-01-28T16:42:26Z

this PR also closes #1614

rcorre · 2025-02-24T15:49:41Z

Are the kebindings configurable?

GNUSheep · 2025-02-24T19:01:16Z

Currently not, but I will add this idea to my to-do list, and implement it.

~~(It is just a case to add moving preview as static command in helix-term/src/commands.rs)~~

Now as I am looking at helix-term/src/commands.rs, it seems like it isn't possible right now to remap any action taken in picker

haxfn · 2025-03-13T09:24:43Z

What is stopping from this being merged to master?

`patchy` is a tool which makes it easy to declaratively manage personal forks by automatically merging pull requests. Check it out here: https://github.com/NikitaRevenco/patchy

n6v26r · 2025-05-23T14:21:03Z

Why aren't we getting this merged?

knowsuchagency · 2025-07-16T04:26:58Z

This would be a very welcome feature as a new user. The current behavior isn't what I would have expected. Looking forward to seeing this merged -- thanks for all the hard work!

EmrysMyrddin · 2025-09-02T13:09:02Z

@GNUSheep Can you rebase your branch ? It has conflicts now ><'

Co-authored-by: GNUSheep <zydekpiotr26@gmail.com> Co-authored-by: Manos Mertzianis <manosmertzianis@gmail.com>

GNUSheep · 2025-09-04T07:54:31Z

@EmrysMyrddin
Hi! Rebase done :)

GNUSheep · 2025-09-04T07:56:08Z

@archseer @the-mikedavis @pascalkuthe @sudormrfbin @dead10ck

EmrysMyrddin · 2025-09-04T08:56:15Z

Thank you ! You're awesome !

the-mikedavis

It seems like this doesn't work well with soft-wrap enabled. Setting editor.soft-wrap.enable to true also enables soft-wrap in the picker and when a file has long lines I can't get to the bottom in the preview with A-d

the-mikedavis · 2025-09-04T13:55:52Z

helix-term/src/ui/picker.rs

-            key!(PageDown) | ctrl!('d') => {
+            key!(PageDown) | ctrl!('d') if !self.show_preview => {
                self.page_down();
            }
-            key!(PageUp) | ctrl!('u') => {
+            key!(PageUp) | ctrl!('u') if !self.show_preview => {


This will leave C-u/C-d unbound when showing the preview. Instead we can separate these out so that C-u/C-d always does page_up/page_down and gate key!(PageUp)/key!(PageDown) on self.show_preview

the-mikedavis · 2025-09-04T13:59:18Z

helix-term/src/ui/picker.rs

+            const fn div_ceil(a: usize, b: usize) -> usize {
+                (a + b - 1) / b
+            }


This can be written with div_ceil from std now, also see

helix/helix-term/src/ui/menu.rs

Line 355 in d1750a7

let scroll_height = win_height.pow(2).div_ceil(len).min(win_height);

It stabilized somewhat recently in 1.73 and we had to define it ourselves before then

the-mikedavis · 2025-09-04T14:03:34Z

helix-term/src/ui/picker.rs

+        match move_direction {
+            Direction::Backward => match current_scroll_direction {
+                Direction::Backward => {
+                    self.preview_scroll_offset.1 = current_scroll_offset.saturating_add(amount);
+                }
+                Direction::Forward => {
+                    if let Some(change) = current_scroll_offset.checked_sub(amount) {
+                        self.preview_scroll_offset.1 = change;
+                    } else {
+                        self.preview_scroll_offset = (
+                            Direction::Backward,
+                            amount.saturating_sub(current_scroll_offset),
+                        );
+                    }
+                }
+            },
+            Direction::Forward => match current_scroll_direction {
+                Direction::Backward => {
+                    if let Some(change) = current_scroll_offset.checked_sub(amount) {
+                        self.preview_scroll_offset.1 = change;
+                    } else {
+                        self.preview_scroll_offset = (
+                            Direction::Forward,
+                            amount.saturating_sub(current_scroll_offset),
+                        );
+                    }
+                }
+                Direction::Forward => {
+                    self.preview_scroll_offset.1 = current_scroll_offset.saturating_add(amount);
+                }
+            },
+        };


I think this can be simplified a bit since the branches are nearly duplicates

Suggested change

match move_direction {

Direction::Backward => match current_scroll_direction {

Direction::Backward => {

self.preview_scroll_offset.1 = current_scroll_offset.saturating_add(amount);

}

Direction::Forward => {

if let Some(change) = current_scroll_offset.checked_sub(amount) {

self.preview_scroll_offset.1 = change;

} else {

self.preview_scroll_offset = (

Direction::Backward,

amount.saturating_sub(current_scroll_offset),

);

}

}

},

Direction::Forward => match current_scroll_direction {

Direction::Backward => {

if let Some(change) = current_scroll_offset.checked_sub(amount) {

self.preview_scroll_offset.1 = change;

} else {

self.preview_scroll_offset = (

Direction::Forward,

amount.saturating_sub(current_scroll_offset),

);

}

}

Direction::Forward => {

self.preview_scroll_offset.1 = current_scroll_offset.saturating_add(amount);

}

},

};

if move_direction == current_scroll_direction {

self.preview_scroll_offset.1 = current_scroll_offset.saturating_add(amount);

} else if let Some(change) = current_scroll_offset.checked_sub(amount) {

self.preview_scroll_offset.1 = change;

} else {

self.preview_scroll_offset =

(move_direction, amount.saturating_sub(current_scroll_offset));

}

GNUSheep · 2025-09-04T15:15:06Z

Hi @the-mikedavis
Thanks you for your review, I will apply your suggestions as soon as I can

GNUSheep changed the title ~~Redone helix-editor#4189, working preview scrolling~~ Redone #4189, working preview scrolling Aug 7, 2024

archseer mentioned this pull request Aug 9, 2024

implementing picker preview scrolling #4189

Closed

GNUSheep force-pushed the scroll_preview_fixed branch 2 times, most recently from ecd415f to a69ea1d Compare August 19, 2024 13:00

kirawi added the A-helix-term Area: Helix term improvements label Aug 28, 2024

GNUSheep force-pushed the scroll_preview_fixed branch from c61eea4 to 40d9547 Compare January 15, 2025 12:36

nik-rev mentioned this pull request Jan 28, 2025

Make picker preview scrollable #12717

Closed

satoqz mentioned this pull request Feb 20, 2025

preview file in file picker #12917

Closed

GNUSheep force-pushed the scroll_preview_fixed branch from eda5aa1 to 16b0db2 Compare May 25, 2025 09:27

feat: Redone preview scrolling from helix-editor#4189

2a1d44f

Co-authored-by: GNUSheep <zydekpiotr26@gmail.com> Co-authored-by: Manos Mertzianis <manosmertzianis@gmail.com>

GNUSheep force-pushed the scroll_preview_fixed branch from 16b0db2 to 2a1d44f Compare September 4, 2025 07:53

the-mikedavis reviewed Sep 4, 2025

View reviewed changes

Uh oh!

Uh oh!

Redone #4189, working preview scrolling #11441

Are you sure you want to change the base?

Redone #4189, working preview scrolling #11441

Uh oh!

Conversation

GNUSheep commented Aug 7, 2024 • edited by the-mikedavis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasaarholt commented Aug 18, 2024

Uh oh!

thomasaarholt commented Aug 18, 2024

Uh oh!

GNUSheep commented Aug 19, 2024

Uh oh!

GNUSheep commented Sep 10, 2024

Uh oh!

EmrysMyrddin commented Sep 18, 2024

Uh oh!

EmrysMyrddin commented Sep 18, 2024

Uh oh!

David-Else commented Sep 18, 2024

Uh oh!

GNUSheep commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GNUSheep commented Sep 20, 2024

Uh oh!

GNUSheep commented Oct 24, 2024

Uh oh!

qiu-x commented Dec 30, 2024

Uh oh!

rockboynton commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GNUSheep commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik-rev commented Jan 28, 2025

Uh oh!

rcorre commented Feb 24, 2025

Uh oh!

GNUSheep commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haxfn commented Mar 13, 2025

Uh oh!

n6v26r commented May 23, 2025

Uh oh!

knowsuchagency commented Jul 16, 2025

Uh oh!

EmrysMyrddin commented Sep 2, 2025

Uh oh!

GNUSheep commented Sep 4, 2025

Uh oh!

GNUSheep commented Sep 4, 2025

Uh oh!

EmrysMyrddin commented Sep 4, 2025

Uh oh!

the-mikedavis left a comment

Choose a reason for hiding this comment

Uh oh!

the-mikedavis Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

the-mikedavis Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

the-mikedavis Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

GNUSheep commented Sep 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

GNUSheep commented Aug 7, 2024 •

edited by the-mikedavis

Loading

GNUSheep commented Sep 20, 2024 •

edited

Loading

rockboynton commented Jan 12, 2025 •

edited

Loading

GNUSheep commented Jan 15, 2025 •

edited

Loading

GNUSheep commented Feb 24, 2025 •

edited

Loading