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

Make slow scrolling possible on some operating systems #819

Closed
wants to merge 1 commit into from

Conversation

Luflosi
Copy link
Contributor

@Luflosi Luflosi commented Aug 22, 2018

On macOS the yoffset in scroll_event() in mouse.c can be as small as 0.1 (in my testing) when scrolling slowly enough. Since the code is multiplying yoffset with OPT(wheel_scroll_multiplier) and then rounding it to the nearest integer, the resulting integer can be equal to zero when scrolling slowly enough or when wheel_scroll_multiplier is set to a small value. This pull request attempts to fix this by storing the difference between the precise and the rounded value and adding it to the next yoffset. This makes it possible to scroll arbitrarily slowly in macOS.
I'm not sure if state.h is the right place to store yoffset_remaining, please let me know it it belongs somewhere else.
One possible issue I can think of with this pull request might be the following situation:
Suppose someone has a mouse with low scrolling resolution and the wheel_scroll_multiplier setting is set to 1. On every change of the scroll wheel kitty might get a yoffset of 1.5. The "old" behaviour will cause kitty to scroll two lines per scroll wheel change (I think) because the value will get rounded up. The "new" behaviour will cause kitty to alternate between scrolling by one line and scrolling by two lines. This motion might be perceived more irregular and unpredictable than before. It may be a good idea to make this behaviour optional or to detect the resolution of the mouse or touchpad somehow and change behaviour based on that.

@kovidgoyal
Copy link
Owner

Is there some actual use case for needing to scroll uber-slowly? Perhaps just use MAX(1, abs(round(yoffset * scroll_multiplier))? I generally like to avoid storing state as much as possible.

Also, wont this have odd effects when switching scroll direction? So if yoffset_remaining is -1 and the next scroll event is +1 there will be no scroll.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 23, 2018

This is not about scrolling uber-slowly, at least on my MacBook I actually have to scroll fairly fast to be able to scroll at all, adjusting the scrolling position to a specific line is not very easy and scrolling in kitty feels quite different than in most other applications. I'm afraid that using MAX(1, abs(round(yoffset * scroll_multiplier)) will cause kitty to scroll quite fast, even when scrolling very slowly, so I don't think that's a good solution.

And yes, when scrolling almost a line and then reversing scroll direction, one has to scroll almost a line back. This is however the behaviour I'd expect because in other applications that are not line based and allow pixel-precise scrolling, I also have to scroll the same amount back that I scrolled in the other direction. With this pull request kitty would behave pretty much the same except that the position would be rounded to the nearest line if that makes sense, sort of like a quantised version of smooth and precise scrolling. In most other applications I have a very good feeling for how I need to move my fingers across the touch pad to end up at a certain position but it's harder to predict where I will end up when scrolling in kitty. I guess not everybody prefers the new behaviour and I suggest adding a config option to switch between the two behaviours.
I found the following article about scrolling very interesting, if you have some time I highly recommend you to read it: https://pavelfatin.com/scrolling-with-pleasure/
I understand that you want to avoid storing state as much as possible but I think that the scroll position is quite important. What about removing yoffset_remaining and instead of storing what was left over from the rounding, storing the precise position by making scrolled_by a double and then rounding when we actually need to determine in which line we are? Would this complicate things or do you see other issues with this?

@kovidgoyal
Copy link
Owner

I think a better approach is to fix glfw to properly report high precision scroll events. Since on macOS scrollingDeltaX/y should be used as a pixel value when high precision events are available and otherwise as a line count, we should do the following:

  1. Modify glfw scrollcallback to take a third parameter named highPrecision
  2. On cocoa simply pass in the scrollingdeltaValues and the value of supports high presision
  3. On cocoa change the default for wheel scroll multiplier to 1
  4. On cocoa implement scrolling using the scrollingdelta values as recommended in https://developer.apple.com/documentation/appkit/nsevent/1535387-scrollingdeltay?language=objc
  5. On other platforms leave hte current logic unchanged.

@kovidgoyal
Copy link
Owner

kovidgoyal commented Aug 24, 2018

To elaborate on (4) above -- it would use your basic approach storing the pixel values and only scrolling by one line if they increase beyond the line height in pixels.

@kovidgoyal
Copy link
Owner

I have implemented it, but not tested it on macOS (only Wayland). Feel free to fine-tune as needed. In particular I'm not sure if retina screens need special handling in terms of the pixels to scroll by.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 26, 2018

I added a comment to 009ef54.

@Luflosi Luflosi deleted the better_scrolling branch August 26, 2018 16:48
@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 27, 2018

In that comment I recommended to change the line to global_state.callback_os_window->pending_scroll_pixels = pixels - s * (int)global_state.callback_os_window->fonts_data->cell_height; but you changed it to global_state.callback_os_window->pending_scroll_pixels = pixels - s * global_state.callback_os_window->fonts_data->cell_height;. Without the cast to int, the multiplication will be unsigned for some reason which causes the scrolling in the downward direction to be messed up.

@kovidgoyal
Copy link
Owner

I can never keep C's type promotion rules straight -- I have added the cast.

@Luflosi
Copy link
Contributor Author

Luflosi commented Aug 27, 2018

Works great now 👍

@dylan-chong
Copy link

@Luflosi fyi i have made an issue relating to this pull request #849

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.

None yet

3 participants