-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[WIP] Improve foldFix further by making more motion prefixed actions fold-aware #2887
Conversation
…p` and `MoveDown` methods. Improves Handling of operators with movements over folded regions.
…dd and yy commands
…ual line and fix bugs with movement when on these lines. Return `Position` from `execAction` methods of `MoveUp`/`MoveDown` to fix visual mode bugs which do not expect `IMovement`.
It would be very nice to have a few unit tests or at least a file that allow us to test behavior around folds https://github.com/VSCodeVim/Vim/blob/master/test/multicursor.test.ts is an example what I think might be nice for starters, just something thin that allows us to have SOME metrics of how we are doing. You should also be able to do something like: |
I agree, I will try to write some fold-specific tests. But it is necessary to repeat most tests with and without foldFix on since it affects a basic motion that is combined with many other commands during tests. I'm not sure how much of the other tests can be incorporated without redundancy in fold-specific tests, but I will try to investigate. |
…rom `execAction` methods of `MoveUp` and `MoveDown`. Fix foldfix adjusted up and down movement bugs in `execActionForOperator` method of `BaseMovement`.
I managed to get some test coverage for fold-aware movement and actions and fixed a few bugs along the way. It was rather tricky but I think there is now a decent setup for adding more tests for foldfix in the future. |
Some things I couldn't quite figure out and might need some help with:
|
O wow, I lost track of the fact you made changes here! Give me a little bit of time to go through the changes, I personally am not a heavy fold user so I am hoping that the changes make sense to you and provide a better experience for something that was a pain point for you in the current/past releases. Regarding your questions
or something like:
|
You can do something like this: https://github.com/VSCodeVim/Vim/blob/master/test/mode/modeNormal.test.ts#L14 You can take a peek inside
If you find out, let us know :). Tests are executed using vscode's harness: However, we do have some custom logic such that if you are using the The above applies to every which way you execute the tests: through vscode, |
I checked out |
Yikes, this part of the codebase I'm unfamiliar with. @Chillee / @xconverge can you CR? |
@jpoon I'll take a look tomorrow. |
I would like to refactor the changes here when I find the time as I did in #2921 (see 5e32f93) by introducing a new subclass to reduce duplication of the |
I also still need to adjust the test configuration to have predictable line wrapping so I can ensure this also fixes traversal of wrapped lines. |
First of all, thanks a lot for the PR! Sorry I didn't get to this earlier, but I'm not sure how I feel about extending upon When I initially added the option, it was meant as a stopgap solution to make VSCodeVim usable for those who needed to use folds. Most of the time, the main frustration was caused by folds opening when trying to move over them with On the other hand, there appear to be a fairly solid test suite, and the What do you think @jpoon ? |
As I noted before, I don't think it will be possible to completely fix folds even if we continue to hack around vscode. But with the abstraction I added to the |
@xmbhasin I don't mind keeping the deleting and yanking, but I would really like the abstraction of foldfix. Thanks! |
663697d
to
f2b9408
Compare
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
|
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
|
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
TravisBuddy Request Identifier: b2792650-ccd3-11e8-9706-8d7bf71fb7b5 |
Sorry it's been a while since I updated this PR, I was fairly busy past few weeks. I think I am mostly satisfied with the refactors now although there may still be room for improvement. I did not pull out the code in Let me know if you want to do another review, otherwise I will remove the [WIP] tag and I think this PR can be merged. |
Closing this PR due to the lack of activity. Please re-open once this is ready for review. |
What this PR does / why we need it:
Improve foldFix further by making more motion prefixed actions fold-aware (including at least deleting and yanking)
Which issue(s) this PR fixes
See #2879 where I am tracking improvements to foldFix.
Special notes for your reviewer:
gulp test
passes all tests with and without forcingfoldFix
to true. I'm not sure how to go about it, but I would like to add a foldFix configuration togulp test
. I don't want to make too many changes there as I'm worried it might break CI.