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

Scroll sensitivity #1689

Merged
merged 13 commits into from
Jan 30, 2023
Merged

Scroll sensitivity #1689

merged 13 commits into from
Jan 30, 2023

Conversation

willmcgugan
Copy link
Collaborator

  • Adds scroll_sensitivity_x and scroll_sensitivity_y to change the number of lines to move the scroll position by when using the scroll wheel and trackpad
  • Adds horizontal scrolling, with shift or ctrl + scroll wheel. Horizontal scrolling uses shift + mouse wheel on macOS, but for some reason ITerm won't send mouse wheel events when shift is pressed. Kitty does. iTerm bug? Don't know, but for now both ctrl and shift will scroll horizontally.

I've doubled the scroll speed which makes the wheel somewhat usable, and the trackpad remains usable if a little faster.

There is no way to know which is being used, which means we need to find settings that works with both.

Please check this out and play with scrolling. I'm concerned that it may work for me, and break others.

@@ -369,6 +369,11 @@ def __init__(
self.css_path = css_paths
self._registry: WeakSet[DOMNode] = WeakSet()

self.scroll_sensitivity_x: float = 4
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Scrolling in x dimension is double to make the perceived distance the same in both directions (given cells are typically twice as tall as they are wide).

Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao Jan 30, 2023

Choose a reason for hiding this comment

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

Wouldn't that make for a good # comment in the original source?

src/textual/app.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
if self.scroll_up(animate=False):
event.stop()
def _on_mouse_scroll_down(self, event: events.MouseScrollDown) -> None:
if event.ctrl or event.shift:
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to steal both control and shift here? I forget which one is usually used for this kind of behaviour.

If we steal both, users wouldn't be able to do something like "hold shift and use mouse wheel to zoom instead of scroll", would they?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally it would be shift+mouse wheel only, but the problem is that iTerm doesn't report shift when pressed with the mouse wheel. Kitty does. I added both shift and ctrl so there is at least one way of horizontal scrolling.

Copy link
Contributor

@davep davep left a comment

Choose a reason for hiding this comment

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

Pease check this out and play with scrolling. I'm concerned that it may work for me, and break others.

Tested on macOS with both a Magic Trackpad and a Logitech M575 trackball. In this version scrolling with the trackpad is obviously faster, but I find it just as comfortable to control; starting and stopping and getting to where I want to be vertically feels about the same (testing with the Textual demo). With the scroll wheel on the M575, while still a bit of a snooze-fest when it comes to scrolling, it's obviously far better than before this change (I had two windows open side-by-side, without and with this change, to compare).

Hopefully this'll be the bedrock for a method of letting people configure it "just so", in which case I see this as really handy and, from my experience, it doesn't impact my current use of Textual in iTerm on macOS.

CHANGELOG.md Outdated Show resolved Hide resolved
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.

4 participants