Updated the implementation for hscroll area #56

maha-sachin · 2024-04-04T18:29:08Z

@panglesd @Julow , I've encountered a bug in the scroll area implementation and would appreciate your guidance in resolving it.

When using a vscroll area inside hscroll area, I've noticed that the horizontal scrolling behaves correctly with the left and right keys. However, there's an issue with vertical scrolling – when using the up and down keys, the behavior is unexpected, and vice versa.

And, Currently, we're planning to implement vertical scrolling using the j and k keys.
I propose extending this functionality to include horizontal scrolling as well, by introducing the v and b keys for scrolling left and right, respectively. With these additional keys, users would have a more comprehensive navigation experience, allowing them to scroll both vertically and horizontally with ease.

vertical scroll:
| `ASCII 'j', [] -> scroll state (-scroll_step)  
| `ASCII 'k', [] -> scroll state (+scroll_step)

horizontal scroll:
| `ASCII 'v', [] -> scroll state (-scroll_step)  
| `ASCII 'b', [] -> scroll state (+scroll_step)

What are your thoughts on incorporating horizontal scroll keys v and b? I am looking forward to hearing your feedback.
Thank you very much for your assistance and support in tackling this issue.

Julow

Nice work! I'm also not sure why the horizontal scroll misbehave. Could you attempt at debugging it ?
I've opened several threads for separate discussion.

Julow · 2024-04-05T08:54:37Z

vendor/lwd/lib/nottui/nottui_widgets.ml

+  | `Arrow `Right, [] -> scroll state (+scroll_step) 
+  | _ -> `Unhandled
+  in
+  (* let scroll_handler state ~x:_ ~y:_ = function


Unfortunately, no scroll handler is possible, you can remove the comment entirely.

Julow · 2024-04-05T08:56:45Z

lib/interactive_viewer.ml

+            ~change:change_scroll_state_h
+          @@ W.vscroll_area
+               ~state:(Lwd.get curr_scroll_state)
+               ~change:change_scroll_state
          @@ current_hunks z_patches;
          Lwd.pure
          @@ Ui.keyboard_area


Could you add a test ? A good test would use the recent cram test and would add a new diff file dedicated to testing scroll. The test would use the sequences of keys that you found to misbehave during your testing.

Ok sure @Julow, Thank you for your suggestion to add a test for the scrolling functionality. I'll work on adding a new cram test

Julow · 2024-04-05T08:57:32Z

lib/interactive_viewer.ml

  let change_scroll_state _action state =
    let off_screen = state.W.position > state.W.bound in
    if off_screen then
      Lwd.set curr_scroll_state { state with position = state.W.bound }
    else Lwd.set curr_scroll_state state
  in
+  let change_scroll_state_h _action state =


The existing scroll related function and variable could be renamed to have _v to make things clear.

The existing scroll related function and variable could be renamed to have _v to make things clear.

I agree that clear and descriptive names are essential for maintaining readability.

Julow · 2024-04-05T09:00:53Z

lib/interactive_viewer.ml

+            ~change:change_scroll_state_h
+          @@ W.vscroll_area
+               ~state:(Lwd.get curr_scroll_state)
+               ~change:change_scroll_state
          @@ current_hunks z_patches;
          Lwd.pure
          @@ Ui.keyboard_area


What are your thoughts on incorporating horizontal scroll keys v and b?

Key bindings would be a great addition ! In what other tools are v and b used for scrolling ? As we plan to have j and k for vertical scrolling, I'd suggest h and l to be consistent in the vim-like keybindings.

Hi @Julow, I understand that we currently use h for opening the help panel. we may need to explore alternative options for incorporating horizontal scrolling keys while ensuring consistency and usability in our application. What are your thoughts on this?

Good catch! Then v and b seem like good options.

Julow · 2024-04-05T09:48:56Z

lib/interactive_viewer.ml

+            ~change:change_scroll_state_h
+          @@ W.vscroll_area
+               ~state:(Lwd.get curr_scroll_state)
+               ~change:change_scroll_state
          @@ current_hunks z_patches;
          Lwd.pure
          @@ Ui.keyboard_area


When using a vscroll area inside hscroll area, I've noticed that the horizontal scrolling behaves correctly with the left and right keys. However, there's an issue with vertical scrolling – when using the up and down keys, the behavior is unexpected, and vice versa.

Can you describe the unexpected behavior and how to get to it ?

I notice one unexpected behavior:
On a hunk that is big enough to scroll both vertically and horizontally, the right key has no effect when pressed but when pressing the down key, update both the vertical and horizontal scroll to the expected position.

I'm not sure why this happens by reading the code. Could you try printing the values of the states. Here's a setup that can work with two terminals: In one terminal run the app with dune exec -- diffcessible example.diff 2> stderr, in the second terminal read this file with tail -f stderr. You can then interact with the app and see debug printing at the same time. Make sure to print to stderr, for example with Printf.eprintf.
Good luck!

Can you describe the unexpected behavior and how to get to it ?

I notice one unexpected behavior: On a hunk that is big enough to scroll both vertically and horizontally, the right key has no effect when pressed but when pressing the down key, update both the vertical and horizontal scroll to the expected position.

I'm not sure why this happens by reading the code. Could you try printing the values of the states. Here's a setup that can work with two terminals: In one terminal run the app with dune exec -- diffcessible example.diff 2> stderr, in the second terminal read this file with tail -f stderr. You can then interact with the app and see debug printing at the same time. Make sure to print to stderr, for example with Printf.eprintf. Good luck!

I'll investigate further and try to understand why the right key is not having any effect, I'll set up the suggested environment with two terminals as you described.
Once I have more information from the debug prints, I'll analyze the behavior and work on resolving the issue accordingly.

Thanks for your guidance, and I'll keep you updated on my progress!

Julow reviewed Apr 5, 2024

View reviewed changes

Updated the implementation for hscroll area

951efa3

maha-sachin force-pushed the 45-Horizontal-scrolling-1 branch from 3f07f34 to 951efa3 Compare April 5, 2024 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated the implementation for hscroll area #56

Updated the implementation for hscroll area #56

maha-sachin commented Apr 4, 2024 •

edited

Loading

Julow left a comment

Julow Apr 5, 2024

Julow Apr 5, 2024

maha-sachin Apr 5, 2024

Julow Apr 5, 2024

maha-sachin Apr 5, 2024

Julow Apr 5, 2024

maha-sachin Apr 5, 2024

Julow Apr 5, 2024

Julow Apr 5, 2024

maha-sachin Apr 5, 2024

Updated the implementation for hscroll area #56

Are you sure you want to change the base?

Updated the implementation for hscroll area #56

Conversation

maha-sachin commented Apr 4, 2024 • edited Loading

Julow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maha-sachin commented Apr 4, 2024 •

edited

Loading