Skip to content
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

Hopefully fixing the rest of our undo issues #2559

Merged
merged 4 commits into from
Apr 24, 2018

Conversation

Chillee
Copy link
Member

@Chillee Chillee commented Apr 23, 2018

As has been noted previously, I think the issue behind this second "undo" problem is because we separate EditorIdentity not only by fileName, but also by viewColumn. This is problematic for several reasons.

  1. The viewColumn isn't how we think about what our current editor is. For example, if one drags a file from column 1 to column 2, one'd expect that to be the same editor.
  2. We'd expect most state to be shared from a file open in 2 different columns. For example, undo doesn't really make sense (we can't undo locally; we're not building a collaborative editor here).

Looking at these 2 problems, we arrive at what is the fundamental problem: There can be multiple undoStates for the same file, indexed by viewColumn. Here are a couple of cases where this is a problem.
Case 1:
The repro @johnfn reported: #928 (comment)
I'll write it out here for clarity. For the sake of succinctness, I'll assume there's only one file in each column so "making a few changes in column 1" has the same meaning as "making a few changes in column 1's version of the file".

  1. Open same file in 2 columns.
  2. Make a few changes in column 1.
  3. Switch to column 2 and hit u. All changes made in first column are undone.

Case 2:

  1. Open the same file in 2 columns
  2. Make a few changes in the the first column.
  3. Close the first colum.
  4. Hit undo. All the changes made have now been undone.

Case 3:

  1. Open 2 columns (but only have the file we're using be in Column 1).
  2. Drag the file over to Column 2.
  3. Make a few changes in Column 2.
  4. Drag the file back to the first column.
  5. Hit undo. All changes made in the second column are now undone.

I suspect that most of the bugs people have seen are variations of one of these.

Luckily, there's a simple fix: Don't separate ModeHandler by viewColumn. I'm not sure what that breaks. You can still have different positions in different columns at the same time just fine; we use VSCode's concept of "saved position" for that. I think somebody mentioned that it lets us have different modes in different files (so insert mode in Column 1, normal mode in Column 2). If that's the only issue, I strongly believe that we should break that for now, and add it in later (perhaps with a separate structure from modeHandler).

cc @jpoon @xconverge @rebornix @johnfn

@TravisBuddy
Copy link

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/src/editorIdentity.ts b/src/editorIdentity.ts
index 5997066..0a2efce 100644
--- a/src/editorIdentity.ts
+++ b/src/editorIdentity.ts
@@ -11,7 +11,6 @@ export class EditorIdentity {
     return this._fileName;
   }
 
-
   public hasSameBuffer(identity: EditorIdentity): boolean {
     return this.fileName === identity.fileName;
   }
Warning: The 'no-use-before-declare' rule requires type information.
Prettier Failed. Run [19:33:23] Using gulpfile ~/build/VSCodeVim/Vim/gulpfile.js
[19:33:23] Starting 'prettier'...
[19:33:23] Starting 'tslint'...
[19:33:24] Starting 'compile'...
[19:33:25] Finished 'prettier' after 1.05 s
[19:33:28] Finished 'tslint' after 4.29 s
[19:33:34] Finished 'compile' after 11 s
[19:33:34] Starting 'default'...
[19:33:34] Finished 'default' after 49 μs or [19:33:35] Using gulpfile ~/build/VSCodeVim/Vim/gulpfile.js
[19:33:35] Starting 'forceprettier'...
[19:33:42] Finished 'forceprettier' after 7.01 s

@xconverge
Copy link
Member

LGTM, lets do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants