Simplify custom Actions: 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,
+  ActionPredicateFn,
-  EnableFn,
   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>;
+  actions = {} as Record<Action["name"], Action>;
-
+  actionPredicates = [] as ActionPredicateFn[];
-  disablers = {} as Record<ActionName, DisableFn[]>;
-  enablers = {} as Record<Action["name"], EnableFn[]>;
 
   updater: (actionResult: ActionResult | Promise<ActionResult>) => void;
 
@@ -75,36 +70,9 @@ export class ActionManager {
     this.app = app;
   }
 
-  registerActionGuards() {
+  registerActionPredicate(predicate: ActionPredicateFn) {
-    const disablers = getActionDisablers();
+    if (!this.actionPredicates.includes(predicate)) {
-    for (const d in disablers) {
+      this.actionPredicates.push(predicate);
-      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);
     }
   }
 
@@ -196,10 +164,7 @@ export class ActionManager {
   /**
    * @param data additional data sent to the PanelComponent
    */
-  renderAction = (
+  renderAction = (name: Action["name"], data?: PanelComponentProps["data"]) => {
-    name: ActionName | 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 &&
-      !_action.predicate(elements, appState, this.app.props, this.app, data)
+      !action.predicate(elements, appState, this.app.props, this.app, data)
     ) {
       return false;
     }
-
+    let enabled = true;
-    if (isActionName(_action.name)) {
+    this.actionPredicates.forEach((fn) => {
-      return !(
+      if (!fn(action, elements, appState, data)) {
-        _action.name in this.disablers &&
+        enabled = false;
-        this.disablers[_action.name].some((fn) =>
-          fn(elements, appState, _action.name as ActionName),
-        )
-      );
       }
-    return (
+    });
-      _action.name in this.enablers &&
+    return enabled;
-      this.enablers[_action.name].some((fn) =>
-        fn(elements, appState, _action.name),
-      )
-    );
   };
 }

src/actions/register.ts

@@ -1,14 +1,8 @@
-import { Action, isActionName } from "./types";
+import { Action } from "./types";
 
-let actions: readonly Action[] = [];
+export let actions: readonly Action[] = [];
-let customActions: readonly Action[] = [];
-export const getCustomActions = () => customActions;
-export const getActions = () => actions;
 
 export const register = <T extends Action>(action: T) => {
-  if (!isActionName(action.name)) {
-    customActions = customActions.concat(action);
-  }
   actions = actions.concat(action);
   return action as T & {
     keyTest?: unknown extends T["keyTest"] ? never : T["keyTest"];

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`
+// Return `true` *unless* `Action` should be disabled
-// should be disabled given `elements` and `appState`.
+// given `elements`, `appState`, and optionally `data`.
-export type DisableFn = (
+export type ActionPredicateFn = (
+  action: Action,
   elements: readonly ExcalidrawElement[],
   appState: AppState,
-  actionName: ActionName,
+  data?: Record<string, any>,
-) => 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"],
 ) => boolean;
 
 export type UpdaterFn = (res: ActionResult) => void;

src/components/App.tsx

@@ -38,7 +38,7 @@ import {
 } from "../actions";
 import { createRedoAction, createUndoAction } from "../actions/actionHistory";
 import { ActionManager } from "../actions/manager";
-import { getActions } from "../actions/register";
+import { actions } from "../actions/register";
 import { ActionResult } from "../actions/types";
 import { trackEvent } from "../analytics";
 import {
@@ -241,6 +241,7 @@ import {
   SubtypePrepFn,
   prepareSubtype,
   selectSubtype,
+  subtypeActionPredicate,
 } from "../subtypes";
 import {
   dataURLToFile,
@@ -490,12 +491,12 @@ class App extends React.Component<AppProps, AppState> {
       onSceneUpdated: this.onSceneUpdated,
     });
     this.history = new History();
-    this.actionManager.registerAll(getActions());
+    this.actionManager.registerAll(actions);
 
     this.actionManager.registerAction(createUndoAction(this.history));
     this.actionManager.registerAction(createRedoAction(this.history));
     // Call `this.addSubtype()` here for `@excalidraw/excalidraw`-specific subtypes
-    this.actionManager.registerActionGuards();
+    this.actionManager.registerActionPredicate(subtypeActionPredicate);
   }
 
   private addSubtype(record: SubtypeRecord, subtypePrepFn: SubtypePrepFn) {
@@ -527,7 +528,6 @@ class App extends React.Component<AppProps, AppState> {
     if (prep.actions) {
       this.actionManager.registerAll(prep.actions);
     }
-    this.actionManager.registerActionGuards();
     return prep;
   }
 

src/subtypes.ts

@@ -8,13 +8,12 @@ import { getSelectedElements } from "./scene";
 import { AppState } from "./types";
 import { registerAuxLangData } from "./i18n";
 
-import { Action, ActionName, DisableFn, EnableFn } from "./actions/types";
+import { Action, ActionName, ActionPredicateFn } from "./actions/types";
 import {
   CustomShortcutName,
   registerCustomShortcuts,
 } from "./actions/shortcuts";
 import { register } from "./actions/register";
-import { registerDisableFn, registerEnableFn } from "./actions/guards";
 import { hasBoundTextElement } from "./element/typeChecks";
 import { getBoundTextElement } from "./element/textElement";
 
@@ -100,16 +99,16 @@ const isForSubtype = (
   return false;
 };
 
-const isActionDisabled: DisableFn = function (elements, appState, actionName) {
+export const subtypeActionPredicate: ActionPredicateFn = function (
-  return !isActionEnabled(elements, appState, actionName);
+  action,
-};
+  elements,
-
+  appState,
-const isActionEnabled: EnableFn = function (elements, appState, actionName) {
+) {
   // We always enable subtype actions.  Also let through standard actions
   // which no subtypes might have disabled.
   if (
-    isSubtypeName(actionName) ||
+    isSubtypeName(action.name) ||
-    (!isSubtypeActionName(actionName) && !isDisabledActionName(actionName))
+    (!isSubtypeActionName(action.name) && !isDisabledActionName(action.name))
   ) {
     return true;
   }
@@ -121,13 +120,13 @@ const isActionEnabled: EnableFn = function (elements, appState, actionName) {
     ? [appState.editingElement, ...selectedElements]
     : selectedElements;
   // Now handle actions added by subtypes
-  if (isSubtypeActionName(actionName)) {
+  if (isSubtypeActionName(action.name)) {
     // Has any ExcalidrawElement enabled this actionName through having
     // its subtype?
     return (
       chosen.some((el) => {
         const e = hasBoundTextElement(el) ? getBoundTextElement(el)! : el;
-        return isForSubtype(e.subtype, actionName, true);
+        return isForSubtype(e.subtype, action.name, true);
       }) ||
       // Or has any active subtype enabled this actionName?
       (appState.activeSubtypes !== undefined &&
@@ -135,20 +134,20 @@ const isActionEnabled: EnableFn = function (elements, appState, actionName) {
           if (!isValidSubtype(subtype, appState.activeTool.type)) {
             return false;
           }
-          return isForSubtype(subtype, actionName, true);
+          return isForSubtype(subtype, action.name, true);
         })) ||
       alwaysEnabledMap.some((value) => {
-        return value.actions.includes(actionName);
+        return value.actions.includes(action.name);
       })
     );
   }
   // Now handle standard actions disabled by subtypes
-  if (isDisabledActionName(actionName)) {
+  if (isDisabledActionName(action.name)) {
     return (
       // Has every ExcalidrawElement not disabled this actionName?
       (chosen.every((el) => {
         const e = hasBoundTextElement(el) ? getBoundTextElement(el)! : el;
-        return !isForSubtype(e.subtype, actionName, false);
+        return !isForSubtype(e.subtype, action.name, false);
       }) &&
         // And has every active subtype not disabled this actionName?
         (appState.activeSubtypes === undefined ||
@@ -156,7 +155,7 @@ const isActionEnabled: EnableFn = function (elements, appState, actionName) {
             if (!isValidSubtype(subtype, appState.activeTool.type)) {
               return true;
             }
-            return !isForSubtype(subtype, actionName, false);
+            return !isForSubtype(subtype, action.name, false);
           }))) ||
       // Or can we find an ExcalidrawElement without a valid subtype
       // which would disable this action if it had a valid subtype?
@@ -166,14 +165,14 @@ const isActionEnabled: EnableFn = function (elements, appState, actionName) {
           (value) =>
             value.parentType === e.type &&
             !isValidSubtype(e.subtype, e.type) &&
-            isForSubtype(value.subtype, actionName, false),
+            isForSubtype(value.subtype, action.name, false),
         );
       }) ||
       chosen.some((el) => {
         const e = hasBoundTextElement(el) ? getBoundTextElement(el)! : el;
         return (
           // Would the subtype of e by inself disable this action?
-          isForSubtype(e.subtype, actionName, false) &&
+          isForSubtype(e.subtype, action.name, false) &&
           // Can we find an ExcalidrawElement which could have the same subtype
           // as e but whose subtype does not disable this action?
           chosen.some((el) => {
@@ -184,7 +183,7 @@ const isActionEnabled: EnableFn = function (elements, appState, actionName) {
               parentTypeMap
                 .filter((val) => val.subtype === e.subtype)
                 .some((val) => val.parentType === e2.type) &&
-              !isForSubtype(e2.subtype, actionName, false)
+              !isForSubtype(e2.subtype, action.name, false)
             );
           })
         );
@@ -381,13 +380,6 @@ export const prepareSubtype = (
     onSubtypeLoaded,
   );
 
-  record.disabledNames?.forEach((name) => {
-    registerDisableFn(name, isActionDisabled);
-  });
-  record.actionNames?.forEach((name) => {
-    registerEnableFn(name, isActionEnabled);
-  });
-  registerEnableFn(record.subtype, isActionEnabled);
   // Register the subtype's methods
   addSubtypeMethods(record.subtype, methods);
   return { actions, methods };

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,
+  actionChangeFontFamily,
-  getActionEnablers,
+  actionChangeFontSize,
-  registerDisableFn,
+} from "../actions/actionProperties";
-  registerEnableFn,
+import { isTextElement } from "../element";
-} from "../actions/guards";
 
 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 el1 = API.createElement({ type: "rectangle", id: "A", y: 0 });
-    const text2 = API.createElement({ type: "rectangle", id: "B", y: 30 });
+    const el2 = API.createElement({ type: "rectangle", id: "B", y: 30 });
-    const text3 = API.createElement({ type: "rectangle", id: "C", y: 60 });
+    const el3 = API.createElement({ type: "text", id: "C", y: 60 });
-    const el12: ExcalidrawElement[] = [text1, text2];
+    const el12: ExcalidrawElement[] = [el1, el2];
-    const el13: ExcalidrawElement[] = [text1, text3];
+    const el13: ExcalidrawElement[] = [el1, el3];
-    const el23: ExcalidrawElement[] = [text2, text3];
+    const el23: ExcalidrawElement[] = [el2, el3];
-    const el123: ExcalidrawElement[] = [text1, text2, text3];
+    const el123: ExcalidrawElement[] = [el1, el2, el3];
     // Set up the custom Action enablers
     const enableName = "custom" as Action["name"];
-    const enabler: EnableFn = function (elements) {
+    const enableAction: Action = {
-      if (elements.some((el) => el.y === 30)) {
+      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 disabled1 = actionChangeFontFamily;
-    const disableName2 = "changeFontSize" as ActionName;
+    const disabled2 = actionChangeFontSize;
-    const disabler: DisableFn = function (elements) {
+    const disabler: ActionPredicateFn = function (action, elements) {
-      if (elements.some((el) => el.y === 0)) {
+      if (
-        return true;
+        action.name === disabled2.name &&
-      }
+        elements.some((el) => el.y === 0 || isTextElement(el))
+      ) {
         return false;
+      }
+      return true;
     };
-    registerDisableFn(disableName1, disabler);
     // Test the custom Action enablers
-    const enablers = getActionEnablers();
+    const am = h.app.actionManager;
-    const isCustomEnabled = function (
+    am.registerActionPredicate(enabler);
-      elements: ExcalidrawElement[],
+    expect(am.isActionEnabled(enableAction, { elements: el12 })).toBe(true);
-      name: string,
+    expect(am.isActionEnabled(enableAction, { elements: el13 })).toBe(false);
-    ) {
+    expect(am.isActionEnabled(enableAction, { elements: el23 })).toBe(true);
-      return (
+    expect(am.isActionEnabled(disabled1, { elements: el12 })).toBe(true);
-        name in enablers &&
+    expect(am.isActionEnabled(disabled1, { elements: el13 })).toBe(true);
-        enablers[name].some((enabler) => enabler(elements, h.state, name))
+    expect(am.isActionEnabled(disabled1, { elements: el23 })).toBe(true);
-      );
-    };
-    expect(isCustomEnabled(el12, enableName)).toBe(true);
-    expect(isCustomEnabled(el13, enableName)).toBe(false);
-    expect(isCustomEnabled(el23, enableName)).toBe(true);
     // Test the standard Action disablers
-    const disablers = getActionDisablers();
+    am.registerActionPredicate(disabler);
-    const isStandardDisabled = function (
+    expect(am.isActionEnabled(disabled1, { elements: el123 })).toBe(true);
-      elements: ExcalidrawElement[],
+    expect(am.isActionEnabled(disabled2, { elements: [el1] })).toBe(false);
-      name: ActionName,
+    expect(am.isActionEnabled(disabled2, { elements: [el2] })).toBe(true);
-    ) {
+    expect(am.isActionEnabled(disabled2, { elements: [el3] })).toBe(false);
-      return (
+    expect(am.isActionEnabled(disabled2, { elements: el12 })).toBe(false);
-        name in disablers &&
+    expect(am.isActionEnabled(disabled2, { elements: el23 })).toBe(false);
-        disablers[name].some((disabler) => disabler(elements, h.state, name))
+    expect(am.isActionEnabled(disabled2, { elements: el13 })).toBe(false);
-      );
-    };
-    expect(isStandardDisabled(el12, disableName1)).toBe(true);
-    expect(isStandardDisabled(el23, disableName1)).toBe(false);
-    expect(isStandardDisabled(el123, disableName2)).toBe(false);
   });
 });

src/tests/helpers/api.ts

@@ -20,6 +20,7 @@ import {
   SubtypeRecord,
   prepareSubtype,
   selectSubtype,
+  subtypeActionPredicate,
 } from "../../subtypes";
 import {
   maybeGetSubtypeProps,
@@ -36,6 +37,7 @@ const { h } = window;
 
 export class API {
   constructor() {
+    h.app.actionManager.registerActionPredicate(subtypeActionPredicate);
     if (true) {
       // Call `prepareSubtype()` here for `@excalidraw/excalidraw`-specific subtypes
     }
@@ -45,7 +47,6 @@ export class API {
     const prep = prepareSubtype(record, subtypePrepFn);
     if (prep.actions) {
       h.app.actionManager.registerAll(prep.actions);
-      h.app.actionManager.registerActionGuards();
     }
     return prep;
   };

src/tests/subtypes.test.tsx

@@ -24,9 +24,11 @@ import { getFontString, getShortcutKey } from "../utils";
 import * as textElementUtils from "../element/textElement";
 import { isTextElement } from "../element";
 import { mutateElement, newElementWith } from "../element/mutateElement";
-import { Action } from "../actions/types";
+import { Action, ActionName } from "../actions/types";
 import { AppState } from "../types";
 import { getShortcutFromShortcutName } from "../actions/shortcuts";
+import { actionChangeSloppiness } from "../actions";
+import { actionChangeRoundness } from "../actions/actionProperties";
 
 const MW = 200;
 const TWIDTH = 200;
@@ -60,13 +62,13 @@ const testSubtypeIcon = ({ theme }: { theme: Theme }) =>
   );
 
 const TEST_ACTION = "testAction";
-const TEST_DISABLE1 = "changeSloppiness";
+const TEST_DISABLE1 = actionChangeSloppiness;
-const TEST_DISABLE3 = "changeRoundness";
+const TEST_DISABLE3 = actionChangeRoundness;
 
 const test1: SubtypeRecord = {
   subtype: "test",
   parents: ["line", "arrow", "rectangle", "diamond", "ellipse"],
-  disabledNames: [TEST_DISABLE1],
+  disabledNames: [TEST_DISABLE1.name as ActionName],
   actionNames: [TEST_ACTION],
 };
 
@@ -106,7 +108,7 @@ const test3: SubtypeRecord = {
     testShortcut: [getShortcutKey("Shift+T")],
   },
   alwaysEnabledNames: ["test3Always"],
-  disabledNames: [TEST_DISABLE3],
+  disabledNames: [TEST_DISABLE3.name as ActionName],
 };
 
 const test3Button = SubtypeButton(