From 82ce068972eda930c72808660efe321a6bb04d79 Mon Sep 17 00:00:00 2001
From: David Luzar <luzar.david@gmail.com>
Date: Thu, 19 Mar 2020 14:51:05 +0100
Subject: [PATCH] fix history (#1009)

* fix history

* tweak withBatchedUpdates typing
---
 src/actions/actionCanvas.tsx            | 11 +++--
 src/actions/actionDeleteSelected.tsx    |  3 +-
 src/actions/actionDuplicateSelection.ts |  1 +
 src/actions/actionExport.tsx            | 15 +++++--
 src/actions/actionFinalize.tsx          |  1 +
 src/actions/actionHistory.tsx           | 15 ++++---
 src/actions/actionMenu.tsx              |  2 +
 src/actions/actionProperties.tsx        | 16 ++++----
 src/actions/actionSelectAll.ts          |  1 +
 src/actions/actionStyles.ts             |  8 ++--
 src/actions/actionZindex.tsx            |  8 ++--
 src/actions/manager.tsx                 | 24 +----------
 src/actions/types.ts                    |  5 +--
 src/components/App.tsx                  | 53 ++++++++++++++-----------
 src/data/index.ts                       |  1 +
 src/history.ts                          |  2 +-
 16 files changed, 86 insertions(+), 80 deletions(-)

diff --git a/src/actions/actionCanvas.tsx b/src/actions/actionCanvas.tsx
index b5e54df9..182da736 100644
--- a/src/actions/actionCanvas.tsx
+++ b/src/actions/actionCanvas.tsx
@@ -14,7 +14,10 @@ import { newElementWith } from "../element/mutateElement";
 export const actionChangeViewBackgroundColor = register({
   name: "changeViewBackgroundColor",
   perform: (_, appState, value) => {
-    return { appState: { ...appState, viewBackgroundColor: value } };
+    return {
+      appState: { ...appState, viewBackgroundColor: value },
+      commitToHistory: true,
+    };
   },
   PanelComponent: ({ appState, updateData }) => {
     return (
@@ -28,18 +31,17 @@ export const actionChangeViewBackgroundColor = register({
       </div>
     );
   },
-  commitToHistory: () => true,
 });
 
 export const actionClearCanvas = register({
   name: "clearCanvas",
-  commitToHistory: () => true,
   perform: elements => {
     return {
       elements: elements.map(element =>
         newElementWith(element, { isDeleted: true }),
       ),
       appState: getDefaultAppState(),
+      commitToHistory: true,
     };
   },
   PanelComponent: ({ updateData }) => (
@@ -81,6 +83,7 @@ export const actionZoomIn = register({
         ...appState,
         zoom: getNormalizedZoom(appState.zoom + ZOOM_STEP),
       },
+      commitToHistory: false,
     };
   },
   PanelComponent: ({ updateData }) => (
@@ -107,6 +110,7 @@ export const actionZoomOut = register({
         ...appState,
         zoom: getNormalizedZoom(appState.zoom - ZOOM_STEP),
       },
+      commitToHistory: false,
     };
   },
   PanelComponent: ({ updateData }) => (
@@ -133,6 +137,7 @@ export const actionResetZoom = register({
         ...appState,
         zoom: 1,
       },
+      commitToHistory: false,
     };
   },
   PanelComponent: ({ updateData }) => (
diff --git a/src/actions/actionDeleteSelected.tsx b/src/actions/actionDeleteSelected.tsx
index e8979274..dcf84341 100644
--- a/src/actions/actionDeleteSelected.tsx
+++ b/src/actions/actionDeleteSelected.tsx
@@ -20,12 +20,11 @@ export const actionDeleteSelected = register({
         elementType: "selection",
         multiElement: null,
       },
+      commitToHistory: isSomeElementSelected(elements, appState),
     };
   },
   contextItemLabel: "labels.delete",
   contextMenuOrder: 3,
-  commitToHistory: (appState, elements) =>
-    isSomeElementSelected(elements, appState),
   keyTest: event => event.key === KEYS.BACKSPACE || event.key === KEYS.DELETE,
   PanelComponent: ({ elements, appState, updateData }) => (
     <ToolButton
diff --git a/src/actions/actionDuplicateSelection.ts b/src/actions/actionDuplicateSelection.ts
index b993df2e..5c057fa1 100644
--- a/src/actions/actionDuplicateSelection.ts
+++ b/src/actions/actionDuplicateSelection.ts
@@ -23,6 +23,7 @@ export const actionDuplicateSelection = register({
         },
         [],
       ),
+      commitToHistory: true,
     };
   },
   contextItemLabel: "labels.duplicateSelection",
diff --git a/src/actions/actionExport.tsx b/src/actions/actionExport.tsx
index 59c4d3db..d790e81f 100644
--- a/src/actions/actionExport.tsx
+++ b/src/actions/actionExport.tsx
@@ -10,7 +10,7 @@ import { register } from "./register";
 export const actionChangeProjectName = register({
   name: "changeProjectName",
   perform: (_elements, appState, value) => {
-    return { appState: { ...appState, name: value } };
+    return { appState: { ...appState, name: value }, commitToHistory: false };
   },
   PanelComponent: ({ appState, updateData }) => (
     <ProjectName
@@ -24,7 +24,10 @@ export const actionChangeProjectName = register({
 export const actionChangeExportBackground = register({
   name: "changeExportBackground",
   perform: (_elements, appState, value) => {
-    return { appState: { ...appState, exportBackground: value } };
+    return {
+      appState: { ...appState, exportBackground: value },
+      commitToHistory: false,
+    };
   },
   PanelComponent: ({ appState, updateData }) => (
     <label>
@@ -42,7 +45,7 @@ export const actionSaveScene = register({
   name: "saveScene",
   perform: (elements, appState, value) => {
     saveAsJSON(elements, appState).catch(error => console.error(error));
-    return {};
+    return { commitToHistory: false };
   },
   PanelComponent: ({ updateData }) => (
     <ToolButton
@@ -63,7 +66,11 @@ export const actionLoadScene = register({
     appState,
     { elements: loadedElements, appState: loadedAppState },
   ) => {
-    return { elements: loadedElements, appState: loadedAppState };
+    return {
+      elements: loadedElements,
+      appState: loadedAppState,
+      commitToHistory: false,
+    };
   },
   PanelComponent: ({ updateData }) => (
     <ToolButton
diff --git a/src/actions/actionFinalize.tsx b/src/actions/actionFinalize.tsx
index 46d0eef8..7ec7c434 100644
--- a/src/actions/actionFinalize.tsx
+++ b/src/actions/actionFinalize.tsx
@@ -52,6 +52,7 @@ export const actionFinalize = register({
         editingElement: null,
         selectedElementIds: {},
       },
+      commitToHistory: false,
     };
   },
   keyTest: (event, appState) =>
diff --git a/src/actions/actionHistory.tsx b/src/actions/actionHistory.tsx
index b5d818d6..1fcdeda3 100644
--- a/src/actions/actionHistory.tsx
+++ b/src/actions/actionHistory.tsx
@@ -1,4 +1,4 @@
-import { Action } from "./types";
+import { Action, ActionResult } from "./types";
 import React from "react";
 import { undo, redo } from "../components/icons";
 import { ToolButton } from "../components/ToolButton";
@@ -13,8 +13,12 @@ import { newElementWith } from "../element/mutateElement";
 const writeData = (
   prevElements: readonly ExcalidrawElement[],
   appState: AppState,
-  updater: () => { elements: ExcalidrawElement[]; appState: AppState } | null,
-) => {
+  updater: () => {
+    elements: ExcalidrawElement[];
+    appState: AppState;
+  } | null,
+): ActionResult => {
+  const commitToHistory = false;
   if (
     !appState.multiElement &&
     !appState.resizingElement &&
@@ -23,7 +27,7 @@ const writeData = (
   ) {
     const data = updater();
     if (data === null) {
-      return {};
+      return { commitToHistory };
     }
 
     const prevElementMap = getElementMap(prevElements);
@@ -47,9 +51,10 @@ const writeData = (
             ),
         ),
       appState: { ...appState, ...data.appState },
+      commitToHistory,
     };
   }
-  return {};
+  return { commitToHistory };
 };
 
 const testUndo = (shift: boolean) => (event: KeyboardEvent) =>
diff --git a/src/actions/actionMenu.tsx b/src/actions/actionMenu.tsx
index dcbfb512..3db133e6 100644
--- a/src/actions/actionMenu.tsx
+++ b/src/actions/actionMenu.tsx
@@ -12,6 +12,7 @@ export const actionToggleCanvasMenu = register({
       ...appState,
       openMenu: appState.openMenu === "canvas" ? null : "canvas",
     },
+    commitToHistory: false,
   }),
   PanelComponent: ({ appState, updateData }) => (
     <ToolButton
@@ -31,6 +32,7 @@ export const actionToggleEditMenu = register({
       ...appState,
       openMenu: appState.openMenu === "shape" ? null : "shape",
     },
+    commitToHistory: false,
   }),
   PanelComponent: ({ elements, appState, updateData }) => (
     <ToolButton
diff --git a/src/actions/actionProperties.tsx b/src/actions/actionProperties.tsx
index 0781d991..c67c1640 100644
--- a/src/actions/actionProperties.tsx
+++ b/src/actions/actionProperties.tsx
@@ -52,9 +52,9 @@ export const actionChangeStrokeColor = register({
         }),
       ),
       appState: { ...appState, currentItemStrokeColor: value },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <>
       <h3 aria-hidden="true">{t("labels.stroke")}</h3>
@@ -83,9 +83,9 @@ export const actionChangeBackgroundColor = register({
         }),
       ),
       appState: { ...appState, currentItemBackgroundColor: value },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <>
       <h3 aria-hidden="true">{t("labels.background")}</h3>
@@ -114,9 +114,9 @@ export const actionChangeFillStyle = register({
         }),
       ),
       appState: { ...appState, currentItemFillStyle: value },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.fill")}</legend>
@@ -151,9 +151,9 @@ export const actionChangeStrokeWidth = register({
         }),
       ),
       appState: { ...appState, currentItemStrokeWidth: value },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.strokeWidth")}</legend>
@@ -186,9 +186,9 @@ export const actionChangeSloppiness = register({
         }),
       ),
       appState: { ...appState, currentItemRoughness: value },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.sloppiness")}</legend>
@@ -221,9 +221,9 @@ export const actionChangeOpacity = register({
         }),
       ),
       appState: { ...appState, currentItemOpacity: value },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <label className="control-label">
       {t("labels.opacity")}
@@ -281,9 +281,9 @@ export const actionChangeFontSize = register({
           appState.currentItemFont.split("px ")[1]
         }`,
       },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.fontSize")}</legend>
@@ -328,9 +328,9 @@ export const actionChangeFontFamily = register({
           appState.currentItemFont.split("px ")[0]
         }px ${value}`,
       },
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.fontFamily")}</legend>
diff --git a/src/actions/actionSelectAll.ts b/src/actions/actionSelectAll.ts
index b2f0e7ca..6ccfd9a8 100644
--- a/src/actions/actionSelectAll.ts
+++ b/src/actions/actionSelectAll.ts
@@ -12,6 +12,7 @@ export const actionSelectAll = register({
           return map;
         }, {} as any),
       },
+      commitToHistory: true,
     };
   },
   contextItemLabel: "labels.selectAll",
diff --git a/src/actions/actionStyles.ts b/src/actions/actionStyles.ts
index e66f4179..fb1f431f 100644
--- a/src/actions/actionStyles.ts
+++ b/src/actions/actionStyles.ts
@@ -17,7 +17,9 @@ export const actionCopyStyles = register({
     if (element) {
       copiedStyles = JSON.stringify(element);
     }
-    return {};
+    return {
+      commitToHistory: false,
+    };
   },
   contextItemLabel: "labels.copyStyles",
   keyTest: event =>
@@ -30,7 +32,7 @@ export const actionPasteStyles = register({
   perform: (elements, appState) => {
     const pastedElement = JSON.parse(copiedStyles);
     if (!isExcalidrawElement(pastedElement)) {
-      return { elements };
+      return { elements, commitToHistory: false };
     }
     return {
       elements: elements.map(element => {
@@ -53,9 +55,9 @@ export const actionPasteStyles = register({
         }
         return element;
       }),
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   contextItemLabel: "labels.pasteStyles",
   keyTest: event =>
     event[KEYS.CTRL_OR_CMD] && event.shiftKey && event.key === "V",
diff --git a/src/actions/actionZindex.tsx b/src/actions/actionZindex.tsx
index 21eeb6d1..5a481ab3 100644
--- a/src/actions/actionZindex.tsx
+++ b/src/actions/actionZindex.tsx
@@ -26,11 +26,11 @@ export const actionSendBackward = register({
         getSelectedIndices(elements, appState),
       ),
       appState,
+      commitToHistory: true,
     };
   },
   contextItemLabel: "labels.sendBackward",
   keyPriority: 40,
-  commitToHistory: () => true,
   keyTest: event =>
     event[KEYS.CTRL_OR_CMD] && !event.shiftKey && event.code === "BracketLeft",
   PanelComponent: ({ updateData }) => (
@@ -54,11 +54,11 @@ export const actionBringForward = register({
         getSelectedIndices(elements, appState),
       ),
       appState,
+      commitToHistory: true,
     };
   },
   contextItemLabel: "labels.bringForward",
   keyPriority: 40,
-  commitToHistory: () => true,
   keyTest: event =>
     event[KEYS.CTRL_OR_CMD] && !event.shiftKey && event.code === "BracketRight",
   PanelComponent: ({ updateData }) => (
@@ -82,10 +82,10 @@ export const actionSendToBack = register({
         getSelectedIndices(elements, appState),
       ),
       appState,
+      commitToHistory: true,
     };
   },
   contextItemLabel: "labels.sendToBack",
-  commitToHistory: () => true,
   keyTest: event => {
     return isDarwin
       ? event[KEYS.CTRL_OR_CMD] && event.altKey && event.code === "BracketLeft"
@@ -118,9 +118,9 @@ export const actionBringToFront = register({
         getSelectedIndices(elements, appState),
       ),
       appState,
+      commitToHistory: true,
     };
   },
-  commitToHistory: () => true,
   contextItemLabel: "labels.bringToFront",
   keyTest: event => {
     return isDarwin
diff --git a/src/actions/manager.tsx b/src/actions/manager.tsx
index 00a09a66..e085cd9d 100644
--- a/src/actions/manager.tsx
+++ b/src/actions/manager.tsx
@@ -50,24 +50,12 @@ export class ActionManager implements ActionsManagerInterface {
     }
 
     event.preventDefault();
-    const commitToHistory =
-      data[0].commitToHistory &&
-      data[0].commitToHistory(this.getAppState(), this.getElements());
-    this.updater(
-      data[0].perform(this.getElements(), this.getAppState(), null),
-      commitToHistory,
-    );
+    this.updater(data[0].perform(this.getElements(), this.getAppState(), null));
     return true;
   }
 
   executeAction(action: Action) {
-    const commitToHistory =
-      action.commitToHistory &&
-      action.commitToHistory(this.getAppState(), this.getElements());
-    this.updater(
-      action.perform(this.getElements(), this.getAppState(), null),
-      commitToHistory,
-    );
+    this.updater(action.perform(this.getElements(), this.getAppState(), null));
   }
 
   getContextMenuItems(actionFilter: ActionFilterFn = action => action) {
@@ -82,12 +70,8 @@ export class ActionManager implements ActionsManagerInterface {
       .map(action => ({
         label: action.contextItemLabel ? t(action.contextItemLabel) : "",
         action: () => {
-          const commitToHistory =
-            action.commitToHistory &&
-            action.commitToHistory(this.getAppState(), this.getElements());
           this.updater(
             action.perform(this.getElements(), this.getAppState(), null),
-            commitToHistory,
           );
         },
       }));
@@ -98,12 +82,8 @@ export class ActionManager implements ActionsManagerInterface {
       const action = this.actions[name];
       const PanelComponent = action.PanelComponent!;
       const updateData = (formState?: any) => {
-        const commitToHistory =
-          action.commitToHistory &&
-          action.commitToHistory(this.getAppState(), this.getElements());
         this.updater(
           action.perform(this.getElements(), this.getAppState(), formState),
-          commitToHistory,
         );
       };
 
diff --git a/src/actions/types.ts b/src/actions/types.ts
index c1fe7251..609c9c2b 100644
--- a/src/actions/types.ts
+++ b/src/actions/types.ts
@@ -5,6 +5,7 @@ import { AppState } from "../types";
 export type ActionResult = {
   elements?: readonly ExcalidrawElement[] | null;
   appState?: AppState | null;
+  commitToHistory: boolean;
 };
 
 type ActionFn = (
@@ -32,10 +33,6 @@ export interface Action {
   ) => boolean;
   contextItemLabel?: string;
   contextMenuOrder?: number;
-  commitToHistory?: (
-    appState: AppState,
-    elements: readonly ExcalidrawElement[],
-  ) => boolean;
 }
 
 export interface ActionsManagerInterface {
diff --git a/src/components/App.tsx b/src/components/App.tsx
index 89cf76ec..606f0b67 100644
--- a/src/components/App.tsx
+++ b/src/components/App.tsx
@@ -105,11 +105,14 @@ import { isLinearElement } from "../element/typeChecks";
 import { rescalePoints } from "../points";
 import { actionFinalize } from "../actions";
 
+/**
+ * @param func handler taking at most single parameter (event).
+ */
 function withBatchedUpdates<
   TFunction extends ((event: any) => void) | (() => void)
->(func: TFunction) {
+>(func: Parameters<TFunction>["length"] extends 0 | 1 ? TFunction : never) {
   return (event => {
-    unstable_batchedUpdates(func, event);
+    unstable_batchedUpdates(func as TFunction, event);
   }) as TFunction;
 }
 
@@ -164,30 +167,28 @@ export class App extends React.Component<any, AppState> {
     this.actionManager.registerAction(createRedoAction(history));
   }
 
-  private syncActionResult = withBatchedUpdates(
-    (res: ActionResult, commitToHistory: boolean = true) => {
-      if (this.unmounted) {
-        return;
-      }
-      if (res.elements) {
-        globalSceneState.replaceAllElements(res.elements);
-        if (commitToHistory) {
-          history.resumeRecording();
-        }
+  private syncActionResult = withBatchedUpdates((res: ActionResult) => {
+    if (this.unmounted) {
+      return;
+    }
+    if (res.elements) {
+      globalSceneState.replaceAllElements(res.elements);
+      if (res.commitToHistory) {
+        history.resumeRecording();
       }
+    }
 
-      if (res.appState) {
-        if (commitToHistory) {
-          history.resumeRecording();
-        }
-        this.setState(state => ({
-          ...res.appState,
-          isCollaborating: state.isCollaborating,
-          collaborators: state.collaborators,
-        }));
+    if (res.appState) {
+      if (res.commitToHistory) {
+        history.resumeRecording();
       }
-    },
-  );
+      this.setState(state => ({
+        ...res.appState,
+        isCollaborating: state.isCollaborating,
+        collaborators: state.collaborators,
+      }));
+    }
+  });
 
   private onCut = withBatchedUpdates((event: ClipboardEvent) => {
     if (isWritableElement(event.target)) {
@@ -917,7 +918,11 @@ export class App extends React.Component<any, AppState> {
               ) {
                 loadFromBlob(file)
                   .then(({ elements, appState }) =>
-                    this.syncActionResult({ elements, appState }),
+                    this.syncActionResult({
+                      elements,
+                      appState,
+                      commitToHistory: false,
+                    }),
                   )
                   .catch(error => console.error(error));
               }
diff --git a/src/data/index.ts b/src/data/index.ts
index b4b5ce0e..1b97a5df 100644
--- a/src/data/index.ts
+++ b/src/data/index.ts
@@ -357,5 +357,6 @@ export async function loadScene(id: string | null, privateKey?: string) {
   return {
     elements: data.elements,
     appState: data.appState && { ...data.appState },
+    commitToHistory: false,
   };
 }
diff --git a/src/history.ts b/src/history.ts
index 925d096a..f12f5231 100644
--- a/src/history.ts
+++ b/src/history.ts
@@ -83,7 +83,7 @@ export class SceneHistory {
   }
 
   undoOnce(): Result | null {
-    if (this.stateHistory.length === 0) {
+    if (this.stateHistory.length === 1) {
       return null;
     }