Skip to content
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.

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

Horizontal scrolling #45

Open
panglesd opened this issue Mar 18, 2024 · 11 comments
Open

Horizontal scrolling #45

panglesd opened this issue Mar 18, 2024 · 11 comments
Assignees

Comments

@panglesd
Copy link
Owner

Vertical scrolling was added in #36, but long lines overflows and the content at the end is not visible.

This intermediate issue is about adding the ability to scroll horizontally.

When assigned this issue, do not hesitate to ask for more information, or help if you are blocked!

@maha-sachin
Copy link
Contributor

@panglesd, I am Maha interested in working on #15 or this one, can you help me assign an issue?

@panglesd
Copy link
Owner Author

You have already completed a good first issue, so we will assign you this one.

@maha-sachin
Copy link
Contributor

maha-sachin commented Mar 20, 2024

@panglesd @Julow, I hope you're doing well. I've been reviewing the code related to issue #36, which focuses on vertical scrolling, and I have a question about expanding this functionality to include horizontal scrolling.

1.I noticed that we have a vscroll_area function for vertical scrolling, but I couldn't find a corresponding function for horizontal scrolling. Is there a reason why a similar function for horizontal scrolling is not available? Could you provide some insights or considerations regarding the implementation of a horizontal scroll area?

Your insights on how to integrate horizontal scrolling would be greatly appreciated. Thank you for your time and assistance.

@panglesd
Copy link
Owner Author

You did well to ask for assistance, this issue is not very easy to start!

I can answer with more details later, but here is a brief first message. Come back for more guidance if you are blocked again!

Indeed, there is no hscroll_area in nottui. But the good news is: we vendor this library, so we can easily modify it!
So I think the first stage of resolving this issue would be to implement hscroll_area, taking inspiration from the vscroll_area function, which is already implemented. (We will see later if upstreaming is desirable.)

@maha-sachin
Copy link
Contributor

Hi @panglesd @Julow , I've encountered an issue that I'm having difficulty resolving.

File "vendor/lwd/lib/nottui/nottui_widgets.ml", line 240, characters 21-43:
240 | |> Ui.mouse_area (scroll_handler state)
                             ^^^^^^^^^^^^^^^^^^^^^^
Error: This expression has type
       x:int ->
       y:int ->
       [> `Scroll of [> `Right ] | `scroll of [> `Left ] ] ->
       [> `Handled | `Unhandled ]
     but an expression was expected of type
       Ui.mouse_handler =
         x:int ->
         y:int ->
         Unescape.button ->
         [ `Grab of (x:int -> y:int -> unit) * (x:int -> y:int -> unit)
         | `Handled
         | `Unhandled ]
     Type [> `Scroll of [> `Right ] | `scroll of [> `Left ] ]
     is not compatible with type
       Unescape.button =
         [ `Left | `Middle | `Right | `Scroll of [ `Down | `Up ] ]
     The second variant type does not allow tag(s) `scroll

I'm unsure how to adjust my implementation to align with the expected type.
but I tried to change :
Mouse_handler;
It seems that the scroll_handler function I've defined is not compatible with the expected type for Ui.mouse_handler. The mouse_area function in Nottui expects a handler of type Ui.mouse_handler

type mouse_handler = x:int -> y:int -> Unescape.button -> [
      | may_handle
      | `Grab of (x:int -> y:int -> unit) * (x:int -> y:int -> unit)
    ]

Here's the issue I'm facing: Unescape module in the Nottui library, which relies on the Notty library for mouse handling functionalities. However, I've encountered a compatibility issue that I'm struggling to resolve).

The Unescape module defines a type button that represents various mouse buttons, including scrolling directions (Up and Down). However, I need to extend this type to include left and right scrolling directions (Left and Right).

The original definition of button is as follows:

type button = [ `Left | `Middle | `Right | `Scroll of [ `Up | `Down ] ]

I would like to modify it to include left and right scrolling directions -
ex:

type button = [ `Left | `Middle | `Right | `Scroll of [ `Up | `Down | `Right | `Left ] ]

@panglesd and @Julow, Could you please provide guidance on how to make this modification to the Unescape module? I'm unsure about the appropriate steps to take and how to ensure compatibility with other parts of the codebase.

Screenshot 2024-03-22 at 11 29 48 AM

I've tried my best to explain the situation, but if there are any shortcomings in my explanation, I apologize. Your guidance and expertise in this matter would be greatly appreciated

@maha-sachin
Copy link
Contributor

Hi @Julow , @panglesd ,I'm reaching out to seek your guidance on #45. Could you please spare some time to provide me with direction on how to proceed? Any advice, resources, or pointers you can offer would be greatly appreciated.

@Julow
Copy link
Collaborator

Julow commented Mar 25, 2024

Hi! Sorry for the slow reply.
It's indeed not possible to represent horizontal mouse wheel scrolling due to this type. It could be a nice contribution to fix that in the future but we can workaround this by not implementing horizontal scrolling with the mouse for now.

Keyboard horizontal scrolling would be a good start. Then mouse scrolling by dragging on the scrollbar should be added, as it's also supported for vertical scrolling. (edit: the scrollbar is not implemented by vscroll_area, let's not implement it now)

@maha-sachin
Copy link
Contributor

Hi! Sorry for the slow reply. It's indeed not possible to represent horizontal mouse wheel scrolling due to this type. It could be a nice contribution to fix that in the future but we can workaround this by not implementing horizontal scrolling with the mouse for now.

Keyboard horizontal scrolling would be a good start. Then mouse scrolling by dragging on the scrollbar should be added, as it's also supported for vertical scrolling. (edit: the scrollbar is not implemented by vscroll_area, let's not implement it now)

@Julow , Thank you for providing these suggestions. I'll start working on implementing keyboard horizontal scrolling. I'll keep you updated on my progress.

@maha-sachin
Copy link
Contributor

Hi! Sorry for the slow reply. It's indeed not possible to represent horizontal mouse wheel scrolling due to this type. It could be a nice contribution to fix that in the future but we can workaround this by not implementing horizontal scrolling with the mouse for now.
Keyboard horizontal scrolling would be a good start. Then mouse scrolling by dragging on the scrollbar should be added, as it's also supported for vertical scrolling. (edit: the scrollbar is not implemented by vscroll_area, let's not implement it now)

@Julow, Thank you for providing these suggestions. I'll start working on implementing keyboard horizontal scrolling. I'll keep you updated on my progress.

@Julow, I'd like to get your input on the approach I'm considering.

My plan involves adjusting the scroll_state type to include additional fields such as x_position, x_bound, and x_visible. With these additions, users would be able to navigate horizontally within the vscroll_area using keyboard input.
Before proceeding further, I just wanted to confirm if this approach is what you would like.

Screenshot 2024-03-26 at 2 43 39 AM

@panglesd
Copy link
Owner Author

Hello @maha-sachin !

Your approach would definitely work to have a single scroll state for both horizontal and vertical scrolling.

However, there are some issues with this approach:

  • When you only want vertical scrolling, you still have the y_* fields for the scroll state,
  • 4 fields are duplicated, which suggests the type is actually a product of two types... which happen to be the original scroll_state! (in other words: your scroll_state contains exactly the same info as {horizontal : scroll_state ; vertical : scroll_state }, but the latter is more modular)

My suggestion would be to start by implementing hscroll_area which only deals with horizontal scrolling. Horizontal + vertical scrolling would be achieved simply by using both hscroll_area and vscroll_area. The scroll_state type does not need to be modified: the same can be used for both direction, nothing in it assumes that the scroll is vertical or horizontal!

@maha-sachin
Copy link
Contributor

Hello @maha-sachin !

Your approach would definitely work to have a single scroll state for both horizontal and vertical scrolling.

However, there are some issues with this approach:

  • When you only want vertical scrolling, you still have the y_* fields for the scroll state,
  • 4 fields are duplicated, which suggests the type is actually a product of two types... which happen to be the original scroll_state! (in other words: your scroll_state contains exactly the same info as {horizontal : scroll_state ; vertical : scroll_state }, but the latter is more modular)

My suggestion would be to start by implementing hscroll_area which only deals with horizontal scrolling. Horizontal + vertical scrolling would be achieved simply by using both hscroll_area and vscroll_area. The scroll_state type does not need to be modified: the same can be used for both directions, nothing in it assumes that the scroll is vertical or horizontal!

okay, thanks. I will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants