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

feat(viewport): sync neovim scroll with vscode #1335

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

theol0403
Copy link
Member

@theol0403 theol0403 commented Jul 23, 2023

This continues the work of #993 and #885.

Will fix #1261.

Currently in a very rough draft. A lot of work is needed, including in reconsidering how change locks are handled. My visual selection PR went in a different direction than this, so I have to figure out what's best.

There are also a lot of edge cases to solve.

@theol0403 theol0403 changed the title fix(veiwport): sync neovim scroll with vscode fix(viewport): sync neovim scroll with vscode Jul 23, 2023
@theol0403 theol0403 marked this pull request as draft July 23, 2023 07:03
@theol0403 theol0403 changed the title fix(viewport): sync neovim scroll with vscode feat(viewport): sync neovim scroll with vscode Jul 25, 2023
@xiyaowong
Copy link
Collaborator

@theol0403 Hi, what other works need to do for this PR? The main issues I encounter in my daily use are related to viewport for now.

@theol0403
Copy link
Member Author

theol0403 commented Sep 4, 2023

@xiyaowong I agree this is one of the biggest areas of improvement (#1261). Unfortunately this PR is very much pre-alpha, I essentially copy/pasted the code from #993 and solved some merge conflicts. However, the way I evolved the cursor manager in 0.4.0 took a different sync approach than this PR uses so I will have to refactor this PR to be compatible with that. Once the PR is properly written, there will undoubtedly be a large bug fixing session.

As I am about to return to an extremely busy year of class, I don't know when I will have time to work on this - you are welcome to take a stab if you want.

1st priority for me atm, other than starting the school year (am I also president of a robotics design team), is to create a CONTRIBUTING.md file with all the information I've learned, which will make it much easier to find new contributors.

@xiyaowong
Copy link
Collaborator

@theol0403 Today I looked into the synchronization issue with the viewport. A simple try: https://github.com/xiyaowong/vscode-neovim/tree/fix/viewport

The main problem is that scrolling in VSCode is not based on lines, so the obtained visible range is not accurate and usually smaller than the actual range. This requires manually adding compensation when synchronizing the vim viewport, hence the vim viewport is generally larger.

In this case, when using native vim commands for jumping or scrolling, the following situation occurs:

vim cursor position changes -> sync VSCode cursor position -> VSCode viewport changes -> sync vim viewport -> vim cursor position changes

This leads to jitter.

@theol0403
Copy link
Member Author

@xiyaowong the solution must be to let neovim handle all scrolling and denounce vscode scrolling

@xiyaowong
Copy link
Collaborator

@theol0403 I’m not sure what you mean by "let nvim handle all scrolling." I think what you're referring to is bidirectional synchronization, which would be the ideal situation. We are using VSCode, and it should always be the primary role with nvim as a secondary one. Besides, how's it possible to make VSCode not handle scrolling?

@theol0403
Copy link
Member Author

@theol0403 I’m not sure what you mean by "let nvim handle all scrolling." I think what you're referring to is bidirectional synchronization, which would be the ideal situation. We are using VSCode, and it should always be the primary role with nvim as a secondary one. Besides, how's it possible to make VSCode not handle scrolling?

I mean that nvim is given priority for all events that change scroll position. When moving cursor, using scrolling commands, etc, nvim should control the vscode scroll (keep in mind that moving cursor does not create scroll event in vscode). However, in the case that the scroll is generated from vscode (mouse wheel, go to definition), it should be denounced for when there are no nvim updates.

Basically, nvim should be primary role, just like cursor sync.

@theol0403
Copy link
Member Author

@xiyaowong I see you're starting to work on viewport sync. Start with 929a8e1 (#1335), and earlier commits, as it deletes all code you won't be needing (especially triggering the viewport scroll from cursor move).

@xiyaowong
Copy link
Collaborator

Hmm. I have finished reading the relevant pr codes and related discussions. I might do it my own way first. The main synchronization logic should not be complicated, but many edge scenes should be considered.

@xiyaowong
Copy link
Collaborator

Made some updates. In short, there are too many situations that cause cursor jitter. Maybe now is not the time for an update, and we can wait for vscode to provide related APIs before optimizing: microsoft/vscode#195406.

@theol0403
Copy link
Member Author

theol0403 commented Oct 29, 2023

@xiyaowong I quickly tested your PR, and it does seem to work pretty well... is the jitter really that bad/unavoidable? What is the fundamental issue? I don't think the linked vscode issue will solve, that's for the vscode window dimensions, not the editor (and it can be years to wait for vscode fixes). Either there is never a solution, or we have to solve the current limitations.

@theol0403
Copy link
Member Author

Conceptually, I think no jitter is possible, if the cursor and viewport changes from nvim get applied atomically, and if the vscode scroll events get debounced before sending to nvim.

@xiyaowong
Copy link
Collaborator

xiyaowong commented Oct 29, 2023

We need to known the editor height, It is not accurate to adjust the viewport by adjusting the window height.
Try the following steps:
unmap zb G zt zb (issue occurs) ctrl+b ctrl+b ctrl+b (The height of the window is constantly adjusted until it is consistent with the height of the editor, then the number of rows per scroll returns to normal)

Some other problems, but mainly the above one.

  1. Use the mouse to scroll. Move the cursor out of the editor viewport and click.
  2. Scroll in visual mode.
  3. ...

@ZeChArtiahSaher
Copy link

ZeChArtiahSaher commented Apr 27, 2024

Eons after - will this ever work

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.

Issue Tracker: Sync viewport scroll
3 participants