Fix gj/gk so it maintains cursor position #3890
Merged
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
hetmankp
force-pushed
the
topic/fix-gj-gk
branch
from
64cb04b to
e14d23d
Compare
|
Can you add tests? |
|
Done. |
hetmankp
force-pushed
the
topic/fix-gj-gk
branch
from
00620d8 to
30ced67
Compare
jpoon
reviewed
Jul 29, 2019
hetmankp
force-pushed
the
topic/fix-gj-gk
branch
from
30ced67 to
53f713a
Compare
|
Is something blocking this PR? I'd really like to see to get this merged :) |
J-Fields
reviewed
Sep 25, 2019
|
|
||
| newTest({ | ||
| title: "Can handle 'gj'", | ||
| start: ['blah', 'duh', '|dur', 'hur'], |
There was a problem hiding this comment.
These tests don't seem like they exhibit the bug in #1323 (since the cursor is at the front of the line each time). Am I wrong? If not, we should have some regression tests.
J-Fields
requested changes
Sep 25, 2019
The previous fix (commit e14d23d) was somewhat naive as it conflated what should have been entirely separate classes. One set for moving the cursor up and down, and the other for moving it up and down while maintaining cursors position in wrapped lines. This conflation would probably be fine except that the former set of classes was in turn being used by the MoveUpFoldFix/MoveDownFoldFix classes. Digging through the commit history, the original confusion might have been introduced by commit 5a9f2f0. We now correctly separate Move(Up|Down)ByScreenLine classes from the Move(Up|Down)ByScreenLineMaintainDesiredColumn classes.
|
I’m not sure if there is a better mechanism but as a maintainer it’s hard to figure out when a PR is ready for another round of reviews. After you’ve addressed all the comments, let us know so we can take another look. |
Travis tests have failedHey @hetmankp, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; finpm testTravisBuddy Request Identifier: 797859a0-e01b-11e9-a161-570cf9dcad8d |
hetmankp
force-pushed
the
topic/fix-gj-gk
branch
from
3cd22f7 to
bcd5471
Compare
|
Yes, I had wanted to add those tests a while ago but then realised visual mode behaviour was completely broken with the existing code and abandoned that effort at the time. I've reworked the original fix with better dependency hygiene, fixed visual mode behaviour so it behaves like VIM and added the requested tests. |
hetmankp
added a commit
to hetmankp/VSCodeVim
that referenced
this pull request
Oct 3, 2019
This more or less reverts the functionality introduced by commit 4b2e079 and raised in issue VSCodeVim#3890. Unfortunately it is not currently possible to fully implement display line movements over wrapped lines that will always preserve cursor position. One of two things is necessary to change this, either the VSCode API needs to be extended to return information about wrapped lines (but this was rejected in issue microsoft/vscode#23045). Alternatively VSCodeVim would need to reïimplement all motions using vscode.commands.executeCommand() commands rather than manipulating vscode.window.activeTextEditor.selections directly, as the former preserves column position while the latter does not. However it's not clear to me if all VIM beahviour could be reïmplemented this way without first trying this out and sounds like a bit of a ground up rewrite. It seems for now we're stuck with gj/gk not preserving column position when jumping across short lines.
J-Fields
pushed a commit
that referenced
this pull request
Oct 5, 2019
Closes #4120 This more or less reverts the functionality introduced by commit 4b2e079 and raised in issue #3890. Unfortunately it is not currently possible to fully implement display line movements over wrapped lines that will always preserve cursor position. One of two things is necessary to change this, either the VSCode API needs to be extended to return information about wrapped lines (but this was rejected in issue microsoft/vscode#23045). Alternatively VSCodeVim would need to reïimplement all motions using vscode.commands.executeCommand() commands rather than manipulating vscode.window.activeTextEditor.selections directly, as the former preserves column position while the latter does not. However it's not clear to me if all VIM beahviour could be reïmplemented this way without first trying this out and sounds like a bit of a ground up rewrite. It seems for now we're stuck with gj/gk not preserving column position when jumping across short lines. Add gj/gk tests for future behaviour These tests had been previously removed as supporting this behaviour is not yet possible due to technical reasons, however they do represent correct VIM behaviour so we keep them until implementation becomes architecturally possible.
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.
This fixes the gj & gk motion commands so that they maintain column position when jumping across short lines. This fixes issue #1323 .