Fixes #1974: U command #2081
Fixes #1974: U command #2081
Conversation
|
Oh wow! To be honest, I didn't think anybody would fix this issue: undo is a pretty tricky thing to deal with. This deserves a more thorough review, so I'll do it over the weekend. |
src/history/historyTracker.ts
Outdated
| @@ -100,7 +100,7 @@ class HistoryStep { | |||
| cursorEnd?: Position[] | undefined; | |||
| marks?: IMark[]; | |||
| }) { | |||
| this.changes = init.changes = []; | |||
| this.changes = init.changes || []; | |||
There was a problem hiding this comment.
Was this just a bug? Could this be the cause for the undo issues we've been experiencing?
There was a problem hiding this comment.
Yes, this was a bug that was preventing me from creating the new HistoryStep near the end of the function. Any list of changes passed in would always be overwritten as an empty array. I'm not aware of the existing undo issues, but it's possible.
| return undefined; | ||
| if (this.currentHistoryStepIndex === 0) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
What's the reasoning behind putting this in the if statement?
There was a problem hiding this comment.
It's not completely necessary, I simply thought it looked odd to have the exact same if statement twice in the same block. Nesting this statement makes the relationship between the second and third if statements more obvious (to me). I did the same change in the new function.
There was a problem hiding this comment.
Ah I didn't see the if statement above.
|
Is this good to merge @Chillee? Edit: Actually, I see that you approved the review. I'm going to update the branch and merge if/when the new build passes. |
|
Argh, build on |
|
Looks like a small bug with the In Vim, when the jumplist is at the most recent state, the first invocation of This was previously working, as indicated by the passing tests in the Vim/src/history/historyTracker.ts Line 103 in e3b9cf1 See my discussion with @Chillee about this change above for the bug in this line. I can work on a fix for this bug. For this PR, should we simply disable these tests? |
|
@westim Yeah that sounds fine to me. |
|
I have decided to revert the small change which caused regressions & failing tests for the |
|
@jpoon this PR is ready for review. |
|
I reviewed this earlier and it all looks good to me! Thanks for the contribution, awesome work! |
This PR adds the
Ucommand, which performs an undo on all changes which occurred on the line of the most recent change.This feature was relatively complicated to implement. The first challenge is that, unlike the
ucommand, theUcommand is itself a change. This means that you can undo and redo aUcommand. Therefore,Uperforms the following steps:undo()on all the changes in the list.HistoryTracker.historySteps.The second challenge was the presence of newlines in some changes. For example, if a change has the text
'\nhello, world!', we want to undo only the text'hello, world!'and not the newline character, so the newline characters must be removed. This must also account for how newline is'\n'on MacOS/Linux and'\r\n'on Windows. This special case also has a starting cursor position offset that must be compensated.I chose to implement workarounds for these challenges rather than modify the architecture elsewhere. The benefit is that any bugs introduced are isolated to this command. The downside is that the code is not as elegant as it could be.
I have added 7 unit tests to cover the major functionality of this command. I have also updated the ROADMAP accordingly.