From 1d3652a96c08ec1fd13d6b7ba991721fbfbdcd0b Mon Sep 17 00:00:00 2001 From: "Daniel J. Geiger" <1852529+DanielJGeiger@users.noreply.github.com> Date: Fri, 27 Jan 2023 14:21:35 -0600 Subject: [PATCH] Simplify: universal Action predicates instead of action-specific guards. --- src/actions/guards.ts | 25 ------ src/actions/manager.tsx | 79 ++++--------------- src/actions/types.ts | 17 ++-- src/components/App.tsx | 1 - src/packages/excalidraw/example/App.tsx | 20 ++--- src/tests/customActions.test.tsx | 100 ++++++++++++------------ 6 files changed, 83 insertions(+), 159 deletions(-) delete mode 100644 src/actions/guards.ts diff --git a/src/actions/guards.ts b/src/actions/guards.ts deleted file mode 100644 index a5a00ab69..000000000 --- a/src/actions/guards.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { Action, ActionName, DisableFn, EnableFn } from "./types"; - -const disablers = {} as Record; -const enablers = {} as Record; - -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); - } -}; diff --git a/src/actions/manager.tsx b/src/actions/manager.tsx index 662528f8f..6fc8ced71 100644 --- a/src/actions/manager.tsx +++ b/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; - - disablers = {} as Record; - enablers = {} as Record; + actions = {} as Record; + actionPredicates = [] as ActionPredicateFn[]; updater: (actionResult: ActionResult | Promise) => 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; @@ -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; }; } diff --git a/src/actions/types.ts b/src/actions/types.ts index 9a695ee5e..81b0322a4 100644 --- a/src/actions/types.ts +++ b/src/actions/types.ts @@ -31,20 +31,13 @@ type ActionFn = ( app: AppClassProperties, ) => ActionResult | Promise; -// 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, ) => boolean; export type UpdaterFn = (res: ActionResult) => void; diff --git a/src/components/App.tsx b/src/components/App.tsx index 7ac179cb9..b1f5c35a7 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -480,7 +480,6 @@ class App extends React.Component { }); this.history = new History(); this.actionManager.registerAll(actions); - this.actionManager.registerActionGuards(); this.actionManager.registerAction(createUndoAction(this.history)); this.actionManager.registerAction(createRedoAction(this.history)); diff --git a/src/packages/excalidraw/example/App.tsx b/src/packages/excalidraw/example/App.tsx index 5080a4bf4..71e3fbbad 100644 --- a/src/packages/excalidraw/example/App.tsx +++ b/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 }, + }, + }); }} > {" "} diff --git a/src/tests/customActions.test.tsx b/src/tests/customActions.test.tsx index c265e965b..84e2bf189 100644 --- a/src/tests/customActions.test.tsx +++ b/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(); // 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); }); });