From ce487ae9d108a39d96cc4acdb0f235b6af033d63 Mon Sep 17 00:00:00 2001 From: Clint Andrew Hall Date: Fri, 22 Feb 2019 02:10:02 -0600 Subject: [PATCH] Fix Shortcut EventEmitter Leak/Re-render leaks (#31779) --- .../public/components/workpad_page/index.js | 184 +++++---------- .../components/workpad_page/workpad_page.js | 84 +++---- .../workpad_page/workpad_shortcuts.tsx | 209 ++++++++++++++++++ yarn.lock | 21 ++ 4 files changed, 307 insertions(+), 191 deletions(-) create mode 100644 x-pack/plugins/canvas/public/components/workpad_page/workpad_shortcuts.tsx diff --git a/x-pack/plugins/canvas/public/components/workpad_page/index.js b/x-pack/plugins/canvas/public/components/workpad_page/index.js index 03da9afa4eb9dd..b3c84781b79ba6 100644 --- a/x-pack/plugins/canvas/public/components/workpad_page/index.js +++ b/x-pack/plugins/canvas/public/components/workpad_page/index.js @@ -7,10 +7,7 @@ import { connect } from 'react-redux'; import PropTypes from 'prop-types'; import { compose, withState, withProps, withHandlers } from 'recompose'; -import { notify } from '../../lib/notify'; import { aeroelastic } from '../../lib/aeroelastic_kibana'; -import { setClipboardData, getClipboardData } from '../../lib/clipboard'; -import { cloneSubgraphs } from '../../lib/clone_subgraphs'; import { removeElements, insertNodes, elementLayer } from '../../state/actions/elements'; import { getFullscreen, canUserWrite } from '../../state/selectors/app'; import { getNodes, isWriteable } from '../../state/selectors/workpad'; @@ -85,139 +82,60 @@ export const WorkpadPage = compose( }; }), withState('updateCount', 'setUpdateCount', 0), // TODO: remove this, see setUpdateCount below - withProps( - ({ - updateCount, - setUpdateCount, - page, - elements: pageElements, - insertNodes, - removeElements, - selectElement, - elementLayer, - }) => { - const { shapes, selectedPrimaryShapes = [], cursor } = aeroelastic.getStore( - page.id - ).currentScene; - const elementLookup = new Map(pageElements.map(element => [element.id, element])); - const recurseGroupTree = shapeId => { - return [ - shapeId, - ...flatten( - shapes - .filter(s => s.parent === shapeId && s.type !== 'annotation') - .map(s => s.id) - .map(recurseGroupTree) - ), - ]; - }; + withProps(({ updateCount, setUpdateCount, page, elements: pageElements }) => { + const { shapes, selectedPrimaryShapes = [], cursor } = aeroelastic.getStore( + page.id + ).currentScene; + const elementLookup = new Map(pageElements.map(element => [element.id, element])); + const recurseGroupTree = shapeId => { + return [ + shapeId, + ...flatten( + shapes + .filter(s => s.parent === shapeId && s.type !== 'annotation') + .map(s => s.id) + .map(recurseGroupTree) + ), + ]; + }; - const selectedPrimaryShapeObjects = selectedPrimaryShapes - .map(id => shapes.find(s => s.id === id)) - .filter(shape => shape); + const selectedPrimaryShapeObjects = selectedPrimaryShapes + .map(id => shapes.find(s => s.id === id)) + .filter(shape => shape); - const selectedPersistentPrimaryShapes = flatten( - selectedPrimaryShapeObjects.map(shape => - shape.subtype === 'adHocGroup' - ? shapes.filter(s => s.parent === shape.id && s.type !== 'annotation').map(s => s.id) - : [shape.id] - ) - ); - const selectedElementIds = flatten(selectedPersistentPrimaryShapes.map(recurseGroupTree)); - const selectedElements = []; - const elements = shapes.map(shape => { - let element = null; - if (elementLookup.has(shape.id)) { - element = elementLookup.get(shape.id); - if (selectedElementIds.indexOf(shape.id) > -1) { - selectedElements.push({ ...element, id: shape.id }); - } + const selectedPersistentPrimaryShapes = flatten( + selectedPrimaryShapeObjects.map(shape => + shape.subtype === 'adHocGroup' + ? shapes.filter(s => s.parent === shape.id && s.type !== 'annotation').map(s => s.id) + : [shape.id] + ) + ); + const selectedElementIds = flatten(selectedPersistentPrimaryShapes.map(recurseGroupTree)); + const selectedElements = []; + const elements = shapes.map(shape => { + let element = null; + if (elementLookup.has(shape.id)) { + element = elementLookup.get(shape.id); + if (selectedElementIds.indexOf(shape.id) > -1) { + selectedElements.push({ ...element, id: shape.id }); } - // instead of just combining `element` with `shape`, we make property transfer explicit - return element ? { ...shape, filter: element.filter } : shape; - }); - return { - elements, - cursor, - commit: (...args) => { - aeroelastic.commit(page.id, ...args); - // TODO: remove this, it's a hack to force react to rerender - setUpdateCount(updateCount + 1); - }, - removeElements: () => { - // currently, handle the removal of one element, exploiting multiselect subsequently - if (selectedElementIds.length) { - removeElements(page.id)(selectedElementIds); - } - }, - copyElements: () => { - if (selectedElements.length) { - setClipboardData({ selectedElements, rootShapes: selectedPrimaryShapes }); - notify.success('Copied element to clipboard'); - } - }, - cutElements: () => { - if (selectedElements.length) { - setClipboardData({ selectedElements, rootShapes: selectedPrimaryShapes }); - removeElements(page.id)(selectedElementIds); - notify.success('Copied element to clipboard'); - } - }, - // TODO: This is slightly different from the duplicateElements function in sidebar/index.js. Should they be doing the same thing? - // This should also be abstracted. - duplicateElements: () => { - const clonedElements = selectedElements && cloneSubgraphs(selectedElements); - if (clonedElements) { - insertNodes(page.id)(clonedElements); - if (selectedPrimaryShapes.length) { - if (selectedElements.length > 1) { - // adHocGroup branch (currently, pasting will leave only the 1st element selected, rather than forming a - // new adHocGroup - todo) - selectElement(clonedElements[0].id); - } else { - // single element or single persistentGroup branch - selectElement( - clonedElements[selectedElements.findIndex(s => s.id === selectedPrimaryShapes[0])] - .id - ); - } - } - } - }, - pasteElements: () => { - const { selectedElements, rootShapes } = JSON.parse(getClipboardData()) || {}; - const clonedElements = selectedElements && cloneSubgraphs(selectedElements); - if (clonedElements) { - // first clone and persist the new node(s) - insertNodes(page.id)(clonedElements); - // then select the cloned node - if (rootShapes.length) { - if (selectedElements.length > 1) { - // adHocGroup branch (currently, pasting will leave only the 1st element selected, rather than forming a - // new adHocGroup - todo) - selectElement(clonedElements[0].id); - } else { - // single element or single persistentGroup branch - selectElement( - clonedElements[selectedElements.findIndex(s => s.id === rootShapes[0])].id - ); - } - } - } - }, - // TODO: Same as above. Abstract these out. This is the same code as in sidebar/index.js - // Note: these layer actions only work when a single element is selected - bringForward: () => - selectedElements.length === 1 && elementLayer(page.id, selectedElements[0], 1), - bringToFront: () => - selectedElements.length === 1 && elementLayer(page.id, selectedElements[0], Infinity), - sendBackward: () => - selectedElements.length === 1 && elementLayer(page.id, selectedElements[0], -1), - sendToBack: () => - selectedElements.length === 1 && elementLayer(page.id, selectedElements[0], -Infinity), - }; - } - ), // Updates states; needs to have both local and global + } + // instead of just combining `element` with `shape`, we make property transfer explicit + return element ? { ...shape, filter: element.filter } : shape; + }); + return { + elements, + cursor, + selectedElementIds, + selectedElements, + selectedPrimaryShapes, + commit: (...args) => { + aeroelastic.commit(page.id, ...args); + // TODO: remove this, it's a hack to force react to rerender + setUpdateCount(updateCount + 1); + }, + }; + }), // Updates states; needs to have both local and global withHandlers({ groupElements: ({ commit }) => () => commit('actionEvent', { diff --git a/x-pack/plugins/canvas/public/components/workpad_page/workpad_page.js b/x-pack/plugins/canvas/public/components/workpad_page/workpad_page.js index 48de8e087a0426..77a4037031b961 100644 --- a/x-pack/plugins/canvas/public/components/workpad_page/workpad_page.js +++ b/x-pack/plugins/canvas/public/components/workpad_page/workpad_page.js @@ -6,7 +6,6 @@ import React, { PureComponent } from 'react'; import PropTypes from 'prop-types'; -import { Shortcuts } from 'react-shortcuts'; import { ElementWrapper } from '../element_wrapper'; import { AlignmentGuide } from '../alignment_guide'; import { HoverAnnotation } from '../hover_annotation'; @@ -14,7 +13,7 @@ import { TooltipAnnotation } from '../tooltip_annotation'; import { RotationHandle } from '../rotation_handle'; import { BorderConnection } from '../border_connection'; import { BorderResizeHandle } from '../border_resize_handle'; -import { isTextInput } from '../../lib/is_text_input'; +import { WorkpadShortcuts } from './workpad_shortcuts'; // NOTE: the data-shared-* attributes here are used for reporting export class WorkpadPage extends PureComponent { @@ -70,6 +69,7 @@ export class WorkpadPage extends PureComponent { height, width, isEditable, + isSelected, onDoubleClick, onKeyDown, onMouseDown, @@ -77,59 +77,34 @@ export class WorkpadPage extends PureComponent { onMouseUp, onAnimationEnd, onWheel, + selectedElementIds, + selectedElements, + selectedPrimaryShapes, + selectElement, + insertNodes, removeElements, - copyElements, - cutElements, - duplicateElements, - pasteElements, - bringForward, - bringToFront, - sendBackward, - sendToBack, + elementLayer, groupElements, ungroupElements, } = this.props; - const keyHandler = (action, event) => { - if (!isTextInput(event.target)) { - event.preventDefault(); - switch (action) { - case 'COPY': - copyElements(); - break; - case 'CLONE': - duplicateElements(); - break; - case 'CUT': - cutElements(); - break; - case 'DELETE': - removeElements(); - break; - case 'PASTE': - pasteElements(); - break; - case 'BRING_FORWARD': - bringForward(); - break; - case 'BRING_TO_FRONT': - bringToFront(); - break; - case 'SEND_BACKWARD': - sendBackward(); - break; - case 'SEND_TO_BACK': - sendToBack(); - break; - case 'GROUP': - groupElements(); - break; - case 'UNGROUP': - ungroupElements(); - break; - } - } - }; + let shortcuts = null; + + if (isEditable && isSelected) { + const shortcutProps = { + elementLayer, + groupElements, + insertNodes, + pageId: page.id, + removeElements, + selectedElementIds, + selectedElements, + selectedPrimaryShapes, + selectElement, + ungroupElements, + }; + shortcuts = ; + } return (
- {isEditable && ( - - )} + {shortcuts} {elements .map(element => { if (element.type === 'annotation') { diff --git a/x-pack/plugins/canvas/public/components/workpad_page/workpad_shortcuts.tsx b/x-pack/plugins/canvas/public/components/workpad_page/workpad_shortcuts.tsx new file mode 100644 index 00000000000000..3bbf1c468fa05f --- /dev/null +++ b/x-pack/plugins/canvas/public/components/workpad_page/workpad_shortcuts.tsx @@ -0,0 +1,209 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import _ from 'lodash'; +import React, { Component } from 'react'; + +import isEqual from 'react-fast-compare'; +// @ts-ignore +import { Shortcuts } from 'react-shortcuts'; +// @ts-ignore +import { getClipboardData, setClipboardData } from '../../lib/clipboard'; +// @ts-ignore +import { cloneSubgraphs } from '../../lib/clone_subgraphs'; +// @ts-ignore +import { notify } from '../../lib/notify'; + +export interface Props { + pageId: string; + selectedElementIds: string[]; + selectedElements: any[]; + selectedPrimaryShapes: any[]; + selectElement: (elementId: string) => void; + insertNodes: (pageId: string) => (selectedElements: any[]) => void; + removeElements: (pageId: string) => (selectedElementIds: string[]) => void; + elementLayer: (pageId: string, selectedElement: any, movement: any) => void; + groupElements: () => void; + ungroupElements: () => void; +} + +export class WorkpadShortcuts extends Component { + public render() { + const { pageId } = this.props; + return ( + this._keyHandler(action, event)} + targetNodeSelector={`#${pageId}`} + global + /> + ); + } + + public shouldComponentUpdate(nextProps: Props) { + return !isEqual(nextProps, this.props); + } + + private _keyHandler(action: string, event: Event) { + event.preventDefault(); + switch (action) { + case 'COPY': + this._copyElements(); + break; + case 'CLONE': + this._duplicateElements(); + break; + case 'CUT': + this._cutElements(); + break; + case 'DELETE': + this._removeElements(); + break; + case 'PASTE': + this._pasteElements(); + break; + case 'BRING_FORWARD': + this._bringForward(); + break; + case 'BRING_TO_FRONT': + this._bringToFront(); + break; + case 'SEND_BACKWARD': + this._sendBackward(); + break; + case 'SEND_TO_BACK': + this._sendToBack(); + break; + case 'GROUP': + this.props.groupElements(); + break; + case 'UNGROUP': + this.props.ungroupElements(); + break; + } + } + + private _removeElements() { + const { pageId, removeElements, selectedElementIds } = this.props; + // currently, handle the removal of one element, exploiting multiselect subsequently + if (selectedElementIds.length) { + removeElements(pageId)(selectedElementIds); + } + } + + private _copyElements() { + const { selectedElements, selectedPrimaryShapes } = this.props; + if (selectedElements.length) { + setClipboardData({ selectedElements, rootShapes: selectedPrimaryShapes }); + notify.success('Copied element to clipboard'); + } + } + + private _cutElements() { + const { + pageId, + removeElements, + selectedElements, + selectedElementIds, + selectedPrimaryShapes, + } = this.props; + + if (selectedElements.length) { + setClipboardData({ selectedElements, rootShapes: selectedPrimaryShapes }); + removeElements(pageId)(selectedElementIds); + notify.success('Copied element to clipboard'); + } + } + + // TODO: This is slightly different from the duplicateElements function in sidebar/index.js. Should they be doing the same thing? + // This should also be abstracted. + private _duplicateElements() { + const { + insertNodes, + pageId, + selectElement, + selectedElements, + selectedPrimaryShapes, + } = this.props; + const clonedElements = selectedElements && cloneSubgraphs(selectedElements); + + if (clonedElements) { + insertNodes(pageId)(clonedElements); + if (selectedPrimaryShapes.length) { + if (selectedElements.length > 1) { + // adHocGroup branch (currently, pasting will leave only the 1st element selected, rather than forming a + // new adHocGroup - todo) + selectElement(clonedElements[0].id); + } else { + // single element or single persistentGroup branch + selectElement( + clonedElements[selectedElements.findIndex(s => s.id === selectedPrimaryShapes[0])].id + ); + } + } + } + } + + private _pasteElements() { + const { insertNodes, pageId, selectElement } = this.props; + const { selectedElements, rootShapes } = JSON.parse(getClipboardData()) || { + selectedElements: [], + rootShapes: [], + }; + + const clonedElements = selectedElements && cloneSubgraphs(selectedElements); + + if (clonedElements) { + // first clone and persist the new node(s) + insertNodes(pageId)(clonedElements); + // then select the cloned node + if (rootShapes.length) { + if (selectedElements.length > 1) { + // adHocGroup branch (currently, pasting will leave only the 1st element selected, rather than forming a + // new adHocGroup - todo) + selectElement(clonedElements[0].id); + } else { + // single element or single persistentGroup branch + selectElement( + clonedElements[ + selectedElements.findIndex((s: { id: string }) => s.id === rootShapes[0]) + ].id + ); + } + } + } + } + + // TODO: Same as above. Abstract these out. This is the same code as in sidebar/index.js + // Note: these layer actions only work when a single element is selected + private _bringForward() { + const { elementLayer, pageId, selectedElements } = this.props; + if (selectedElements.length === 1) { + elementLayer(pageId, selectedElements[0], 1); + } + } + + private _bringToFront() { + const { elementLayer, pageId, selectedElements } = this.props; + if (selectedElements.length === 1) { + elementLayer(pageId, selectedElements[0], Infinity); + } + } + + private _sendBackward() { + const { elementLayer, pageId, selectedElements } = this.props; + if (selectedElements.length === 1) { + elementLayer(pageId, selectedElements[0], -1); + } + } + + private _sendToBack() { + const { elementLayer, pageId, selectedElements } = this.props; + if (selectedElements.length === 1) { + elementLayer(pageId, selectedElements[0], -Infinity); + } + } +} diff --git a/yarn.lock b/yarn.lock index baefbb58ff32b9..732cf604f5a34c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17625,10 +17625,31 @@ react-dropzone@^4.2.9: attr-accept "^1.1.3" prop-types "^15.5.7" +<<<<<<< HEAD +======= +react-error-overlay@^5.1.0: + version "5.1.0" + resolved "https://registry.yarnpkg.com/react-error-overlay/-/react-error-overlay-5.1.0.tgz#c516995a5652e7bfbed8b497910d5280df74a7e8" + integrity sha512-akMy/BQT5m1J3iJIHkSb4qycq2wzllWsmmolaaFVnb+LPV9cIJ/nTud40ZsiiT0H3P+/wXIdbjx2fzF61OaeOQ== + +>>>>>>> 923f360fab... Fix Shortcut EventEmitter Leak/Re-render leaks (#31779) react-fast-compare@^2.0.4: version "2.0.4" resolved "https://registry.yarnpkg.com/react-fast-compare/-/react-fast-compare-2.0.4.tgz#e84b4d455b0fec113e0402c329352715196f81f9" integrity sha512-suNP+J1VU1MWFKcyt7RtjiSWUjvidmQSlqu+eHslq+342xCbGTYmC0mEhPCOHxlW0CywylOC1u2DFAT+bv4dBw== +<<<<<<< HEAD +======= + +react-fuzzy@^0.5.2: + version "0.5.2" + resolved "https://registry.yarnpkg.com/react-fuzzy/-/react-fuzzy-0.5.2.tgz#fc13bf6f0b785e5fefe908724efebec4935eaefe" + integrity sha512-qIZZxaCheb/HhcBi5fABbiCFg85+K5r1TCps1D4uaL0LAMMD/1zm/x1/kNR130Tx7nnY9V7mbFyY0DquPYeLAw== + dependencies: + babel-runtime "^6.23.0" + classnames "^2.2.5" + fuse.js "^3.0.1" + prop-types "^15.5.9" +>>>>>>> 923f360fab... Fix Shortcut EventEmitter Leak/Re-render leaks (#31779) react-grid-layout@^0.16.2: version "0.16.6"