From e9f5175f51d2cf7d06117789f9a183802194439f Mon Sep 17 00:00:00 2001 From: Pete Hunt Date: Mon, 16 Mar 2020 19:07:47 -0700 Subject: [PATCH] Fix performance bug (#984) --- src/components/App.tsx | 210 +++++++++++++++++++---------------- src/components/Popover.tsx | 3 +- src/element/mutateElement.ts | 3 +- 3 files changed, 121 insertions(+), 95 deletions(-) diff --git a/src/components/App.tsx b/src/components/App.tsx index 19c43abf..0f84ba29 100644 --- a/src/components/App.tsx +++ b/src/components/App.tsx @@ -97,6 +97,16 @@ import { ScrollBars } from "../scene/types"; import { generateCollaborationLink, getCollaborationLinkData } from "../data"; import { mutateElement, newElementWith } from "../element/mutateElement"; import { invalidateShapeForElement } from "../renderer/renderElement"; +import { unstable_batchedUpdates } from "react-dom"; +import { SceneStateCallbackRemover } from "../scene/globalScene"; + +function withBatchedUpdates< + TFunction extends ((event: any) => void) | (() => void) +>(func: TFunction) { + return (event => { + unstable_batchedUpdates(func, event); + }) as TFunction; +} // ----------------------------------------------------------------------------- // TEST HOOKS @@ -159,6 +169,7 @@ export class App extends React.Component { roomID: string | null = null; roomKey: string | null = null; lastBroadcastedOrReceivedSceneVersion: number = -1; + removeSceneCallback: SceneStateCallbackRemover | null = null; actionManager: ActionManager; canvasOnlyActions = ["selectAll"]; @@ -201,7 +212,7 @@ export class App extends React.Component { } }; - private onCut = (event: ClipboardEvent) => { + private onCut = withBatchedUpdates((event: ClipboardEvent) => { if (isWritableElement(event.target)) { return; } @@ -214,20 +225,21 @@ export class App extends React.Component { history.resumeRecording(); this.setState({ ...appState }); event.preventDefault(); - }; - private onCopy = (event: ClipboardEvent) => { + }); + + private onCopy = withBatchedUpdates((event: ClipboardEvent) => { if (isWritableElement(event.target)) { return; } copyToAppClipboard(globalSceneState.getAllElements(), this.state); event.preventDefault(); - }; + }); - private onUnload = () => { + private onUnload = withBatchedUpdates(() => { isHoldingSpace = false; this.saveDebounced(); this.saveDebounced.flush(); - }; + }); private disableEvent: EventHandlerNonNull = event => { event.preventDefault(); @@ -454,7 +466,7 @@ export class App extends React.Component { } } - private handleSceneCallback = () => { + private onSceneUpdated = () => { this.setState({}); }; @@ -469,7 +481,9 @@ export class App extends React.Component { }); } - globalSceneState.addCallback(this.handleSceneCallback); + this.removeSceneCallback = globalSceneState.addCallback( + this.onSceneUpdated, + ); document.addEventListener("copy", this.onCopy); document.addEventListener("paste", this.pasteFromClipboard); @@ -528,6 +542,8 @@ export class App extends React.Component { public componentWillUnmount() { this.unmounted = true; + this.removeSceneCallback!(); + document.removeEventListener("copy", this.onCopy); document.removeEventListener("paste", this.pasteFromClipboard); document.removeEventListener("cut", this.onCut); @@ -561,19 +577,21 @@ export class App extends React.Component { public state: AppState = getDefaultAppState(); - private onResize = () => { + private onResize = withBatchedUpdates(() => { globalSceneState .getAllElements() .forEach(element => invalidateShapeForElement(element)); this.setState({}); - }; + }); - private updateCurrentCursorPosition = (event: MouseEvent) => { - cursorX = event.x; - cursorY = event.y; - }; + private updateCurrentCursorPosition = withBatchedUpdates( + (event: MouseEvent) => { + cursorX = event.x; + cursorY = event.y; + }, + ); - private onKeyDown = (event: KeyboardEvent) => { + private onKeyDown = withBatchedUpdates((event: KeyboardEvent) => { if ( (isWritableElement(event.target) && event.key !== KEYS.ESCAPE) || // case: using arrows to move between buttons @@ -629,9 +647,9 @@ export class App extends React.Component { isHoldingSpace = true; document.documentElement.style.cursor = CURSOR_TYPE.GRABBING; } - }; + }); - private onKeyUp = (event: KeyboardEvent) => { + private onKeyUp = withBatchedUpdates((event: KeyboardEvent) => { if (event.key === KEYS.SPACE) { if (this.state.elementType === "selection") { resetCursor(); @@ -644,7 +662,7 @@ export class App extends React.Component { } isHoldingSpace = false; } - }; + }); private copyToAppClipboard = () => { copyToAppClipboard(globalSceneState.getAllElements(), this.state); @@ -666,55 +684,57 @@ export class App extends React.Component { ); }; - private pasteFromClipboard = async (event: ClipboardEvent | null) => { - // #686 - const target = document.activeElement; - const elementUnderCursor = document.elementFromPoint(cursorX, cursorY); - if ( - // if no ClipboardEvent supplied, assume we're pasting via contextMenu - // thus these checks don't make sense - !event || - (elementUnderCursor instanceof HTMLCanvasElement && - !isWritableElement(target)) - ) { - const data = await getClipboardContent(event); - if (data.elements) { - this.addElementsFromPaste(data.elements); - } else if (data.text) { - const { x, y } = viewportCoordsToSceneCoords( - { clientX: cursorX, clientY: cursorY }, - this.state, - this.canvas, - window.devicePixelRatio, - ); + private pasteFromClipboard = withBatchedUpdates( + async (event: ClipboardEvent | null) => { + // #686 + const target = document.activeElement; + const elementUnderCursor = document.elementFromPoint(cursorX, cursorY); + if ( + // if no ClipboardEvent supplied, assume we're pasting via contextMenu + // thus these checks don't make sense + !event || + (elementUnderCursor instanceof HTMLCanvasElement && + !isWritableElement(target)) + ) { + const data = await getClipboardContent(event); + if (data.elements) { + this.addElementsFromPaste(data.elements); + } else if (data.text) { + const { x, y } = viewportCoordsToSceneCoords( + { clientX: cursorX, clientY: cursorY }, + this.state, + this.canvas, + window.devicePixelRatio, + ); - const element = newTextElement( - newElement( - "text", - x, - y, - this.state.currentItemStrokeColor, - this.state.currentItemBackgroundColor, - this.state.currentItemFillStyle, - this.state.currentItemStrokeWidth, - this.state.currentItemRoughness, - this.state.currentItemOpacity, - ), - data.text, - this.state.currentItemFont, - ); + const element = newTextElement( + newElement( + "text", + x, + y, + this.state.currentItemStrokeColor, + this.state.currentItemBackgroundColor, + this.state.currentItemFillStyle, + this.state.currentItemStrokeWidth, + this.state.currentItemRoughness, + this.state.currentItemOpacity, + ), + data.text, + this.state.currentItemFont, + ); - globalSceneState.replaceAllElements([ - ...globalSceneState.getAllElements(), - element, - ]); - this.setState({ selectedElementIds: { [element.id]: true } }); - history.resumeRecording(); + globalSceneState.replaceAllElements([ + ...globalSceneState.getAllElements(), + element, + ]); + this.setState({ selectedElementIds: { [element.id]: true } }); + history.resumeRecording(); + } + this.selectShapeTool("selection"); + event?.preventDefault(); } - this.selectShapeTool("selection"); - event?.preventDefault(); - } - }; + }, + ); private selectShapeTool(elementType: AppState["elementType"]) { if (!isHoldingSpace) { @@ -730,21 +750,23 @@ export class App extends React.Component { } } - private onGestureStart = (event: GestureEvent) => { + private onGestureStart = withBatchedUpdates((event: GestureEvent) => { event.preventDefault(); gesture.initialScale = this.state.zoom; - }; - private onGestureChange = (event: GestureEvent) => { + }); + + private onGestureChange = withBatchedUpdates((event: GestureEvent) => { event.preventDefault(); this.setState({ zoom: getNormalizedZoom(gesture.initialScale! * event.scale), }); - }; - private onGestureEnd = (event: GestureEvent) => { + }); + + private onGestureEnd = withBatchedUpdates((event: GestureEvent) => { event.preventDefault(); gesture.initialScale = null; - }; + }); setAppState = (obj: any) => { this.setState(obj); @@ -1174,7 +1196,7 @@ export class App extends React.Component { isPanning = true; document.documentElement.style.cursor = CURSOR_TYPE.GRABBING; let { clientX: lastX, clientY: lastY } = event; - const onPointerMove = (event: PointerEvent) => { + const onPointerMove = withBatchedUpdates((event: PointerEvent) => { const deltaX = lastX - event.clientX; const deltaY = lastY - event.clientY; lastX = event.clientX; @@ -1188,17 +1210,19 @@ export class App extends React.Component { this.state.scrollY - deltaY / this.state.zoom, ), }); - }; - const teardown = (lastPointerUp = () => { - lastPointerUp = null; - isPanning = false; - if (!isHoldingSpace) { - setCursorForShape(this.state.elementType); - } - window.removeEventListener("pointermove", onPointerMove); - window.removeEventListener("pointerup", teardown); - window.removeEventListener("blur", teardown); }); + const teardown = withBatchedUpdates( + (lastPointerUp = () => { + lastPointerUp = null; + isPanning = false; + if (!isHoldingSpace) { + setCursorForShape(this.state.elementType); + } + window.removeEventListener("pointermove", onPointerMove); + window.removeEventListener("pointerup", teardown); + window.removeEventListener("blur", teardown); + }), + ); window.addEventListener("blur", teardown); window.addEventListener("pointermove", onPointerMove, { passive: true, @@ -1264,7 +1288,7 @@ export class App extends React.Component { isDraggingScrollBar = true; lastX = event.clientX; lastY = event.clientY; - const onPointerMove = (event: PointerEvent) => { + const onPointerMove = withBatchedUpdates((event: PointerEvent) => { const target = event.target; if (!(target instanceof HTMLElement)) { return; @@ -1288,15 +1312,15 @@ export class App extends React.Component { }); lastY = y; } - }; + }); - const onPointerUp = () => { + const onPointerUp = withBatchedUpdates(() => { isDraggingScrollBar = false; setCursorForShape(this.state.elementType); lastPointerUp = null; window.removeEventListener("pointermove", onPointerMove); window.removeEventListener("pointerup", onPointerUp); - }; + }); lastPointerUp = onPointerUp; @@ -1624,7 +1648,7 @@ export class App extends React.Component { } }; - const onPointerMove = (event: PointerEvent) => { + const onPointerMove = withBatchedUpdates((event: PointerEvent) => { const target = event.target; if (!(target instanceof HTMLElement)) { return; @@ -1997,9 +2021,9 @@ export class App extends React.Component { }, })); } - }; + }); - const onPointerUp = (event: PointerEvent) => { + const onPointerUp = withBatchedUpdates((event: PointerEvent) => { const { draggingElement, resizingElement, @@ -2150,7 +2174,7 @@ export class App extends React.Component { draggingElement: null, }); } - }; + }); lastPointerUp = onPointerUp; @@ -2158,7 +2182,7 @@ export class App extends React.Component { window.addEventListener("pointerup", onPointerUp); }; - private handleWheel = (event: WheelEvent) => { + private handleWheel = withBatchedUpdates((event: WheelEvent) => { event.preventDefault(); const { deltaX, deltaY } = event; @@ -2181,9 +2205,9 @@ export class App extends React.Component { scrollX: normalizeScroll(scrollX - deltaX / zoom), scrollY: normalizeScroll(scrollY - deltaY / zoom), })); - }; + }); - private beforeUnload = (event: BeforeUnloadEvent) => { + private beforeUnload = withBatchedUpdates((event: BeforeUnloadEvent) => { if ( this.state.isCollaborating && hasNonDeletedElements(globalSceneState.getAllElements()) @@ -2192,7 +2216,7 @@ export class App extends React.Component { // NOTE: modern browsers no longer allow showing a custom message here event.returnValue = ""; } - }; + }); private addElementsFromPaste = ( clipboardElements: readonly ExcalidrawElement[], diff --git a/src/components/Popover.tsx b/src/components/Popover.tsx index c033040d..0ee5da00 100644 --- a/src/components/Popover.tsx +++ b/src/components/Popover.tsx @@ -1,5 +1,6 @@ import React, { useLayoutEffect, useRef, useEffect } from "react"; import "./Popover.css"; +import { unstable_batchedUpdates } from "react-dom"; type Props = { top?: number; @@ -39,7 +40,7 @@ export function Popover({ if (onCloseRequest) { const handler = (e: Event) => { if (!popoverRef.current?.contains(e.target as Node)) { - onCloseRequest(); + unstable_batchedUpdates(() => onCloseRequest()); } }; document.addEventListener("pointerdown", handler, false); diff --git a/src/element/mutateElement.ts b/src/element/mutateElement.ts index 14236122..58df4699 100644 --- a/src/element/mutateElement.ts +++ b/src/element/mutateElement.ts @@ -10,7 +10,8 @@ type ElementUpdate = Omit< // This function tracks updates of text elements for the purposes for collaboration. // The version is used to compare updates when more than one user is working in -// the same drawing. +// the same drawing. Note: this will trigger the component to update. Make sure you +// are calling it either from a React event handler or within unstable_batchedUpdates(). export function mutateElement( element: TElement, updates: ElementUpdate,