feat(scrolling): sync neovim scroll with vscode

[zbw8388]

See #983 for more discussion.
This PR takes the first approach in that issue to fix content scrolling. Creating this PR to link issue and branch together.

Tracking:
Legend: not planned

• synchronize viewport as soon as a buffer is created
• (good to have) when executing lua, specify winId as currently the message is not passed on instantly
• (good to have) merge set cursor with the lua as winrestview natively support them
• fix mouse wheel scrolling
• support folding
• fix quick-preview induced buffer height change
• tests

[theol0403]

Thank you so much for continuing to work on this!! I've found a few issues with the WIP, but I'm sure you are aware and are continuing to work at it.

Please let me know if there is anything you want me to test! I'll let you work on this and focus on some other tasks for now.

[zbw8388]

@theol0403 I think this is mostly stable if people don't use code folding as that would mess up height computation. I made it so that mouse related events (clicking, scrolling by mouse wheel, scrolling by clicking on scrollbar, scrolling by dragging minimap) to be as close to vanilla vscode experience as possible. I'll be using it at work next week for discovering bugs. It'll be great if you can try it out as well.
I also hope people with mac can help testing it out as this extension seem to have issues on those machines.

[zbw8388]

Update on folding:

I created a new branch dealing with code folding: https://github.com/vscode-neovim/vscode-neovim/tree/experimental-scroll-fold
using techniques found in https://github.com/kevinhwang91/nvim-ufo/blob/56a8538d65ed69657b7a4d4163fc6cf87cbcf192/lua/ufo/fold/driver.lua
This lets vscode handle all folding related stuff and sync folded lines (basically the idea from #58)
Note that I chose to let vscode handle it as it is dependent on language syntax, and we should allow only one thing handle syntax

---

However, this branch suffers from multiple issues:

• Cannot process global folding (meaning C-d cannot correctly detect folded region if it is outside of current viewport)
• Costly updates as every syncing of viewport requires a window update
• Need to handle highlighting etc.

Possible fixes:

• Check if folding has changed for the current document every time visible region changes
  • Might be costly
  • Need to skip folded regions for highlighting
• Stop passing folded region to neovim and let vscode handle possible fold region collision
  • Will break neovim jumplist but we're not using it anyways
  • Will break highlighting (what if lightspeed labeled something in the folded region?)

---

Status: cannot implement the first solution due to upstream issue: microsoft/vscode#81498. Guess we will not support folding until this is resolved.

[theol0403]

@zbw8388 these updates look very promising!
It's too bad about the folding, but I think for now its a feature that we can't support. Btw, have you seen #502? Also, VSCodeVim seems to have figured out a solution to folds, maybe you can look at that (VSCodeVim/Vim#1552). Chances are we are talking about totally different issues though.

I've been trying out your PR.
If I disable vscode -> neovim scrolling, scrolling (in neovim) works very well! C-u/C-d, zt/zb/zz, j/k, lightspeed, etc work perfectly, and scrolling inside vscode feels native.

However, I've been experiencing problems with two-way sync enabled.
Essentially, any scrolling motion results in the viewport jumping around sporadically, almost like it is being interrupted mid-scroll.

Vscode -> neovim disabled:

https://i.imgur.com/NRoujfJ.gif

Vscode -> neovim enabled:

https://i.imgur.com/Twe9Lhy.gif

https://i.imgur.com/yK5T4aK.gif

Let me know if you want any help reproducing this!

[theol0403]

I've found that increasing the 20ms delay to 100ms almost fixes the problem for me. Perhaps this is a workaround that can be removed?

Edit:
With a timeout of 200ms, if I double-press C-u, I still get the jumpy viewport (which settles in the wrong spot).

[zbw8388]

I've found that increasing the 20ms delay to 100ms almost fixes the problem for me. Perhaps this is a workaround that can be removed?

Edit:
With a timeout of 200ms, if I double-press C-u, I still get the jumpy viewport (which settles in the wrong spot).

Yeah I can reproduce this scrolling bug. I always had smoothScrolling disabled so I did not notice it when I was working on it. It's kinda stupid: VSCode would issue onDidChangeTextEditorVisibleRanges during the animation, and due to microsoft/vscode#84351, we don't really know if we need to deal with it or not. A simple fix would be finding out the animation time hidden in their repo and replace viewport.ts:324 with an this.vscodeScrollingLock = new Promise((resolve) => setTimeout(resolve, ...)), which asks vscode-side code to ignore requests by onDidChangeTextEditorVisibleRanges for at least that amount of time when it is scrolling.

I also imagine other issues will pop up due to this weird behavior such as going to definition-induced scroll.

Btw, have you seen #502? Also, VSCodeVim seems to have figured out a solution to folds, maybe you can look at that (VSCodeVim/Vim#1552). Chances are we are talking about totally different issues though.

I read through VSCodeVim/Vim#1552. They use multiple chained vscode-side window scroll commands to fix this issue, which would probably also cause animation issue.
I think my other branch https://github.com/vscode-neovim/vscode-neovim/tree/experimental-scroll-fold would cover the solution in #502: only the same-screen folds would get taken care of. That might be a nice partial solution though. Doing some careful optimization we can also let neovim register all folds generated in the visibleRegions.

Another thing is that both solutions introduce vscode scrolling logic, which we would like to get rid of in the first place, so I think opting for them might be a bit extra.

---

I might not have time for at least this week to work on this. Feel free to work on it though! I also found a couple bugs that needs to get addressed:

• Checking (if vscode -> neovim message is necessary) should also get viewport height involved (will need some refactoring of code)
• Switching to a desynced tab requires ignore first neovim viewport request
• Removing <= 1 checks (good for when there's only one folded line on top)
• (might be good to have) Stopping neovim from echoing vscode's scrolling messages back
• ... or just sequence all messages but I haven't thought it through and might be an overkill

[theol0403]

Thanks for all the great info! Yeah, that is a bit dumb.

Unfortunately, there is still a bit of trouble with smooth scrolling w/ two-way (occasional jitter, although correct end pos), and scrolling with the mouse is still sometimes jittery even with 200ms delay (the momentum gets stopped).

Requiring no smooth scrolling might be a concevable compromise.

All good about being busy! You've already done so much. I'll see what I can improve when I have a chance, but the techincal bits feel a bit beyond me currently. No rush to get this shipped. I might even release master as-is, because the only bug is lightspeed being broken immediately after C-u/C-d.

[zbw8388]

Try the latest commit and see if it solves the issue :) Maybe it'd be better to look at config prior to changing setting that timeout (but who would scroll by mouse in 126ms after clicking on their keyboard?)

I would be a bit hesitant to release master now, as it involves (I think) a major change to user experience, and we are expecting another change once this PR is ready. Maybe one way to do it would be excluding 29233d5 for releasing until we are fixed on how people scroll stuff

[theol0403]

That seems to fix the issue with C-u/C-d! I'm still running into the momentum clamping with mouse scroll, but it is better.

Sounds good about the release!

[theol0403]

I'm doing these tests on my work computer using windows.

It's somewhat irregular to produce, but here are the logs and screenshot:

https://pastebin.com/WRPnEPgU

https://i.imgur.com/6H8m7DC.gif

I also noticed that it happens more regularly when I scroll down beyond last line.

Edit: sorry, saw you wanted some extra event logs too. I'm not sure how to log these, it crashes when I try to json stringify and it contains a lot of data.

Are your latest commits ideal? It seems to make a lot of use of the magic 125 number, and seems to wait a lot. Would directly debouncing/timeout the incoming vsocde scroll position be a cleaner solution? For me it required less code.

[theol0403]

Here we are, events using util.inspect:

https://i.imgur.com/8Er5QA3.gif

https://pastebin.com/Y0856qzP

[zbw8388]

Thank you for the info!

Are your latest commits ideal? It seems to make a lot of use of the magic 125 number, and seems to wait a lot. Would directly debouncing/timeout the incoming vsocde scroll position be a cleaner solution? For me it required less code.

This 125 is vscode's default scrolling duration as you can see below. For some reason, the smooth scrolling duration for your vscode is a tad bit longer than 125ms: 677 - 530 = 147ms for the first scroll you made, which causes the "bouncy" behavior at your end. This should also explain your issue.

https://github.com/microsoft/vscode/blob/125ae4af2999c13096d8c1bb9d188b4c6e24bd0d/src/vs/base/browser/ui/list/listView.ts#L350
https://github.com/microsoft/vscode/blob/380ad48e3240676b48d96343f8ad565d4fea8063/src/vs/editor/common/viewLayout/viewLayout.ts#L16

I looked around and found that there's a way to stop autocommand, which might be the most ideal solution: https://neovim.io/doc/user/autocmd.html#:noautocmd
I think once we implemented this we then don't need any of the 125 and we could use debounce instead.

[wenfangdu]

@zbw8388 @theol0403
I found this PR through this comment, seems like it's causing workbench.action.compareEditor.nextChange and workbench.action.compareEditor.previousChange not working properly (instead of jumping to the change, the viewport twitches), v0.0.90 has this issue, v0.0.89 does not, I suppose this got merged into master in v0.0.90?

I use these two shortcuts all the time when comparing diffs, so it was easy for me to spot:

[theol0403]

@wenfangdu this PR has not been merged yet. I was referring to #885 and #919. Either way, nothing merged in master should affect those commands.

[wenfangdu]

@theol0403 I did a commit bisect, the code from #919 caused that issue.

[theol0403]

@zbw8388
Hey, how's it going? I assume you are busy (as I am), but do you know if you will have time to finish this up?

Currently the release is in a weird place because there were the first scrolling PRs that were merged but this one fixes them, so I can't release without this. I might make a hotfix release for the other PRs though.

This PR has a lot of potential so I would love to see it merged. If not, it will be a while until I can try to tackle it myself.

Thanks!

[zbw8388]

Hi! I'm sorry about the inactivity. I'm super busy these days but I always wanted to finish this PR. It's hard to find a large chunk of time currently. I remembered I was dealing with a VSCode API limitation last time I was working on this. I might be able to look at it in November but it depends. Please release a hotfix if this is currently blocking the development! At the same time, I'm wondering if it would be possible to revert and split the merged scrolling modification commit into an individual branch so that normal development can still happen.

[theol0403]

@zbw8388 thanks for the reply! I totally understand, especially regarding finding a large chunk of time to work on it lol. This project is not the kind of project you can expect to make significant progress with two-hour chunks.

I've released all pending PRs and have put the commits holding up development back in master (sync viewport, delete c-u). I imagine I won't have time to make many more PRs until december so hopefully this lines up with this PR being complete.

I've gone ahead and merged master back into this PR, to reduce friction for you to get started when you have time. I modified a bit of code doing so, but if you don't like the changes I don't mind if you change it back.

[MohamedOsman1998]

hi guys, great work here, any updates on this?

[archilkarchava]

These look somewhat related – #1131 (comment)

[adueck]

Hey everyone don't want to pressure the awesome people working on this but I would LOVE to see this happen. 🙇‍♂️