Skip to content

Implemented smooth scrolling.#3616

Closed
PoetaKodu wants to merge 6 commits intoxtermjs:masterfrom
PoetaKodu:smooth-scrolling
Closed

Implemented smooth scrolling.#3616
PoetaKodu wants to merge 6 commits intoxtermjs:masterfrom
PoetaKodu:smooth-scrolling

Conversation

@PoetaKodu
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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