Fixes #2754. Ctrl+d/u pull cursor along when screen moves past cursor #3658
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jpoon Pretty sure this PR is ready for merge, assuming the tradeoffs are acceptable. Got time for a 👍/👎? 😄 |
jpoon
requested changes
Apr 22, 2019
| let newPosition; | ||
| let newPosition: Position; | ||
| if (!smoothScrolling) { | ||
| const newFirstLine = editor.visibleRanges[0].start.line; |
There was a problem hiding this comment.
Same as newFirstLine from before this commit (line 681 above). firstLine is the first line before the scroll, newFirstLine is the first line after the scroll.
|
Thanks for the review @jpoon - I believe everything should be fixed/addressed now. |
jpoon
approved these changes
Apr 30, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Current implementation of ctrl+d/ctrl+u does not allow scrolling past the cursor's original position when smooth scrolling is enabled (see #2754). With this PR, when ctrl+d/ctrl+u scroll the screen such that the cursor would be off-screen, the cursor is placed at the first/last line respectively rather than the screen snapping back to keep the cursor in view.
Which issue(s) this PR fixes
#2754
Special notes for your reviewer:
As I understand it, when VSCodeVim executes ctrl+d or ctrl+u, it checks the range of lines on the screen and then places the cursor at an offset (relative to the new first line) equal to the offset prior to scrolling. This is the ideal behavior normally, but when smooth scrolling is enabled the new range of lines is not available to the VSCodeVim command (due to scrolling happening asynchronously). It ends up accessing the original range of lines instead, which means the new position is calculated relative to (and identical to) the original position. This causes the cursor to not be moved and prevents the screen from scrolling any further than the cursor.
This change "fixes" the issue by using a built-in feature of VSCode's scrolling command. By setting
revealCursorto true in the scroll command VSCode will ensure the cursor gets "pulled along" when scrolling it off-screen, and telling VSCodeVim to use that new position ensures VSCodeVim doesn't reset it back.Caveat: This doesn't keep the proper vertical offset when scrolling, so isn't quite a "true" fix, especially as the functionality differs slightly between having smooth scrolling on and off (it as at least usable now, though).