-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Fix code editor scrolling experience on track pads #73502
Fix code editor scrolling experience on track pads #73502
Conversation
Reposting what I discussed on the Godot Contributors Chat: Rounding scrolling to lines is probably not desired behavior nowadays, even with a mouse wheel (and even without smooth scrolling). For example, VS Code doesn't round to lines when scrolling with the wheel, so I think we can get rid of the behavior entirely 🙂 |
7cf106d
to
07be795
Compare
Updated to get rid of line snapping. |
07be795
to
4dbe82b
Compare
Some tests were failing because scrolling to a caret position can now result in fractional scroll values. That actually feels really weird, so I fixed it by rounding the scroll values in The One test had to be changed since checking for equality of two doubles isn't working there because of a rounding error. |
Any update on this one ? Without it the editor really feels clunky with a touchpad. |
@KeatsPeeks Currently just waiting for review I guess, PR should be good to go. |
I tested this one on my Wayland branch (#57025) and this works fine even there! This PR really improves the code editing situation considerably on a track pad and the patch looks concise and clean. |
Looks fine from the input team's perspective, but we'd like some assessment from those more familiar with TextEdit or the expected behavior of trackpads. |
@Kurble I created an new pull request #82260 with your changes in it with the updates from the godot master branch. It seems this PR has conflicts. If you have the time to resolve the conflicts I think this PR could get pulled in soon. However, If not I have the new PR with your changes in it. I hope Im not stepping on any toes. I am new to doing these things and really would love to see this update pulled into Godot. |
@Kurble can you rebase your PR and fix the merge conflicts |
4dbe82b
to
6ffa9b0
Compare
Rebased on master and now the tests even pass without any changes! |
I suggest testing the pull request locally and make sure it works on your end. (I don't have a macOS device with a trackpad to test this PR.) |
Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
Built this from source on my Macbook Pro with M1 Max running MacOS Sonoma 14.0 and found no issues. I hope this gets merged soon! |
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.
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.
Tested again on top of the Wayland branch (#57025), which implements InputEventPanGesture
, and it still works great as last time!
Other than that I'll reiterate that the patch looks really nice and concise, great job!
Thanks! And congrats for your first merged Godot contribution 🎉 |
Fixes #28149
This PR fixes two closely related problems:
The first problem is caused by the step size that's set on the
VScrollBar
, any scroll event going throughTextEdit::_scroll_up
andTextEdit::_scroll_down
got rounded down to an integer number of lines. Since scrolling on a track pad happens every frame, the individual deltas are relatively small, and they would be rounded down to zero unless you're scrolling quite fast. This is addressed differently for the smooth scrolling enabled and disabled case:The second problem becomes quickly apparent when the first problem is fixed: scroll delta's less than 1 line are applied immediately, while scroll delta's larger than 1 line are animated. This feels really snappy as long as the target scroll value is less than 1 line away from the current scroll value, only to suddenly change to a fixed speed when the delta becomes too big. This has been addressed by directly applying any scroll delta coming from
InputEventPanGesture
events, instead of allowing it to animate.