From c8d4e8c421a0dabeac5f17aeb832de5e60ae7042 Mon Sep 17 00:00:00 2001 From: "Daniel J. Geiger" <1852529+DanielJGeiger@users.noreply.github.com> Date: Fri, 27 Jan 2023 13:23:40 -0600 Subject: [PATCH] Simplify custom Actions: universal Action predicates instead of action-specific guards. --- src/actions/guards.ts | 25 -------- src/actions/manager.tsx | 79 ++++++------------------ src/actions/register.ts | 10 +--- src/actions/types.ts | 17 ++---- src/components/App.tsx | 8 +-- src/subtypes.ts | 44 ++++++-------- src/tests/customActions.test.tsx | 100 +++++++++++++++---------------- src/tests/helpers/api.ts | 3 +- src/tests/subtypes.test.tsx | 12 ++-- 9 files changed, 105 insertions(+), 193 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/register.ts b/src/actions/register.ts index e25dc93b4..ccc9cdbf9 100644 --- a/src/actions/register.ts +++ b/src/actions/register.ts @@ -1,14 +1,8 @@ -import { Action, isActionName } from "./types"; +import { Action } from "./types"; -let actions: readonly Action[] = []; -let customActions: readonly Action[] = []; -export const getCustomActions = () => customActions; -export const getActions = () => actions; +export let actions: readonly Action[] = []; export const register = (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"]; 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 3fadfd488..68d7f4d71 100644 --- a/src/components/App.tsx +++ b/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 { 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 { if (prep.actions) { this.actionManager.registerAll(prep.actions); } - this.actionManager.registerActionGuards(); return prep; } diff --git a/src/subtypes.ts b/src/subtypes.ts index f58fe01da..20e505c5a 100644 --- a/src/subtypes.ts +++ b/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) { - return !isActionEnabled(elements, appState, actionName); -}; - -const isActionEnabled: EnableFn = function (elements, appState, actionName) { +export const subtypeActionPredicate: ActionPredicateFn = function ( + action, + elements, + appState, +) { // We always enable subtype actions. Also let through standard actions // which no subtypes might have disabled. if ( - isSubtypeName(actionName) || - (!isSubtypeActionName(actionName) && !isDisabledActionName(actionName)) + isSubtypeName(action.name) || + (!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 }; 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); }); }); diff --git a/src/tests/helpers/api.ts b/src/tests/helpers/api.ts index e9abf917d..2461677ea 100644 --- a/src/tests/helpers/api.ts +++ b/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; }; diff --git a/src/tests/subtypes.test.tsx b/src/tests/subtypes.test.tsx index 604b5fbbf..ebb8f81dd 100644 --- a/src/tests/subtypes.test.tsx +++ b/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_DISABLE3 = "changeRoundness"; +const TEST_DISABLE1 = actionChangeSloppiness; +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(