Excalidraw: Undoing a change created after undo skips history

[dwelle]

Not sure what's really going on, so the title may be incorrect.

Steps:

1. create a shape
2. move the shape
3. undo the move change
4. move the shape again
5. undo that move again

The last undo removes the shape completely, instead of undoing the last move.

[enzoferey]

Hi @dwelle, I was just working on https://github.com/excalidraw/excalidraw/pull/309 and I was facing the same bug. It is caused because here we are allowing to pop 2 elements in the history stack when we should only pop one element, put it into the redo stack, and restore grab last element of the history (previous second last).

This is what happens:

1. Create a shape --> push entry --> [createShape]
2. Move the shape --> push entry --> [createShape, moveShape]
3. Undo the move change --> undoOnce --> pop --> [createShape] --> sees current state is equal to popped element --> pop --> []
4. Move the shape again --> [moveShape]
5. Undo that move again --> []

I suppose there are historical reasons why this logic was introduced (which I'm not aware). Looking at the code as it is today, I see no reason why we can't have a simplified undoOnce like this:

undoOnce() {
    if (this.stateHistory.length === 0) {
      return null;
    }

    const currentEntry = this.stateHistory.pop();
    const entryToRestore = this.stateHistory[this.stateHistory.length - 1];

    if (currentEntry !== undefined) {
      this.redoStack.push(currentEntry);
      return this.restoreEntry(entryToRestore);
    }

    return null;
}

This fixes the bug. Let me know if you want me to submit a PR where I can also revisit the redoOnce and restoreEntry functions 👍

[vjeux]

For context, I've never written an undo/redo action before, so I tried something with pushing random stuff so that it kind of worked. Looks like I didn't get it right. Thanks @enzoferey for the correct version!

[enzoferey]

@vjeux As I wrote at the end of my comment, there are other not working as expected things and to be simplified things in the history. I will send a PR solving them in the next hours.