Simplify: universal Action predicates instead of action-specific guards.

src/actions/guards.ts

@@ -1,25 +0,0 @@
-import { Action, ActionName, DisableFn, EnableFn } from "./types";
-
-const disablers = {} as Record<ActionName, DisableFn[]>;
-const enablers = {} as Record<Action["name"], EnableFn[]>;
-
-export const getActionDisablers = () => disablers;
-export const getActionEnablers = () => enablers;
-
-export const registerDisableFn = (name: ActionName, disabler: DisableFn) => {
-  if (!(name in disablers)) {
-    disablers[name] = [] as DisableFn[];
-  }
-  if (!disablers[name].includes(disabler)) {
-    disablers[name].push(disabler);
-  }
-};
-
-export const registerEnableFn = (name: Action["name"], enabler: EnableFn) => {
-  if (!(name in enablers)) {
-    enablers[name] = [] as EnableFn[];
-  }
-  if (!enablers[name].includes(enabler)) {
-    enablers[name].push(enabler);
-  }
-};

src/actions/manager.tsx

@@ -2,15 +2,12 @@ import React from "react";
 import {
   Action,
   UpdaterFn,
-  ActionName,
   ActionResult,
   PanelComponentProps,
   ActionSource,
-  DisableFn,
-  EnableFn,
+  ActionPredicateFn,
   isActionName,
 } from "./types";
-import { getActionDisablers, getActionEnablers } from "./guards";
 import { ExcalidrawElement } from "../element/types";
 import { AppClassProperties, AppState } from "../types";
 import { trackEvent } from "../analytics";
@@ -44,10 +41,8 @@ const trackAction = (
 };
 
 export class ActionManager {
-  actions = {} as Record<ActionName | Action["name"], Action>;
-
-  disablers = {} as Record<ActionName, DisableFn[]>;
-  enablers = {} as Record<Action["name"], EnableFn[]>;
+  actions = {} as Record<Action["name"], Action>;
+  actionPredicates = [] as ActionPredicateFn[];
 
   updater: (actionResult: ActionResult | Promise<ActionResult>) => void;
 
@@ -75,36 +70,9 @@ export class ActionManager {
     this.app = app;
   }
 
-  registerActionGuards() {
-    const disablers = getActionDisablers();
-    for (const d in disablers) {
-      const dName = d as ActionName;
-      disablers[dName].forEach((disabler) =>
-        this.registerDisableFn(dName, disabler),
-      );
-    }
-    const enablers = getActionEnablers();
-    for (const e in enablers) {
-      const eName = e as Action["name"];
-      enablers[e].forEach((enabler) => this.registerEnableFn(eName, enabler));
-    }
-  }
-
-  registerDisableFn(name: ActionName, disabler: DisableFn) {
-    if (!(name in this.disablers)) {
-      this.disablers[name] = [] as DisableFn[];
-    }
-    if (!this.disablers[name].includes(disabler)) {
-      this.disablers[name].push(disabler);
-    }
-  }
-
-  registerEnableFn(name: Action["name"], enabler: EnableFn) {
-    if (!(name in this.enablers)) {
-      this.enablers[name] = [] as EnableFn[];
-    }
-    if (!this.enablers[name].includes(enabler)) {
-      this.enablers[name].push(enabler);
+  registerActionPredicate(predicate: ActionPredicateFn) {
+    if (!this.actionPredicates.includes(predicate)) {
+      this.actionPredicates.push(predicate);
     }
   }
 
@@ -196,10 +164,7 @@ export class ActionManager {
   /**
    * @param data additional data sent to the PanelComponent
    */
-  renderAction = (
-    name: ActionName | Action["name"],
-    data?: PanelComponentProps["data"],
-  ) => {
+  renderAction = (name: Action["name"], data?: PanelComponentProps["data"]) => {
     const canvasActions = this.app.props.UIOptions.canvasActions;
 
     if (
@@ -243,7 +208,7 @@ export class ActionManager {
   };
 
   isActionEnabled = (
-    action: Action | ActionName,
+    action: Action,
     opts?: {
       elements?: readonly ExcalidrawElement[];
       data?: Record<string, any>;
@@ -254,29 +219,19 @@ export class ActionManager {
     const appState = this.getAppState();
     const data = opts?.data;
 
-    const _action = isActionName(action) ? this.actions[action] : action;
-
     if (
       !opts?.guardsOnly &&
-      _action.predicate &&
-      !_action.predicate(elements, appState, this.app.props, this.app, data)
+      action.predicate &&
+      !action.predicate(elements, appState, this.app.props, this.app, data)
     ) {
       return false;
     }
-
-    if (isActionName(_action.name)) {
-      return !(
-        _action.name in this.disablers &&
-        this.disablers[_action.name].some((fn) =>
-          fn(elements, appState, _action.name as ActionName),
-        )
-      );
-    }
-    return (
-      _action.name in this.enablers &&
-      this.enablers[_action.name].some((fn) =>
-        fn(elements, appState, _action.name),
-      )
-    );
+    let enabled = true;
+    this.actionPredicates.forEach((fn) => {
+      if (!fn(action, elements, appState, data)) {
+        enabled = false;
+      }
+    });
+    return enabled;
   };
 }

src/actions/types.ts

@@ -31,20 +31,13 @@ type ActionFn = (
   app: AppClassProperties,
 ) => ActionResult | Promise<ActionResult>;
 
-// Return `true` to indicate the standard Action with name `actionName`
-// should be disabled given `elements` and `appState`.
-export type DisableFn = (
+// Return `true` *unless* `Action` should be disabled
+// given `elements`, `appState`, and optionally `data`.
+export type ActionPredicateFn = (
+  action: Action,
   elements: readonly ExcalidrawElement[],
   appState: AppState,
-  actionName: ActionName,
-) => boolean;
-
-// Return `true` to indicate the custom Action with name `actionName`
-// should be enabled given `elements` and `appState`.
-export type EnableFn = (
-  elements: readonly ExcalidrawElement[],
-  appState: AppState,
-  actionName: Action["name"],
+  data?: Record<string, any>,
 ) => boolean;
 
 export type UpdaterFn = (res: ActionResult) => void;

src/components/App.tsx

@@ -480,7 +480,6 @@ class App extends React.Component<AppProps, AppState> {
     });
     this.history = new History();
     this.actionManager.registerAll(actions);
-    this.actionManager.registerActionGuards();
 
     this.actionManager.registerAction(createUndoAction(this.history));
     this.actionManager.registerAction(createRedoAction(this.history));

src/packages/excalidraw/example/App.tsx

@@ -30,7 +30,7 @@ import { NonDeletedExcalidrawElement } from "../../../element/types";
 import { ImportedLibraryData } from "../../../data/types";
 import CustomFooter from "./CustomFooter";
 import MobileFooter from "./MobileFooter";
-import { Action, EnableFn } from "../../../actions/types";
+import { Action, ActionPredicateFn } from "../../../actions/types";
 import { ContextMenuItems } from "../../../components/ContextMenu";
 
 const exampleAction: Action = {
@@ -43,8 +43,9 @@ const exampleAction: Action = {
     data === undefined || data.source === "custom",
   contextItemLabel: "labels.untitled",
 };
-const exampleEnableFn: EnableFn = (elements, appState, actionName) =>
-  actionName === "example";
+const examplePredicateFn: ActionPredicateFn = (action, elements) =>
+  action.name !== "example" ||
+  !elements.some((el) => el.type === "text" && !el.isDeleted);
 
 declare global {
   interface Window {
@@ -128,7 +129,7 @@ export default function App() {
       return;
     }
     excalidrawAPI.actionManager.registerAction(exampleAction);
-    excalidrawAPI.actionManager.registerEnableFn("example", exampleEnableFn);
+    excalidrawAPI.actionManager.registerActionPredicate(examplePredicateFn);
     const fetchData = async () => {
       const res = await fetch("/rocket.jpeg");
       const imageData = await res.blob();
@@ -177,11 +178,12 @@ export default function App() {
             excalidrawAPI?.actionManager
               .getCustomActions({ data: { source: "custom" } })
               .forEach((action) => items.push(action));
-            excalidrawAPI?.updateScene({
-              appState: {
-                contextMenu: { top, left, items },
-              },
-            });
+            items.length > 0 &&
+              excalidrawAPI?.updateScene({
+                appState: {
+                  contextMenu: { top, left, items },
+                },
+              });
           }}
         >
           {" "}

src/tests/customActions.test.tsx

@@ -1,18 +1,19 @@
 import { ExcalidrawElement } from "../element/types";
 import { getShortcutKey } from "../utils";
 import { API } from "./helpers/api";
+import { render } from "./test-utils";
+import ExcalidrawApp from "../excalidraw-app";
 import {
   CustomShortcutName,
   getShortcutFromShortcutName,
   registerCustomShortcuts,
 } from "../actions/shortcuts";
-import { Action, ActionName, DisableFn, EnableFn } from "../actions/types";
+import { Action, ActionPredicateFn, ActionResult } from "../actions/types";
 import {
-  getActionDisablers,
-  getActionEnablers,
-  registerDisableFn,
-  registerEnableFn,
-} from "../actions/guards";
+  actionChangeFontFamily,
+  actionChangeFontSize,
+} from "../actions/actionProperties";
+import { isTextElement } from "../element";
 
 const { h } = window;
 
@@ -25,61 +26,60 @@ describe("regression tests", () => {
     expect(getShortcutFromShortcutName("test")).toBe("Ctrl+1");
   });
 
-  it("should follow action guards", () => {
+  it("should apply universal action predicates", async () => {
+    await render(<ExcalidrawApp />);
     // Create the test elements
-    const text1 = API.createElement({ type: "rectangle", id: "A", y: 0 });
-    const text2 = API.createElement({ type: "rectangle", id: "B", y: 30 });
-    const text3 = API.createElement({ type: "rectangle", id: "C", y: 60 });
-    const el12: ExcalidrawElement[] = [text1, text2];
-    const el13: ExcalidrawElement[] = [text1, text3];
-    const el23: ExcalidrawElement[] = [text2, text3];
-    const el123: ExcalidrawElement[] = [text1, text2, text3];
+    const el1 = API.createElement({ type: "rectangle", id: "A", y: 0 });
+    const el2 = API.createElement({ type: "rectangle", id: "B", y: 30 });
+    const el3 = API.createElement({ type: "text", id: "C", y: 60 });
+    const el12: ExcalidrawElement[] = [el1, el2];
+    const el13: ExcalidrawElement[] = [el1, el3];
+    const el23: ExcalidrawElement[] = [el2, el3];
+    const el123: ExcalidrawElement[] = [el1, el2, el3];
     // Set up the custom Action enablers
     const enableName = "custom" as Action["name"];
-    const enabler: EnableFn = function (elements) {
-      if (elements.some((el) => el.y === 30)) {
+    const enableAction: Action = {
+      name: enableName,
+      perform: (): ActionResult => {
+        return {} as ActionResult;
+      },
+      trackEvent: false,
+    };
+    const enabler: ActionPredicateFn = function (action, elements) {
+      if (action.name !== enableName || elements.some((el) => el.y === 30)) {
         return true;
       }
       return false;
     };
-    registerEnableFn(enableName, enabler);
     // Set up the standard Action disablers
-    const disableName1 = "changeFontFamily" as ActionName;
-    const disableName2 = "changeFontSize" as ActionName;
-    const disabler: DisableFn = function (elements) {
-      if (elements.some((el) => el.y === 0)) {
-        return true;
+    const disabled1 = actionChangeFontFamily;
+    const disabled2 = actionChangeFontSize;
+    const disabler: ActionPredicateFn = function (action, elements) {
+      if (
+        action.name === disabled2.name &&
+        elements.some((el) => el.y === 0 || isTextElement(el))
+      ) {
+        return false;
       }
-      return false;
+      return true;
     };
-    registerDisableFn(disableName1, disabler);
     // Test the custom Action enablers
-    const enablers = getActionEnablers();
-    const isCustomEnabled = function (
-      elements: ExcalidrawElement[],
-      name: string,
-    ) {
-      return (
-        name in enablers &&
-        enablers[name].some((enabler) => enabler(elements, h.state, name))
-      );
-    };
-    expect(isCustomEnabled(el12, enableName)).toBe(true);
-    expect(isCustomEnabled(el13, enableName)).toBe(false);
-    expect(isCustomEnabled(el23, enableName)).toBe(true);
+    const am = h.app.actionManager;
+    am.registerActionPredicate(enabler);
+    expect(am.isActionEnabled(enableAction, { elements: el12 })).toBe(true);
+    expect(am.isActionEnabled(enableAction, { elements: el13 })).toBe(false);
+    expect(am.isActionEnabled(enableAction, { elements: el23 })).toBe(true);
+    expect(am.isActionEnabled(disabled1, { elements: el12 })).toBe(true);
+    expect(am.isActionEnabled(disabled1, { elements: el13 })).toBe(true);
+    expect(am.isActionEnabled(disabled1, { elements: el23 })).toBe(true);
     // Test the standard Action disablers
-    const disablers = getActionDisablers();
-    const isStandardDisabled = function (
-      elements: ExcalidrawElement[],
-      name: ActionName,
-    ) {
-      return (
-        name in disablers &&
-        disablers[name].some((disabler) => disabler(elements, h.state, name))
-      );
-    };
-    expect(isStandardDisabled(el12, disableName1)).toBe(true);
-    expect(isStandardDisabled(el23, disableName1)).toBe(false);
-    expect(isStandardDisabled(el123, disableName2)).toBe(false);
+    am.registerActionPredicate(disabler);
+    expect(am.isActionEnabled(disabled1, { elements: el123 })).toBe(true);
+    expect(am.isActionEnabled(disabled2, { elements: [el1] })).toBe(false);
+    expect(am.isActionEnabled(disabled2, { elements: [el2] })).toBe(true);
+    expect(am.isActionEnabled(disabled2, { elements: [el3] })).toBe(false);
+    expect(am.isActionEnabled(disabled2, { elements: el12 })).toBe(false);
+    expect(am.isActionEnabled(disabled2, { elements: el23 })).toBe(false);
+    expect(am.isActionEnabled(disabled2, { elements: el13 })).toBe(false);
   });
 });