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

Record origin position of scrollbars to improve grab mechanics #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ScoreUnder
Copy link
Contributor

@ScoreUnder ScoreUnder commented Oct 16, 2021

Test scrollbars with:

open Nottui

module W = Nottui_widgets

let () =
  W.string "Hello world"
  |> Ui.resize ~w:200 ~h:100
  |> Lwd.pure
  |> W.scrollbox
  |> W.h_pane @@ Lwd.pure Ui.empty
  |> W.v_pane @@ Lwd.pure Ui.empty
  |> Ui_loop.run

@c-cube
Copy link
Contributor

c-cube commented Oct 16, 2021

I'm not going to comment on the code, but the behavior on this little test is nice. Maybe there could be a directory of small examples, with this as scrollbox.exe?

Also, unrelated, but the colors are bad, it's nearly impossible to see the cursor with dark grey on light gray. I suggest:

let scrollbar_bg = Notty.A.gray 15
let scrollbar_fg = Notty.A.lightblue

@let-def
Copy link
Owner

let-def commented Oct 18, 2021

The behavior would be nice to have, but I am not fond of using permanent_sensor for this use-case.
It will essentially force the renderer to visit sub-trees and update every scrollbar if they move, while we are only interested in their movement if grabbing is on.

In practice, scrolling a list of scroll bars (ok, that does not happen every day) changes from being O(1) to O(n) (wrt to the number of scroll bars).

It would be more elegant to switch the sensor on only when grabbing the control.
I will write a patch later, and maybe rethink the sensor/grabbing mechanism.

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

Successfully merging this pull request may close these issues.

3 participants