From 7275d9d4dc327259832d28c03b4f89a2ae99c995 Mon Sep 17 00:00:00 2001 From: Bruce Martin Date: Mon, 8 Apr 2019 15:53:12 -0700 Subject: [PATCH] Graph selection state management and history bug fixes (#679) * save graph selection in redux state * fix old graph brush select regressions * refactor graph brush selection to work with undo/redo * update tests to match new crossfilter spatial select API * graph selection state now in redux * remove dead code * sync graph selection with redux state; improvements to undoable machinery * fix regression in undoable * differentiate graph selection cancel from deselect action * simplify calculation * remove debugging code * fix responsive repaint bug in graph selection tool * undoable debugging and code cleanliness * undoable action filter state now merges, rather than replaces * improve comments * add debounce to undoable action filter; improve comments and debug sanity check code * comments * fix undoable bug with clear scatterplot actions * disable undoable debug flag * cleanup API and comments around statemachine * add test id attribute to lasso * add better error handling for gene fetch requests --- .../util/typedCrossfilter/crossfilter.test.js | 6 +- client/src/actions/index.js | 6 + .../components/brushableHistogram/index.js | 7 +- client/src/components/geneExpression/index.js | 41 ++- client/src/components/graph/graph.js | 340 ++++++++++++++---- client/src/components/graph/setupLasso.js | 20 +- .../src/components/graph/setupSVGandBrush.js | 59 +-- client/src/components/graph/util.js | 26 -- client/src/reducers/crossfilter.js | 43 ++- client/src/reducers/graphSelection.js | 59 +++ client/src/reducers/index.js | 46 +-- client/src/reducers/undoable.js | 177 +++++++-- client/src/reducers/undoableConfig.js | 250 +++++++++++++ client/src/reducers/undoableFsm.js | 206 +++++++++++ client/src/util/statemachine/index.js | 95 +++++ .../src/util/typedCrossfilter/crossfilter.js | 6 +- 16 files changed, 1150 insertions(+), 237 deletions(-) delete mode 100644 client/src/components/graph/util.js create mode 100644 client/src/reducers/graphSelection.js create mode 100644 client/src/reducers/undoableConfig.js create mode 100644 client/src/reducers/undoableFsm.js create mode 100644 client/src/util/statemachine/index.js diff --git a/client/__tests__/util/typedCrossfilter/crossfilter.test.js b/client/__tests__/util/typedCrossfilter/crossfilter.test.js index fefa1410..b3c9e5c1 100644 --- a/client/__tests__/util/typedCrossfilter/crossfilter.test.js +++ b/client/__tests__/util/typedCrossfilter/crossfilter.test.js @@ -304,15 +304,15 @@ describe("ImmutableTypedCrossfilter", () => { }); test.each([[0, 0, 1, 1], [0, 0, 0.5, 0.5], [0.5, 0.5, 1, 1]])( "within-rect %d %d %d %d", - (x0, y0, x1, y1) => { + (minX, minY, maxX, maxY) => { expect( p - .select("coords", { mode: "within-rect", x0, y0, x1, y1 }) + .select("coords", { mode: "within-rect", minX, minY, maxX, maxY }) .allSelected() ).toEqual( _.filter(someData, d => { const [x, y] = d.coords; - return x0 <= x && x < x1 && y0 <= y && y < y1; + return minX <= x && x < maxX && minY <= y && y < maxY; }) ); } diff --git a/client/src/actions/index.js b/client/src/actions/index.js index 0865cd57..e4e0562e 100644 --- a/client/src/actions/index.js +++ b/client/src/actions/index.js @@ -302,6 +302,9 @@ const requestDifferentialExpression = (set1, set2, num_genes = 10) => async ( const resetInterface = () => (dispatch, getState) => { const { universe } = getState(); + dispatch({ + type: "user reset start" + }); dispatch({ type: "clear all user defined genes" }); @@ -321,6 +324,9 @@ const resetInterface = () => (dispatch, getState) => { dispatch({ type: "increment graph render counter" }); + dispatch({ + type: "user reset end" + }); }; export default { diff --git a/client/src/components/brushableHistogram/index.js b/client/src/components/brushableHistogram/index.js index 37636060..a49ca8a2 100644 --- a/client/src/components/brushableHistogram/index.js +++ b/client/src/components/brushableHistogram/index.js @@ -124,7 +124,7 @@ class HistogramBrush extends React.Component { const dX0 = Math.abs(x0 - selection[0]); const dX1 = Math.abs(x1 - selection[1]); /* - only update the brush if it is grossly incorrect, + only update the brush if it is grossly incorrect, as defined by the moveDeltaThreshold */ if (dX0 > moveDeltaThreshold || dX1 > moveDeltaThreshold) { @@ -216,14 +216,13 @@ class HistogramBrush extends React.Component { }); } else { dispatch({ - type: "continuous metadata histogram end", + type: "continuous metadata histogram cancel", selection: field, continuousNamespace: { isObs, isUserDefined, isDiffExp - }, - range: null + } }); } }; diff --git a/client/src/components/geneExpression/index.js b/client/src/components/geneExpression/index.js index 37a289ae..6ab58788 100644 --- a/client/src/components/geneExpression/index.js +++ b/client/src/components/geneExpression/index.js @@ -119,8 +119,10 @@ class GeneExpression extends React.Component { postUserErrorToast("That doesn't appear to be a valid gene name."); } else { dispatch({ type: "single user defined gene start" }); - dispatch(actions.requestUserDefinedGene(gene)); - dispatch({ type: "single user defined gene complete" }); + dispatch(actions.requestUserDefinedGene(gene)).then( + () => dispatch({ type: "single user defined gene complete" }), + () => dispatch({ type: "single user defined gene error" }) + ); } } @@ -136,22 +138,25 @@ class GeneExpression extends React.Component { const genes = _.pull(_.uniq(bulkAdd.split(/[ ,]+/)), ""); dispatch({ type: "bulk user defined gene start" }); - genes.forEach(gene => { - if (gene.length === 0) { - keepAroundErrorToast("Must enter a gene name."); - } else if (userDefinedGenes.indexOf(gene) !== -1) { - keepAroundErrorToast("That gene already exists"); - } else if ( - world.varAnnotations.col("name").indexOf(gene) === undefined - ) { - keepAroundErrorToast( - `${gene} doesn't appear to be a valid gene name.` - ); - } else { - dispatch(actions.requestUserDefinedGene(gene)); - } - }); - dispatch({ type: "bulk user defined gene complete" }); + Promise.all( + genes.map(gene => { + if (gene.length === 0) { + return keepAroundErrorToast("Must enter a gene name."); + } + if (userDefinedGenes.indexOf(gene) !== -1) { + return keepAroundErrorToast("That gene already exists"); + } + if (world.varAnnotations.col("name").indexOf(gene) === undefined) { + return keepAroundErrorToast( + `${gene} doesn't appear to be a valid gene name.` + ); + } + return dispatch(actions.requestUserDefinedGene(gene)); + }) + ).then( + () => dispatch({ type: "bulk user defined gene complete" }), + () => dispatch({ type: "bulk user defined gene error" }) + ); } this.setState({ bulkAdd: "" }); diff --git a/client/src/components/graph/graph.js b/client/src/components/graph/graph.js index 32a19fc7..fd8ab625 100644 --- a/client/src/components/graph/graph.js +++ b/client/src/components/graph/graph.js @@ -40,15 +40,16 @@ import { World } from "../../util/stateManager"; scatterplotYYaccessor: state.controls.scatterplotYYaccessor, celllist1: state.differential.celllist1, celllist2: state.differential.celllist2, - library_versions: _.get(state.config, "library_versions", null), + libraryVersions: state.config?.library_versions, // eslint-disable-line camelcase undoDisabled: state["@@undoable/past"].length === 0, - redoDisabled: state["@@undoable/future"].length === 0 + redoDisabled: state["@@undoable/future"].length === 0, + selectionTool: state.graphSelection.tool, + currentSelection: state.graphSelection.selection })) class Graph extends React.Component { constructor(props) { super(props); this.count = 0; - this.inverse = mat4.identity([]); this.graphPaddingTop = 0; this.graphPaddingBottom = 45; this.graphPaddingRight = globals.leftSidebarWidth; @@ -59,8 +60,9 @@ class Graph extends React.Component { }; this.state = { svg: null, - brush: null, - mode: "lasso" + tool: null, + container: null, + mode: "select" }; } @@ -102,9 +104,16 @@ class Graph extends React.Component { }); } - componentDidUpdate(prevProps) { + componentDidUpdate(prevProps, prevState) { const { renderCache } = this; - const { world, crossfilter, colorRGB, responsive } = this.props; + const { + world, + crossfilter, + colorRGB, + responsive, + selectionTool, + currentSelection + } = this.props; const { reglRender, mode, @@ -116,6 +125,7 @@ class Graph extends React.Component { sizeBuffer, svg } = this.state; + let stateChanges = {}; if (reglRender && this.reglRenderState === "rendering" && mode !== "zoom") { reglRender.cancel(); @@ -148,9 +158,7 @@ class Graph extends React.Component { dimension: 2 }); - this.setState({ - offset - }); + stateChanges.offset = offset; } // Colors for each point - a cached value that only changes when @@ -193,21 +201,58 @@ class Graph extends React.Component { prevProps.responsive.height !== responsive.height || prevProps.responsive.width !== responsive.width || /* first time */ - (responsive.height && responsive.width && !svg) + (responsive.height && responsive.width && !svg) || + selectionTool !== prevProps.selectionTool ) { /* clear out whatever was on the div, even if nothing, but usually the brushes etc */ d3.select("#graphAttachPoint") .selectAll("svg") .remove(); - const { svg: newSvg, brush } = setupSVGandBrushElements( - this.handleBrushSelectAction.bind(this), - this.handleBrushDeselectAction.bind(this), + + let handleStart; + let handleDrag; + let handleEnd; + let handleCancel; + if (selectionTool === "brush") { + handleStart = this.handleBrushStartAction.bind(this); + handleDrag = this.handleBrushDragAction.bind(this); + handleEnd = this.handleBrushEndAction.bind(this); + } else { + handleStart = this.handleLassoStart.bind(this); + handleEnd = this.handleLassoEnd.bind(this); + handleCancel = this.handleLassoCancel.bind(this); + } + const { svg: newSvg, tool, container } = setupSVGandBrushElements( + selectionTool, + handleStart, + handleDrag, + handleEnd, + handleCancel, responsive, - this.graphPaddingRight, - this.handleLassoStart.bind(this), - this.handleLassoEnd.bind(this) + this.graphPaddingRight ); - this.setState({ svg: newSvg, brush }); + stateChanges = { ...stateChanges, svg: newSvg, tool, container }; + } + + /* + if the selection tool or state has changed, ensure that the selection + tool correctly reflects the underlying selection. + */ + if ( + currentSelection !== prevProps.currentSelection || + mode !== prevState.mode || + stateChanges.svg + ) { + const { tool, container, offset } = this.state; + this.selectionToolUpdate( + stateChanges.tool ? stateChanges.tool : tool, + stateChanges.container ? stateChanges.container : container, + stateChanges.offset ? stateChanges.offset : offset + ); + } + + if (Object.keys(stateChanges).length > 0) { + this.setState(stateChanges); } } @@ -261,6 +306,86 @@ class Graph extends React.Component { dispatch(actions.resetInterface()); }; + brushToolUpdate(tool, container, offset) { + /* + this is called from componentDidUpdate(), so be very careful using + anything from this.state, which may be updated asynchronously. + */ + const { currentSelection } = this.props; + if (container) { + const toolCurrentSelection = d3.brushSelection(container.node()); + + if (currentSelection.mode === "within-rect") { + /* + if there is a selection, make sure the brush tool matches + */ + const screenCoords = [ + this.mapPointToScreen(currentSelection.brushCoords.northwest, offset), + this.mapPointToScreen(currentSelection.brushCoords.southeast, offset) + ]; + if (!toolCurrentSelection) { + /* tool is not selected, so just move the brush */ + container.call(tool.move, screenCoords); + } else { + /* there is an active selection and a brush - make sure they match */ + /* this just sums the difference of each dimension, of each point */ + let delta = 0; + for (let x = 0; x < 2; x += 1) { + for (let y = 0; y < 2; y += 1) { + delta += Math.abs( + screenCoords[x][y] - toolCurrentSelection[x][y] + ); + } + } + if (delta > 0) { + container.call(tool.move, screenCoords); + } + } + } else if (toolCurrentSelection) { + /* no selection, so clear the brush tool if it is set */ + container.call(tool.move, null); + } + } + } + + lassoToolUpdate(tool, container, offset) { + /* + this is called from componentDidUpdate(), so be very careful using + anything from this.state, which may be updated asynchronously. + */ + const { currentSelection } = this.props; + if (currentSelection.mode === "within-polygon") { + /* + if there is a current selection, make sure the lasso tool matches + */ + const polygon = currentSelection.polygon.map(p => + this.mapPointToScreen(p, offset) + ); + tool.move(polygon); + } else { + tool.reset(); + } + } + + selectionToolUpdate(tool, container, offset) { + /* + this is called from componentDidUpdate(), so be very careful using + anything from this.state, which may be updated asynchronously. + */ + const { selectionTool } = this.props; + switch (selectionTool) { + case "brush": + this.brushToolUpdate(tool, container, offset); + break; + case "lasso": + this.lassoToolUpdate(tool, container, offset); + break; + default: + /* punt? */ + break; + } + } + reglDraw(regl, drawPoints, sizeBuffer, colorBuffer, pointBuffer, camera) { regl.clear({ depth: 1, @@ -304,7 +429,11 @@ class Graph extends React.Component { }); } - invertPoint(pin) { + mapScreenToPoint(pin) { + /* + Map an XY coordinates from screen domain to cell/point range, + accounting for current pan/zoom camera. + */ const { responsive } = this.props; const { regl, camera, offset } = this.state; @@ -323,14 +452,45 @@ class Graph extends React.Component { x * inverse[14] * aspect + inverse[12], y * inverse[14] + inverse[13] ]; + return [(pout[0] + 1) / 2 + offset[0], (pout[1] + 1) / 2 + offset[1]]; } - handleBrushSelectAction() { + mapPointToScreen(xyCell, offset) { /* - This conditional handles procedural brush deselect. Brush emits - an event on procedural deselect because it is move: null + Map an XY coordinate from cell/point domain to screen range. Inverse + of mapScreenToPoint() */ + const { responsive } = this.props; + const { regl, camera } = this.state; + + const gl = regl._gl; + + // get aspect ratio + const aspect = gl.drawingBufferWidth / gl.drawingBufferHeight; + + // compute inverse view matrix + const inverse = mat4.invert([], camera.view()); + + // variable names are choosen to reflect inverse of those used + // in mapScreenToPoint(). + const pout = [ + (xyCell[0] - offset[0]) * 2 - 1, + (xyCell[1] - offset[1]) * 2 - 1 + ]; + const x = (pout[0] - inverse[12]) / aspect / inverse[14]; + const y = (pout[1] - inverse[13]) / inverse[14]; + + const pin = [ + Math.round(((x + 1) * (responsive.width - this.graphPaddingRight)) / 2), + Math.round( + -((y + 1) / 2 - 1) * (responsive.height - this.graphPaddingTop) + ) + ]; + return pin; + } + + handleBrushDragAction() { /* event describing brush position: @-------| @@ -338,79 +498,105 @@ class Graph extends React.Component { | | |-------@ */ + // ignore programatically generated events + if (d3.event.sourceEvent === null || !d3.event.selection) return; + + const { dispatch } = this.props; + const s = d3.event.selection; + const brushCoords = { + northwest: this.mapScreenToPoint([s[0][0], s[0][1]]), + southeast: this.mapScreenToPoint([s[1][0], s[1][1]]) + }; + + dispatch({ + type: "graph brush change", + brushCoords + }); + } + + handleBrushStartAction() { + // Ignore programatically generated events. + if (!d3.event.sourceEvent) return; + + const { dispatch } = this.props; + dispatch({ type: "graph brush start" }); + } + + handleBrushEndAction() { + // Ignore programatically generated events. + if (!d3.event.sourceEvent) return; + /* - No idea why d3 event scope works like this - but apparently - it does - https://bl.ocks.org/EfratVil/0e542f5fc426065dd1d4b6daaa345a9f + coordinates will be included if selection made, null + if selection cleared. */ const { dispatch } = this.props; - - if (d3.event.sourceEvent !== null) { - const s = d3.event.selection; - + const s = d3.event.selection; + if (s) { const brushCoords = { - northwest: this.invertPoint([s[0][0], s[0][1]]), - southeast: this.invertPoint([s[1][0], s[1][1]]) + northwest: this.mapScreenToPoint(s[0]), + southeast: this.mapScreenToPoint(s[1]) }; - dispatch({ - type: "graph brush selection change", + type: "graph brush end", brushCoords }); + } else { + dispatch({ + type: "graph brush deselect" + }); } } handleBrushDeselectAction() { const { dispatch } = this.props; - const { svg, brush } = this.state; - - if (d3.event && !d3.event.selection) { - dispatch({ - type: "graph brush deselect" - }); - } - - if (!d3.event) { - /* - this line clears the brush procedurally, ie., zoom button clicked, - not a click away from brush on svg - */ - svg.select(".graph_brush").call(brush.move, null); - dispatch({ - type: "graph brush deselect" - }); - } + dispatch({ + type: "graph brush deselect" + }); } handleLassoStart() { const { dispatch } = this.props; - // reset selected points when starting a new polygon - // making it easier for the user to make the next selection dispatch({ - type: "lasso started" + type: "graph lasso start" }); } // when a lasso is completed, filter to the points within the lasso polygon handleLassoEnd(polygon) { - const minimumPolygoneArea = 10; + const minimumPolygonArea = 10; const { dispatch } = this.props; if ( polygon.length < 3 || - Math.abs(d3.polygonArea(polygon)) < minimumPolygoneArea + Math.abs(d3.polygonArea(polygon)) < minimumPolygonArea ) { // if less than three points, or super small area, treat as a clear selection. - dispatch({ type: "lasso deselect" }); + dispatch({ type: "graph lasso deselect" }); } else { dispatch({ - type: "lasso selection", - polygon: polygon.map(xy => this.invertPoint(xy)) // transform the polygon + type: "graph lasso end", + polygon: polygon.map(xy => this.mapScreenToPoint(xy)) // transform the polygon }); } } + handleLassoCancel() { + const { dispatch } = this.props; + dispatch({ type: "graph lasso cancel" }); + } + + handleLassoDeselectAction() { + const { dispatch } = this.props; + dispatch({ type: "graph lasso deselect" }); + } + + handleDeselectAction() { + const { selectionTool } = this.props; + if (selectionTool === "brush") this.handleBrushDeselectAction(); + if (selectionTool === "lasso") this.handleLassoDeselectAction(); + } + handleOpacityRangeChange(e) { const { dispatch } = this.props; dispatch({ @@ -425,11 +611,24 @@ class Graph extends React.Component { responsive, crossfilter, resettingInterface, - library_versions, + libraryVersions, undoDisabled, - redoDisabled + redoDisabled, + selectionTool } = this.props; const { mode } = this.state; + + // constants used to create selection tool button + let selectionTooltip; + let selectionButtonClass; + if (selectionTool === "brush") { + selectionTooltip = "Brush selection"; + selectionButtonClass = "bp3-icon-select"; + } else { + selectionTooltip = "Lasso selection"; + selectionButtonClass = "bp3-icon-polygon-filter"; + } + return (
- +