Fix issues related to history (#701)

* Separate UI from Canvas

* Explicitly define history recording

* ActionManager: Set syncActionState during construction instead of in every call

* Add commit to history flag to necessary actions

* Disable undoing during multiElement

* Write custom equality function for UI component to render it only when specific props and elements change

* Remove stale comments about history skipping

* Stop undo/redoing when in resizing element mode

* wip

* correctly reset resizingElement & add undo check

* Separate selection element from the rest of the array and stop redrawing the UI when dragging the selection

* Remove selectionElement from local storage

* Remove unnecessary readonly type casting in actionFinalize

* Fix undo / redo for multi points

* Fix an issue that did not update history when elements were locked

* Disable committing to history for noops

- deleteSelected without deleting anything
- Basic selection

* Use generateEntry only inside history and pass elements and appstate to history

* Update component after every history resume

* Remove last item from the history only if in multi mode

* Resume recording when element type is not selection

* ensure we prevent hotkeys only on writable elements

* Remove selection clearing from history

* Remove one point arrows as they are invisibly small

* Remove shape of elements from local storage

* Fix removing invisible element from the array

* add missing history resuming cases & simplify slice

* fix lint

* don't regenerate elements if no elements deselected

* regenerate elements array on selection

* reset state.selectionElement unconditionally

* Use getter instead of passing appState and scene data through functions to actions

* fix import

Co-authored-by: David Luzar <luzar.david@gmail.com>

src/actions/actionCanvas.tsx

@@ -8,7 +8,7 @@ import { t } from "../i18n";
 
 export const actionChangeViewBackgroundColor: Action = {
   name: "changeViewBackgroundColor",
-  perform: (elements, appState, value) => {
+  perform: (_, appState, value) => {
     return { appState: { ...appState, viewBackgroundColor: value } };
   },
   PanelComponent: ({ appState, updateData }) => {
@@ -23,10 +23,12 @@ export const actionChangeViewBackgroundColor: Action = {
       </div>
     );
   },
+  commitToHistory: () => true,
 };
 
 export const actionClearCanvas: Action = {
   name: "clearCanvas",
+  commitToHistory: () => true,
   perform: () => {
     return {
       elements: [],

src/actions/actionDeleteSelected.tsx

@@ -12,5 +12,6 @@ export const actionDeleteSelected: Action = {
   },
   contextItemLabel: "labels.delete",
   contextMenuOrder: 3,
+  commitToHistory: (_, elements) => elements.some(el => el.isSelected),
   keyTest: event => event.key === KEYS.BACKSPACE || event.key === KEYS.DELETE,
 };

src/actions/actionFinalize.tsx

@@ -1,10 +1,12 @@
 import { Action } from "./types";
 import { KEYS } from "../keys";
 import { clearSelection } from "../scene";
+import { isInvisiblySmallElement } from "../element";
 
 export const actionFinalize: Action = {
   name: "finalize",
   perform: (elements, appState) => {
+    let newElements = clearSelection(elements);
     if (window.document.activeElement instanceof HTMLElement) {
       window.document.activeElement.blur();
     }
@@ -13,10 +15,13 @@ export const actionFinalize: Action = {
         0,
         appState.multiElement.points.length - 1,
       );
+      if (isInvisiblySmallElement(appState.multiElement)) {
+        newElements = newElements.slice(0, -1);
+      }
       appState.multiElement.shape = null;
     }
     return {
-      elements: clearSelection(elements),
+      elements: newElements,
       appState: {
         ...appState,
         elementType: "selection",

src/actions/actionProperties.tsx

@@ -47,6 +47,7 @@ export const actionChangeStrokeColor: Action = {
       appState: { ...appState, currentItemStrokeColor: value },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <>
       <h3 aria-hidden="true">{t("labels.stroke")}</h3>
@@ -77,6 +78,7 @@ export const actionChangeBackgroundColor: Action = {
       appState: { ...appState, currentItemBackgroundColor: value },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <>
       <h3 aria-hidden="true">{t("labels.background")}</h3>
@@ -107,6 +109,7 @@ export const actionChangeFillStyle: Action = {
       appState: { ...appState, currentItemFillStyle: value },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.fill")}</legend>
@@ -143,6 +146,7 @@ export const actionChangeStrokeWidth: Action = {
       appState: { ...appState, currentItemStrokeWidth: value },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.strokeWidth")}</legend>
@@ -177,6 +181,7 @@ export const actionChangeSloppiness: Action = {
       appState: { ...appState, currentItemRoughness: value },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.sloppiness")}</legend>
@@ -211,6 +216,7 @@ export const actionChangeOpacity: Action = {
       appState: { ...appState, currentItemOpacity: value },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <label className="control-label">
       {t("labels.opacity")}
@@ -272,6 +278,7 @@ export const actionChangeFontSize: Action = {
       },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.fontSize")}</legend>
@@ -320,6 +327,7 @@ export const actionChangeFontFamily: Action = {
       },
     };
   },
+  commitToHistory: () => true,
   PanelComponent: ({ elements, appState, updateData }) => (
     <fieldset>
       <legend>{t("labels.fontFamily")}</legend>

src/actions/actionStyles.ts

@@ -45,6 +45,7 @@ export const actionPasteStyles: Action = {
       }),
     };
   },
+  commitToHistory: () => true,
   contextItemLabel: "labels.pasteStyles",
   keyTest: event => event[KEYS.META] && event.shiftKey && event.key === "V",
   contextMenuOrder: 1,

src/actions/actionZindex.tsx

@@ -18,6 +18,7 @@ export const actionSendBackward: Action = {
   },
   contextItemLabel: "labels.sendBackward",
   keyPriority: 40,
+  commitToHistory: () => true,
   keyTest: event => event[KEYS.META] && event.altKey && event.key === "B",
 };
 
@@ -31,6 +32,7 @@ export const actionBringForward: Action = {
   },
   contextItemLabel: "labels.bringForward",
   keyPriority: 40,
+  commitToHistory: () => true,
   keyTest: event => event[KEYS.META] && event.altKey && event.key === "F",
 };
 
@@ -43,6 +45,7 @@ export const actionSendToBack: Action = {
     };
   },
   contextItemLabel: "labels.sendToBack",
+  commitToHistory: () => true,
   keyTest: event => event[KEYS.META] && event.shiftKey && event.key === "B",
 };
 
@@ -54,6 +57,7 @@ export const actionBringToFront: Action = {
       appState,
     };
   },
+  commitToHistory: () => true,
   contextItemLabel: "labels.bringToFront",
   keyTest: event => event[KEYS.META] && event.shiftKey && event.key === "F",
 };

src/actions/manager.tsx

@@ -12,29 +12,37 @@ import { t } from "../i18n";
 export class ActionManager implements ActionsManagerInterface {
   actions: { [keyProp: string]: Action } = {};
 
-  updater:
-    | ((elements: ExcalidrawElement[], appState: AppState) => void)
-    | null = null;
+  updater: UpdaterFn;
 
-  setUpdater(
-    updater: (elements: ExcalidrawElement[], appState: AppState) => void,
+  resumeHistoryRecording: () => void;
+
+  getAppState: () => AppState;
+
+  getElements: () => readonly ExcalidrawElement[];
+
+  constructor(
+    updater: UpdaterFn,
+    resumeHistoryRecording: () => void,
+    getAppState: () => AppState,
+    getElements: () => readonly ExcalidrawElement[],
   ) {
     this.updater = updater;
+    this.resumeHistoryRecording = resumeHistoryRecording;
+    this.getAppState = getAppState;
+    this.getElements = getElements;
   }
 
   registerAction(action: Action) {
     this.actions[action.name] = action;
   }
 
-  handleKeyDown(
-    event: KeyboardEvent,
-    elements: readonly ExcalidrawElement[],
-    appState: AppState,
-  ) {
+  handleKeyDown(event: KeyboardEvent) {
     const data = Object.values(this.actions)
       .sort((a, b) => (b.keyPriority || 0) - (a.keyPriority || 0))
       .filter(
-        action => action.keyTest && action.keyTest(event, appState, elements),
+        action =>
+          action.keyTest &&
+          action.keyTest(event, this.getAppState(), this.getElements()),
       );
 
     if (data.length === 0) {
@@ -42,15 +50,16 @@ export class ActionManager implements ActionsManagerInterface {
     }
 
     event.preventDefault();
-    return data[0].perform(elements, appState, null);
+    if (
+      data[0].commitToHistory &&
+      data[0].commitToHistory(this.getAppState(), this.getElements())
+    ) {
+      this.resumeHistoryRecording();
+    }
+    return data[0].perform(this.getElements(), this.getAppState(), null);
   }
 
-  getContextMenuItems(
-    elements: readonly ExcalidrawElement[],
-    appState: AppState,
-    updater: UpdaterFn,
-    actionFilter: ActionFilterFn = action => action,
-  ) {
+  getContextMenuItems(actionFilter: ActionFilterFn = action => action) {
     return Object.values(this.actions)
       .filter(actionFilter)
       .filter(action => "contextItemLabel" in action)
@@ -62,28 +71,40 @@ export class ActionManager implements ActionsManagerInterface {
       .map(action => ({
         label: action.contextItemLabel ? t(action.contextItemLabel) : "",
         action: () => {
-          updater(action.perform(elements, appState, null));
+          if (
+            action.commitToHistory &&
+            action.commitToHistory(this.getAppState(), this.getElements())
+          ) {
+            this.resumeHistoryRecording();
+          }
+          this.updater(
+            action.perform(this.getElements(), this.getAppState(), null),
+          );
         },
       }));
   }
 
-  renderAction(
-    name: string,
-    elements: readonly ExcalidrawElement[],
-    appState: AppState,
-    updater: UpdaterFn,
-  ) {
+  renderAction(name: string) {
     if (this.actions[name] && "PanelComponent" in this.actions[name]) {
       const action = this.actions[name];
       const PanelComponent = action.PanelComponent!;
       const updateData = (formState: any) => {
-        updater(action.perform(elements, appState, formState));
+        if (
+          action.commitToHistory &&
+          action.commitToHistory(this.getAppState(), this.getElements()) ===
+            true
+        ) {
+          this.resumeHistoryRecording();
+        }
+        this.updater(
+          action.perform(this.getElements(), this.getAppState(), formState),
+        );
       };
 
       return (
         <PanelComponent
-          elements={elements}
-          appState={appState}
+          elements={this.getElements()}
+          appState={this.getAppState()}
           updateData={updateData}
         />
       );

src/actions/types.ts

@@ -3,7 +3,7 @@ import { ExcalidrawElement } from "../element/types";
 import { AppState } from "../types";
 
 export type ActionResult = {
-  elements?: ExcalidrawElement[];
+  elements?: readonly ExcalidrawElement[];
   appState?: AppState;
 };
 
@@ -32,6 +32,10 @@ export interface Action {
   ) => boolean;
   contextItemLabel?: string;
   contextMenuOrder?: number;
+  commitToHistory?: (
+    appState: AppState,
+    elements: readonly ExcalidrawElement[],
+  ) => boolean;
 }
 
 export interface ActionsManagerInterface {
@@ -39,21 +43,9 @@ export interface ActionsManagerInterface {
     [keyProp: string]: Action;
   };
   registerAction: (action: Action) => void;
-  handleKeyDown: (
-    event: KeyboardEvent,
-    elements: readonly ExcalidrawElement[],
-    appState: AppState,
-  ) => ActionResult | null;
+  handleKeyDown: (event: KeyboardEvent) => ActionResult | null;
   getContextMenuItems: (
-    elements: readonly ExcalidrawElement[],
-    appState: AppState,
-    updater: UpdaterFn,
     actionFilter: ActionFilterFn,
   ) => { label: string; action: () => void }[];
-  renderAction: (
-    name: string,
-    elements: readonly ExcalidrawElement[],
-    appState: AppState,
-    updater: UpdaterFn,
-  ) => React.ReactElement | null;
+  renderAction: (name: string) => React.ReactElement | null;
 }