Skip to content

Updated the implementation for hscroll area #56

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.

Sign up for GitHub

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

Draft
maha-sachin wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Updated the implementation for hscroll area #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions lib/interactive_viewer.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,12 +122,19 @@ let view (patches : Patch.t list) =
| None -> failwith "zipper_of_list: empty list"
in
let curr_scroll_state = Lwd.var W.default_scroll_state in
let curr_scroll_state_h = Lwd.var W.default_scroll_state in
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 =
Copy link
Collaborator

@Julow Julow Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@maha-sachin maha-sachin Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let off_screen = state.W.position > state.W.bound in
if off_screen then
Lwd.set curr_scroll_state_h { state with position = state.W.bound }
else Lwd.set curr_scroll_state_h state
in
let ui =
let$* help_visible = Lwd.get help in
if help_visible then
Expand All @@ -149,9 +156,19 @@ let view (patches : Patch.t list) =
operation_info z_patches;
change_summary z_patches;
current_operation z_patches;
W.vscroll_area
~state:(Lwd.get curr_scroll_state)
~change:change_scroll_state
(* W.vscroll_area
~state:(Lwd.get curr_scroll_state)
~change:change_scroll_state
@@ W.hscroll_area
~state:(Lwd.get curr_scroll_state_h)
~change:change_scroll_state_h
@@ current_hunks z_patches; *)
W.hscroll_area
~state:(Lwd.get curr_scroll_state_h)
~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
Copy link
Collaborator

@Julow Julow Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@maha-sachin maha-sachin Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@Julow Julow Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@maha-sachin maha-sachin Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@Julow Julow Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@Julow Julow Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@maha-sachin maha-sachin Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Expand Down
42 changes: 42 additions & 0 deletions vendor/lwd/lib/nottui/nottui_widgets.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,48 @@ let vscroll_area ~state ~change t =
|> Ui.mouse_area (scroll_handler state)
|> Ui.keyboard_area (focus_handler state)
end
let hscroll_area ~state ~change t =
let visible = ref (-1) in
let total = ref (-1) in
let scroll state delta =
let position = state.position + delta in
let position = clampi position ~min:0 ~max:state.bound in
if position <> state.position then
change `Action {state with position};
`Handled
in
let focus_handler state = function
| `Arrow `Left , [] -> scroll state (-scroll_step)
| `Arrow `Right, [] -> scroll state (+scroll_step)
| _ -> `Unhandled
in
(* let scroll_handler state ~x:_ ~y:_ = function
Copy link
Collaborator

@Julow Julow Apr 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| `Scroll `Up -> scroll state (0)
| `Scroll `Down -> scroll state (0)
| _ -> `Unhandled
in *)
Lwd.map2 t state ~f:begin fun t state ->
t
|> Ui.shift_area state.position 0
|> Ui.resize ~w:0 ~sw:1
|> Ui.size_sensor (fun ~w ~h:_ ->
let tchange =
if !total <> (Ui.layout_spec t).Ui.w
then (total := (Ui.layout_spec t).Ui.w; true)
else false
in
let vchange =
if !visible <> w
then (visible := w; true)
else false
in
if tchange || vchange then
change `Content {state with visible = !visible; total = !total;
bound = maxi 0 (!total - !visible); }
)
(* |> Ui.mouse_area (scroll_handler state) *)
|> Ui.keyboard_area (focus_handler state)
end

let scroll_area ?(offset=0,0) t =
let offset = Lwd.var offset in
Expand Down
4 changes: 4 additions & 0 deletions vendor/lwd/lib/nottui/nottui_widgets.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ val vscroll_area :
state:scroll_state Lwd.t ->
change:([> `Action | `Content ] -> scroll_state -> unit) ->
ui Lwd.t -> ui Lwd.t
val hscroll_area :
state:scroll_state Lwd.t ->
change:([> `Action | `Content ] -> scroll_state -> unit) ->
ui Lwd.t -> ui Lwd.t

val scroll_area :
?offset:int * int -> ui Lwd.t -> ui Lwd.t
Expand Down