maxmustermann/kibana
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix Shortcut EventEmitter Leak/Re-render leaks (#31779)

Browse source
This commit is contained in:
Clint Andrew Hall 2019-02-22 02:10:02 -06:00 committed by Robert Monfera
parent 1bfde5ec4d
commit 923f360fab
5 changed files with 292 additions and 191 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
x-pack/package.json
View file

@ -244,6 +244,7 @@
"react-datetime": "^2.14.0",
"react-dom": "^16.8.0",
"react-dropzone": "^4.2.9",
"react-fast-compare": "^2.0.4",
"react-markdown-renderer": "^1.4.0",
"react-portal": "^3.2.0",
"react-redux": "^5.0.7",

184
x-pack/plugins/canvas/public/components/workpad_page/index.js
View file

@ -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', {

84
x-pack/plugins/canvas/public/components/workpad_page/workpad_page.js
View file

@ -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 = <WorkpadShortcuts {...shortcutProps} />;
}
return (
<div
@ -153,14 +128,7 @@ export class WorkpadPage extends PureComponent {
onAnimationEnd={onAnimationEnd}
onWheel={onWheel}
>
{isEditable && (
<Shortcuts
name="ELEMENT"
handler={keyHandler}
targetNodeSelector={`#${page.id}`}
global
/>
)}
{shortcuts}
{elements
.map(element => {
if (element.type === 'annotation') {

209
x-pack/plugins/canvas/public/components/workpad_page/workpad_shortcuts.tsx Normal file
View file

@ -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<Props> {
public render() {
const { pageId } = this.props;
return (
<Shortcuts
name="ELEMENT"
handler={(action: string, event: Event) => 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);
}
}
}

5
yarn.lock
View file

@ -19044,6 +19044,11 @@ react-error-overlay@^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==
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==
react-fuzzy@^0.5.2:
version "0.5.2"
resolved "https://registry.yarnpkg.com/react-fuzzy/-/react-fuzzy-0.5.2.tgz#fc13bf6f0b785e5fefe908724efebec4935eaefe"