Merge history changes into a single operation. Fixes #427

[infogulch]

Adds a method HistoryStep.merge that merges all of this.changes into as few DocumentChanges as possible, and call it in finishCurrentStep(). This makes undoing much faster.

I did some manual testing and I'm pretty sure the logic is sound, but it should probably be reviewed carefully.

See #427.

[johnfn]

The way I would test this would be to mash keys while also mashing backspace for about 30 seconds, then hit esc and then i and then do it again a few times. Then use u and c-r to see if all my gibberish was properly maintained. Would you mind doing a test like that?

As you pointed out, I am very cautious about making changes to history tracking, since that's one thing I definitely don't want to break. However if we can be confident that this works, then I'm excited to get it merged in.

In any case, this is a really awesome and big change in terms of VSCodeVim usability! Thanks for the contribution!

[infogulch]

I just pushed again, fixing a bug (more missed optimization) and adding comments to make the merge code a bit more understandable. Also, since I added method DocumentChange.end(), I refactored DocumentChange.do() to use it.

It seems very stable so far. One way to test it that gives a bit of visibility into what it's doing is to add console.log(this.changes); at the beginning and end of the HistoryStep.merge() method. I'll see if I can add some tests like you described.

One part that I haven't tested is where there are multiple DocumentChanges in different parts of the same document in a single HistoryStep. I'm not 100% sure this is possible, and this commit should support it, but I don't know how to trigger this condition.

[johnfn]

One part that I haven't tested is where there are multiple DocumentChanges in different parts of the same document in a single HistoryStep. I'm not 100% sure this is possible, and this commit should support it, but I don't know how to trigger this condition.

Yeah, this is tough. Possibly using search and replace with ctrl-f?

[infogulch]

Hmm, the VSCode replace function doesn't appear to call finishCurrentStep(), so merge is never called in this case.

[rebornix]

this prev represents changes that we are going to merge or merging, maybe we can give it a more intuitive name?

[infogulch]

This loop is basically an Array.reduce(), but we can't use that directly because it can output more than one DocumentChange. e.g. changes in different parts of the document or a del+add (see below). reduce passes two arguments to the function: previous and current.

Here we have prev and change instead.

[johnfn]

Ah, I see. Since we can't use reduce, I'd suggest names that more closely map to what's going on here. Maybe currentChange for prev and nextChange or perhaps toMerge for change.

[infogulch]

I think I'll go for previous and current to match most of the examples on MDN, after making it a bit clearer that this loop is a manual reduce().

[johnfn]

Did you consider something like this?

prev = this.changes[0]
for (const change of this.changes) {

[infogulch]

Yes I considered that at first, but then on the first iteration prev and change would be the same object. change needs to skip the first element, which is why I make an iterator, get the first element, then iterate through the rest.

Other options include using var prev = this.changes.splice(0, 1)[0]; or maybe var prev = this.changes[0]; for (const next of this.changes.slice(1)). I'd prefer the slice method just because it doesn't alter the original array, and it looks a bit clearer to me.

I went with the manual iterator because it doesn't change the original array, it only loops through the array once, and it doesn't create another object. I fully admit these are quite arbitrary reasons, two of which pretty clearly premature optimization. 😄

What would you prefer?

[johnfn]

Oh, right right.

Of all the ideas listed, I prefer

prev = this.changes[0];
for (const next of this.changes.slice(1)) {

I think it's the most readable. 😄 I always favor readability over optimization, unless we've found a bottleneck.

[johnfn]

This is completely awesome. It will definitely improve everyone's Vim experience. If you can fix up the few minor things I mentioned, we should be good to go here.

[infogulch]

Updated to fix the stuff you mentioned. Tests pass fine locally.

[johnfn]

I just realized the most beautiful possible way to handle getting the first element of a list and the rest would have been:

let [current, ...rest] = changes

rather than that splicing thing we did.

I'm gonna just merge this because I'm impatient. I'll fix it on master sometime.

[johnfn]

Anyways, again, thanks for the awesome pull request. This is really great work.