-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Better support for smooth scrolling trackpads and mice #7941
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
base: master
Are you sure you want to change the base?
Conversation
| // ticks based on the mouse x-position where the scroll wheel was used | ||
| int ticks = x / m_ppb; | ||
| // what would be the ticks in the new zoom level on the very same mouse x | ||
| int newTicks = x / (DEFAULT_PR_PPB * m_zoomLevels[z]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have to think about "what would be the ticks in the new zoom level". We can just change it and evaluate x / m_ppb again.
|
|
||
| const float temp = m_dbRange; | ||
| const float dbRangeNew = m_dbRange - copysignf(COMP_GRID_SPACING, event->angleDelta().y()); | ||
| m_dbRange = round(qBound(COMP_GRID_SPACING, dbRangeNew, COMP_GRID_MAX) / COMP_GRID_SPACING) * COMP_GRID_SPACING; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since scroll.getSteps() returns an int I don't think we need to round to the nearest multiple of COMP_GRID_SPACING because the new value will always be a multiple of COMP_GRID_SPACING.
|
I tested this, and it works pretty great! OSI'm on Arch LInux with GNOME HardwareI'm using a touchpad built in to the laptop I use. From Testing
Things I noticed
|
|
Thanks for the thorough testing @regulus79
Good catch! I've fixed it now. It doesn't scroll linearly though, but it didn't before either and I'm not gonna fix that in this PR.
I'll check that off the TODO list
That was intentional... I think
This seems to be a QScrollbar thing. I tested holding shift while scrolling the project notes and it scrolled faster. Good to know...
Sorry, I tricked you. It should be Alt+Shift not Ctrl+Shift.
There's only a difference if the range is greater than 1000 steps, like the FREQ knob in Envelope tab.
There's not much to do, I'm afraid. If we slowed it down for you it would require two steps on a regular mouse wheel to move it one step.
Interesting find, I don't know if we should change it though. |
regulus79
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't do a thorough review, but I looked over the code and it looked pretty good!
include/AutomationEditor.h
Outdated
| static const int DEFAULT_Y_DELTA = 6; | ||
| static const int DEFAULT_STEPS_PER_BAR = 16; | ||
| static const int DEFAULT_PPB = 12 * DEFAULT_STEPS_PER_BAR; | ||
| static const int PIXELS_PER_SCROLL = 36; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to define this somewhere where all editors can access it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried to add it to the Editor base class, but realized that is the base class of PianoRollWindow, SongEditorWindow so that wouldn't work... Do you have any other suggestion? Should we define it in Scroll.h?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fine. It feels like it would make sense to have some sort of standard pixels-per-scroll value, if in the future people implement scrolling things which need to move consistently with the all the editors and stuff, or we ever want to add a setting for scroll sensitivity idk.
regulus79
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I have the mental energy to review the code 100% and understand exactly what every line is doing, but my past self did look though the code and thoroughly tested it (above), and it seems to work very well.
Given what JohannesLorenz mentioned on the discord, careful code review is not as high a priority for GUI changes, so I feel comfortable approving this.
|
We just need a macOS tester for the natural scrolling... @tresf where are you? |
I'll test and report my findings. |
|
macOS testing on trackpad:
The good:
The bad: (including a few observations that may be contributing to the failing of the third test)
macOS uses Command for zoom modifier and that works just fine however horizontal scrolling via modifier seems to be bound to Shift, not
The accuracy of this on trackpad is GREATLY improved and the trackpad works fine for this, but sometimes the controls are passed to the parent (such as the Song Editor) and it scrolls the track away from the cursor. I've uploaded a video of this to show the (somewhat) erratic the behavior is. lcd_scroll.mov
The accuracy of these on trackpad are GREATLY improved! Testing notes:
|
|
Just adding that pinch/zoom still does not work at all with this PR. I'm not sure if that's in scope or not. |
|
Thanks @tresf! Qt uses Alt to swap orientation on Windows and Linux. I had a vague memory that this wasn't the case on macOS so I coded an exception to disregard Alt on macOS. Maybe they've changed it? Could you test if you test if you can use Alt to swap orientation in the project notes or the mixer? (testing on any recent build will be fine) |
|
Notes to self
Glancing through the Qt source code for macOS I see no mention of an Alt modifier [1] like there is on Windows. [2]. To match that system behavior, this PR does not use Alt to swap orientation on macOS [3].
This is a thing with
Seems to be something macOS does on system level [5]. This complicates things quite a bit... I don't think there's much we can do about that. Questions to @tresf
|
|
And also, in which direction does "combo boxes" scroll in other apps on macOS? |
Just to focus on this one issue a bit... (Trackpad only)
alt_pressed.mov
Correct, with the trackpad, knobs and sliders are inverted with Alt (Option). This may be normal but I wanted to share.
Yeah it's just odd to have Shift do something different between knobs / sliders (faster) and scroll bars (invert). I feel the same way about Alt (Option) being used for both slow control as well as inversion modifier, but if no one else cares, I won't complain. With exception of pinch/zoom missing, navigation on a trackpad is so much better than a conventional mouse and this PR makes that even better. |
I tested this in GarageBand and Safari and macOS combo boxes don't respond at all to trackpad "scroll" events. |
I think you nailed it
Like X/Y swapped or up/down swapped? |
|
I'm testing scrolling in Windows 11 on my old laptop with a synaptics touchpad. These are my (not LMMS specific) findings:
I have developed a new hatred towards this issue. Edit: I heard from other Windows users that Alt works fine for them. Maybe all of these issues are just because I have an old touchpad with a sketchy driver. |
When Alt (Option) is used on the knobs, it slows down the sensitivity AND inverts, so the knobs can only be adjusted by swiping the wrong way (up/down won't adjust the knob, only left/right will). When Alt (Option) is used on the sliders, it doesn't seem to impact sensitivity but still inverts (up/down do nothing whereas left/right is needed to adjust the slider). Re-quoting:
|
|
Huh, well that makes things easier, then I can remove the macOS specific code |
|
Tested on Windows 11 on an ASUS Precision Touchpad and Logitech M170/M171 mice. Works fine, Shift and Alt changes the scroll direction to left/right. Diagonal scrolling also works fine |
|
Thank you to everyone who tested this! @tresf whenever you get around to it, make a final test that Since the problems seems to be so OS dependent, there are probably more edge cases to be found. But the current state of LMMS is so horrible on a trackpad / smooth scrolling mouse, so the sooner we get it merged the better. And then I'm sure those bug reports will drop in eventually :) GUI code is cheap to fix, no backwards compatibility to worry about. |
Knobs
Faders
I agree, this is all a vast improvement, I just have serious issues with dual function keybindings. All it takes is for a fader to be left/right and bindings like Shift start to break my brain. Furthemore, bindings such as Super + Scroll for Zoom otherwise break any future possibility for a decelerator key. (I understand that complaining about this is much less effort -- and less productive -- than helping choose a better shortcut system, but I have a fear that if we don't iron this out now, we'll be struggling with this for years to come) |
|
I'd also like to make a prominent note that Shift SLOWS the controls when the mouse is used but SPEEDS the controls when the trackpad is used, another head-scratcher. |
RainbowShatter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I acrually didn't looked at anything, but I love Smooth Scrolling!
Downloads are available here: https://lmms.io/download/pull-request/7941 |
|
We need to the bottom of how Qt receives WheelEvents on macOS before we can continue.
First of all, let's make sure we both use the same terminology. To make it easier (for me), let's call the keys by the names that are used in the codebase:
Secondly, I've hacked together a tool that will print out some debug info for WheelEvents on different widgets. Download the build and open Project notes. https://github.com/allejok96/lmms/actions/runs/16240581001#artifacts |
|
For the record, these are my findings on Linux with trackpad. Left/right scroll not supported 😢 (by my hardware) No modifiers, scrolling up
|
Wow. macOS support is starting to look sunny. ☀️ |
|
@allejok96 can you merge this with upstream, just to boot up Actions? |
Testing noteManjaro Linux
|

Continuation of #6700. Many of the those things have already been fixed in other PRs since then.
Main issue
Scrolling in LMMS has not been designed for high-resolution trackpads and mice. It either violently reacts on every tiny movement, or completely ignores it. This PR introduces a
Scrollclass used to calculate scroll values. It handles high-resolution scrolling, natural scrolling and modifier keys that swap scroll direction.Other small improvements
Changes
Remove Shift+scroll for horizontal scrolling in the editorsAlt+scroll already does the same thing and is built-in system-wide in Qt on Windows and Linux.This PR brings Alt+scroll (Option+scroll) to macOS too.Is this how macOS does it on other drop down menus?Things to test
Hey tester. There's a lot of widgets to test, I'm glad if you can help. If you have both a touchpad and mouse test if there's any difference. Here's some stuff you should look out for:
Widgets that have been edited and needs testing
Reported problems