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: allow vscode sync viewport with neovim #919

Merged
merged 12 commits into from Jul 31, 2022
Merged

feat: allow vscode sync viewport with neovim #919

merged 12 commits into from Jul 31, 2022

Conversation

windwp
Copy link
Contributor

@windwp windwp commented May 23, 2022

Problem:
Currently, the neovim viewport is always 200 and it uses scrolloff=100 to make the cursor always on center.

Solution:
this PR remove scrolloff=100 . It resize the height and scroll the neovim viewport.

PS: i use it for test.

vim.keymap.set('n', '<leader>ua', function()
    local line_start = vim.fn.line('w0')
    local line_end = vim.fn.line('w$')
    print('S:' .. line_start .. ' E:' .. line_end)
end)

@theol0403
Copy link
Member

theol0403 commented May 25, 2022

Thanks for this PR!! It seems to work really well and also makes #885 work great!

One thing I noticed is that when I do the lightspeed jump, it seems to consistently think the viewport is 1-1.5 lines smaller than it actually is: https://user-images.githubusercontent.com/16546293/170319389-f75da218-d496-46ea-ad13-229cb258d3ca.png

@theol0403
Copy link
Member

Also, could you try to remove

private NEOVIM_WIN_HEIGHT = 201;
and
neovimViewportHeight: 201,

It would be ideal if the nvim window could be created with the correct height.

@theol0403
Copy link
Member

I've found another bug with this PR: "jump to definition" or using the outline panel causes the viewport to jump to the new position and then instantly jump back.

remove neovimViewPortHeight
// );
// private commitScrollingFast = throttle(this.updateScreenRowFromScrolling, 200, { leading: false });
private onDidChangeVisibleRange = async (e: TextEditorVisibleRangesChangeEvent): Promise<void> => {
if (e.textEditor !== window.activeTextEditor || !this.modeManager.isNormalMode) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be expanded to also include visual mode? Why is this check needed?

@theol0403
Copy link
Member

Unfortnately I'm not sure if the new commit solved the problem with navigating in the outline. It has to trigger a scroll for the issue to appear.

Additionaly, would it be possible to rebase your commits on #775? I'm planning on merging it soon and it greatly simplifies the cursor setting logic. There is an obvious place to put this.scrollNeovim(editor.visibleRanges);. However, it doesn't solve the problem of being unable to jump using vscode commands (outline, jumplist, etc).

@windwp
Copy link
Contributor Author

windwp commented May 30, 2022

you can merge it to master and I will rebase from the master.

I click on the outline panel and it works.

@theol0403
Copy link
Member

I think this is a timing problem. For me the issue is not from updateCursorPosInEditor, the issue is from onDidChangeVisibleRange. I think what is happening is that both the onDidChangeVisibleRange and onSelectionChanged triggers at the same time, and then when scroll_viewport is called, it causes nvim to update vscode with the old cursor position.

For me it happens about 80% of the time, especially on big files and big jumps. It jumps to the new position and then instantly jumps back.

@theol0403
Copy link
Member

Pressing <C-o> which uses vscode jumplist to try to jump me to the top of the file:

@windwp
Copy link
Contributor Author

windwp commented May 31, 2022

i just fix it. sorry, i am not using vscode daily. thank for helping me test

@theol0403
Copy link
Member

Thanks! It seems to work for me now, I'll use it at work today and then merge if everything seems fine.

@theol0403
Copy link
Member

Sorry for the long delay, got busy with work. Unfortunately, the new changes are pretty buggy, but I'm not sure how to fully reproduce the issue.

Essentially, when I navigate around using lightspeed, and I jump to the botton of the file, and then jump in reverse, it doesn't make the whole screen grey, and I can't jump to the top ~5 lines. Moving the cursor does not fix this, only scrolling the viewport works.

@theol0403
Copy link
Member

It seems as though the test, shown in the PR description, works perfectly. However, it seems the problem is somehow caused by lightspeed.

theol0403 and others added 6 commits July 30, 2022 10:20
sometimes, vscode emits a range changed event that contains one line. This might be caused by vscode-neovim, not sure. This ignores it.
if you scroll a editor without having the cursor inside, this will make it update when you switch back
all new buffers are created with the same height, and then are synced later. It does not make sense to create all new buffers with the height of the first open editor
@theol0403
Copy link
Member

Thanks @windwp for the initial work! I think this works very well now.

@theol0403 theol0403 merged commit 3036a5a into vscode-neovim:master Jul 31, 2022
@windwp
Copy link
Contributor Author

windwp commented Aug 1, 2022

@theol0403 nice work 👍

@wenfangdu
Copy link
Contributor

@windwp @theol0403 Please take a look at #993 (comment), this PR is causing that issue.

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.

None yet

3 participants