Skip to content

Commit

Permalink
Fix Shortcut EventEmitter Leak/Re-render leaks (elastic#31779)
Browse files Browse the repository at this point in the history
  • Loading branch information
@clintandrewhall
clintandrewhall committed Feb 27, 2019
1 parent 84a0579 commit ce487ae
Show file tree
Hide file tree
Showing 4 changed files with 307 additions and 191 deletions.
184 changes: 51 additions & 133 deletions x-pack/plugins/canvas/public/components/workpad_page/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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', {
Expand Down
84 changes: 26 additions & 58 deletions x-pack/plugins/canvas/public/components/workpad_page/workpad_page.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,14 @@

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';
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 {
Expand Down Expand Up @@ -70,66 +69,42 @@ export class WorkpadPage extends PureComponent {
height,
width,
isEditable,
isSelected,
onDoubleClick,
onKeyDown,
onMouseDown,
onMouseMove,
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
Expand All @@ -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') {
Expand Down
Loading

0 comments on commit ce487ae

Please sign in to comment.