add history.shouldCreateEntry resolver (#1622)

src/actions/actionHistory.tsx

@@ -3,7 +3,7 @@ import React from "react";
 import { undo, redo } from "../components/icons";
 import { ToolButton } from "../components/ToolButton";
 import { t } from "../i18n";
-import { SceneHistory } from "../history";
+import { SceneHistory, HistoryEntry } from "../history";
 import { ExcalidrawElement } from "../element/types";
 import { AppState } from "../types";
 import { KEYS } from "../keys";
@@ -13,10 +13,7 @@ import { newElementWith } from "../element/mutateElement";
 const writeData = (
   prevElements: readonly ExcalidrawElement[],
   appState: AppState,
-  updater: () => {
+  updater: () => HistoryEntry | null,
-    elements: ExcalidrawElement[];
-    appState: AppState;
-  } | null,
 ): ActionResult => {
   const commitToHistory = false;
   if (
@@ -52,6 +49,7 @@ const writeData = (
         ),
       appState: { ...appState, ...data.appState },
       commitToHistory,
+      syncHistory: true,
     };
   }
   return { commitToHistory };

src/actions/types.ts

@@ -6,6 +6,7 @@ export type ActionResult = {
   elements?: readonly ExcalidrawElement[] | null;
   appState?: AppState | null;
   commitToHistory: boolean;
+  syncHistory?: boolean;
 };
 
 type ActionFn = (

src/appState.ts

@@ -70,26 +70,6 @@ export const clearAppStateForLocalStorage = (appState: AppState) => {
   return exportedState;
 };
 
-export const clearAppStatePropertiesForHistory = (
-  appState: AppState,
-): Partial<AppState> => {
-  return {
-    selectedElementIds: appState.selectedElementIds,
-    exportBackground: appState.exportBackground,
-    shouldAddWatermark: appState.shouldAddWatermark,
-    currentItemStrokeColor: appState.currentItemStrokeColor,
-    currentItemBackgroundColor: appState.currentItemBackgroundColor,
-    currentItemFillStyle: appState.currentItemFillStyle,
-    currentItemStrokeWidth: appState.currentItemStrokeWidth,
-    currentItemRoughness: appState.currentItemRoughness,
-    currentItemOpacity: appState.currentItemOpacity,
-    currentItemFont: appState.currentItemFont,
-    currentItemTextAlign: appState.currentItemTextAlign,
-    viewBackgroundColor: appState.viewBackgroundColor,
-    name: appState.name,
-  };
-};
-
 export const cleanAppStateForExport = (appState: AppState) => {
   return {
     viewBackgroundColor: appState.viewBackgroundColor,

src/components/App.tsx

@@ -276,12 +276,23 @@ class App extends React.Component<any, AppState> {
       if (res.commitToHistory) {
         history.resumeRecording();
       }
-      this.setState((state) => ({
+      this.setState(
-        ...res.appState,
+        (state) => ({
-        editingElement: editingElement || res.appState?.editingElement || null,
+          ...res.appState,
-        isCollaborating: state.isCollaborating,
+          editingElement:
-        collaborators: state.collaborators,
+            editingElement || res.appState?.editingElement || null,
-      }));
+          isCollaborating: state.isCollaborating,
+          collaborators: state.collaborators,
+        }),
+        () => {
+          if (res.syncHistory) {
+            history.setCurrentState(
+              this.state,
+              globalSceneState.getElementsIncludingDeleted(),
+            );
+          }
+        },
+      );
     }
   });
 

src/data/restore.ts

@@ -74,6 +74,7 @@ export const restore = (
         // all elements must have version > 0 so getDrawingVersion() will pick up newly added elements
         version: element.version || 1,
         id: element.id || randomId(),
+        isDeleted: false,
         fillStyle: element.fillStyle || "hachure",
         strokeWidth: element.strokeWidth || 1,
         strokeStyle: element.strokeStyle ?? "solid",

src/history.ts

@@ -1,18 +1,28 @@
 import { AppState } from "./types";
 import { ExcalidrawElement } from "./element/types";
-import { clearAppStatePropertiesForHistory } from "./appState";
 import { newElementWith } from "./element/mutateElement";
 import { isLinearElement } from "./element/typeChecks";
 
-type Result = {
+export type HistoryEntry = {
-  appState: AppState;
+  appState: ReturnType<typeof clearAppStatePropertiesForHistory>;
   elements: ExcalidrawElement[];
 };
 
+type HistoryEntrySerialized = string;
+
+const clearAppStatePropertiesForHistory = (appState: AppState) => {
+  return {
+    selectedElementIds: appState.selectedElementIds,
+    viewBackgroundColor: appState.viewBackgroundColor,
+    name: appState.name,
+  };
+};
+
 export class SceneHistory {
   private recording: boolean = true;
-  private stateHistory: string[] = [];
+  private stateHistory: HistoryEntrySerialized[] = [];
-  private redoStack: string[] = [];
+  private redoStack: HistoryEntrySerialized[] = [];
+  private lastEntry: HistoryEntry | null = null;
 
   getSnapshotForTest() {
     return {
@@ -25,6 +35,20 @@ export class SceneHistory {
   clear() {
     this.stateHistory.length = 0;
     this.redoStack.length = 0;
+    this.lastEntry = null;
+  }
+
+  private parseEntry(
+    entrySerialized: HistoryEntrySerialized | undefined,
+  ): HistoryEntry | null {
+    if (entrySerialized === undefined) {
+      return null;
+    }
+    try {
+      return JSON.parse(entrySerialized);
+    } catch {
+      return null;
+    }
   }
 
   private generateEntry = (
@@ -48,57 +72,96 @@ export class SceneHistory {
             return elements;
           }
 
-          elements.push(
+          elements.push({
-            newElementWith(element, {
+            ...element,
-              // don't store last point if not committed
+            // don't store last point if not committed
-              points:
+            points:
-                element.lastCommittedPoint !==
+              element.lastCommittedPoint !==
-                element.points[element.points.length - 1]
+              element.points[element.points.length - 1]
-                  ? element.points.slice(0, -1)
+                ? element.points.slice(0, -1)
-                  : element.points,
+                : element.points,
-              // don't regenerate versionNonce else this will short-circuit our
+          });
-              //  bail-on-no-change logic in pushEntry()
-              versionNonce: element.versionNonce,
-            }),
-          );
         } else {
-          elements.push(
+          elements.push(element);
-            newElementWith(element, { versionNonce: element.versionNonce }),
-          );
         }
         return elements;
       }, [] as Mutable<typeof elements>),
     });
 
-  pushEntry(appState: AppState, elements: readonly ExcalidrawElement[]) {
+  shouldCreateEntry(nextEntry: HistoryEntry): boolean {
-    const newEntry = this.generateEntry(appState, elements);
+    const { lastEntry } = this;
-    if (
+
-      this.stateHistory.length > 0 &&
+    if (!lastEntry) {
-      this.stateHistory[this.stateHistory.length - 1] === newEntry
+      return true;
-    ) {
-      // If the last entry is the same as this one, ignore it
-      return;
     }
 
-    this.stateHistory.push(newEntry);
+    if (nextEntry.elements.length !== lastEntry.elements.length) {
+      return true;
+    }
 
-    // As a new entry was pushed, we invalidate the redo stack
+    // loop from right to left as changes are likelier to happen on new elements
-    this.clearRedoStack();
+    for (let i = nextEntry.elements.length - 1; i > -1; i--) {
+      const prev = nextEntry.elements[i];
+      const next = lastEntry.elements[i];
+      if (
+        !prev ||
+        !next ||
+        prev.id !== next.id ||
+        prev.version !== next.version ||
+        prev.versionNonce !== next.versionNonce
+      ) {
+        return true;
+      }
+    }
+
+    // note: this is safe because entry's appState is guaranteed no excess props
+    let key: keyof typeof nextEntry.appState;
+    for (key in nextEntry.appState) {
+      if (key === "selectedElementIds") {
+        continue;
+      }
+      if (nextEntry.appState[key] !== lastEntry.appState[key]) {
+        return true;
+      }
+    }
+
+    return false;
   }
 
-  restoreEntry(entry: string) {
+  pushEntry(appState: AppState, elements: readonly ExcalidrawElement[]) {
-    try {
+    const newEntrySerialized = this.generateEntry(appState, elements);
-      return JSON.parse(entry);
+    const newEntry: HistoryEntry | null = this.parseEntry(newEntrySerialized);
-    } catch {
+
-      return null;
+    if (newEntry) {
+      if (!this.shouldCreateEntry(newEntry)) {
+        return;
+      }
+
+      this.stateHistory.push(newEntrySerialized);
+      this.lastEntry = newEntry;
+      // As a new entry was pushed, we invalidate the redo stack
+      this.clearRedoStack();
     }
   }
 
+  private restoreEntry(
+    entrySerialized: HistoryEntrySerialized,
+  ): HistoryEntry | null {
+    const entry = this.parseEntry(entrySerialized);
+    if (entry) {
+      entry.elements = entry.elements.map((element) => {
+        // renew versions
+        return newElementWith(element, {});
+      });
+    }
+    return entry;
+  }
+
   clearRedoStack() {
     this.redoStack.splice(0, this.redoStack.length);
   }
 
-  redoOnce(): Result | null {
+  redoOnce(): HistoryEntry | null {
     if (this.redoStack.length === 0) {
       return null;
     }
@@ -113,7 +176,7 @@ export class SceneHistory {
     return null;
   }
 
-  undoOnce(): Result | null {
+  undoOnce(): HistoryEntry | null {
     if (this.stateHistory.length === 1) {
       return null;
     }
@@ -130,6 +193,19 @@ export class SceneHistory {
     return null;
   }
 
+  /**
+   * Updates history's `lastEntry` to latest app state. This is necessary
+   *  when doing undo/redo which itself doesn't commit to history, but updates
+   *  app state in a way that would break `shouldCreateEntry` which relies on
+   *  `lastEntry` to reflect last comittable history state.
+   * We can't update `lastEntry` from within history when calling undo/redo
+   *  because the action potentially mutates appState/elements before storing
+   *  it.
+   */
+  setCurrentState(appState: AppState, elements: readonly ExcalidrawElement[]) {
+    this.lastEntry = this.parseEntry(this.generateEntry(appState, elements));
+  }
+
   // Suspicious that this is called so many places. Seems error-prone.
   resumeRecording() {
     this.recording = true;

src/tests/__snapshots__/move.test.tsx.snap

@@ -10,13 +10,13 @@ Object {
   "isDeleted": false,
   "opacity": 100,
   "roughness": 1,
-  "seed": 2019559783,
+  "seed": 401146281,
   "strokeColor": "#000000",
   "strokeStyle": "solid",
   "strokeWidth": 1,
   "type": "rectangle",
   "version": 4,
-  "versionNonce": 1150084233,
+  "versionNonce": 2019559783,
   "width": 30,
   "x": 30,
   "y": 20,
@@ -39,7 +39,7 @@ Object {
   "strokeWidth": 1,
   "type": "rectangle",
   "version": 5,
-  "versionNonce": 1014066025,
+  "versionNonce": 1116226695,
   "width": 30,
   "x": -10,
   "y": 60,
@@ -62,7 +62,7 @@ Object {
   "strokeWidth": 1,
   "type": "rectangle",
   "version": 3,
-  "versionNonce": 401146281,
+  "versionNonce": 453191,
   "width": 30,
   "x": 0,
   "y": 40,

src/tests/__snapshots__/multiPointCreate.test.tsx.snap

@@ -34,7 +34,7 @@ Object {
   "strokeWidth": 1,
   "type": "arrow",
   "version": 7,
-  "versionNonce": 1116226695,
+  "versionNonce": 1150084233,
   "width": 70,
   "x": 30,
   "y": 30,
@@ -75,7 +75,7 @@ Object {
   "strokeWidth": 1,
   "type": "line",
   "version": 7,
-  "versionNonce": 1116226695,
+  "versionNonce": 1150084233,
   "width": 70,
   "x": 30,
   "y": 30,

src/tests/__snapshots__/resize.test.tsx.snap

@@ -16,7 +16,7 @@ Object {
   "strokeWidth": 1,
   "type": "rectangle",
   "version": 3,
-  "versionNonce": 1150084233,
+  "versionNonce": 401146281,
   "width": 30,
   "x": 29,
   "y": 47,
@@ -39,7 +39,7 @@ Object {
   "strokeWidth": 1,
   "type": "rectangle",
   "version": 3,
-  "versionNonce": 1150084233,
+  "versionNonce": 401146281,
   "width": 30,
   "x": 29,
   "y": 47,

src/tests/regressionTests.test.tsx

@@ -162,6 +162,11 @@ const getSelectedElement = (): ExcalidrawElement => {
   return selectedElements[0];
 };
 
+function getStateHistory() {
+  // @ts-ignore
+  return h.history.stateHistory;
+}
+
 type HandlerRectanglesRet = keyof ReturnType<typeof handlerRectangles>;
 const getResizeHandles = () => {
   const rects = handlerRectangles(
@@ -569,6 +574,46 @@ describe("regression tests", () => {
     expect(h.elements.filter((element) => !element.isDeleted).length).toBe(2);
   });
 
+  it("noop interaction after undo shouldn't create history entry", () => {
+    // NOTE: this will fail if this test case is run in isolation. There's
+    //  some leaking state or race conditions in initialization/teardown
+    //  (couldn't figure out)
+    expect(getStateHistory().length).toBe(0);
+
+    clickTool("rectangle");
+    pointerDown(10, 10);
+    pointerMove(20, 20);
+    pointerUp();
+
+    clickTool("rectangle");
+    pointerDown(30, 10);
+    pointerMove(40, 20);
+    pointerUp();
+
+    expect(getStateHistory().length).toBe(2);
+
+    keyPress("z", true);
+    expect(getStateHistory().length).toBe(1);
+
+    // clicking an element shouldn't addu to history
+    pointerDown(10, 10);
+    pointerUp();
+    expect(getStateHistory().length).toBe(1);
+
+    keyPress("z", true, true);
+    expect(getStateHistory().length).toBe(2);
+
+    // clicking an element shouldn't addu to history
+    pointerDown(10, 10);
+    pointerUp();
+    expect(getStateHistory().length).toBe(2);
+
+    // same for clicking the element just redo-ed
+    pointerDown(30, 10);
+    pointerUp();
+    expect(getStateHistory().length).toBe(2);
+  });
+
   it("zoom hotkeys", () => {
     expect(h.state.zoom).toBe(1);
     fireEvent.keyDown(document, { code: "Equal", ctrlKey: true });