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

Scrollable #115

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Scrollable #115

wants to merge 6 commits into from

Conversation

alt-tosc
Copy link
Contributor

@alt-tosc alt-tosc commented Dec 2, 2020

Dear treeform,

This merge request is related to #108.

I would like to have your review on this proposed implementation.
All your feedbacks are welcome: I'm not used neither to Nim, nor to contribute to public open source projects on github.

Current changes summary:

  • From a user point of view: scrollable and scrollSpeed attributes added to Node, and corresponding setters.
  • It supports both vertical and horizontal scrolling.
  • It works with htmlbackend.
  • minimal_scroll example.

Todo:

  • Which kind of automated tests should I add? I think you work with automated screenshot comparison. For this feature it is a bit more complicated as there is mouse interaction and maybe one screenshot is not sufficient. Please tell me what you think :)

scrollable, scroll and scrollSpeed attributes addded to Node, and corresponding setters

minimal_scroll.nim added in examples
Several variables are now Vec2 instead of float (eg Mouse.wheelDelta), or tuple of boolean instead of boolean (eg Node.scrollable) to store both horizontal and vertical attributes
@treeform
Copy link
Owner

Hey, sorry I am currently working on fidget 2 - the complete rewrite. I don't know if I will be doing anything with fidget 1.

I want this to connect with scrollBars it seems like you went for a more complex system scroll system.

I think the speed of the scroll should depend OS not user setting? I don't know how to get that though.

@alt-tosc
Copy link
Contributor Author

Hello,
I'm curious and I can't wait for your new implementation to be published :)

I have a lot to say so I'll add numbers to ease replies:

  1. I want this to connect with scrollBars it seems like you went for a more complex system scroll system.

    I also tried to connect this to some scrollBars, I've considered the approach of scrollbars drawn as Fidget nodes.
    With this approach I see two advantages:

    • scrollBars are customizable
    • the rendering is uniform between desktop and browser.
  2. With what I propose in this PR I have mixed feelings about scrollSpeed which is set at the level of the child node:

    • on one side it allows to easily achieve some scroll effects like parallax scroll effect, probably also 'sticky' nodes
    • on the other side it is more difficult to add scrollBars as it is more difficult to know what is inside the parent screenBox and what is outside when the scrollBar is drawn: I have the impression the scrollbar should be drawn at the end of computeLayout which also seems a bad idea.
  3. I think the speed of the scroll should depend OS not user setting? I don't know how to get that though.

    I've just done a little experiment (I work on Gnome desktop): when I change scroll setting to "natural scroll", the change is reflected on the input given to the callback proc onScroll(window: staticglfw.Window, xoffset, yoffset: float64) set by window.setScrollCallback(onScroll). I don't have settings in Gnome to change scroll speed (and from a quick search it seems complicated to change on Linux desktops).

    It will probably be a good practice to not change the scrollSpeed inside Fidget and let by default scrollSpeed=1. But as mentioned earlier in point 2, scrollSpeed at the level of child node gives some interesting flexibility.

  4. In some applications (like in browsers) I have the feeling the scroll speed is not linear with respect to the 'wheel displacement'. Also scrolling does not stop immediately at the end the 'wheel displacement'. But I suppose this could be added in a further iteration.

Let me know if I can help, even If I'm not the most qualified for this, I will have some free time in the next few weeks ;)

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.

2 participants