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

fix(scrolling): fix c-u/c-y/c-f/c-b by deletion #885

Merged
merged 5 commits into from Jul 31, 2022
Merged

Conversation

theol0403
Copy link
Member

@theol0403 theol0403 commented Apr 28, 2022

Problem

Window scroll commands don't move the cursor and instead just scroll the window. This differs from nvim behavior. This is because the extension intercepts the keybindings and applies vscode commands instead. The solution from #528 and #855 simply improves the logic in the plugin to move the cursor. However, this adds complexity and increases the separation from nvim.

Solution

Remove all custom code for cursor scrolling and allow nvim to handle it itself. However, I have found two problems:

  • due to scrolloff=100, the scrolling commands behave strangely, and don't reach the top of the screen. c-d scrolls twice as far as it should. However, this can't be avoided according to
    // to not deal with screenrow positioning, we set height to high value and scrolloff to value / 2. so screenrow will be always constant
    . @justinmk can you think of another solution that will avoid the enforcing of scrolloff?
    set scrolloff=100
  • C-b behaves very strangely, even with scrolloff=0. Sometimes, it moves down a line. It never makes it to the top.

Fixes #293, #580, #528, and #855.

@theol0403
Copy link
Member Author

theol0403 commented Apr 29, 2022

I found an explanation for why neovim was not originally handling the scrolling. #361

Yep, unfortunately sending scroll commands from neovim produces jitter (at least it did last time when i've checked). It's possible to restore behaviour by disabling default ctrl+e/ctrl+y bindings and using vscode-neovim.send command instead and notifying back the extension from neovim.

I don't notice any jitter - maybe its safe to use this now?

Edit:

Maybe I'm wrong, this won't actually fix c-e and c-y.

@theol0403
Copy link
Member Author

This actually incorporates what I showed in #580, forgot about that.

I'll update with H/M/L. This has the same problem though, as it is affected with scrolloff and the non-synced viewport.

@justinmk
Copy link
Collaborator

justinmk commented Apr 30, 2022

// to not deal with screenrow positioning, we set height to high value and scrolloff to value / 2. so screenrow will be always constant

I'm missing context here, but I wonder if this hack was done before win_viewport UI event was added to Nvim?

@theol0403
Copy link
Member Author

I'm missing context here, but I wonder if this hack was done before win_viewport UI event was added to Nvim?

I'm also quite lacking in knowledge, but this seems related to #868 (comment) . I think the solution to one would be a solution to the other (as well as the solution to many other issues regarding viewport).

@justinmk
Copy link
Collaborator

justinmk commented Apr 30, 2022

If it's possible to remove the hacks while making some small tradeoffs (e.g. <c-b> is broken, but <c-u>/<c-d> work better), I'd lean towards that. And then figure out what is needed from Neovim (or VSCode?) to resolve the tradeoffs.

@theol0403
Copy link
Member Author

Yeah, I'd say all that we need to merge this is for the scrolloff hack to be removed.

fix scrolling commands by removing custom code and reverting to default nvim behavior
@theol0403 theol0403 merged commit 29233d5 into master Jul 31, 2022
@theol0403 theol0403 deleted the delete-c-u branch July 31, 2022 02:05
theol0403 added a commit that referenced this pull request Oct 12, 2022
abhiskk pushed a commit to abhiskk/vscode-neovim that referenced this pull request Nov 6, 2022
abhiskk pushed a commit to abhiskk/vscode-neovim that referenced this pull request Nov 6, 2022
abhiskk added a commit to abhiskk/vscode-neovim that referenced this pull request Nov 6, 2022
@trkoch
Copy link
Collaborator

trkoch commented Nov 15, 2022

I'm having trouble following. Can you bring us up to date what the status of this PR is?

@theol0403 theol0403 restored the delete-c-u branch June 25, 2023 07:38
@theol0403
Copy link
Member Author

I'm having trouble following. Can you bring us up to date what the status of this PR is?

Sorry. Here is what happened:

So, the current state is "waiting for #993, currently reverted".

@justinmk justinmk deleted the delete-c-u branch June 25, 2023 13:14
@theol0403 theol0403 restored the delete-c-u branch July 23, 2023 05:39
@theol0403 theol0403 deleted the delete-c-u branch July 23, 2023 05:39
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.

Page movements don't affect cursor
3 participants