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

u restores multiple edits #928

Closed
octref opened this issue Oct 17, 2016 · 44 comments
Closed

u restores multiple edits #928

octref opened this issue Oct 17, 2016 · 44 comments

Comments

@octref
Copy link
Contributor

octref commented Oct 17, 2016

  • Click thumbs-up 👍 on this issue if you want it!
  • Click confused 😕 on this issue if not having it makes VSCodeVim unusable.

The VSCodeVim team prioritizes issues based on reaction count.


F2 on a word and rename it. Then make one more edit. Press u.

Both the F2 rename and the "one more edit" get undo-ed.

  • VSCode Version: 1.7 Insider
  • VsCodeVim Version: 0.3.7
  • OS: OS X 10.11.6
@johnfn
Copy link
Member

johnfn commented Oct 17, 2016

Interesting. Seems like we should be listening inside didUpdateSelectionEvent or whatever for a text editor change and if so trigger an undo point.

@xconverge
Copy link
Member

we have onDidChangeTextDocument, it is probably something in there..also this happens during normal mode which is interesting, we probably have logic in there for snippets/insert mode only

@offero
Copy link

offero commented May 12, 2017

I am seeing similar behavior. The undo is actually replaying history from more than 1 change back. It's really confusing. It's actually bad enough that I have no choice but to switch away from this extension at the moment.

@jvbianchi
Copy link

I also suffered from this bug

@Chillee
Copy link
Member

Chillee commented May 12, 2017

When you guys say you suffer from this bug, do you mean this specific bug, or undo behavior in general?

@offero
Copy link

offero commented May 12, 2017

In general for me. I didn't press F2 or use rename. I did have a split window open, though. I'm not sure if that has any effect.

@Chillee
Copy link
Member

Chillee commented May 12, 2017

@offero I do think our current undo behavior is fairly wonky, but do you have a specific repro case?

@jvbianchi
Copy link

In my case is the specific bug (using F2 and rename)

@johnfn
Copy link
Member

johnfn commented May 13, 2017

Another great way to repro this is to use TypeScript, use the helper tool to auto insert an import, then undo.

It seems like cases where text is inserted not through VSCodeVim that we have a problem with.

@jleclanche
Copy link

Here's another issue with multiple edits

Prerequisite: Create a file with the following contents:

One
Two
Three
  1. Position your cursor on the beginning of the second line (T).
  2. Hit dd; the file now looks like One\nThree
  3. Hit ; cursor is now at the beginning of file
  4. hit P; the file now looks like Two\nOne\nThree
  5. Hit ; the cursor is now on the second line
  6. Hit dd; the file now looks like Two\nThree

At this point, hitting u restores the file to two edits up rather than one edit up, to One\nThree.

@Chillee
Copy link
Member

Chillee commented Jun 21, 2017

@jleclanche That one is surprising to me. Those are all regular operations.

@jleclanche
Copy link

Yup surprising to me too. I've been noticing u being super unreliable lately; there's more of these multi-edit restores, it's just a little cumbersome to actually make a reduction to report here. But vscode's undo works fine.

I really hope you guys can look into revamping undo entirely. It works differently than vscode's undo and differently than vim's undo, and it's slow, and it sometimes misses things, and hitting ctrl+z after an undo undoes the undo, and so on and so on.

In #1490 there's discussion about how the undo history is maintained separately because of light differences in undo stops, I don't really get it. The custom implementation is riddled with far bigger issues than undo stops imho.

@Chillee
Copy link
Member

Chillee commented Jun 21, 2017

@jleclanche Has undo become worse lately? We had some issues related to the most recent VSCode update, but we should have fixed those.

I just took a look, and the specific bug in this case was that P wasn't creating an undo stop for whatever reason.

As for potential improvements in the future. :

  1. VScode undo stop API: A good potential choice, but prevents us from leveraging Neovim undo features (like an undo tree!). This would also solve the multi-edit restore problem, and lets us sync up ctrl+z and u.

  2. Neovim undo tree: This'll let us get an undo tree! Should also speed it up significantly. However, multi-edit would still need to be handled separately, and ctrl+z will never by synced.

@jleclanche
Copy link

Has undo become worse lately?

Hard to tell. I think so as I've been noticing more missed undos, but I can't be sure.

How likely is it that we get a vim.manageUndo setting which lets us toggle on/off a natively-managed undo?

@Chillee
Copy link
Member

Chillee commented Jun 21, 2017

@jlecanche To be honest, not likely (assuming you're talking about between neovim and vscode). Each flag we have is another combination of things we need to maintain, and for something as integral as undo, it's very likely we'll be break things.

@jleclanche
Copy link

Right, yes. Hm =\ It's a shame. I genuinely believe undo is the biggest pain point of vscode-vim right now and pursuing an undo history managed by vim is causing more pain points than it's theoretically solving.

Regarding your second point on syncing Ctrl+Z, I suppose this could theoretically be done if vscode makes undo management an API feature, couldn't it? This way, undo history could be managed by an extension and vscode-vim could take it over entirely. Native vscode ctrl+z would then just map to a u operation (and same for redo).

@Chillee
Copy link
Member

Chillee commented Jun 21, 2017

@jlecanche Vscode already provides api endpoints for setting undo stops, which is all we need. It's definitely a good solution, and the only reason we didn't do it initially was that the api didn't exist when we implemented undo.

Thinking about it more, it may actually be possible to implement the positive points of both.

@jleclanche
Copy link

The main problem is that hitting Ctrl+Z would create an edit point in vim, which could then be undone in vim again. I don't think setting synced undo stops is enough, you have to make sure that there is only one single "undo"/"redo" operation available to both vim and vscode.

@Chillee
Copy link
Member

Chillee commented Jun 21, 2017

I don't think that's really an issue. Why would you be using both?

@jleclanche
Copy link

I guess most of the time you wouldn't so it's not a huge deal, but I could see some people getting burned by it; or even other extensions that perform functionality which depend on undo/redo, which would then be "incompatible" with vscode-vim undo.

It just seems so much saner to ensure there's only one undo op.

@Chillee
Copy link
Member

Chillee commented Jun 21, 2017

If we use vscode undo stops, that would allow us to use only one open.

Ideally, we'd be able to bind u to ctrl-z and be done with it.

@qguv
Copy link

qguv commented Jul 25, 2017

Behavior for me is similar to @offero: undo for me sometimes reverts hundreds of discrete actions, sometimes hours worth of changes. Unusable without restarting VS Code regularly.

@Chillee
Copy link
Member

Chillee commented Jul 25, 2017

@qguv Wait, so undo sometimes reverts a ton of changes? As in, you'll type up an entire file of code, and then randomly, undo will just revert all the changes you made in the file?

@jleclanche
Copy link

@Chillee I've seen this happen as well. It's like vscode vim sometimes stops creating edit points, and if you hit u it reverts a ton of things at once; if you keep doing changes, it'll revert those as well.

@qguv
Copy link

qguv commented Jul 26, 2017

@Chillee Yes, although this tends to happen with files I've kept open for a while.

@qguv
Copy link

qguv commented Jul 26, 2017

@Chillee here's an example!

demonstrating multiple undo/redos on a single command

@pvaibhav
Copy link

I've suffered through this problem past few days! I've lost too many large edits due to this despite the fact that I save the file pretty much every few secs :( Somehow it modifies both Vim's and VScode's history in a way that's difficult to recover from.

@petejkim
Copy link

petejkim commented Aug 3, 2017

Yep same here: I hit u once and ctrl+r once but many changes happen at once:

undo redo

@Chillee
Copy link
Member

Chillee commented Aug 3, 2017

Hmmm... This is not good. I'm pretty sure what's happening is what @jleclanche said. Could you guys open up developer tools when this happens and report what you guy see?

@Chillee
Copy link
Member

Chillee commented Aug 13, 2017

@BRHS @petejkim Has this ever occurred before v0.9.0?

We recently made a change that prevented us from capping the number of changes we stored. This was a problem if you made a huge change (say, a huge find and replace), and then tried to undo it.

However, if this bug existed before, then the change would only have capped how damaging this bug would be, not eliminated it.

@johnfn
Copy link
Member

johnfn commented Aug 24, 2017

definitely been seeing this one more, must have something to do with our handling of the same file twice with separate modehandlers

@qguv
Copy link

qguv commented Aug 31, 2017

Agreed. I can't use undo anymore with this plugin installed.

@johnfn
Copy link
Member

johnfn commented Sep 7, 2017

The repro for this one is:

  1. Open same file in 2 tabs
  2. Make a few changes in one tab
  3. Switch to other tab and hit u. All changes made in first tab are undone.

@Chillee
Copy link
Member

Chillee commented Sep 7, 2017

@johnfn Is the issue only happening when the same file is open in 2 tabs?

@johnfn
Copy link
Member

johnfn commented Sep 7, 2017

@Chillee I'm not sure about 'only' but that does seem to be 1 repro

@qguv
Copy link

qguv commented Sep 12, 2017 via email

@Chillee
Copy link
Member

Chillee commented Sep 15, 2017

Putting this inside vim.otherModesKeyBindings is a good temporary fix if this is a big issue for you:

        {
            "before": [
                "u"
            ],
            "after": [],
            "commands": [
                {
                    "command": "undo"
                }
            ]
        },
        {
            "before": [
                "r"
            ],
            "after": [],
            "commands": [
                {
                    "command": "redo"
                }
            ]
        }

@xconverge
Copy link
Member

Next time this happens to someone, can you check to see if you have the same file open twice in vscode please, then either thumbs up this for yes, or thumbs down it for no

@xconverge
Copy link
Member

xconverge commented Sep 30, 2017

I am almost certain this is in extension.ts

export async function getAndUpdateModeHandler(): Promise<ModeHandler> {

I think the case of having the same file open twice is slightly different than just general undoing extra things.

We create a unique modeHandler for each instance of the file so that one can be in insert, and the other is in normal etc. This means that we do not sync the undo history, so type in one a few times, press u in the other and it restores the file based on the 2nds HistoryTracker (back to the starting state). Perhaps we could just share the HistoryTracker for all mode handlers of the same filename, this might fix both cases... A map of HistoryTrackers per filename in GlobalState, the modehandler uses the history tracker here instead of inside the modehandler

I am not sure what the 2nd case is of just 1 file open but suspect it might be similar, a stale modehandler or something

Any thoughts @Chillee @johnfn

@Chillee
Copy link
Member

Chillee commented Oct 6, 2017

@xconverge Was there a change recently to those functions that would cause this? I seem to remember a couple PR's that touched those functions.

@xconverge
Copy link
Member

yea at one point I would dispose mode handlers when not used, but then that impacted peek behavior, so I know they were touched a bit, but I don't think we ever thought about the impact on undo with 2 separate mode handlers for the same file. It was undesirable to share a modehandler because if you went into insert on one pane, the other pane would go into insert, etc

@JesseObrien
Copy link

I'm seeing this issue as well, and from what I'm experiencing, and some testing I think it's only showing up when I have the same file open twice.

@zhiwei-nu
Copy link

another instance of this happened in my C# project with the usage of refactoring command:

  1. Make an indentation error somewhere
  2. ctrl+shift+i to refactor file
  3. Make a change: i helloworld
  4. u should undo both 3 and 2

@jpoon
Copy link
Member

jpoon commented Jul 4, 2018

Duplicate of #2007

@jpoon jpoon marked this as a duplicate of #2007 Jul 4, 2018
@jpoon jpoon closed this as completed Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests