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

Implemented smooth scrolling. #3616

Closed
wants to merge 6 commits into from

Conversation

PoetaKodu
Copy link

@PoetaKodu PoetaKodu commented Jan 26, 2022

Hello everyone.

I implemented smooth scrolling. I was annoyed that every time I scroll terminal in VS Code I lose track (see #1140 and microsoft/vscode#125950).
The option is disabled by default but can be enabled using terminal options.

Preview:
https://youtu.be/BCYdZwydmLw

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

@PoetaKodu very cool 👍 will try get a review done this week/early next week. The main thing I want to check out is how the settings work, trackpad vs wheel and how close the experience is to monaco.

@Tyriar Tyriar added this to the 4.17.0 milestone Jan 26, 2022
@Eugeny
Copy link
Member

Eugeny commented Jan 28, 2022

Tested and trackpad scrolling isn't affected by the new option 👍

@Tyriar Tyriar modified the milestones: 4.17.0, 4.18.0 Feb 3, 2022
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Looking into this now, my first impressions are:

  • The scroll speed is too slow, looks like monaco has a smooth scroll duration of 125ms and it feels a lot more responsive because of that
  • smoothScrollingStepInterval seems like we don't need it and it should instead be replaced by requestAnimationFrame
  • A duration would be preferable to smoothScrollingSpeed, so the pointer device tells the terminal how much to scroll and smooth scroll animates that over x milliseconds. If the animation happens based on a number of lines each frame then scrolling a large amount of lines seems like it would be a bad experience?

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Yeah the above is my main feedback, it would be better to move to duration-based scrolling and we should use requestAnimationFrame instead of setTimeout/setInterval. Interested in your thoughts, thanks!

@Tyriar Tyriar self-assigned this Feb 7, 2022
@meganrogge meganrogge modified the milestones: 4.18.0, 4.19.0 Feb 28, 2022
@Tyriar Tyriar removed this from the 4.19.0 milestone Mar 7, 2022
@Tyriar Tyriar marked this pull request as draft June 26, 2022 14:47
@Tyriar Tyriar modified the milestone: 4.20.0 Jul 27, 2022
@Tyriar
Copy link
Member

Tyriar commented Jul 27, 2022

Implemented the needed changes in #3940

@Tyriar Tyriar closed this Jul 27, 2022
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