Fix the transforms in undo/redo

[rhysbrettbowen]

fixes #2105

Added test that fails with previous implementation. Initially in the issue I added a simple way to fix the transforms - however this is not sufficient to fix things. The undo of an undo needs to be calculated when it is applied - you can't just save the initial operation as a redo and transform. This is because position information is lost when a delete is made and you apply transforms. So instead undos and redos are passed through the normal record (this is so the state is available for calculating diffs)

[jhchen]

Hey Rhys, thanks for the PR. The failing test is valid though I'm concerned about the implementation. It sounds like you may have already tried the simple fix though I'm not understanding your explanation for why it does not work. Can you elaborate in general but specifically what positional information is lost?

[rhysbrettbowen]

Sure - I did give it a go and a test to show it's not working is to have the client put in B then the server to add in A & C around it. Then undo and redo. You'll get ACB.

Reason is that the redo isn't really a redo - just an insert. It hasn't actually been played yet so it gets transformed as coming after the items.

another way to think of it would be with a delete. If you remove a character and keep an undo for it, there is no way for the server to place characters directly after it when it's undone as the insert will be transformed to coming after the incoming operations.

Happy to go through it with you in more detail - I'm probably not doing a great job explaining.

[rhysbrettbowen]

@jhchen

Might make it easier in a diagram. Because of the removal in the undo there is no longer any marker down the red path that it should be inserted there (otherwise we could have some sort of undo/redo operation instead of a normal insert). With transforms everything is still consistent and gets to the same outcome, it might just not be what you expect. I highlighted the red as it's easiest to see the effect there.

If however we follow the green path (which follows what happens on the test) and instead generate the undo of the undo at the point it meets the red lines we get a closer representation of what the user wanted as we will have information on the state at that point which will include the information of previous operations already applied to it (i.e. the state already has applied the server ops and we can see that we want to undo the remove operation). This is what this PR does.

[rhysbrettbowen]

Fixed with #2500 thankyou!

[jhchen]

Yup thanks for bringing this to our attention and the exploration!