Fixed undo issue given in #2545

[Chillee]

Thanks to @Diadlo for this repro. #2545.

Basically, we don't always want to create a prevHandler even if it doesn't exist. To be honest, this code is messy and I'd like to rewrite it entirely, if it wasn't for the fact that this kind of frail code is hard to test.

This should fix many of the reports given in #2007. However, the first reports of the issue started before this repro was introduced. That suggests to me that there's actually 2 separate bugs. One where the file gets replaced by another file (issue 1), and one where the file actually drops back to an earlier undo state (issue 2).

Issue 1 was introduced around January 12th in this seemingly innocuous commit: c050066 .

The first report of this (replaced with another file is here): #2007 (comment) Many of the following comments were also expressing this issue.

However, the first report of issue 2 were given here: #928 The issue is slightly confused by the fact that the initial bug that @octref reports is a much more innocuous "Certain VSCode actions aren't given their own undo stop", while issue 2 is really "VSCodeVim is wiping out a ton of work". The first real report of issue 2 appears to be #928 (comment), on July 26th. I believe that this bug was likely introduced by this commit: #1629 on May 3rd.

This commit fixes issue 1. More work is needed to locate the cause of issue 2.

@jpoon @johnfn @xconverge @rebornix

[TravisBuddy]

Travis tests have failed

Hey @Chillee,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

Node.js: 8

if [[ $(git diff-index HEAD --) ]]; then git diff; echo "Prettier Failed. Run `gulp` or `gulp forceprettier`"; exit 1; fi

diff --git a/extension.ts b/extension.ts
index 4b4dfe9..a59c8e2 100644
--- a/extension.ts
+++ b/extension.ts
@@ -363,6 +363,6 @@ async function handleActiveEditorChange(): Promise<void> {
   });
 }
 
-process.on('unhandledRejection', function (reason: any, p: any) {
+process.on('unhandledRejection', function(reason: any, p: any) {
   console.log('Unhandled Rejection at: Promise ', p, ' reason: ', reason);
 });
diff --git a/src/mode/modeHandler.ts b/src/mode/modeHandler.ts
index 5408887..1d1f8da 100644
--- a/src/mode/modeHandler.ts
+++ b/src/mode/modeHandler.ts
@@ -509,8 +509,8 @@ export class ModeHandler implements vscode.Disposable {
         x =>
           x.start.isEarlierThan(x.stop)
             ? x.withNewStop(
-              x.stop.isLineEnd() ? x.stop.getRightThroughLineBreaks() : x.stop.getRight()
-            )
+                x.stop.isLineEnd() ? x.stop.getRightThroughLineBreaks() : x.stop.getRight()
+              )
             : x
       );
     }
@@ -772,7 +772,7 @@ export class ModeHandler implements vscode.Disposable {
       if (
         recordedState.operators.length > 1 &&
         recordedState.operators.reverse()[0].constructor ===
-        recordedState.operators.reverse()[1].constructor
+          recordedState.operators.reverse()[1].constructor
       ) {
         resultVimState = await recordedState.operator.runRepeat(
           resultVimState,
@@ -1327,8 +1327,8 @@ export class ModeHandler implements vscode.Disposable {
     const easyMotionHighlightRanges =
       this.currentMode.name === ModeName.EasyMotionInputMode
         ? vimState.easyMotion.searchAction
-          .getMatches(vimState.cursorPosition, vimState)
-          .map(x => x.toRange())
+            .getMatches(vimState.cursorPosition, vimState)
+            .map(x => x.toRange())
         : [];
     this.vimState.editor.setDecorations(Decoration.EasyMotion, easyMotionHighlightRanges);
 
Warning: The 'no-use-before-declare' rule requires type information.
Prettier Failed. Run [23:31:16] Using gulpfile ~/build/VSCodeVim/Vim/gulpfile.js
[23:31:16] Starting 'prettier'...
[23:31:16] Starting 'tslint'...
[23:31:16] Starting 'compile'...
[23:31:18] Finished 'prettier' after 1.84 s
[23:31:21] Finished 'tslint' after 4.74 s
[23:31:28] Finished 'compile' after 12 s
[23:31:28] Starting 'default'...
[23:31:28] Finished 'default' after 43 μs or [23:31:29] Using gulpfile ~/build/VSCodeVim/Vim/gulpfile.js
[23:31:29] Starting 'forceprettier'...
[23:31:36] Finished 'forceprettier' after 7.86 s

[jpoon]

Basically, we don't always want to create a prevHandler even if it doesn't exist.

Why?

Edit: Looking at the code answered my question. Thanks also for the explanation @Chillee

[Chillee]

@jpoon When we close a file, we delete its handler from ModeHandlerMap. When we were calling ModeHandlerMap.getOrCreate in getAndUpdateModeHandler, prevHandler did not exist at that step. So, it would create a new handler for the file we just closed. If you take a look at the code for creating a modehandler, it creates the vim state out of the current file.

This caused us to initialize the undo history of the closed file with the text of the current file. Then, when you reopened that file, pressing undo would set you to the text of the other file.

So, in this repro: #2545

1. We have the correct state for document A.
2. We have the correct state for document A & B.
3. Closing document B first deletes the current modeHandler we have, but getOrCreate creates another modeHandler initialized with the vimState of document A. So, we have the correct state for document A but the wrong one for document B. Only document A is currently open.
4. Opening this file and changing the contents is treated as an undoable change.
5. Undoing overwrites the contents of document B with document A.

[jpoon]

Maybe we can change this to a create function, but I can do that refactor in a later commit.

[jpoon]

Can probably do this all in one line.

[johnfn]

Oh man what a hero!

[johnfn]

I agree with your conclusion, too. The ModeHandler stuff is a mess and has been responsible for more bugs than I could count. Worst part is, the biggest benefit of ModeHandler is per-file state, and I can't even remember why we need that. Nearly everything is per-project.

[xconverge]

Because you might be in insert mode in one file and another file be in visual? What do you mean per-project, there is definitely a need for per-file state...

[jcjolley]

@Chillee, you're amazing.

[jpoon]

Does this actually return a promise?

[Chillee]

... No

[jpoon]

@Chillee ok to merge?

[Chillee]

Yep!

[jpoon]

I'll cut a release for it.

[rifotu]

great guys!
thanks a lot :)