From 40f2b3786b634441e66f04994909e5b896cb6532 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 1 Mar 2024 17:13:14 -0500 Subject: [PATCH 01/29] Add toolbar buttons --- src/clue/app-config.json | 5 +- src/plugins/graph/assets/add-points-icon.svg | 3 + src/plugins/graph/assets/select-tool-icon.svg | 6 ++ .../components/graph-toolbar-registration.tsx | 66 ++++++++++++++++++- 4 files changed, 78 insertions(+), 2 deletions(-) create mode 100644 src/plugins/graph/assets/add-points-icon.svg create mode 100644 src/plugins/graph/assets/select-tool-icon.svg diff --git a/src/clue/app-config.json b/src/clue/app-config.json index 95ba81332c..9ff8996336 100644 --- a/src/clue/app-config.json +++ b/src/clue/app-config.json @@ -272,7 +272,10 @@ "add-points-by-hand", "|", "fit-all", - "toggle-lock" + "toggle-lock", + "|", + "select-points", + "add-points" ] }, "simulator": { diff --git a/src/plugins/graph/assets/add-points-icon.svg b/src/plugins/graph/assets/add-points-icon.svg new file mode 100644 index 0000000000..94b419c5f7 --- /dev/null +++ b/src/plugins/graph/assets/add-points-icon.svg @@ -0,0 +1,3 @@ + + + diff --git a/src/plugins/graph/assets/select-tool-icon.svg b/src/plugins/graph/assets/select-tool-icon.svg new file mode 100644 index 0000000000..5de5dc6003 --- /dev/null +++ b/src/plugins/graph/assets/select-tool-icon.svg @@ -0,0 +1,6 @@ + + + + + + diff --git a/src/plugins/graph/components/graph-toolbar-registration.tsx b/src/plugins/graph/components/graph-toolbar-registration.tsx index bac3a7901a..9088b18ea9 100644 --- a/src/plugins/graph/components/graph-toolbar-registration.tsx +++ b/src/plugins/graph/components/graph-toolbar-registration.tsx @@ -14,6 +14,8 @@ import FitAllIcon from "../assets/fit-all-icon.svg"; import LockAxesIcon from "../assets/lock-axes-icon.svg"; import UnlockAxesIcon from "../assets/unlock-axes-icon.svg"; import AddPointsByHandIcon from "../assets/add-points-by-hand-icon.svg"; +import SelectToolIcon from "../assets/select-tool-icon.svg"; +import AddPointsIcon from "../assets/add-points-icon.svg"; function LinkTileButton(name: string, title: string, allowMultiple: boolean) { @@ -91,7 +93,6 @@ const ToggleLockAxesButton = observer(function ToggleLockAxesButton({name}: IToo const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IToolbarButtonComponentProps) { const graph = useGraphModelContext(); - const hasEditableLayers = graph.getEditableLayers().length > 0; // Enable button if axes are numeric or undefined. @@ -117,6 +118,60 @@ const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IT }); +const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProps) { + const graph = useGraphModelContext(); + const editableLayers = graph.getEditableLayers(); + const hasEditableLayers = editableLayers.length > 0; + + function handleClick() { + // no action + } + + return ( + + + + ); +}); + +const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) { + const graph = useGraphModelContext(); + const editableLayers = graph.getEditableLayers(); + const hasEditableLayers = editableLayers.length > 0; + + // Determine color of the editable dots + let color = "#000000"; + if (editableLayers.length > 0) { + const yAttributes = editableLayers[0].config.yAttributeIDs; + if (yAttributes.length > 0) { + color = graph.getColorForId(yAttributes[0]); + } + } + + const iconStyle = { fill: color }; + + function handleClick() { + // no action + } + + return ( + + + + ); + +}); + registerTileToolbarButtons("graph", [ { @@ -138,5 +193,14 @@ registerTileToolbarButtons("graph", { name: 'add-points-by-hand', component: AddPointsByHandButton + }, + { + name: 'add-points', + component: AddPointsButton + }, + { + name: 'select-points', + component: SelectPointsButton } + ]); From f65b48b6618acaed59f315501758d176839d0c81 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Sun, 3 Mar 2024 23:57:40 -0500 Subject: [PATCH 02/29] Name of data card changed --- cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js index 18ea57a2b6..5938f7526e 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -229,13 +229,13 @@ context('XYPlot Tool Tile', function () { cy.log("renders Data Card tool tile"); clueCanvas.addTile("datacard"); dataCard.getTile().should("exist"); - + for (let i=1; i Date: Mon, 4 Mar 2024 10:17:17 -0500 Subject: [PATCH 03/29] Add point on click --- src/plugins/graph/components/background.tsx | 25 +++++++--- .../components/graph-toolbar-registration.tsx | 6 ++- .../components/graph-wrapper-component.tsx | 47 ++++++++++++++----- .../graph/hooks/use-graph-editing-context.ts | 21 +++++++++ 4 files changed, 81 insertions(+), 18 deletions(-) create mode 100644 src/plugins/graph/hooks/use-graph-editing-context.ts diff --git a/src/plugins/graph/components/background.tsx b/src/plugins/graph/components/background.tsx index aa5c2cbec7..ed416a40e9 100644 --- a/src/plugins/graph/components/background.tsx +++ b/src/plugins/graph/components/background.tsx @@ -11,6 +11,7 @@ import {MarqueeState} from "../models/marquee-state"; import {IGraphModel} from "../models/graph-model"; import {useInstanceIdContext} from "../imports/hooks/use-instance-id-context"; import { useGraphModelContext } from "../hooks/use-graph-model-context"; +import { useGraphEditingContext } from "../hooks/use-graph-editing-context"; interface IProps { marqueeState: MarqueeState @@ -58,6 +59,7 @@ export const Background = forwardRef((props, ref) => { instanceId = useInstanceIdContext() || 'background', layout = useGraphLayoutContext(), graphModel = useGraphModelContext(), + graphEditMode = useGraphEditingContext(), bgRef = ref as MutableRefObject, startX = useRef(0), startY = useRef(0), @@ -66,6 +68,21 @@ export const Background = forwardRef((props, ref) => { selectionTree = useRef(null), previousMarqueeRect = useRef(); + const onClick = useCallback((event: { offsetX: number, offsetY: number, shiftKey: boolean }) => { + if (!graphEditMode.addPointsMode) { + if (!event.shiftKey) { + graphModel.clearAllSelectedCases(); + } + return; + } + const plotBounds = layout.computedBounds.plot; + const relX = event.offsetX - plotBounds.left; + const relY = event.offsetY - plotBounds.top; + const { data: xVal } = layout.getAxisMultiScale("bottom").getDataCoordinate(relX); + const { data: yVal } = layout.getAxisMultiScale("left").getDataCoordinate(relY); + graphEditMode.addPoint(xVal, yVal); + }, [graphEditMode, graphModel, layout]); + const onDragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { const {computedBounds} = layout, plotBounds = computedBounds.plot; @@ -126,11 +143,7 @@ export const Background = forwardRef((props, ref) => { groupElement = bgRef.current; select(groupElement) // clicking on the background deselects all cases - .on('click', (event) => { - if (!event.shiftKey) { - graphModel.clearAllSelectedCases(); - } - }) + .on('click', onClick) .selectAll('rect') .data(range(numRows * numCols)) .join('rect') @@ -144,7 +157,7 @@ export const Background = forwardRef((props, ref) => { .style('fill-opacity', isTransparent ? 0 : 1) .call(dragBehavior); }); - }, [bgRef, dragBehavior, graphModel, layout]); + }, [bgRef, dragBehavior, graphModel, layout, onClick]); return ( diff --git a/src/plugins/graph/components/graph-toolbar-registration.tsx b/src/plugins/graph/components/graph-toolbar-registration.tsx index 9088b18ea9..432dae08bf 100644 --- a/src/plugins/graph/components/graph-toolbar-registration.tsx +++ b/src/plugins/graph/components/graph-toolbar-registration.tsx @@ -7,6 +7,7 @@ import { IToolbarButtonComponentProps, registerTileToolbarButtons } from "../../../components/toolbar/toolbar-button-manager"; import { GraphControllerContext } from "../models/graph-controller"; import { useGraphModelContext } from "../hooks/use-graph-model-context"; +import { useGraphEditingContext } from "../hooks/use-graph-editing-context"; import LinkTableIcon from "../../../clue/assets/icons/geometry/link-table-icon.svg"; import AddIcon from "../../../assets/icons/add-data-graph-icon.svg"; @@ -144,6 +145,8 @@ const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) const editableLayers = graph.getEditableLayers(); const hasEditableLayers = editableLayers.length > 0; + const graphEditMode = useGraphEditingContext(); + // Determine color of the editable dots let color = "#000000"; if (editableLayers.length > 0) { @@ -156,7 +159,7 @@ const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) const iconStyle = { fill: color }; function handleClick() { - // no action + graphEditMode.setAddPointsMode(!graphEditMode.addPointsMode); } return ( @@ -164,6 +167,7 @@ const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) name={name} title="Add point" onClick={handleClick} + selected={graphEditMode.addPointsMode} disabled={!hasEditableLayers} > diff --git a/src/plugins/graph/components/graph-wrapper-component.tsx b/src/plugins/graph/components/graph-wrapper-component.tsx index dc31ba8785..314c2bcc7f 100644 --- a/src/plugins/graph/components/graph-wrapper-component.tsx +++ b/src/plugins/graph/components/graph-wrapper-component.tsx @@ -1,4 +1,4 @@ -import React, { useCallback, useEffect } from "react"; +import React, { useCallback, useEffect, useState } from "react"; import classNames from "classnames"; import { observer } from "mobx-react-lite"; @@ -19,6 +19,9 @@ import { GraphComponent } from "./graph-component"; import { isNumericAxisModel } from "../imports/components/axis/models/axis-model"; import { Point } from "../graph-types"; import { ScaleLinear } from "d3"; +import { GraphEditingContext, IGraphEditMode } from "../hooks/use-graph-editing-context"; +import { ICaseCreation } from "../../../models/data/data-set-types"; +import { addCanonicalCasesToDataSet } from "../../../models/data/data-set"; import "./graph-toolbar-registration"; @@ -36,6 +39,26 @@ export const GraphWrapperComponent: React.FC = observer(function(pro const xAttrType = content.config.attributeType("x"); const yAttrType = content.config.attributeType("y"); + function addPoint(x: number, y: number) { + const layers = content.getEditableLayers(); + if (layers.length > 0) { + const dataConfig = layers[0].config; + const dataset = dataConfig.dataset; + const xAttr = dataConfig.attributeID("x"); + const yAttr = dataConfig.attributeID("y"); + if (dataset && xAttr && yAttr) { + const newCase: ICaseCreation = {}; + newCase[xAttr] = x; + newCase[yAttr] = y; + addCanonicalCasesToDataSet(dataset, [newCase]); + return; + } + } + console.log("Failed to add point"); + } + const [addPointsMode, setAddPointsMode] = useState(false); + const graphEditMode: IGraphEditMode = { addPointsMode, setAddPointsMode, addPoint }; + // This is used for locating Sparrow endpoints. const getDotCenter = useCallback((dotId: string) => { // FIXME Currently, getScreenX and getScreenY only handle numeric axes, so just bail if they are a different type. @@ -153,16 +176,18 @@ export const GraphWrapperComponent: React.FC = observer(function(pro return ( -
- - -
+ +
+ + +
+ ); }); diff --git a/src/plugins/graph/hooks/use-graph-editing-context.ts b/src/plugins/graph/hooks/use-graph-editing-context.ts new file mode 100644 index 0000000000..40d22e4fbb --- /dev/null +++ b/src/plugins/graph/hooks/use-graph-editing-context.ts @@ -0,0 +1,21 @@ +import { createContext, useContext } from "react"; + +/** + * This context holds state relevant to editing points on the Graph tile. + */ + +export interface IGraphEditMode { + addPointsMode: boolean; + setAddPointsMode: (val: boolean) => void; + addPoint: (x: number, y: number) => void; +} + +const kDefaultGraphEditMode: IGraphEditMode = { + addPointsMode: false, + setAddPointsMode: val => { }, + addPoint: (x, y) => { } +}; + +export const GraphEditingContext = createContext(kDefaultGraphEditMode); + +export const useGraphEditingContext = () => useContext(GraphEditingContext); From ffac53e22bb892a395abed979c85a2652ff2c225 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 4 Mar 2024 12:41:56 -0500 Subject: [PATCH 04/29] useEffect was missing some case additions --- src/plugins/graph/hooks/use-plot.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/graph/hooks/use-plot.ts b/src/plugins/graph/hooks/use-plot.ts index fa9369bca9..96976e081f 100644 --- a/src/plugins/graph/hooks/use-plot.ts +++ b/src/plugins/graph/hooks/use-plot.ts @@ -1,6 +1,6 @@ import React, {useCallback, useEffect, useRef} from "react"; import {autorun, reaction} from "mobx"; -import { isSetCaseValuesAction } from "../../../models/data/data-set-actions"; +import { isAddCasesAction, isRemoveCasesAction, isSetCaseValuesAction } from "../../../models/data/data-set-actions"; import {IDotsRef, GraphAttrRoles} from "../graph-types"; import {INumericAxisModel} from "../imports/components/axis/models/axis-model"; import {useGraphLayoutContext} from "../models/graph-layout"; @@ -191,7 +191,7 @@ export const usePlotResponders = (props: IPlotResponderProps) => { // respond to added or removed cases and change in attribute type useEffect(function handleAddRemoveCases() { const disposer = dataConfiguration?.onAction(action => { - if (['addCases', 'removeCases', 'setAttributeType'].includes(action.name)) { + if (isAddCasesAction(action) || isRemoveCasesAction(action) || action.name === 'setAttributeType') { matchCirclesToData({ dataConfiguration, pointRadius: graphModel.getPointRadius(), From e5c9672444949d996f9c7c102ef9f309930d08a1 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 4 Mar 2024 12:55:18 -0500 Subject: [PATCH 05/29] Define add and edit modes --- .../components/graph-toolbar-registration.tsx | 15 +++++++++++---- .../graph/components/graph-wrapper-component.tsx | 12 ++++++++++-- .../graph/hooks/use-graph-editing-context.ts | 13 ++++++++----- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/plugins/graph/components/graph-toolbar-registration.tsx b/src/plugins/graph/components/graph-toolbar-registration.tsx index 432dae08bf..320709dfb5 100644 --- a/src/plugins/graph/components/graph-toolbar-registration.tsx +++ b/src/plugins/graph/components/graph-toolbar-registration.tsx @@ -94,6 +94,7 @@ const ToggleLockAxesButton = observer(function ToggleLockAxesButton({name}: IToo const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IToolbarButtonComponentProps) { const graph = useGraphModelContext(); + const graphEditMode = useGraphEditingContext(); const hasEditableLayers = graph.getEditableLayers().length > 0; // Enable button if axes are numeric or undefined. @@ -104,6 +105,7 @@ const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IT function handleClick() { graph.createEditableLayer(); + graphEditMode.setEditMode("add"); } return ( @@ -121,11 +123,13 @@ const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IT const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProps) { const graph = useGraphModelContext(); + const graphEditModeContext = useGraphEditingContext(); + const editableLayers = graph.getEditableLayers(); const hasEditableLayers = editableLayers.length > 0; function handleClick() { - // no action + graphEditModeContext.setEditMode(graphEditModeContext.editPointsMode ? "none" : "edit"); } return ( @@ -133,6 +137,7 @@ const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProp name={name} title="Select/Move point" onClick={handleClick} + selected={graphEditModeContext.editPointsMode} disabled={!hasEditableLayers} > @@ -142,10 +147,11 @@ const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProp const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) { const graph = useGraphModelContext(); + const graphEditModeContext = useGraphEditingContext(); + const editableLayers = graph.getEditableLayers(); const hasEditableLayers = editableLayers.length > 0; - const graphEditMode = useGraphEditingContext(); // Determine color of the editable dots let color = "#000000"; @@ -159,7 +165,8 @@ const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) const iconStyle = { fill: color }; function handleClick() { - graphEditMode.setAddPointsMode(!graphEditMode.addPointsMode); + // Toggle the mode + graphEditModeContext.setEditMode(graphEditModeContext.addPointsMode ? "none" : "add"); } return ( @@ -167,7 +174,7 @@ const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) name={name} title="Add point" onClick={handleClick} - selected={graphEditMode.addPointsMode} + selected={graphEditModeContext.addPointsMode} disabled={!hasEditableLayers} > diff --git a/src/plugins/graph/components/graph-wrapper-component.tsx b/src/plugins/graph/components/graph-wrapper-component.tsx index 314c2bcc7f..e59b4b05dd 100644 --- a/src/plugins/graph/components/graph-wrapper-component.tsx +++ b/src/plugins/graph/components/graph-wrapper-component.tsx @@ -19,7 +19,7 @@ import { GraphComponent } from "./graph-component"; import { isNumericAxisModel } from "../imports/components/axis/models/axis-model"; import { Point } from "../graph-types"; import { ScaleLinear } from "d3"; -import { GraphEditingContext, IGraphEditMode } from "../hooks/use-graph-editing-context"; +import { GraphEditingContext, IGraphEditMode, IGraphEditModeContext } from "../hooks/use-graph-editing-context"; import { ICaseCreation } from "../../../models/data/data-set-types"; import { addCanonicalCasesToDataSet } from "../../../models/data/data-set"; @@ -57,7 +57,15 @@ export const GraphWrapperComponent: React.FC = observer(function(pro console.log("Failed to add point"); } const [addPointsMode, setAddPointsMode] = useState(false); - const graphEditMode: IGraphEditMode = { addPointsMode, setAddPointsMode, addPoint }; + const [editPointsMode, setEditPointsMode] = useState(false); + + function setEditMode(string: IGraphEditMode) { + setEditPointsMode(string === "edit"); + setAddPointsMode(string === "add"); + } + + const graphEditMode: IGraphEditModeContext = + { editPointsMode, addPointsMode, setEditMode, addPoint }; // This is used for locating Sparrow endpoints. const getDotCenter = useCallback((dotId: string) => { diff --git a/src/plugins/graph/hooks/use-graph-editing-context.ts b/src/plugins/graph/hooks/use-graph-editing-context.ts index 40d22e4fbb..a55b466dd4 100644 --- a/src/plugins/graph/hooks/use-graph-editing-context.ts +++ b/src/plugins/graph/hooks/use-graph-editing-context.ts @@ -4,18 +4,21 @@ import { createContext, useContext } from "react"; * This context holds state relevant to editing points on the Graph tile. */ -export interface IGraphEditMode { +export type IGraphEditMode = "none"|"edit"|"add"; +export interface IGraphEditModeContext { + editPointsMode: boolean; addPointsMode: boolean; - setAddPointsMode: (val: boolean) => void; + setEditMode: (mode: IGraphEditMode) => void; addPoint: (x: number, y: number) => void; } -const kDefaultGraphEditMode: IGraphEditMode = { +const kDefaultGraphEditModeContext: IGraphEditModeContext = { + editPointsMode: false, addPointsMode: false, - setAddPointsMode: val => { }, + setEditMode: mode => { }, addPoint: (x, y) => { } }; -export const GraphEditingContext = createContext(kDefaultGraphEditMode); +export const GraphEditingContext = createContext(kDefaultGraphEditModeContext); export const useGraphEditingContext = () => useContext(GraphEditingContext); From 0391aa7160a526c3cbe11d3b8552a866c5691c66 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 4 Mar 2024 14:15:11 -0500 Subject: [PATCH 06/29] Drag on background --- src/plugins/graph/components/background.tsx | 80 ++++++++++++++----- .../components/graph-toolbar-registration.tsx | 12 +-- .../components/graph-wrapper-component.tsx | 14 +++- .../graph/hooks/use-graph-editing-context.ts | 4 +- 4 files changed, 76 insertions(+), 34 deletions(-) diff --git a/src/plugins/graph/components/background.tsx b/src/plugins/graph/components/background.tsx index ed416a40e9..3da78fbede 100644 --- a/src/plugins/graph/components/background.tsx +++ b/src/plugins/graph/components/background.tsx @@ -1,10 +1,10 @@ import {autorun} from "mobx"; -import React, {forwardRef, MutableRefObject, useCallback, useEffect, useMemo, useRef} from "react"; +import React, {forwardRef, MutableRefObject, useCallback, useEffect, useMemo, useRef, useState} from "react"; import {drag, select, color, range} from "d3"; import RTreeLib from 'rtree'; type RTree = ReturnType; import {CaseData} from "../d3-types"; -import {InternalizedData, rTreeRect} from "../graph-types"; +import {InternalizedData, Point, rTreeRect} from "../graph-types"; import {useGraphLayoutContext} from "../models/graph-layout"; import {rectangleSubtract, rectNormalize} from "../utilities/graph-utils"; import {MarqueeState} from "../models/marquee-state"; @@ -66,24 +66,31 @@ export const Background = forwardRef((props, ref) => { width = useRef(0), height = useRef(0), selectionTree = useRef(null), - previousMarqueeRect = useRef(); + previousMarqueeRect = useRef(), + [potentialPoint, setPotentialPoint] = useState(undefined); + + const pointCoordinates = useCallback((offsetX: number, offsetY: number) => { + const plotBounds = layout.computedBounds.plot; + const relX = offsetX - plotBounds.left; + const relY = offsetY - plotBounds.top; + const { data: x } = layout.getAxisMultiScale("bottom").getDataCoordinate(relX); + const { data: y } = layout.getAxisMultiScale("left").getDataCoordinate(relY); + return { x, y }; + }, [layout]); const onClick = useCallback((event: { offsetX: number, offsetY: number, shiftKey: boolean }) => { if (!graphEditMode.addPointsMode) { if (!event.shiftKey) { + // Clicking on background deselects all cases graphModel.clearAllSelectedCases(); } return; } - const plotBounds = layout.computedBounds.plot; - const relX = event.offsetX - plotBounds.left; - const relY = event.offsetY - plotBounds.top; - const { data: xVal } = layout.getAxisMultiScale("bottom").getDataCoordinate(relX); - const { data: yVal } = layout.getAxisMultiScale("left").getDataCoordinate(relY); - graphEditMode.addPoint(xVal, yVal); - }, [graphEditMode, graphModel, layout]); - - const onDragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { + const {x, y} = pointCoordinates(event.offsetX, event.offsetY); + graphEditMode.addPoint(x, y); + }, [graphEditMode, graphModel, pointCoordinates]); + + const selectModeDragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { const {computedBounds} = layout, plotBounds = computedBounds.plot; selectionTree.current = prepareTree(`.${instanceId}`, 'circle'); @@ -97,7 +104,7 @@ export const Background = forwardRef((props, ref) => { marqueeState.setMarqueeRect({x: startX.current, y: startY.current, width: 0, height: 0}); }, [graphModel, instanceId, layout, marqueeState]), - onDrag = useCallback((event: { dx: number; dy: number }) => { + selectModeDrag = useCallback((event: { dx: number; dy: number }) => { if (event.dx !== 0 || event.dy !== 0) { previousMarqueeRect.current = rectNormalize( {x: startX.current, y: startY.current, w: width.current, h: height.current}); @@ -118,14 +125,41 @@ export const Background = forwardRef((props, ref) => { } }, [graphModel, marqueeState]), - onDragEnd = useCallback(() => { + selectModeDragEnd = useCallback(() => { marqueeState.setMarqueeRect({x: 0, y: 0, width: 0, height: 0}); selectionTree.current = null; - }, [marqueeState]), - dragBehavior = useMemo(() => drag() - .on("start", onDragStart) - .on("drag", onDrag) - .on("end", onDragEnd), [onDrag, onDragEnd, onDragStart]); + }, [marqueeState]); + + const + createModeDragStart = useCallback((event: { x: number; y: number; }) => { + setPotentialPoint(event); + }, []), + + createModeDrag = useCallback((event: { x: number; y: number; }) => { + setPotentialPoint(event); + }, []), + + createModeDragEnd = useCallback((event: { x: number; y: number; }) => { + const point = pointCoordinates(event.x, event.y); + graphEditMode.addPoint(point.x, point.y); + setPotentialPoint(undefined); + }, [graphEditMode, pointCoordinates]); + + + const dragBehavior = useMemo(() => { + if (graphEditMode.addPointsMode) { + return drag() + .on("start", createModeDragStart) + .on("drag", createModeDrag) + .on("end", createModeDragEnd); + } else { + return drag() + .on("start", selectModeDragStart) + .on("drag", selectModeDrag) + .on("end", selectModeDragEnd); + } + }, [createModeDrag, createModeDragEnd, createModeDragStart, graphEditMode.addPointsMode, + selectModeDrag, selectModeDragEnd, selectModeDragStart]); useEffect(() => { return autorun(() => { @@ -142,7 +176,6 @@ export const Background = forwardRef((props, ref) => { col = (index: number) => index % numCols, groupElement = bgRef.current; select(groupElement) - // clicking on the background deselects all cases .on('click', onClick) .selectAll('rect') .data(range(numRows * numCols)) @@ -160,7 +193,12 @@ export const Background = forwardRef((props, ref) => { }, [bgRef, dragBehavior, graphModel, layout, onClick]); return ( - + + + {potentialPoint && + } + ); }); Background.displayName = "Background"; diff --git a/src/plugins/graph/components/graph-toolbar-registration.tsx b/src/plugins/graph/components/graph-toolbar-registration.tsx index 320709dfb5..b0a9eda3e2 100644 --- a/src/plugins/graph/components/graph-toolbar-registration.tsx +++ b/src/plugins/graph/components/graph-toolbar-registration.tsx @@ -152,17 +152,7 @@ const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) const editableLayers = graph.getEditableLayers(); const hasEditableLayers = editableLayers.length > 0; - - // Determine color of the editable dots - let color = "#000000"; - if (editableLayers.length > 0) { - const yAttributes = editableLayers[0].config.yAttributeIDs; - if (yAttributes.length > 0) { - color = graph.getColorForId(yAttributes[0]); - } - } - - const iconStyle = { fill: color }; + const iconStyle = { fill: graphEditModeContext.getEditablePointsColor() }; function handleClick() { // Toggle the mode diff --git a/src/plugins/graph/components/graph-wrapper-component.tsx b/src/plugins/graph/components/graph-wrapper-component.tsx index e59b4b05dd..90a8328856 100644 --- a/src/plugins/graph/components/graph-wrapper-component.tsx +++ b/src/plugins/graph/components/graph-wrapper-component.tsx @@ -64,8 +64,20 @@ export const GraphWrapperComponent: React.FC = observer(function(pro setAddPointsMode(string === "add"); } + function getEditablePointsColor() { + let color = "#000000"; + const editableLayers = content.getEditableLayers(); + if (editableLayers.length > 0) { + const yAttributes = editableLayers[0].config.yAttributeIDs; + if (yAttributes.length > 0) { + color = content.getColorForId(yAttributes[0]); + } + } + return color; + } + const graphEditMode: IGraphEditModeContext = - { editPointsMode, addPointsMode, setEditMode, addPoint }; + { editPointsMode, addPointsMode, setEditMode, addPoint, getEditablePointsColor }; // This is used for locating Sparrow endpoints. const getDotCenter = useCallback((dotId: string) => { diff --git a/src/plugins/graph/hooks/use-graph-editing-context.ts b/src/plugins/graph/hooks/use-graph-editing-context.ts index a55b466dd4..3d55d9335b 100644 --- a/src/plugins/graph/hooks/use-graph-editing-context.ts +++ b/src/plugins/graph/hooks/use-graph-editing-context.ts @@ -10,13 +10,15 @@ export interface IGraphEditModeContext { addPointsMode: boolean; setEditMode: (mode: IGraphEditMode) => void; addPoint: (x: number, y: number) => void; + getEditablePointsColor: () => string; } const kDefaultGraphEditModeContext: IGraphEditModeContext = { editPointsMode: false, addPointsMode: false, setEditMode: mode => { }, - addPoint: (x, y) => { } + addPoint: (x, y) => { }, + getEditablePointsColor: () => "#000000" }; export const GraphEditingContext = createContext(kDefaultGraphEditModeContext); From b6af59c906e885082b537529421da1995e913fb7 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Tue, 5 Mar 2024 07:56:48 -0500 Subject: [PATCH 07/29] Update existing drag code to work again & not reset values. Fix selection; prevent autoscaling while dragging. --- src/plugins/graph/components/graph-axis.tsx | 53 +++------ src/plugins/graph/components/graph.tsx | 1 + src/plugins/graph/components/scatterdots.tsx | 110 ++++++++----------- src/plugins/graph/d3-types.ts | 26 +++++ src/plugins/graph/models/graph-model.ts | 5 +- src/plugins/graph/utilities/graph-utils.ts | 6 +- 6 files changed, 100 insertions(+), 101 deletions(-) diff --git a/src/plugins/graph/components/graph-axis.tsx b/src/plugins/graph/components/graph-axis.tsx index 3e0875306b..de51cee65c 100644 --- a/src/plugins/graph/components/graph-axis.tsx +++ b/src/plugins/graph/components/graph-axis.tsx @@ -18,22 +18,22 @@ import {GraphPlace} from "../imports/components/axis-graph-shared"; import {AttributeLabel} from "./attribute-label"; import {useDropHintString} from "../imports/hooks/use-drop-hint-string"; import { isAddCasesAction, isSetCaseValuesAction } from "../../../models/data/data-set-actions"; -import { computeNiceNumericBounds } from "../utilities/graph-utils"; -import { isNumericAxisModel } from "../imports/components/axis/models/axis-model"; import { DroppableAxis } from "./droppable-axis"; import { useGraphSettingsContext } from "../hooks/use-graph-settings-context"; +import { GraphController } from "../models/graph-controller"; interface IProps { - place: AxisPlace - enableAnimation: MutableRefObject - autoAdjust?: React.MutableRefObject - onDropAttribute?: (place: GraphPlace, dataSet: IDataSet, attrId: string) => void - onRemoveAttribute?: (place: GraphPlace, attrId: string) => void - onTreatAttributeAs?: (place: GraphPlace, attrId: string, treatAs: AttributeType) => void + place: AxisPlace; + enableAnimation: MutableRefObject; + autoAdjust?: React.MutableRefObject; + controller: GraphController; + onDropAttribute?: (place: GraphPlace, dataSet: IDataSet, attrId: string) => void; + onRemoveAttribute?: (place: GraphPlace, attrId: string) => void; + onTreatAttributeAs?: (place: GraphPlace, attrId: string, treatAs: AttributeType) => void; } export const GraphAxis = observer(function GraphAxis({ - place, enableAnimation, autoAdjust, onDropAttribute, onRemoveAttribute, onTreatAttributeAs + place, enableAnimation, autoAdjust, controller, onDropAttribute, onRemoveAttribute, onTreatAttributeAs }: IProps) { const dataConfig = useDataConfigurationContext(), isDropAllowed = dataConfig?.graphPlaceCanAcceptAttributeIDDrop ?? (() => true), @@ -89,37 +89,18 @@ export const GraphAxis = observer(function GraphAxis({ useEffect(() => { if (autoAdjust?.current) { - // TODO multi dataset - this should consider all layers dataConfig?.onAction(action => { - if ( - isAlive(graphModel) && - !graphModel.lockAxes && - (isAddCasesAction(action) || isSetCaseValuesAction(action)) - ) - { - const _axisModel = graphModel?.getAxis(place); - const xValues = dataConfig.numericValuesForAttrRole("x"); - const yValues = dataConfig.numericValuesForAttrRole("y"); - - if (_axisModel && isNumericAxisModel(_axisModel)) { - if (xValues.length > 0 && place === "bottom") { - const minX = Math.min(...xValues); - const maxX = Math.max(...xValues); - const newXBounds = computeNiceNumericBounds(minX, maxX); - _axisModel.setDomain(newXBounds.min, newXBounds.max); - } - - if (yValues.length > 0 && place === "left") { - const minY = Math.min(...yValues); - const maxY = Math.max(...yValues); - const newYBounds = computeNiceNumericBounds(minY, maxY); - _axisModel.setDomain(newYBounds.min, newYBounds.max); - } - } + if (isAlive(graphModel) && + !graphModel.lockAxes && + !graphModel.interactionInProgress && + (isAddCasesAction(action) || isSetCaseValuesAction(action))) { + controller.autoscaleAllAxes(); } }); } - }, [autoAdjust, dataConfig, graphModel, layout, place]); + // we just want this to run once to set up the handlers, not every time something changes. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [autoAdjust]); useEffect(function cleanup () { return () => { diff --git a/src/plugins/graph/components/graph.tsx b/src/plugins/graph/components/graph.tsx index 593b5d6e95..560180e5fc 100644 --- a/src/plugins/graph/components/graph.tsx +++ b/src/plugins/graph/components/graph.tsx @@ -183,6 +183,7 @@ export const Graph = observer( return axes.map((place: AxisPlace) => { return ([]), pointRadiusRef = useRef(0), @@ -32,9 +33,7 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { yScaleRef = useRef(), [dragID, setDragID] = useState(''), currPos = useRef({x: 0, y: 0}), - didDrag = useRef(false), target = useRef(), - selectedDataObjects = useRef>({}), plotNumRef = useRef(0); secondaryAttrIDsRef.current = dataConfiguration?.yAttributeIDs || []; @@ -44,70 +43,74 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { yScaleRef.current = layout.getAxisScale("left") as ScaleNumericBaseType; const onDragStart = useCallback((event: MouseEvent) => { - target.current = select(event.target as SVGSVGElement); + if (graphEditingContext.editPointsMode || graphEditingContext.addPointsMode) { + const targetDot = event.target && inGraphDot(event.target as SVGSVGElement); + if (!targetDot) return; + target.current = select(targetDot as SVGSVGElement); const aCaseData: CaseData = target.current.node().__data__; - if (!aCaseData) return; + if (!aCaseData || !dataConfiguration || aCaseData.dataConfigID !== dataConfiguration.id) return; + event.stopPropagation(); + graphModel.setInteractionInProgress(true); dataset?.beginCaching(); - secondaryAttrIDsRef.current = dataConfiguration?.yAttributeIDs || []; + secondaryAttrIDsRef.current = dataConfiguration.yAttributeIDs || []; enableAnimation.current = false; // We don't want to animate points until end of drag - didDrag.current = false; const tItsID = aCaseData.caseID; plotNumRef.current = target.current.datum()?.plotNum ?? 0; - if (target.current.node()?.nodeName === 'circle') { - target.current - .property('isDragging', true) - .transition() - .attr('r', dragPointRadiusRef.current); - setDragID(tItsID); - currPos.current = {x: event.clientX, y: event.clientY}; - - handleClickOnDot(event, aCaseData, dataConfiguration); - // Record the current values, so we can change them during the drag and restore them when done - const { caseSelection } = dataConfiguration || {}, - xAttrID = dataConfiguration?.attributeID('x') ?? ''; - caseSelection?.forEach(anID => { - selectedDataObjects.current[anID] = { - x: dataset?.getNumeric(anID, xAttrID) ?? 0, - y: dataset?.getNumeric(anID, secondaryAttrIDsRef.current[plotNumRef.current]) ?? 0 - }; - }); - } - }, [dataConfiguration, dataset, enableAnimation]), - - onDrag = useCallback((event: MouseEvent) => { - const xAxisScale = layout.getAxisScale('bottom') as ScaleLinear, - xAttrID = dataConfiguration?.attributeID('x') ?? ''; + // TODO the radius transition doesn't actually do anything + target.current + .property('isDragging', true) + .transition() + .attr('r', dragPointRadiusRef.current); + setDragID(tItsID); + currPos.current = { x: event.clientX, y: event.clientY }; + + handleClickOnDot(event, aCaseData, dataConfiguration); + } + }, [dataConfiguration, dataset, enableAnimation, graphEditingContext.addPointsMode, + graphEditingContext.editPointsMode, graphModel]), + + updatePositions = useCallback((event: MouseEvent) => { if (dragID !== '') { + const xAxisScale = layout.getAxisScale('bottom') as ScaleLinear, + xAttrID = dataConfiguration?.attributeID('x') ?? ''; const newPos = {x: event.clientX, y: event.clientY}, dx = newPos.x - currPos.current.x, dy = newPos.y - currPos.current.y; currPos.current = newPos; if (dx !== 0 || dy !== 0) { - didDrag.current = true; const deltaX = Number(xAxisScale.invert(dx)) - Number(xAxisScale.invert(0)), deltaY = Number(yScaleRef.current?.invert(dy)) - Number(yScaleRef.current?.invert(0)), - caseValues: ICase[] = [], - { caseSelection } = dataConfiguration || {}; - caseSelection?.forEach(anID => { - const currX = Number(dataset?.getNumeric(anID, xAttrID)), - currY = Number(dataset?.getNumeric(anID, secondaryAttrIDsRef.current[plotNumRef.current])); + caseValues: ICase[] = []; + const cellSelection = dataConfiguration?.dataset?.selectedCells; + cellSelection?.forEach(cell => { + const currX = Number(dataset?.getNumeric(cell.caseId, xAttrID)), + currY = Number(dataset?.getNumeric(cell.caseId, cell.attributeId)); if (isFinite(currX) && isFinite(currY)) { caseValues.push({ - __id__: anID, + __id__: cell.caseId, [xAttrID]: currX + deltaX, - [secondaryAttrIDsRef.current[plotNumRef.current]]: currY + deltaY + [cell.attributeId]: currY + deltaY }); } }); caseValues.length && - dataset?.setCaseValues(caseValues, - [xAttrID, secondaryAttrIDsRef.current[plotNumRef.current]]); + dataset?.setCanonicalCaseValues(caseValues); } } }, [layout, dataConfiguration, dataset, dragID]), - onDragEnd = useCallback(() => { + onDrag = useCallback((event: MouseEvent) => { + event.stopPropagation(); + event.preventDefault(); + updatePositions(event); + }, [updatePositions]), + + onDragEnd = useCallback((event: MouseEvent) => { + graphModel.setInteractionInProgress(false); + // Final update does a rescale if appropriate + updatePositions(event); if (dragID !== '') { + // TODO the radius transition doesn't actually do anything target.current .classed('dragging', false) .property('isDragging', false) @@ -115,25 +118,8 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { .attr('r', selectedPointRadiusRef.current); setDragID(() => ''); target.current = null; - - if (didDrag.current) { - const caseValues: ICase[] = [], - { caseSelection } = dataConfiguration || {}, - xAttrID = dataConfiguration?.attributeID('x') ?? ''; - caseSelection?.forEach(anID => { - caseValues.push({ - __id__: anID, - [xAttrID]: selectedDataObjects.current[anID].x, - [secondaryAttrIDsRef.current[plotNumRef.current]]: selectedDataObjects.current[anID].y - }); - }); - startAnimation(enableAnimation); // So points will animate back to original positions - caseValues.length && dataset?.setCaseValues(caseValues, - [xAttrID, secondaryAttrIDsRef.current[plotNumRef.current]]); - didDrag.current = false; - } } - }, [dataConfiguration, dataset, dragID, enableAnimation,]); + }, [dragID, graphModel, updatePositions]); useDragHandlers(window, {start: onDragStart, drag: onDrag, end: onDragEnd}); diff --git a/src/plugins/graph/d3-types.ts b/src/plugins/graph/d3-types.ts index c2a873bdfb..0ee8a684b6 100644 --- a/src/plugins/graph/d3-types.ts +++ b/src/plugins/graph/d3-types.ts @@ -38,3 +38,29 @@ export function selectInnerCircles(svg: DotsElt): DotSelection | null { ? select(svg).selectAll("g.graph-dot .inner-circle") : null; } + +/** + * Returns true if node is a graph dot. + * @param node + * @returns boolean + */ +export function isGraphDot(node: Node) { + return (node.nodeName === "g" + && node instanceof SVGElement + && node.classList.contains("graph-dot")); +} + +/** + * Returns true if node is a graph dot, or part of a graph dot. + * @param node + * @returns the graph dot element, or undefined + */ +export function inGraphDot(node: Node): SVGElement | undefined { + if (isGraphDot(node)) return node as SVGElement; + const parent = node.parentNode; + if (parent && parent instanceof SVGElement) { + return inGraphDot(parent); + } else { + return undefined; + } +} diff --git a/src/plugins/graph/models/graph-model.ts b/src/plugins/graph/models/graph-model.ts index d465e3846c..62c0607e46 100644 --- a/src/plugins/graph/models/graph-model.ts +++ b/src/plugins/graph/models/graph-model.ts @@ -81,7 +81,7 @@ export const GraphModel = TileContentModel yAttributeLabel: types.optional(types.string, kDefaultAxisLabel) }) .volatile(self => ({ - // prevDataSetId: "", + interactionInProgress: false, })) .preProcessSnapshot((snapshot: any) => { const hasLayerAlready:boolean = (snapshot?.layers?.length || 0) > 0; @@ -370,6 +370,9 @@ export const GraphModel = TileContentModel }, setYAttributeLabel(label: string) { self.yAttributeLabel = label; + }, + setInteractionInProgress(value: boolean) { + self.interactionInProgress = value; } })) .actions(self => ({ diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index d4905b1778..476bb7a3e9 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -130,6 +130,7 @@ export function getPointTipText(caseID: string, attributeIDs: (string|undefined) export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConfiguration?: IDataConfigurationModel) { if (!dataConfiguration) return; + event.stopPropagation(); const dataset = dataConfiguration.dataset; const yAttributeId = dataConfiguration.yAttributeID(caseData.plotNum); const yCell = { attributeId: yAttributeId, caseId: caseData.caseID }; @@ -144,6 +145,7 @@ export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConf } else if (extendSelection) { // y cell is selected and Shift key is down => deselect cell dataset?.selectCells([yCell], false); } + console.log("Selected:", dataset?.selectedCells, "among", dataset?.caseIDMap); } export interface IMatchAllCirclesProps { @@ -180,7 +182,7 @@ export interface IMatchCirclesProps { export function matchCirclesToData(props: IMatchCirclesProps) { const { dataConfiguration, enableAnimation, instanceId, dotsElement } = props; const allCaseData = dataConfiguration.joinedCaseDataArrays; - const caseDataKeyFunc = (d: CaseData) => `${d.dataConfigID}-${d.plotNum}-${d.caseID}`; + const caseDataKeyFunc = (d: CaseData) => `${d.dataConfigID}_${instanceId}_${d.plotNum}_${d.caseID}`; // Create the circles const allCircles = selectGraphDots(dotsElement); @@ -193,7 +195,7 @@ export function matchCirclesToData(props: IMatchCirclesProps) { enter => { const g = enter.append('g') .attr('class', `graph-dot`) - .property('id', (d: CaseData) => `${d.dataConfigID}_${instanceId}_${d.caseID}`); + .property('id', (d: CaseData) => `${d.dataConfigID}_${instanceId}_${d.plotNum}_${d.caseID}`); g.append('circle') .attr('class', 'outer-circle'); g.append('circle') From 06cab9f54ed0b2c463c581cfee477f7d8715b60b Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Tue, 5 Mar 2024 09:57:14 -0500 Subject: [PATCH 08/29] Fix rectangle selection --- src/plugins/graph/components/background.tsx | 90 +++++++++++++++---- src/plugins/graph/d3-types.ts | 4 +- .../graph/models/data-configuration-model.ts | 3 + 3 files changed, 79 insertions(+), 18 deletions(-) diff --git a/src/plugins/graph/components/background.tsx b/src/plugins/graph/components/background.tsx index 3da78fbede..f57e03363c 100644 --- a/src/plugins/graph/components/background.tsx +++ b/src/plugins/graph/components/background.tsx @@ -3,8 +3,8 @@ import React, {forwardRef, MutableRefObject, useCallback, useEffect, useMemo, us import {drag, select, color, range} from "d3"; import RTreeLib from 'rtree'; type RTree = ReturnType; -import {CaseData} from "../d3-types"; -import {InternalizedData, Point, rTreeRect} from "../graph-types"; +import {CaseData, graphDotSelector} from "../d3-types"; +import { Point, rTreeRect} from "../graph-types"; import {useGraphLayoutContext} from "../models/graph-layout"; import {rectangleSubtract, rectNormalize} from "../utilities/graph-utils"; import {MarqueeState} from "../models/marquee-state"; @@ -12,46 +12,102 @@ import {IGraphModel} from "../models/graph-model"; import {useInstanceIdContext} from "../imports/hooks/use-instance-id-context"; import { useGraphModelContext } from "../hooks/use-graph-model-context"; import { useGraphEditingContext } from "../hooks/use-graph-editing-context"; +import { ICell } from "../../../models/data/data-types"; interface IProps { marqueeState: MarqueeState } +function getTransformToElement(source: SVGGraphicsElement, elem: SVGGraphicsElement) { + const elemCTM = elem.getScreenCTM(); + const sourceCTM = source.getScreenCTM(); + if (elemCTM && sourceCTM) { + return elemCTM.inverse().multiply(sourceCTM); + } else { + console.warn("Can't get CTM on element"); + } +} + +// Get bounding box INCLUDING any transform="..." on the element itself. +// From https://stackoverflow.com/a/64909822 +function boundingBoxRelativeToElement(fromSpace: SVGGraphicsElement, toSpace: SVGGraphicsElement) { + const bbox = fromSpace.getBBox(); + const m = getTransformToElement(fromSpace, toSpace); + let bbC = new DOMPoint(); bbC.x = bbox.x; bbC.y = bbox.y; + let bbD = new DOMPoint(); bbD.x = bbox.x + bbox.width; bbD.y = bbox.y + bbox.height; + bbC = bbC.matrixTransform(m); + bbD = bbD.matrixTransform(m); + return { x: bbC.x, y: bbC.y, width: Math.abs(bbD.x - bbC.x), height: Math.abs(bbD.y - bbC.y)}; +} + const prepareTree = (areaSelector: string, circleSelector: string): RTree => { const selectionTree = RTreeLib(10); - select(areaSelector).selectAll(circleSelector) - .each((datum: InternalizedData, index, groups) => { - const element: any = groups[index], + select(areaSelector).selectAll(circleSelector) + .each((datum: CaseData, index, groups) => { + const + element: any = groups[index], + bbox = boundingBoxRelativeToElement(element, element.parentElement), rect = { - x: Number(element.cx.baseVal.value), - y: Number(element.cy.baseVal.value), - w: 1, h: 1 + x: bbox.x, + y: bbox.y, + w: bbox.width, + h: bbox.height }; - selectionTree.insert(rect, (element.__data__ as CaseData).caseID); + selectionTree.insert(rect, (element.__data__ as CaseData)); }); return selectionTree; }, + /** + * Searches the new area, and returns cases found in it. + */ getCasesForDelta = (tree: any, newRect: rTreeRect, prevRect: rTreeRect) => { const diffRects = rectangleSubtract(newRect, prevRect); - let caseIDs: string[] = []; + let allCases: CaseData[] = []; diffRects.forEach(aRect => { - const newlyFoundIDs = tree.search(aRect); - caseIDs = caseIDs.concat(newlyFoundIDs); + const newlyFoundCases = tree.search(aRect); + allCases = allCases.concat(newlyFoundCases); }); - return caseIDs; + return allCases; + }, + + /** + * Take list of CaseData objects, return map from data configuration id to cell info. + */ + sortByDataConfiguration = (graphModel: IGraphModel, cases: CaseData[]) => { + const caseDatas = new Map(); + for (const c of cases) { + if (!caseDatas.has(c.dataConfigID)) { + caseDatas.set(c.dataConfigID, []); + } + const dataConfiguration = graphModel.layerForDataConfigurationId(c.dataConfigID)?.config; + if (dataConfiguration) { + const attributeId = dataConfiguration.attributeIdforPlotNumber(c.plotNum); + caseDatas.get(c.dataConfigID)?.push({caseId: c.caseID, attributeId}); + } + } + return caseDatas; }, updateSelections = (graphModel: IGraphModel, tree: any, newRect: rTreeRect, prevRect: rTreeRect) => { const newSelection = getCasesForDelta(tree, newRect, prevRect); const newDeselection = getCasesForDelta(tree, prevRect, newRect); if (newSelection.length) { - graphModel.layers[0].config.dataset?.selectCases(newSelection, true); // FIXME multi dataset + for (const [dataConfId, cells] of sortByDataConfiguration(graphModel, newSelection).entries()) { + const dataConfiguration = graphModel.layerForDataConfigurationId(dataConfId); + if (dataConfiguration) { + dataConfiguration.config.dataset?.selectCells(cells, true); + } + } } if (newDeselection.length) { - graphModel.layers[0].config.dataset?.selectCases(newDeselection, false); + for (const [dataConfId, cells] of sortByDataConfiguration(graphModel, newDeselection).entries()) { + const dataConfiguration = graphModel.layerForDataConfigurationId(dataConfId); + if (dataConfiguration) { + dataConfiguration.config.dataset?.selectCells(cells, false); + } + } } - }; export const Background = forwardRef((props, ref) => { @@ -93,7 +149,7 @@ export const Background = forwardRef((props, ref) => { const selectModeDragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { const {computedBounds} = layout, plotBounds = computedBounds.plot; - selectionTree.current = prepareTree(`.${instanceId}`, 'circle'); + selectionTree.current = prepareTree(`.${instanceId}`, graphDotSelector); startX.current = event.x - plotBounds.left; startY.current = event.y - plotBounds.top; width.current = 0; diff --git a/src/plugins/graph/d3-types.ts b/src/plugins/graph/d3-types.ts index 0ee8a684b6..4f6b0c931d 100644 --- a/src/plugins/graph/d3-types.ts +++ b/src/plugins/graph/d3-types.ts @@ -14,10 +14,12 @@ export type DotsElt = SVGSVGElement | null; // unknown: type of data attached to parent element (none in this case) export type DotSelection = Selection; +export const graphDotSelector = "g.graph-dot"; + // selects all all g elements, which contain inner and outer circles export function selectGraphDots(svg: DotsElt): DotSelection | null { return svg - ? select(svg).selectAll("g.graph-dot") + ? select(svg).selectAll(graphDotSelector) : null; } diff --git a/src/plugins/graph/models/data-configuration-model.ts b/src/plugins/graph/models/data-configuration-model.ts index 2dc546f684..f97fa1b545 100644 --- a/src/plugins/graph/models/data-configuration-model.ts +++ b/src/plugins/graph/models/data-configuration-model.ts @@ -90,6 +90,9 @@ export const DataConfigurationModel = types get yAttributeIDs() { return this.yAttributeDescriptions.map((d: IAttributeDescriptionSnapshot) => d.attributeID); }, + attributeIdforPlotNumber(plotNum: number) { + return this.yAttributeIDs[plotNum]; + }, /** * Returns the sequential number of the given Y attribute ID. * This includes the rightNumeric attribute if any. From 4fb7ef7e5d6765400e89b07c2e8d3ed6f1a7767f Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Tue, 5 Mar 2024 13:02:53 -0500 Subject: [PATCH 09/29] Improve rescaling --- src/plugins/graph/components/graph-axis.tsx | 19 +++++++++++-------- src/plugins/graph/components/scatterdots.tsx | 8 ++++---- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/plugins/graph/components/graph-axis.tsx b/src/plugins/graph/components/graph-axis.tsx index de51cee65c..6d83921121 100644 --- a/src/plugins/graph/components/graph-axis.tsx +++ b/src/plugins/graph/components/graph-axis.tsx @@ -35,7 +35,7 @@ interface IProps { export const GraphAxis = observer(function GraphAxis({ place, enableAnimation, autoAdjust, controller, onDropAttribute, onRemoveAttribute, onTreatAttributeAs }: IProps) { - const dataConfig = useDataConfigurationContext(), + const dataConfig = useDataConfigurationContext(), // FIXME mult-dataset. isDropAllowed = dataConfig?.graphPlaceCanAcceptAttributeIDDrop ?? (() => true), graphModel = useGraphModelContext(), instanceId = useInstanceIdContext(), @@ -89,14 +89,17 @@ export const GraphAxis = observer(function GraphAxis({ useEffect(() => { if (autoAdjust?.current) { - dataConfig?.onAction(action => { - if (isAlive(graphModel) && - !graphModel.lockAxes && - !graphModel.interactionInProgress && - (isAddCasesAction(action) || isSetCaseValuesAction(action))) { + // Set up listener on each layer for changes that require a rescale + for (const layer of graphModel.layers) { + layer.config?.onAction(action => { + if (isAlive(graphModel) && + !graphModel.lockAxes && + !graphModel.interactionInProgress && + (isAddCasesAction(action) || isSetCaseValuesAction(action))) { controller.autoscaleAllAxes(); - } - }); + } + }); + } } // we just want this to run once to set up the handlers, not every time something changes. // eslint-disable-next-line react-hooks/exhaustive-deps diff --git a/src/plugins/graph/components/scatterdots.tsx b/src/plugins/graph/components/scatterdots.tsx index d234d07175..7af8619fb7 100644 --- a/src/plugins/graph/components/scatterdots.tsx +++ b/src/plugins/graph/components/scatterdots.tsx @@ -69,7 +69,7 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { }, [dataConfiguration, dataset, enableAnimation, graphEditingContext.addPointsMode, graphEditingContext.editPointsMode, graphModel]), - updatePositions = useCallback((event: MouseEvent) => { + updatePositions = useCallback((event: MouseEvent, forceUpdate: boolean) => { if (dragID !== '') { const xAxisScale = layout.getAxisScale('bottom') as ScaleLinear, xAttrID = dataConfiguration?.attributeID('x') ?? ''; @@ -77,7 +77,7 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { dx = newPos.x - currPos.current.x, dy = newPos.y - currPos.current.y; currPos.current = newPos; - if (dx !== 0 || dy !== 0) { + if (forceUpdate || dx !== 0 || dy !== 0) { const deltaX = Number(xAxisScale.invert(dx)) - Number(xAxisScale.invert(0)), deltaY = Number(yScaleRef.current?.invert(dy)) - Number(yScaleRef.current?.invert(0)), caseValues: ICase[] = []; @@ -102,13 +102,13 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { onDrag = useCallback((event: MouseEvent) => { event.stopPropagation(); event.preventDefault(); - updatePositions(event); + updatePositions(event, false); }, [updatePositions]), onDragEnd = useCallback((event: MouseEvent) => { graphModel.setInteractionInProgress(false); // Final update does a rescale if appropriate - updatePositions(event); + updatePositions(event, true); if (dragID !== '') { // TODO the radius transition doesn't actually do anything target.current From 59389c930e37c0c184bd9b92a69c9913db99a165 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Tue, 5 Mar 2024 15:37:55 -0500 Subject: [PATCH 10/29] Fix ability to undo/redo adding editable layer --- src/models/history/tree-monitor.ts | 10 +++++++++- src/plugins/graph/components/legend/multi-legend.tsx | 9 ++++++++- src/plugins/graph/hooks/use-plot.ts | 7 +++++-- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/models/history/tree-monitor.ts b/src/models/history/tree-monitor.ts index 9500fed235..939fcd6bad 100644 --- a/src/models/history/tree-monitor.ts +++ b/src/models/history/tree-monitor.ts @@ -1,6 +1,6 @@ import { addDisposer, addMiddleware, getSnapshot, IJsonPatch, Instance, IPatchRecorder, - isActionContextThisOrChildOf, isAlive, recordPatches + isActionContextThisOrChildOf, isAlive, recordPatches, tryResolve } from "mobx-state-tree"; import { nanoid } from "nanoid"; import { TreeManagerAPI } from "./tree-manager-api"; @@ -222,6 +222,14 @@ export class TreeMonitor { // inform the tiles of all changes at the same time. for (const [sharedModelPath, numModifications] of Object.entries(sharedModelModifications)) { if (numModifications > 0) { + // If a shared model has been deleted, we can't run these callbacks without errors, + // so we bail out now. May need improvement if tiles need to be notified about + // deleted shared models. + try { + tryResolve(this.tree, `${sharedModelPath}/sharedModel`); + } catch { + continue; + } // Run the callbacks tracking changes to the shared model. // We need to wait for these to complete because the manager // needs to know when this history entry is complete. If it diff --git a/src/plugins/graph/components/legend/multi-legend.tsx b/src/plugins/graph/components/legend/multi-legend.tsx index 19b64a7880..a6c9c6772f 100644 --- a/src/plugins/graph/components/legend/multi-legend.tsx +++ b/src/plugins/graph/components/legend/multi-legend.tsx @@ -43,7 +43,14 @@ export const MultiLegend = observer(function MultiLegend(props: IMultiLegendProp useEffect(function RespondToLayoutChange() { layout.setDesiredExtent("legend", totalHeight); - onRequestRowHeight?.(instanceId, kGraphDefaultHeight + totalHeight); + // This setTimeout makes sure that this model change does not happen in the + // context of an "undo" operation. Without it, undoing an operation that + // changed the legend height caused an extra height change to be added to the + // undo stack, so from the user's point of view undo would work but then + // redo was not available. + setTimeout(() => { + onRequestRowHeight?.(instanceId, kGraphDefaultHeight + totalHeight); + }, 1); }, [instanceId, layout, onRequestRowHeight, totalHeight]); return ( diff --git a/src/plugins/graph/hooks/use-plot.ts b/src/plugins/graph/hooks/use-plot.ts index 96976e081f..bdeba4aba4 100644 --- a/src/plugins/graph/hooks/use-plot.ts +++ b/src/plugins/graph/hooks/use-plot.ts @@ -10,6 +10,7 @@ import {useCurrent} from "../../../hooks/use-current"; import {useInstanceIdContext} from "../imports/hooks/use-instance-id-context"; import {onAnyAction} from "../../../utilities/mst-utils"; import { IGraphLayerModel } from "../models/graph-layer-model"; +import { mstReaction } from "../../../utilities/mst-reaction"; interface IDragHandlers { start: (event: MouseEvent) => void @@ -129,12 +130,14 @@ export const usePlotResponders = (props: IPlotResponderProps) => { // respond to attribute assignment changes useEffect(() => { - const disposer = reaction( + const disposer = mstReaction( () => GraphAttrRoles.map((aRole) => dataConfiguration?.attributeID(aRole)), () => { startAnimation(enableAnimation); callRefreshPointPositions(false); - } + }, + { name: "usePlot.attribute assignment reaction" }, + dataConfiguration ); return () => disposer(); }, [callRefreshPointPositions, dataConfiguration, enableAnimation]); From 5d411e62374c35cc2f4f2accd90ea3732212478b Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Wed, 6 Mar 2024 08:02:15 -0500 Subject: [PATCH 11/29] Toolbar naming & doc --- docs/unit-configuration.md | 7 +++++-- src/clue/app-config.json | 2 +- .../graph/components/graph-toolbar-registration.tsx | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/unit-configuration.md b/docs/unit-configuration.md index f8c0107e22..b40648cc13 100644 --- a/docs/unit-configuration.md +++ b/docs/unit-configuration.md @@ -165,13 +165,16 @@ Not updated to common toolbar framework and does not support toolbar configurati - `emptyPlotIsNumeric`: boolean, default true - `scalePlotOnValueChange`: boolean, default true -Common toolbar framework; default toolbar buttons: +Uses the common toolbar framework. Default toolbar buttons: - `link-tile` (opens dialog to replace dataset with a new one) +- `add-points-by-hand` (creates a dataset owned by the graph) - `fit-all` (rescale axes to fit all points in view) - `toggle-lock` (lock axes so they won't automatically rescale) +- `move-points` (mode where points can be moved) +- `add-points` (mode where points can be added) -Also available: +Additional buttons available not in default set: - `link-tile-multiple` (opens dialog to add an additional dataset or link variables) diff --git a/src/clue/app-config.json b/src/clue/app-config.json index 7eb62b68da..167f4902a9 100644 --- a/src/clue/app-config.json +++ b/src/clue/app-config.json @@ -274,7 +274,7 @@ "fit-all", "toggle-lock", "|", - "select-points", + "move-points", "add-points" ] }, diff --git a/src/plugins/graph/components/graph-toolbar-registration.tsx b/src/plugins/graph/components/graph-toolbar-registration.tsx index b0a9eda3e2..99ff8514c6 100644 --- a/src/plugins/graph/components/graph-toolbar-registration.tsx +++ b/src/plugins/graph/components/graph-toolbar-registration.tsx @@ -200,7 +200,7 @@ registerTileToolbarButtons("graph", component: AddPointsButton }, { - name: 'select-points', + name: 'move-points', component: SelectPointsButton } From 060df0e29c763c84f4c46f0c4ab07822e235eeba Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Wed, 6 Mar 2024 14:18:33 -0500 Subject: [PATCH 12/29] Don't animate when dots are added. --- src/plugins/graph/utilities/graph-utils.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index 4f49ed0e6e..3c6a7867f9 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -180,14 +180,13 @@ export interface IMatchCirclesProps { } export function matchCirclesToData(props: IMatchCirclesProps) { - const { dataConfiguration, enableAnimation, instanceId, dotsElement } = props; + const { dataConfiguration, instanceId, dotsElement } = props; const allCaseData = dataConfiguration.joinedCaseDataArrays; const caseDataKeyFunc = (d: CaseData) => `${d.dataConfigID}_${instanceId}_${d.plotNum}_${d.caseID}`; // Create the circles const allCircles = selectGraphDots(dotsElement); if (!allCircles) return; - startAnimation(enableAnimation); allCircles .data(allCaseData, caseDataKeyFunc) From 877de6c090a206d5eea49a5be3e00cbf782a5fde Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Wed, 6 Mar 2024 16:37:45 -0500 Subject: [PATCH 13/29] Better fix for animation issue --- src/plugins/graph/utilities/graph-utils.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index 3c6a7867f9..0c5f1dd8d9 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -145,7 +145,6 @@ export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConf } else if (extendSelection) { // y cell is selected and Shift key is down => deselect cell dataset?.selectCells([yCell], false); } - console.log("Selected:", dataset?.selectedCells, "among", dataset?.caseIDMap); } export interface IMatchAllCirclesProps { @@ -180,13 +179,14 @@ export interface IMatchCirclesProps { } export function matchCirclesToData(props: IMatchCirclesProps) { - const { dataConfiguration, instanceId, dotsElement } = props; + const { dataConfiguration, enableAnimation, instanceId, dotsElement } = props; const allCaseData = dataConfiguration.joinedCaseDataArrays; const caseDataKeyFunc = (d: CaseData) => `${d.dataConfigID}_${instanceId}_${d.plotNum}_${d.caseID}`; // Create the circles const allCircles = selectGraphDots(dotsElement); if (!allCircles) return; + startAnimation(enableAnimation); allCircles .data(allCaseData, caseDataKeyFunc) @@ -488,9 +488,14 @@ export function setPointCoordinates(props: ISetPointCoordinates) { const setPositions = (dots: DotSelection | null) => { if (dots !== null) { + // This utilizes a transition() to move the dots smoothly to new positions. + // However, any dots that don't have a position already should just move + // immediately; otherwise they enter from the top left for no reason. dots .transition() - .duration(duration) + .duration((d, i, nodes) => { + return nodes[i].getAttribute('transform') ? duration : 0; + }) .attr('transform', transformForCase) // The rest of this should not be necessary, but works around an apparent Chrome bug. // At least in Chrome v120 on MacOS, if the points are animated from a position far off-screen, From dd0b41d1cddea23649f9459994e490903cf82e4f Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Thu, 7 Mar 2024 09:52:33 -0500 Subject: [PATCH 14/29] Better add-points handling --- src/models/data/data-set.ts | 7 +++++-- src/plugins/graph/components/graph-wrapper-component.tsx | 5 ++++- src/plugins/graph/hooks/use-plot.ts | 4 ++-- src/plugins/graph/models/data-configuration-model.ts | 1 + src/plugins/graph/utilities/graph-utils.ts | 4 ++-- 5 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/models/data/data-set.ts b/src/models/data/data-set.ts index f504f6fbe8..d1dbebf7d5 100644 --- a/src/models/data/data-set.ts +++ b/src/models/data/data-set.ts @@ -221,6 +221,7 @@ export const DataSet = types.model("DataSet", { self.cases.push(newCase); beforeIndex = self.cases.length - 1; } + return newCase; } // `affectedAttributes` are not used in the function, but are present as a potential @@ -689,14 +690,16 @@ export const DataSet = types.model("DataSet", { }, addCanonicalCasesWithIDs(cases: ICase[], beforeID?: string | string[]) { + const newCases: ICase[] = []; cases.forEach((aCase, index) => { const beforeIndex = beforeIndexForInsert(index, beforeID); self.attributes.forEach((attr: IAttribute) => { const value = aCase[attr.id]; attr.addValue(value != null ? value : undefined, beforeIndex); }); - insertCaseIDAtIndex(aCase.__id__, beforeIndex); + newCases.push(insertCaseIDAtIndex(aCase.__id__, beforeIndex)); }); + return newCases; }, setCaseValues(cases: ICase[], affectedAttributes?: string[]) { @@ -904,7 +907,7 @@ export function addCanonicalCasesToDataSet(dataset: IDataSet, cases: ICaseCreati aCase.__id__ = newCaseId(); } }); - dataset.addCanonicalCasesWithIDs(newCases, beforeID); + return dataset.addCanonicalCasesWithIDs(newCases, beforeID); } export function getDataSetBounds(dataSet: IDataSet) { diff --git a/src/plugins/graph/components/graph-wrapper-component.tsx b/src/plugins/graph/components/graph-wrapper-component.tsx index 90a8328856..8322542082 100644 --- a/src/plugins/graph/components/graph-wrapper-component.tsx +++ b/src/plugins/graph/components/graph-wrapper-component.tsx @@ -50,7 +50,10 @@ export const GraphWrapperComponent: React.FC = observer(function(pro const newCase: ICaseCreation = {}; newCase[xAttr] = x; newCase[yAttr] = y; - addCanonicalCasesToDataSet(dataset, [newCase]); + const caseAdded = addCanonicalCasesToDataSet(dataset, [newCase]); + // The values are already correct, but various reactions in the graph code + // expect there to be a value setting action after case creation. + dataset.setCanonicalCaseValues(caseAdded); return; } } diff --git a/src/plugins/graph/hooks/use-plot.ts b/src/plugins/graph/hooks/use-plot.ts index bdeba4aba4..7f431996f1 100644 --- a/src/plugins/graph/hooks/use-plot.ts +++ b/src/plugins/graph/hooks/use-plot.ts @@ -1,6 +1,6 @@ import React, {useCallback, useEffect, useRef} from "react"; import {autorun, reaction} from "mobx"; -import { isAddCasesAction, isRemoveCasesAction, isSetCaseValuesAction } from "../../../models/data/data-set-actions"; +import { isSetCaseValuesAction } from "../../../models/data/data-set-actions"; import {IDotsRef, GraphAttrRoles} from "../graph-types"; import {INumericAxisModel} from "../imports/components/axis/models/axis-model"; import {useGraphLayoutContext} from "../models/graph-layout"; @@ -194,7 +194,7 @@ export const usePlotResponders = (props: IPlotResponderProps) => { // respond to added or removed cases and change in attribute type useEffect(function handleAddRemoveCases() { const disposer = dataConfiguration?.onAction(action => { - if (isAddCasesAction(action) || isRemoveCasesAction(action) || action.name === 'setAttributeType') { + if (['addCases', 'removeCases', 'setAttributeType'].includes(action.name)) { matchCirclesToData({ dataConfiguration, pointRadius: graphModel.getPointRadius(), diff --git a/src/plugins/graph/models/data-configuration-model.ts b/src/plugins/graph/models/data-configuration-model.ts index f97fa1b545..68dc7f2dbe 100644 --- a/src/plugins/graph/models/data-configuration-model.ts +++ b/src/plugins/graph/models/data-configuration-model.ts @@ -606,6 +606,7 @@ export const DataConfigurationModel = types // this is called by the FilteredCases object with additional information about // whether the value changes result in adding/removing any cases from the filtered set // a single call to setCaseValues can result in up to three calls to the handlers + console.log("handling:", cases); if (cases.added.length) { const newCases = self.dataset?.getCases(cases.added); self.handlers.forEach(handler => handler({name: "addCases", args: [newCases]})); diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index 0c5f1dd8d9..934fc820c9 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -182,7 +182,7 @@ export function matchCirclesToData(props: IMatchCirclesProps) { const { dataConfiguration, enableAnimation, instanceId, dotsElement } = props; const allCaseData = dataConfiguration.joinedCaseDataArrays; const caseDataKeyFunc = (d: CaseData) => `${d.dataConfigID}_${instanceId}_${d.plotNum}_${d.caseID}`; - + console.warn("matchCircles", allCaseData); // Create the circles const allCircles = selectGraphDots(dotsElement); if (!allCircles) return; @@ -494,7 +494,7 @@ export function setPointCoordinates(props: ISetPointCoordinates) { dots .transition() .duration((d, i, nodes) => { - return nodes[i].getAttribute('transform') ? duration : 0; + return nodes[i].getAttribute('transform') ? duration : 1; }) .attr('transform', transformForCase) // The rest of this should not be necessary, but works around an apparent Chrome bug. From 5481fc210dae8face6dbcbce115bcad48a088924 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Thu, 7 Mar 2024 15:05:25 -0500 Subject: [PATCH 15/29] Can add a graph dot on top of another dot. --- .../components/graph-wrapper-component.tsx | 48 +++++++------------ src/plugins/graph/graph-types.ts | 3 ++ .../graph/hooks/use-graph-editing-context.ts | 4 +- .../graph/models/data-configuration-model.ts | 25 ++++++++-- src/plugins/graph/models/graph-model.ts | 27 ++++++++++- src/plugins/graph/utilities/graph-utils.ts | 37 +++++++++----- 6 files changed, 96 insertions(+), 48 deletions(-) diff --git a/src/plugins/graph/components/graph-wrapper-component.tsx b/src/plugins/graph/components/graph-wrapper-component.tsx index 8322542082..19ee886cb1 100644 --- a/src/plugins/graph/components/graph-wrapper-component.tsx +++ b/src/plugins/graph/components/graph-wrapper-component.tsx @@ -1,6 +1,7 @@ -import React, { useCallback, useEffect, useState } from "react"; +import React, { useCallback, useEffect } from "react"; import classNames from "classnames"; import { observer } from "mobx-react-lite"; +import { ScaleLinear } from "d3"; import { kSmallAnnotationNodeRadius } from "../../../components/annotations/annotation-utilities"; import { BasicEditableTileTitle } from "../../../components/tiles/basic-editable-tile-title"; @@ -17,11 +18,8 @@ import { IGraphModel } from "../models/graph-model"; import { decipherDotId } from "../utilities/graph-utils"; import { GraphComponent } from "./graph-component"; import { isNumericAxisModel } from "../imports/components/axis/models/axis-model"; -import { Point } from "../graph-types"; -import { ScaleLinear } from "d3"; -import { GraphEditingContext, IGraphEditMode, IGraphEditModeContext } from "../hooks/use-graph-editing-context"; -import { ICaseCreation } from "../../../models/data/data-set-types"; -import { addCanonicalCasesToDataSet } from "../../../models/data/data-set"; +import { GraphEditMode, Point } from "../graph-types"; +import { GraphEditingContext, IGraphEditModeContext } from "../hooks/use-graph-editing-context"; import "./graph-toolbar-registration"; @@ -40,31 +38,14 @@ export const GraphWrapperComponent: React.FC = observer(function(pro const yAttrType = content.config.attributeType("y"); function addPoint(x: number, y: number) { - const layers = content.getEditableLayers(); - if (layers.length > 0) { - const dataConfig = layers[0].config; - const dataset = dataConfig.dataset; - const xAttr = dataConfig.attributeID("x"); - const yAttr = dataConfig.attributeID("y"); - if (dataset && xAttr && yAttr) { - const newCase: ICaseCreation = {}; - newCase[xAttr] = x; - newCase[yAttr] = y; - const caseAdded = addCanonicalCasesToDataSet(dataset, [newCase]); - // The values are already correct, but various reactions in the graph code - // expect there to be a value setting action after case creation. - dataset.setCanonicalCaseValues(caseAdded); - return; - } + const layer = content.editingLayer; + if (layer) { + const dataConfig = layer.config; + dataConfig.addPoint(0, x, y); // Point is added in layer 0 -- we only support editing one trace so far. } - console.log("Failed to add point"); } - const [addPointsMode, setAddPointsMode] = useState(false); - const [editPointsMode, setEditPointsMode] = useState(false); - - function setEditMode(string: IGraphEditMode) { - setEditPointsMode(string === "edit"); - setAddPointsMode(string === "add"); + function setEditMode(string: GraphEditMode) { + content.setEditingMode(string); } function getEditablePointsColor() { @@ -79,8 +60,13 @@ export const GraphWrapperComponent: React.FC = observer(function(pro return color; } - const graphEditMode: IGraphEditModeContext = - { editPointsMode, addPointsMode, setEditMode, addPoint, getEditablePointsColor }; + const graphEditMode: IGraphEditModeContext = { + addPointsMode: content.editingMode==="add", + editPointsMode: content.editingMode==="edit", + setEditMode, + addPoint, + getEditablePointsColor + }; // This is used for locating Sparrow endpoints. const getDotCenter = useCallback((dotId: string) => { diff --git a/src/plugins/graph/graph-types.ts b/src/plugins/graph/graph-types.ts index 9d9b055e5e..44054b819e 100644 --- a/src/plugins/graph/graph-types.ts +++ b/src/plugins/graph/graph-types.ts @@ -108,3 +108,6 @@ export const kGraphAdornmentsClassSelector = `.${kGraphAdornmentsClass}`; export const kDefaultNumericAxisBounds = [0, 10] as const; export const kGraphPortalClass = ".canvas-area"; + +export const GraphEditModes = ["none", "edit", "add"]; +export type GraphEditMode = typeof GraphEditModes[number]; diff --git a/src/plugins/graph/hooks/use-graph-editing-context.ts b/src/plugins/graph/hooks/use-graph-editing-context.ts index 3d55d9335b..64edf62f1a 100644 --- a/src/plugins/graph/hooks/use-graph-editing-context.ts +++ b/src/plugins/graph/hooks/use-graph-editing-context.ts @@ -1,14 +1,14 @@ import { createContext, useContext } from "react"; +import { GraphEditMode } from "../graph-types"; /** * This context holds state relevant to editing points on the Graph tile. */ -export type IGraphEditMode = "none"|"edit"|"add"; export interface IGraphEditModeContext { editPointsMode: boolean; addPointsMode: boolean; - setEditMode: (mode: IGraphEditMode) => void; + setEditMode: (mode: GraphEditMode) => void; addPoint: (x: number, y: number) => void; getEditablePointsColor: () => string; } diff --git a/src/plugins/graph/models/data-configuration-model.ts b/src/plugins/graph/models/data-configuration-model.ts index 68dc7f2dbe..a63f24c28d 100644 --- a/src/plugins/graph/models/data-configuration-model.ts +++ b/src/plugins/graph/models/data-configuration-model.ts @@ -2,8 +2,8 @@ import { observable } from "mobx"; import {scaleQuantile, ScaleQuantile, schemeBlues} from "d3"; import { getSnapshot, Instance, ISerializedActionCall, SnapshotIn, types} from "mobx-state-tree"; import {AttributeType, attributeTypes} from "../../../models/data/attribute"; -import {ICase} from "../../../models/data/data-set-types"; -import {DataSet, IDataSet } from "../../../models/data/data-set"; +import { ICase, ICaseCreation } from "../../../models/data/data-set-types"; +import { DataSet, IDataSet, addCanonicalCasesToDataSet } from "../../../models/data/data-set"; import {getCategorySet, ISharedCaseMetadata, SharedCaseMetadata} from "../../../models/shared/shared-case-metadata"; import {isRemoveAttributeAction, isSetCaseValuesAction} from "../../../models/data/data-set-actions"; import {FilteredCases, IFilteredChangedCases} from "../../../models/data/filtered-cases"; @@ -240,6 +240,26 @@ export const DataConfigurationModel = types } else { self._attributeDescriptions.delete(iRole); } + }, + /** + * Add a point in the given plotNum with the given x and y values. + * @param plotNum + * @param x + * @param y + */ + addPoint(plotNum: number, x: number, y: number) { + const dataset = self.dataset; + const xAttr = self.attributeID("x"); + const yAttr = self.yAttributeIDs[plotNum]; + if (dataset && xAttr && yAttr) { + const newCase: ICaseCreation = {}; + newCase[xAttr] = x; + newCase[yAttr] = y; + const caseAdded = addCanonicalCasesToDataSet(dataset, [newCase]); + // The values are already correct, but various reactions in the graph code + // expect there to be a value setting action after case creation. + dataset.setCanonicalCaseValues(caseAdded); + } } })) .actions(self => ({ @@ -606,7 +626,6 @@ export const DataConfigurationModel = types // this is called by the FilteredCases object with additional information about // whether the value changes result in adding/removing any cases from the filtered set // a single call to setCaseValues can result in up to three calls to the handlers - console.log("handling:", cases); if (cases.added.length) { const newCases = self.dataset?.getCases(cases.added); self.handlers.forEach(handler => handler({name: "addCases", args: [newCases]})); diff --git a/src/plugins/graph/models/graph-model.ts b/src/plugins/graph/models/graph-model.ts index 1503497f39..e1b3048677 100644 --- a/src/plugins/graph/models/graph-model.ts +++ b/src/plugins/graph/models/graph-model.ts @@ -10,7 +10,7 @@ import { } from "../imports/components/axis/models/axis-model"; import { GraphPlace } from "../imports/components/axis-graph-shared"; import { - GraphAttrRole, hoverRadiusFactor, kDefaultAxisLabel, kDefaultNumericAxisBounds, kGraphTileType, + GraphAttrRole, GraphEditMode, hoverRadiusFactor, kDefaultAxisLabel, kDefaultNumericAxisBounds, kGraphTileType, PlotType, PlotTypes, pointRadiusMax, pointRadiusSelectionAddend } from "../graph-types"; import { withoutUndo } from "../../../models/history/without-undo"; @@ -81,7 +81,10 @@ export const GraphModel = TileContentModel yAttributeLabel: types.optional(types.string, kDefaultAxisLabel) }) .volatile(self => ({ + // True if a dragging operation is ongoing - automatic rescaling is deferred until drag is done. interactionInProgress: false, + editingMode: "none" as GraphEditMode, + editingLayerId: undefined as string|undefined })) .preProcessSnapshot((snapshot: any) => { const hasLayerAlready:boolean = (snapshot?.layers?.length || 0) > 0; @@ -204,6 +207,9 @@ export const GraphModel = TileContentModel attributeType(role: GraphAttrRole) { return self.layers[0].config.attributeType(role); }, + getLayerById(layerId: string) { + return self.layers.find(layer => layer.id === layerId); + }, layerForDataConfigurationId(dataConfID: string) { return self.layers.find(layer => layer.config.id === dataConfID); }, @@ -229,6 +235,12 @@ export const GraphModel = TileContentModel getEditableLayers() { return self.layers.filter(l => l.editable); }, + /** + * Return the layer currently being edited, or undefined if none. + */ + get editingLayer() { + return self.editingLayerId && this.getLayerById(self.editingLayerId); + }, /** * Find all tooltip-related attributes from all layers. * Returned as a list of { role, attribute } pairs. @@ -371,6 +383,19 @@ export const GraphModel = TileContentModel setYAttributeLabel(label: string) { self.yAttributeLabel = label; }, + setEditingMode(mode: GraphEditMode, layer?: IGraphLayerModel) { + self.editingMode = mode; + if (mode === "none") { + self.editingLayerId = undefined; + } else { + if (layer) { + self.editingLayerId = layer && layer.id; + } else { + const editables = self.getEditableLayers(); + self.editingLayerId = editables.length>0 ? editables[0].id : undefined; + } + } + }, setInteractionInProgress(value: boolean) { self.interactionInProgress = value; } diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index 934fc820c9..f190d45747 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -1,7 +1,7 @@ import {extent, format, select, timeout} from "d3"; import React from "react"; import { isInteger} from "lodash"; -import { SnapshotOut } from "mobx-state-tree"; +import { SnapshotOut, getParentOfType } from "mobx-state-tree"; import { IClueObjectSnapshot } from "../../../models/annotations/clue-object"; import { PartialSharedModelEntry } from "../../../models/document/document-content-types"; @@ -131,19 +131,35 @@ export function getPointTipText(caseID: string, attributeIDs: (string|undefined) export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConfiguration?: IDataConfigurationModel) { if (!dataConfiguration) return; event.stopPropagation(); + const graphModel = getParentOfType(dataConfiguration, GraphModel); const dataset = dataConfiguration.dataset; const yAttributeId = dataConfiguration.yAttributeID(caseData.plotNum); const yCell = { attributeId: yAttributeId, caseId: caseData.caseID }; - const extendSelection = event.shiftKey, - cellIsSelected = dataset?.isCellSelected(yCell); - if (!cellIsSelected) { - if (extendSelection) { // y cell is not selected and Shift key is down => add y cell to selection - dataset?.selectCells([yCell]); - } else { // y cell is not selected and Shift key is up => only this y cell should be selected - dataset?.setSelectedCells([yCell]); + + if (graphModel.editingMode==="add" + && graphModel.editingLayer && graphModel.editingLayer.config !== dataConfiguration) { + // We add a case to the editable dataset at the same values as the existing case clicked on. + const existingCase = dataset?.getCanonicalCase(caseData.caseID); + if (existingCase) { + const xAttributeId = dataConfiguration.xAttributeID; + const x = dataset?.getNumeric(caseData.caseID, xAttributeId); + const y = dataset?.getNumeric(caseData.caseID, yAttributeId); + if (x !== undefined && y !== undefined) { + graphModel.editingLayer.config.addPoint(0, x, y); + } + } + } else { + const extendSelection = event.shiftKey, + cellIsSelected = dataset?.isCellSelected(yCell); + if (!cellIsSelected) { + if (extendSelection) { // y cell is not selected and Shift key is down => add y cell to selection + dataset?.selectCells([yCell]); + } else { // y cell is not selected and Shift key is up => only this y cell should be selected + dataset?.setSelectedCells([yCell]); + } + } else if (extendSelection) { // y cell is selected and Shift key is down => deselect cell + dataset?.selectCells([yCell], false); } - } else if (extendSelection) { // y cell is selected and Shift key is down => deselect cell - dataset?.selectCells([yCell], false); } } @@ -204,7 +220,6 @@ export function matchCirclesToData(props: IMatchCirclesProps) { ); dotsElement && select(dotsElement).on('click', (event: MouseEvent) => { - event.stopPropagation(); const target = select(event.target as SVGSVGElement); if (target.node()?.nodeName === 'circle') { handleClickOnDot(event, target.datum() as CaseData, dataConfiguration); From 4bd3e59741b3c6788490efcf873174f96ff1df37 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 8 Mar 2024 11:39:29 -0500 Subject: [PATCH 16/29] Fix some remaining bugs & add test. Make use-plot responder handle all add/remove cases; get rid of duplicative use-graph-model responder. Add new buttons to qa-config-subtabs unit, which is used in tests. --- .../tile_tests/xy_plot_tool_spec.js | 78 ++++++++++++++++--- .../support/elements/tile/XYPlotToolTile.js | 3 + src/plugins/graph/components/background.tsx | 18 ++--- .../components/graph-wrapper-component.tsx | 3 +- src/plugins/graph/hooks/use-graph-model.ts | 48 ++++++------ src/plugins/graph/hooks/use-plot.ts | 9 ++- .../graph/models/data-configuration-model.ts | 24 +----- src/plugins/graph/models/graph-layer-model.ts | 25 +++++- src/plugins/graph/utilities/graph-utils.ts | 2 +- 9 files changed, 137 insertions(+), 73 deletions(-) diff --git a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js index c120361061..b118103fbf 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -40,6 +40,23 @@ function buildTable(data) { }); } +// Parse `transform` attributes (used for point positioning) +function xAttributeOfTransform(matcher) { + return attributeOfTransform(matcher, 1); +} +function yAttributeOfTransform(matcher) { + return attributeOfTransform(matcher, 2); +} +function attributeOfTransform(matcher, n) { + return matcher + .invoke('attr', 'transform') + .then(transform => { + return transform.match(/translate\((-?[0-9.]+), *(-?[0-9.]+)\)/)[n]; + }) + .then(parseFloat); +} + + function beforeTest(params) { cy.clearQAData('all'); cy.visit(params); @@ -469,27 +486,64 @@ context('XYPlot Tool Tile', function () { xyTile.getRemoveVariablesButtons().should("have.length", 1); }); - it("Test points by hand", () => { + it('Test points by hand', () => { beforeTest(queryParamsMultiDataset); - cy.log("Add XY Plot Tile"); + cy.log('Add XY Plot Tile'); cy.collapseResourceTabs(); - clueCanvas.addTile("graph"); - xyTile.getTile().should('be.visible'); - - clueCanvas.clickToolbarButton("graph", "add-points-by-hand"); - xyTile.getXAttributesLabel().should('have.length', 1).should("contain.text", "X Variable"); - xyTile.getYAttributesLabel().should('have.length', 1).should("contain.text", "Y Variable 1"); - xyTile.getLayerName().should('have.length', 1).should("contain.text", "Added by hand"); + clueCanvas.addTile('graph'); + xyTile.getTile('.primary-workspace').should('be.visible'); + clueCanvas.toolbarButtonIsDisabled('graph', 'move-points'); + clueCanvas.toolbarButtonIsNotSelected('graph', 'move-points'); + clueCanvas.toolbarButtonIsDisabled('graph', 'add-points'); + clueCanvas.toolbarButtonIsNotSelected('graph', 'add-points'); + + // Create manual layer + clueCanvas.clickToolbarButton('graph', 'add-points-by-hand'); + clueCanvas.toolbarButtonIsDisabled('graph', 'add-points-by-hand'); // only one manual set + clueCanvas.toolbarButtonIsEnabled('graph', 'move-points'); + clueCanvas.toolbarButtonIsEnabled('graph', 'add-points'); + clueCanvas.toolbarButtonIsSelected('graph', 'add-points'); // automatically turns on add mode + xyTile.getXAttributesLabel().should('have.length', 1).should('contain.text', 'X Variable'); + xyTile.getYAttributesLabel().should('have.length', 1).should('contain.text', 'Y Variable 1'); + xyTile.getLayerName().should('have.length', 1).should('contain.text', 'Added by hand'); xyTile.getLayerNameInput().should('not.be.visible'); + // Rename manual layer xyTile.getLayerNameEditButton().click(); xyTile.getLayerNameEditButton().should('have.length', 0); xyTile.getLayerNameInput().should('be.visible').type('Renamed{enter}'); xyTile.getLayerNameInput().should('not.be.visible'); - xyTile.getLayerName().should('have.length', 1).should("contain.text", "Renamed"); - }); + xyTile.getLayerName().should('have.length', 1).should('contain.text', 'Renamed'); - }); + // Add points + xyTile.getGraphDot().should('have.length', 0); + xyTile.getTile('.primary-workspace').should('have.length', 1); + xyTile.getGraphBackground().should('have.length', 1).click(150, 50); + xyTile.getGraphBackground().click(200, 100); + xyTile.getGraphDot().should('have.length', 2); + // Switch to 'select/move' mode + clueCanvas.clickToolbarButton('graph', 'move-points'); + clueCanvas.toolbarButtonIsSelected('graph', 'move-points'); + clueCanvas.toolbarButtonIsNotSelected('graph', 'add-points'); + xyTile.getGraphBackground().click(250, 100); // should not add a point + xyTile.getGraphDot().should('have.length', 2); + // Drag a point to reposition. Should start out where we initially clicked + xAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 150, 10); + yAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 50, 10); + xyTile.getGraphDot().eq(0).children('circle').eq(1) + .trigger("mousedown", 150, 50, { force: true }) + .trigger("drag", 175, 75, { force: true }) + .trigger("mouseup", 175, 75, { force: true }); + cy.wait(1000); + xAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 175, 10); + yAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 75, 10); + + // Click toolbar button again to leave edit mode + clueCanvas.clickToolbarButton('graph', 'move-points'); + clueCanvas.toolbarButtonIsNotSelected('graph', 'move-points'); + clueCanvas.toolbarButtonIsNotSelected('graph', 'add-points'); + }); + }); }); diff --git a/cypress/support/elements/tile/XYPlotToolTile.js b/cypress/support/elements/tile/XYPlotToolTile.js index aa97dd1a87..f015a1e50c 100644 --- a/cypress/support/elements/tile/XYPlotToolTile.js +++ b/cypress/support/elements/tile/XYPlotToolTile.js @@ -6,6 +6,9 @@ class XYPlotToolTile { getTile(workspaceClass) { return cy.get(`${wsClass(workspaceClass)} .canvas-area .graph-tool-tile`); } + getGraphBackground(workspaceClass) { + return cy.get(`${wsClass(workspaceClass)} svg [data-testid=graph-background]`); + } getXYPlotTitle(workspaceClass) { return cy.get(`${wsClass(workspaceClass)} .canvas-area .editable-tile-title`); } diff --git a/src/plugins/graph/components/background.tsx b/src/plugins/graph/components/background.tsx index f57e03363c..a8e16f69cd 100644 --- a/src/plugins/graph/components/background.tsx +++ b/src/plugins/graph/components/background.tsx @@ -135,15 +135,15 @@ export const Background = forwardRef((props, ref) => { }, [layout]); const onClick = useCallback((event: { offsetX: number, offsetY: number, shiftKey: boolean }) => { - if (!graphEditMode.addPointsMode) { - if (!event.shiftKey) { - // Clicking on background deselects all cases - graphModel.clearAllSelectedCases(); - } - return; + if (graphEditMode.addPointsMode) { + const {x, y} = pointCoordinates(event.offsetX, event.offsetY); + graphEditMode.addPoint(x, y); + } else { + // If not in add mode or shifted, clicking on background deselects everything + if (!event.shiftKey) { + graphModel.clearAllSelectedCases(); + } } - const {x, y} = pointCoordinates(event.offsetX, event.offsetY); - graphEditMode.addPoint(x, y); }, [graphEditMode, graphModel, pointCoordinates]); const selectModeDragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { @@ -249,7 +249,7 @@ export const Background = forwardRef((props, ref) => { }, [bgRef, dragBehavior, graphModel, layout, onClick]); return ( - + {potentialPoint && = observer(function(pro function addPoint(x: number, y: number) { const layer = content.editingLayer; if (layer) { - const dataConfig = layer.config; - dataConfig.addPoint(0, x, y); // Point is added in layer 0 -- we only support editing one trace so far. + layer.addPoint(0, x, y); // Point is added in layer 0 -- we only support editing one trace so far. } } function setEditMode(string: GraphEditMode) { diff --git a/src/plugins/graph/hooks/use-graph-model.ts b/src/plugins/graph/hooks/use-graph-model.ts index 9dd2e9126f..29979f4f14 100644 --- a/src/plugins/graph/hooks/use-graph-model.ts +++ b/src/plugins/graph/hooks/use-graph-model.ts @@ -1,7 +1,6 @@ import {MutableRefObject, useCallback, useEffect} from "react"; -import {isAddCasesAction, isRemoveCasesAction} from "../../../models/data/data-set-actions"; import {IGraphModel, isGraphVisualPropsAction} from "../models/graph-model"; -import {matchAllCirclesToData, matchCirclesToData, setNiceDomain, startAnimation} from "../utilities/graph-utils"; +import {matchAllCirclesToData, setNiceDomain, startAnimation} from "../utilities/graph-utils"; import {onAnyAction} from "../../../utilities/mst-utils"; interface IProps { @@ -13,16 +12,16 @@ interface IProps { export function useGraphModel(props: IProps) { const { graphModel, enableAnimation, instanceId } = props; - const callMatchCirclesToData = useCallback((layer) => { - matchCirclesToData({ - dataConfiguration: layer.config, - pointRadius: graphModel.getPointRadius(), - pointColor: graphModel.pointColor, - pointStrokeColor: graphModel.pointStrokeColor, - dotsElement: layer.dotsElt, - enableAnimation, instanceId - }); - }, [graphModel, enableAnimation, instanceId]); + // const callMatchCirclesToData = useCallback((layer) => { + // matchCirclesToData({ + // dataConfiguration: layer.config, + // pointRadius: graphModel.getPointRadius(), + // pointColor: graphModel.pointColor, + // pointStrokeColor: graphModel.pointStrokeColor, + // dotsElement: layer.dotsElt, + // enableAnimation, instanceId + // }); + // }, [graphModel, enableAnimation, instanceId]); const callMatchAllCirclesToData = useCallback(() => { matchAllCirclesToData({ @@ -31,17 +30,20 @@ export function useGraphModel(props: IProps) { }, [graphModel, enableAnimation, instanceId]); // respond to added/removed cases - useEffect(function installAddRemoveCaseHandler() { - const disposers: (()=>void)[] = []; - for (const layer of graphModel.layers) { - disposers.push(layer.config.onAction(action => { - if (isAddCasesAction(action) || isRemoveCasesAction(action)) { - callMatchCirclesToData(layer); - } - })); - } - return () => { disposers.forEach((d) => { d(); }); }; - }, [graphModel.layers, callMatchCirclesToData]); + // TODO seems redundant with use-plot.ts handleAddRemoveCases + // useEffect(function installAddRemoveCaseHandler() { + // const disposers: (()=>void)[] = []; + // for (const layer of graphModel.layers) { + // console.log("registering layer responder"); + // disposers.push(layer.config.onAction(action => { + // console.log('layer responder examining', action); + // if (isAddCasesAction(action) || isRemoveCasesAction(action)) { + // callMatchCirclesToData(layer); + // } + // })); + // } + // return () => { disposers.forEach((d) => { d(); }); }; + // }, [graphModel.layers, callMatchCirclesToData]); // respond to change in plotType useEffect(function installPlotTypeAction() { diff --git a/src/plugins/graph/hooks/use-plot.ts b/src/plugins/graph/hooks/use-plot.ts index 7f431996f1..dce3b9eb14 100644 --- a/src/plugins/graph/hooks/use-plot.ts +++ b/src/plugins/graph/hooks/use-plot.ts @@ -1,6 +1,7 @@ import React, {useCallback, useEffect, useRef} from "react"; import {autorun, reaction} from "mobx"; -import { isSetCaseValuesAction } from "../../../models/data/data-set-actions"; +import { isAddCasesAction, isRemoveAttributeAction, isRemoveCasesAction, isSetCaseValuesAction } + from "../../../models/data/data-set-actions"; import {IDotsRef, GraphAttrRoles} from "../graph-types"; import {INumericAxisModel} from "../imports/components/axis/models/axis-model"; import {useGraphLayoutContext} from "../models/graph-layout"; @@ -194,7 +195,11 @@ export const usePlotResponders = (props: IPlotResponderProps) => { // respond to added or removed cases and change in attribute type useEffect(function handleAddRemoveCases() { const disposer = dataConfiguration?.onAction(action => { - if (['addCases', 'removeCases', 'setAttributeType'].includes(action.name)) { + if (isAddCasesAction(action) + || isRemoveCasesAction(action) + || isRemoveAttributeAction(action) + || ['addCases', 'removeCases', 'setAttributeType'].includes(action.name)) { + matchCirclesToData({ dataConfiguration, pointRadius: graphModel.getPointRadius(), diff --git a/src/plugins/graph/models/data-configuration-model.ts b/src/plugins/graph/models/data-configuration-model.ts index a63f24c28d..95be8dff56 100644 --- a/src/plugins/graph/models/data-configuration-model.ts +++ b/src/plugins/graph/models/data-configuration-model.ts @@ -2,8 +2,8 @@ import { observable } from "mobx"; import {scaleQuantile, ScaleQuantile, schemeBlues} from "d3"; import { getSnapshot, Instance, ISerializedActionCall, SnapshotIn, types} from "mobx-state-tree"; import {AttributeType, attributeTypes} from "../../../models/data/attribute"; -import { ICase, ICaseCreation } from "../../../models/data/data-set-types"; -import { DataSet, IDataSet, addCanonicalCasesToDataSet } from "../../../models/data/data-set"; +import { ICase } from "../../../models/data/data-set-types"; +import { DataSet, IDataSet } from "../../../models/data/data-set"; import {getCategorySet, ISharedCaseMetadata, SharedCaseMetadata} from "../../../models/shared/shared-case-metadata"; import {isRemoveAttributeAction, isSetCaseValuesAction} from "../../../models/data/data-set-actions"; import {FilteredCases, IFilteredChangedCases} from "../../../models/data/filtered-cases"; @@ -240,26 +240,6 @@ export const DataConfigurationModel = types } else { self._attributeDescriptions.delete(iRole); } - }, - /** - * Add a point in the given plotNum with the given x and y values. - * @param plotNum - * @param x - * @param y - */ - addPoint(plotNum: number, x: number, y: number) { - const dataset = self.dataset; - const xAttr = self.attributeID("x"); - const yAttr = self.yAttributeIDs[plotNum]; - if (dataset && xAttr && yAttr) { - const newCase: ICaseCreation = {}; - newCase[xAttr] = x; - newCase[yAttr] = y; - const caseAdded = addCanonicalCasesToDataSet(dataset, [newCase]); - // The values are already correct, but various reactions in the graph code - // expect there to be a value setting action after case creation. - dataset.setCanonicalCaseValues(caseAdded); - } } })) .actions(self => ({ diff --git a/src/plugins/graph/models/graph-layer-model.ts b/src/plugins/graph/models/graph-layer-model.ts index 37128bb9c3..a4e4bdcfc2 100644 --- a/src/plugins/graph/models/graph-layer-model.ts +++ b/src/plugins/graph/models/graph-layer-model.ts @@ -1,4 +1,4 @@ -import { getParentOfType, Instance, SnapshotIn, types } from "@concord-consortium/mobx-state-tree"; +import { getParentOfType, Instance, SnapshotIn, types } from "mobx-state-tree"; import { typedId } from "../../../utilities/js-utils"; import { onAnyAction } from "../../../utilities/mst-utils"; import { DataConfigurationModel, IDataConfigurationModel } from "./data-configuration-model"; @@ -7,9 +7,10 @@ import { GraphPlace } from "../imports/components/axis-graph-shared"; import { GraphAttrRole } from "../graph-types"; import { IUpdateCategoriesOptions } from "../adornments/adornment-models"; import { GraphModel } from "./graph-model"; -import { IDataSet } from "../../../models/data/data-set"; +import { IDataSet, addCanonicalCasesToDataSet } from "../../../models/data/data-set"; import { ISharedCaseMetadata } from "../../../models/shared/shared-case-metadata"; import { DotsElt } from "../d3-types"; +import { ICaseCreation } from "../../../models/data/data-set-types"; export const GraphLayerModel = types .model('GraphLayerModel') @@ -63,6 +64,26 @@ export const GraphLayerModel = types clearAutoAssignedAttributes() { self.autoAssignedAttributes = []; }, + /** + * Add a point in the given plotNum with the given x and y values. + * @param plotNum + * @param x + * @param y + */ + addPoint(plotNum: number, x: number, y: number) { + const dataset = self.config.dataset; + const xAttr = self.config.attributeID("x"); + const yAttr = self.config.yAttributeIDs[plotNum]; + if (dataset && xAttr && yAttr) { + const newCase: ICaseCreation = {}; + newCase[xAttr] = x; + newCase[yAttr] = y; + const caseAdded = addCanonicalCasesToDataSet(dataset, [newCase]); + // The values are already correct, but various reactions in the graph code + // expect there to be a value setting action after case creation. + dataset.setCanonicalCaseValues(caseAdded); + } + }, configureLinkedLayer() { if (!self.config) { console.warn("GraphLayerModel.configureLinkedLayer requires a dataset"); diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index f190d45747..d85355871e 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -145,7 +145,7 @@ export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConf const x = dataset?.getNumeric(caseData.caseID, xAttributeId); const y = dataset?.getNumeric(caseData.caseID, yAttributeId); if (x !== undefined && y !== undefined) { - graphModel.editingLayer.config.addPoint(0, x, y); + graphModel.editingLayer.addPoint(0, x, y); } } } else { From 853bb2611b22cd92612fd0fd6a1555f8826b0eff Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 8 Mar 2024 11:39:42 -0500 Subject: [PATCH 17/29] Update testing unit --- src/public/demo/units/qa-config-subtabs/content.json | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/public/demo/units/qa-config-subtabs/content.json b/src/public/demo/units/qa-config-subtabs/content.json index 5694d5ad91..1b84cf4c83 100644 --- a/src/public/demo/units/qa-config-subtabs/content.json +++ b/src/public/demo/units/qa-config-subtabs/content.json @@ -39,8 +39,12 @@ "tools": [ "link-tile-multiple", "add-points-by-hand", + "|", "fit-all", - "toggle-lock" + "toggle-lock", + "|", + "move-points", + "add-points" ] }, "diagram": { From 858d9d092e199b3cccf9d4e55deb8251b5e15293 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 8 Mar 2024 15:48:15 -0500 Subject: [PATCH 18/29] Fit graph copying bug. The duplicateTiles method was sending MST objects, rather than snapshots, which caused problems when cloned and contained unwanted references. To simplify the types needed for passing snapshots, remove unnecessary PartialTile and PartialSharedModelEntry types, and use standard snapshots. --- docs/mst-detached-error.md | 2 +- src/models/document/document-content-types.ts | 8 ----- src/models/document/document-content.ts | 29 ++++++++++++------- src/models/tiles/table/table-utils.ts | 6 ++-- src/models/tiles/tile-content-info.ts | 6 ++-- src/plugins/data-card/data-card-content.ts | 4 +-- src/plugins/graph/utilities/graph-utils.ts | 6 ++-- 7 files changed, 30 insertions(+), 31 deletions(-) diff --git a/docs/mst-detached-error.md b/docs/mst-detached-error.md index 77f6634297..40227591eb 100644 --- a/docs/mst-detached-error.md +++ b/docs/mst-detached-error.md @@ -12,7 +12,7 @@ Below is a strategy for tracking down the cause and a solution for one particula Hopefully from the message you can figure out the rough area of the code where the problem is happening. If you can pin point the UI action that causes the warning to be printed then you can use the following approach. -In your local code find `kEnableLivelinessChecking` located in `index.tsx` and set to be `true`. This will cause these console warnings to actually be thrown errors instead. In many cases these thrown errors will be caught and ignored so after you set this to true you probably won't see any messages in the console if you try to duplicate the problem again. +In your local code find `kEnableLivelinessChecking` located in `initialize-app.tsx` and set to be `true`. This will cause these console warnings to actually be thrown errors instead. In many cases these thrown errors will be caught and ignored so after you set this to true you probably won't see any messages in the console if you try to duplicate the problem again. Instead of looking in the console, what you should do is use the dev tools debugger to track it down. Setup the app to just before the error would normally be printed to the console. Then in the debug panel turn on "pause on exceptions" and check the box "pause on caught exceptions". Now trigger the error. The app should pause and you should be in the debugger. diff --git a/src/models/document/document-content-types.ts b/src/models/document/document-content-types.ts index a4fef9f9e8..f046fb7d1e 100644 --- a/src/models/document/document-content-types.ts +++ b/src/models/document/document-content-types.ts @@ -1,5 +1,4 @@ import { IArrowAnnotation } from "../annotations/arrow-annotation"; -import { SharedModelSnapshotType } from "../shared/shared-model"; import { IDragSharedModelItem } from "../shared/shared-model-manager"; import { IDragTileItem } from "../tiles/tile-model"; import { IDropRowInfo } from "./tile-row"; @@ -46,10 +45,3 @@ export interface IDragTilesData { annotations: IArrowAnnotation[]; } -export interface PartialTile { - id: string; -} -export interface PartialSharedModelEntry { - sharedModel: SharedModelSnapshotType; - tiles: PartialTile[]; -} diff --git a/src/models/document/document-content.ts b/src/models/document/document-content.ts index 3d8d9fbd8b..051af27fb3 100644 --- a/src/models/document/document-content.ts +++ b/src/models/document/document-content.ts @@ -1,7 +1,7 @@ import stringify from "json-stringify-pretty-compact"; import { applySnapshot, getSnapshot, Instance, SnapshotIn } from "mobx-state-tree"; import { cloneDeep, each } from "lodash"; -import { IDragTilesData, PartialSharedModelEntry, PartialTile, +import { IDragTilesData, IDocumentContentAddTileOptions } from "./document-content-types"; import { DocumentContentModelWithTileDragging } from "./drag-tiles"; import { IDropRowInfo, TileRowModelType, TileRowSnapshotOutType } from "./tile-row"; @@ -14,6 +14,7 @@ import { getTileContentInfo, IDocumentExportOptions } from "../tiles/tile-conten import { IDragTileItem, IDropTileItem, ITileModel, ITileModelSnapshotOut } from "../tiles/tile-model"; import { uniqueId } from "../../utilities/js-utils"; import { comma, StringBuilder } from "../../utilities/string-builder"; +import { SharedModelEntrySnapshotType } from "./shared-model-entry"; // Imports related to hard coding shared model duplication import { @@ -231,13 +232,13 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" // Copies tiles and shared models into the specified row, giving them all new ids copyTiles( tiles: IDragTileItem[], - sharedModelEntries: PartialSharedModelEntry[], + sharedModelEntries: SharedModelEntrySnapshotType[], annotations: IArrowAnnotationSnapshot[], rowInfo: IDropRowInfo ) { // Update shared models with new names and ids const updatedSharedModelMap: Record = {}; - const newSharedModelEntries: PartialSharedModelEntry[] = []; + const newSharedModelEntries: SharedModelEntrySnapshotType[] = []; sharedModelEntries.forEach(sharedModelEntry => { // For now, only duplicate shared data sets if (isSharedDataSetSnapshot(sharedModelEntry.sharedModel)) { @@ -266,7 +267,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" }); const findTileSharedModelEntries = (tileId: string) => { - return sharedModelEntries.filter(entry => entry.tiles?.map(t => t.id).includes(tileId)); + return sharedModelEntries.filter(entry => entry.tiles?.includes(tileId)); }; // Update tile content with new shared model ids @@ -302,7 +303,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" // Update tile ids for shared models and add those references to document. // The shared datasets have already been added above. newSharedModelEntries.forEach(sharedModelEntry => { - const updatedTileIds: string[] = sharedModelEntry.tiles.map((oldTile: PartialTile) => tileIdMap[oldTile.id]) + const updatedTileIds: string[] = (sharedModelEntry.tiles||[]).map((oldTile) => tileIdMap[oldTile]) .filter((tileId: string | undefined) => tileId !== undefined); if (isSharedDataSetSnapshot(sharedModelEntry.sharedModel)) { const oldProvider = sharedModelEntry.sharedModel.providerId; @@ -332,7 +333,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" }); // Update tile and object ids for annotations and add copies to document - const updateObject = (object?: IClueObjectSnapshot) => { + const updateAnnotationObject = (object?: IClueObjectSnapshot) => { if (object) { const tile = tiles.find(t => t.tileId === object?.tileId); if (tile) { @@ -344,11 +345,12 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" } } }; + annotations.forEach(annotation => { if (isArrowAnnotationSnapshot(annotation)) { const newAnnotationSnapshot = cloneDeep(annotation); - updateObject(newAnnotationSnapshot.sourceObject); - updateObject(newAnnotationSnapshot.targetObject); + updateAnnotationObject(newAnnotationSnapshot.sourceObject); + updateAnnotationObject(newAnnotationSnapshot.targetObject); updateArrowAnnotationTileIds(newAnnotationSnapshot, tileIdMap); newAnnotationSnapshot.id = uniqueId(); self.addArrow(ArrowAnnotation.create(newAnnotationSnapshot)); @@ -361,7 +363,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" const { tiles, sharedModels, annotations } = dragTiles; // Convert IDragSharedModelItems to partial SharedModelEnries - const sharedModelEntries: PartialSharedModelEntry[] = []; + const sharedModelEntries: SharedModelEntrySnapshotType[] = []; sharedModels.forEach(dragSharedModel => { try { const content = JSON.parse(dragSharedModel.content); @@ -370,7 +372,7 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" if (sharedModel) { sharedModelEntries.push({ sharedModel, - tiles: dragSharedModel.tileIds.map(tileId => ({ id: tileId })) + tiles: dragSharedModel.tileIds }); } } catch (e) { @@ -389,9 +391,14 @@ export const DocumentContentModel = DocumentContentModelWithTileDragging.named(" const sharedModelEntries = Object.values(self.getSharedModelsUsedByTiles(tileIds)); const annotations = Object.values(self.getAnnotationsUsedByTiles(tileIds, true)); + // Make sure we are only passing snapshots to copyTiles, since they need to be cloned & modified. + const snapshots = sharedModelEntries.map(sme => { + return getSnapshot(sme); + }); + self.copyTiles( tiles, - sharedModelEntries, + snapshots, annotations, { rowInsertIndex: rowIndex } ); diff --git a/src/models/tiles/table/table-utils.ts b/src/models/tiles/table/table-utils.ts index bea9af3c17..0d5bf4b807 100644 --- a/src/models/tiles/table/table-utils.ts +++ b/src/models/tiles/table/table-utils.ts @@ -1,7 +1,7 @@ import { TableContentSnapshotType } from "./table-content"; import { IClueObjectSnapshot } from "../../annotations/clue-object"; -import { PartialSharedModelEntry } from "../../document/document-content-types"; import { UpdatedSharedDataSetIds } from "../../shared/shared-data-set"; +import { SharedModelEntrySnapshotType } from "../../document/shared-model-entry"; const cellIdRegEx = /^cell:{(.+)}:{(.+)}$/; @@ -21,7 +21,7 @@ export function decipherCellId(cellId: string) { export function updateTableContentWithNewSharedModelIds( content: TableContentSnapshotType, - sharedDataSetEntries: PartialSharedModelEntry[], + sharedDataSetEntries: SharedModelEntrySnapshotType[], updatedSharedModelMap: Record ) { // Column widths uses attribute ids, so we have to update them when updating shared dataset ids @@ -44,7 +44,7 @@ export function updateTableContentWithNewSharedModelIds( export function updateTableObjectWithNewSharedModelIds( object: IClueObjectSnapshot, - sharedDataSetEntries: PartialSharedModelEntry[], + sharedDataSetEntries: SharedModelEntrySnapshotType[], updatedSharedModelMap: Record ) { if (object.objectType === "cell") { diff --git a/src/models/tiles/tile-content-info.ts b/src/models/tiles/tile-content-info.ts index 08f4b786cf..35f872286a 100644 --- a/src/models/tiles/tile-content-info.ts +++ b/src/models/tiles/tile-content-info.ts @@ -2,8 +2,8 @@ import { ITileMetadataModel, TileMetadataModel } from "./tile-metadata"; import { TileContentModel, ITileContentModel } from "./tile-content"; import { IClueObjectSnapshot } from "../annotations/clue-object"; import { AppConfigModelType } from "../stores/app-config-model"; -import { PartialSharedModelEntry } from "../document/document-content-types"; import { UpdatedSharedDataSetIds } from "../shared/shared-data-set"; +import { SharedModelEntrySnapshotType } from "../document/shared-model-entry"; export interface IDefaultContentOptions { // title is only currently used by the Geometry and Table tiles @@ -22,13 +22,13 @@ type TileContentSnapshotPostProcessor = type TileContentNewSharedModelIdUpdater = ( content: any, - sharedModelEntries: PartialSharedModelEntry[], + sharedModelEntries: SharedModelEntrySnapshotType[], updatedSharedModelMap: Record ) => any; type ClueObjectNewSharedModelIdUpdater = ( clueObject: IClueObjectSnapshot, - sharedModelEntries: PartialSharedModelEntry[], + sharedModelEntries: SharedModelEntrySnapshotType[], updatedSharedModelMap: Record ) => any; diff --git a/src/plugins/data-card/data-card-content.ts b/src/plugins/data-card/data-card-content.ts index 0958091c4b..58af9ad881 100644 --- a/src/plugins/data-card/data-card-content.ts +++ b/src/plugins/data-card/data-card-content.ts @@ -17,8 +17,8 @@ import { import { updateSharedDataSetColors } from "../../models/shared/shared-data-set-colors"; import { SharedModelType } from "../../models/shared/shared-model"; import { uniqueId, uniqueTitle } from "../../utilities/js-utils"; -import { PartialSharedModelEntry } from "../../models/document/document-content-types"; import { getTileModel } from "../../models/tiles/tile-model"; +import { SharedModelEntrySnapshotType } from "../../models/document/shared-model-entry"; export function defaultDataSet(name?: string) { const dataSet = DataSet.create({name}); @@ -278,7 +278,7 @@ export type DataCardContentSnapshotType = SnapshotIn ) { const updatedContent = cloneDeep(content); diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index e6d8900503..b8bb1ca19f 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -4,7 +4,6 @@ import { isInteger} from "lodash"; import { SnapshotOut } from "mobx-state-tree"; import { IClueObjectSnapshot } from "../../../models/annotations/clue-object"; -import { PartialSharedModelEntry } from "../../../models/document/document-content-types"; import { UpdatedSharedDataSetIds, replaceJsonStringsWithUpdatedIds } from "../../../models/shared/shared-data-set"; import { CaseData, DotSelection, DotsElt, selectGraphDots, selectInnerCircles, selectOuterCircles @@ -24,6 +23,7 @@ import {IDataConfigurationModel} from "../models/data-configuration-model"; import {measureText} from "../../../components/tiles/hooks/use-measure-text"; import { GraphModel, IGraphModel } from "../models/graph-model"; import { isFiniteNumber } from "../../../utilities/math-utils"; +import { SharedModelEntrySnapshotType } from "../../../models/document/shared-model-entry"; /** * Utility routines having to do with graph entities @@ -570,7 +570,7 @@ export function decipherDotId(dotId: string) { export function updateGraphContentWithNewSharedModelIds( content: SnapshotOut, - sharedDataSetEntries: PartialSharedModelEntry[], + sharedDataSetEntries: SharedModelEntrySnapshotType[], updatedSharedModelMap: Record ) { return replaceJsonStringsWithUpdatedIds(content, ...Object.values(updatedSharedModelMap)); @@ -578,7 +578,7 @@ export function updateGraphContentWithNewSharedModelIds( export function updateGraphObjectWithNewSharedModelIds( object: IClueObjectSnapshot, - sharedDataSetEntries: PartialSharedModelEntry[], + sharedDataSetEntries: SharedModelEntrySnapshotType[], updatedSharedModelMap: Record ) { if (object.objectType === "dot") { From 8ac14145e5af721c567e469030a5a913a92292c4 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Fri, 8 Mar 2024 16:27:10 -0500 Subject: [PATCH 19/29] Remove verbose logging --- src/plugins/graph/utilities/graph-utils.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index d85355871e..44954a4e70 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -198,7 +198,6 @@ export function matchCirclesToData(props: IMatchCirclesProps) { const { dataConfiguration, enableAnimation, instanceId, dotsElement } = props; const allCaseData = dataConfiguration.joinedCaseDataArrays; const caseDataKeyFunc = (d: CaseData) => `${d.dataConfigID}_${instanceId}_${d.plotNum}_${d.caseID}`; - console.warn("matchCircles", allCaseData); // Create the circles const allCircles = selectGraphDots(dotsElement); if (!allCircles) return; From 7d242490d883393a08c6d1a364a71e141186c5ba Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 11 Mar 2024 13:00:44 -0400 Subject: [PATCH 20/29] Fix issue that broke dragging of tiles --- src/plugins/graph/components/scatterdots.tsx | 10 +++++----- src/plugins/graph/hooks/use-plot.ts | 6 ++++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/plugins/graph/components/scatterdots.tsx b/src/plugins/graph/components/scatterdots.tsx index 7af8619fb7..781d2bb0ed 100644 --- a/src/plugins/graph/components/scatterdots.tsx +++ b/src/plugins/graph/components/scatterdots.tsx @@ -71,6 +71,8 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { updatePositions = useCallback((event: MouseEvent, forceUpdate: boolean) => { if (dragID !== '') { + event.stopPropagation(); + event.preventDefault(); const xAxisScale = layout.getAxisScale('bottom') as ScaleLinear, xAttrID = dataConfiguration?.attributeID('x') ?? ''; const newPos = {x: event.clientX, y: event.clientY}, @@ -100,16 +102,14 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { }, [layout, dataConfiguration, dataset, dragID]), onDrag = useCallback((event: MouseEvent) => { - event.stopPropagation(); - event.preventDefault(); updatePositions(event, false); }, [updatePositions]), onDragEnd = useCallback((event: MouseEvent) => { - graphModel.setInteractionInProgress(false); - // Final update does a rescale if appropriate - updatePositions(event, true); if (dragID !== '') { + graphModel.setInteractionInProgress(false); + // Final update does a rescale if appropriate + updatePositions(event, true); // TODO the radius transition doesn't actually do anything target.current .classed('dragging', false) diff --git a/src/plugins/graph/hooks/use-plot.ts b/src/plugins/graph/hooks/use-plot.ts index dce3b9eb14..b02dc5a745 100644 --- a/src/plugins/graph/hooks/use-plot.ts +++ b/src/plugins/graph/hooks/use-plot.ts @@ -12,6 +12,7 @@ import {useInstanceIdContext} from "../imports/hooks/use-instance-id-context"; import {onAnyAction} from "../../../utilities/mst-utils"; import { IGraphLayerModel } from "../models/graph-layer-model"; import { mstReaction } from "../../../utilities/mst-reaction"; +import { useReadOnlyContext } from "../../../components/document/read-only-context"; interface IDragHandlers { start: (event: MouseEvent) => void @@ -20,8 +21,9 @@ interface IDragHandlers { } export const useDragHandlers = (target: any, {start, drag, end}: IDragHandlers) => { + const readOnly = useReadOnlyContext(); useEffect(() => { - if (target) { + if (target && !readOnly) { target.addEventListener('mousedown', start); target.addEventListener('mousemove', drag); target.addEventListener('mouseup', end); @@ -32,7 +34,7 @@ export const useDragHandlers = (target: any, {start, drag, end}: IDragHandlers) target.removeEventListener('mouseup', end); }; } - }, [target, start, drag, end]); + }, [target, start, drag, end, readOnly]); }; export interface IPlotResponderProps { From f895f0a437ee89568c07c25604675c94dbbbdc1f Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 11 Mar 2024 13:29:58 -0400 Subject: [PATCH 21/29] Try to fix mysterious cypress failure. Based on this: https://docs.cypress.io/guides/core-concepts/retry-ability#Actions-should-be-at-the-end-of-chains-not-the-middle --- cypress/support/elements/tile/TableToolTile.js | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/cypress/support/elements/tile/TableToolTile.js b/cypress/support/elements/tile/TableToolTile.js index e293336f58..f415391897 100644 --- a/cypress/support/elements/tile/TableToolTile.js +++ b/cypress/support/elements/tile/TableToolTile.js @@ -64,14 +64,12 @@ class TableToolTile{ return cy.get('.rdg-row .rdg-cell .rdg-text-editor'); } typeInTableCellXY(row, col, text) { - this.getTableCellXY(row, col).dblclick().then(() => { - this.getTableCellEdit().type(`${text}{enter}`); - }); + this.getTableCellXY(row, col).dblclick(); + this.getTableCellEdit().type(`${text}{enter}`); } typeInTableCell(i, text) { - this.getTableCell().eq(i).dblclick().then(() => { - this.getTableCellEdit().type(`${text}{enter}`); - }); + this.getTableCell().eq(i).dblclick(); + this.getTableCellEdit().type(`${text}{enter}`); } getTableCellWithColIndex(colIndex, colValue){ return cy.get('.rdg-row').contains('.rdg-cell[aria-colindex="' + colIndex + '"]', colValue); From 67d2e571bc3f72c8a79604c2cc541f266fe1e679 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 11 Mar 2024 14:29:07 -0400 Subject: [PATCH 22/29] Cleanups and comments. --- .../e2e/functional/tile_tests/xy_plot_tool_spec.js | 1 + .../utilities/editable-label-with-button.tsx | 6 ++++++ src/plugins/graph/components/background.tsx | 11 ++++++++++- src/plugins/graph/utilities/graph-utils.ts | 10 ++++++---- src/public/demo/units/qa/content.json | 1 - 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js index b118103fbf..53a45de4cc 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -532,6 +532,7 @@ context('XYPlot Tool Tile', function () { // Drag a point to reposition. Should start out where we initially clicked xAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 150, 10); yAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 50, 10); + // {force: true} seems to be necessary, not sure why xyTile.getGraphDot().eq(0).children('circle').eq(1) .trigger("mousedown", 150, 50, { force: true }) .trigger("drag", 175, 75, { force: true }) diff --git a/src/components/utilities/editable-label-with-button.tsx b/src/components/utilities/editable-label-with-button.tsx index 5b107da8b8..90365ee861 100644 --- a/src/components/utilities/editable-label-with-button.tsx +++ b/src/components/utilities/editable-label-with-button.tsx @@ -11,6 +11,12 @@ interface IProps { onSubmit: (value:string) => void; } +/** + * Generic component for an in-place editable label. + * + * Differs from stock in that there's an explicit edit button rather + * than invoking edit mode by clicking on the label text itself. + */ export const EditableLabelWithButton = observer(function EditableDataSetName({defaultValue, onSubmit}: IProps) { function EditButton() { diff --git a/src/plugins/graph/components/background.tsx b/src/plugins/graph/components/background.tsx index a8e16f69cd..0e62262539 100644 --- a/src/plugins/graph/components/background.tsx +++ b/src/plugins/graph/components/background.tsx @@ -18,6 +18,15 @@ interface IProps { marqueeState: MarqueeState } +/** + * Gets the transformation matrix that transforms from the user coordinate + * system on the source element to the user coordinate system on the + * target element. + * Replaces deprecated SVG method of the same name. + * @param source + * @param elem + * @returns + */ function getTransformToElement(source: SVGGraphicsElement, elem: SVGGraphicsElement) { const elemCTM = elem.getScreenCTM(); const sourceCTM = source.getScreenCTM(); @@ -29,7 +38,7 @@ function getTransformToElement(source: SVGGraphicsElement, elem: SVGGraphicsElem } // Get bounding box INCLUDING any transform="..." on the element itself. -// From https://stackoverflow.com/a/64909822 +// Adapted from https://stackoverflow.com/a/64909822 function boundingBoxRelativeToElement(fromSpace: SVGGraphicsElement, toSpace: SVGGraphicsElement) { const bbox = fromSpace.getBBox(); const m = getTransformToElement(fromSpace, toSpace); diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index 3cdcc0815a..67818de3b2 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -138,7 +138,8 @@ export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConf if (graphModel.editingMode==="add" && graphModel.editingLayer && graphModel.editingLayer.config !== dataConfiguration) { - // We add a case to the editable dataset at the same values as the existing case clicked on. + // We're in "Add points" mode, and clicked on a case that is not in the editable dataset: + // add a case to the editable dataset at the same values as the existing case clicked on. const existingCase = dataset?.getCanonicalCase(caseData.caseID); if (existingCase) { const xAttributeId = dataConfiguration.xAttributeID; @@ -149,15 +150,16 @@ export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConf } } } else { + // Otherwise, clicking on a dot means updating the selection. const extendSelection = event.shiftKey, cellIsSelected = dataset?.isCellSelected(yCell); if (!cellIsSelected) { - if (extendSelection) { // y cell is not selected and Shift key is down => add y cell to selection + if (extendSelection) { // Dot is not selected and Shift key is down => add to selection dataset?.selectCells([yCell]); - } else { // y cell is not selected and Shift key is up => only this y cell should be selected + } else { // Dot is not selected and Shift key is up => only this dot should be selected dataset?.setSelectedCells([yCell]); } - } else if (extendSelection) { // y cell is selected and Shift key is down => deselect cell + } else if (extendSelection) { // Dot is selected and Shift key is down => deselect dataset?.selectCells([yCell], false); } } diff --git a/src/public/demo/units/qa/content.json b/src/public/demo/units/qa/content.json index 975f93ac20..aab3c3d309 100644 --- a/src/public/demo/units/qa/content.json +++ b/src/public/demo/units/qa/content.json @@ -57,7 +57,6 @@ "connectPointsByDefault": true, "tools": [ "link-tile-multiple", - "add-points-by-hand", "fit-all", "toggle-lock" ] From 956617585859f5112e650c10688eb157a31604d0 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 11 Mar 2024 14:32:52 -0400 Subject: [PATCH 23/29] Try delay and scroll to fix mysterious cypress failure. --- cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js | 3 ++- cypress/support/elements/tile/TableToolTile.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js index 53a45de4cc..d96262ce8d 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -97,6 +97,7 @@ context('XYPlot Tool Tile', function () { cy.log("Link Table"); clueCanvas.clickToolbarButton('graph', 'link-tile-multiple'); xyTile.linkTable("Table Data 1"); + cy.wait(2000); cy.log("shows edit boxes on axes"); xyTile.getEditableAxisBox("bottom", "min").should("exist"); @@ -109,7 +110,7 @@ context('XYPlot Tool Tile', function () { cy.log("Add Second Row Table Cell"); cy.get(".primary-workspace").within((workspace) => { - tableToolTile.typeInTableCell(5, '7'); + tableToolTile.typeInTableCell(5, '7'); // Failing tableToolTile.getTableCell().eq(5).should('contain', '7'); tableToolTile.typeInTableCell(6, '6'); tableToolTile.getTableCell().eq(6).should('contain', '6'); diff --git a/cypress/support/elements/tile/TableToolTile.js b/cypress/support/elements/tile/TableToolTile.js index f415391897..edb80e2d74 100644 --- a/cypress/support/elements/tile/TableToolTile.js +++ b/cypress/support/elements/tile/TableToolTile.js @@ -68,7 +68,7 @@ class TableToolTile{ this.getTableCellEdit().type(`${text}{enter}`); } typeInTableCell(i, text) { - this.getTableCell().eq(i).dblclick(); + this.getTableCell().eq(i).scrollIntoView().dblclick(); this.getTableCellEdit().type(`${text}{enter}`); } getTableCellWithColIndex(colIndex, colValue){ From e1539a6c96ad91ac6b375f9e22054568b66d4243 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 11 Mar 2024 15:10:47 -0400 Subject: [PATCH 24/29] GraphEditingContext no longer needed. The relevant information and methods are now all in the model layer. --- .../tile_tests/xy_plot_tool_spec.js | 5 +- .../support/elements/tile/TableToolTile.js | 2 +- src/plugins/graph/components/background.tsx | 19 +++---- .../components/graph-toolbar-registration.tsx | 19 +++---- .../components/graph-wrapper-component.tsx | 55 ++++--------------- src/plugins/graph/components/scatterdots.tsx | 7 +-- .../graph/hooks/use-graph-editing-context.ts | 26 --------- src/plugins/graph/models/graph-layer-model.ts | 8 ++- src/plugins/graph/models/graph-model.ts | 19 ++++++- src/plugins/graph/utilities/graph-utils.ts | 2 +- 10 files changed, 56 insertions(+), 106 deletions(-) delete mode 100644 src/plugins/graph/hooks/use-graph-editing-context.ts diff --git a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js index d96262ce8d..514300e9f9 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -97,7 +97,8 @@ context('XYPlot Tool Tile', function () { cy.log("Link Table"); clueCanvas.clickToolbarButton('graph', 'link-tile-multiple'); xyTile.linkTable("Table Data 1"); - cy.wait(2000); + cy.wait(1000); // Needs a little extra time, probably due to legend resizing. + // Otherwise the upcoming typeInTableCell fails. cy.log("shows edit boxes on axes"); xyTile.getEditableAxisBox("bottom", "min").should("exist"); @@ -110,7 +111,7 @@ context('XYPlot Tool Tile', function () { cy.log("Add Second Row Table Cell"); cy.get(".primary-workspace").within((workspace) => { - tableToolTile.typeInTableCell(5, '7'); // Failing + tableToolTile.typeInTableCell(5, '7'); tableToolTile.getTableCell().eq(5).should('contain', '7'); tableToolTile.typeInTableCell(6, '6'); tableToolTile.getTableCell().eq(6).should('contain', '6'); diff --git a/cypress/support/elements/tile/TableToolTile.js b/cypress/support/elements/tile/TableToolTile.js index edb80e2d74..f415391897 100644 --- a/cypress/support/elements/tile/TableToolTile.js +++ b/cypress/support/elements/tile/TableToolTile.js @@ -68,7 +68,7 @@ class TableToolTile{ this.getTableCellEdit().type(`${text}{enter}`); } typeInTableCell(i, text) { - this.getTableCell().eq(i).scrollIntoView().dblclick(); + this.getTableCell().eq(i).dblclick(); this.getTableCellEdit().type(`${text}{enter}`); } getTableCellWithColIndex(colIndex, colValue){ diff --git a/src/plugins/graph/components/background.tsx b/src/plugins/graph/components/background.tsx index 0e62262539..f1f4a9e426 100644 --- a/src/plugins/graph/components/background.tsx +++ b/src/plugins/graph/components/background.tsx @@ -11,7 +11,6 @@ import {MarqueeState} from "../models/marquee-state"; import {IGraphModel} from "../models/graph-model"; import {useInstanceIdContext} from "../imports/hooks/use-instance-id-context"; import { useGraphModelContext } from "../hooks/use-graph-model-context"; -import { useGraphEditingContext } from "../hooks/use-graph-editing-context"; import { ICell } from "../../../models/data/data-types"; interface IProps { @@ -124,7 +123,6 @@ export const Background = forwardRef((props, ref) => { instanceId = useInstanceIdContext() || 'background', layout = useGraphLayoutContext(), graphModel = useGraphModelContext(), - graphEditMode = useGraphEditingContext(), bgRef = ref as MutableRefObject, startX = useRef(0), startY = useRef(0), @@ -144,16 +142,16 @@ export const Background = forwardRef((props, ref) => { }, [layout]); const onClick = useCallback((event: { offsetX: number, offsetY: number, shiftKey: boolean }) => { - if (graphEditMode.addPointsMode) { + if (graphModel.editingMode==="add") { const {x, y} = pointCoordinates(event.offsetX, event.offsetY); - graphEditMode.addPoint(x, y); + graphModel.editingLayer?.addPoint(x, y); } else { // If not in add mode or shifted, clicking on background deselects everything if (!event.shiftKey) { graphModel.clearAllSelectedCases(); } } - }, [graphEditMode, graphModel, pointCoordinates]); + }, [graphModel, pointCoordinates]); const selectModeDragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { const {computedBounds} = layout, @@ -206,13 +204,13 @@ export const Background = forwardRef((props, ref) => { createModeDragEnd = useCallback((event: { x: number; y: number; }) => { const point = pointCoordinates(event.x, event.y); - graphEditMode.addPoint(point.x, point.y); + graphModel.editingLayer?.addPoint(point.x, point.y); setPotentialPoint(undefined); - }, [graphEditMode, pointCoordinates]); + }, [graphModel.editingLayer, pointCoordinates]); const dragBehavior = useMemo(() => { - if (graphEditMode.addPointsMode) { + if (graphModel.editingMode==="add") { return drag() .on("start", createModeDragStart) .on("drag", createModeDrag) @@ -223,7 +221,8 @@ export const Background = forwardRef((props, ref) => { .on("drag", selectModeDrag) .on("end", selectModeDragEnd); } - }, [createModeDrag, createModeDragEnd, createModeDragStart, graphEditMode.addPointsMode, + }, [createModeDrag, createModeDragEnd, createModeDragStart, + graphModel.editingMode, selectModeDrag, selectModeDragEnd, selectModeDragStart]); useEffect(() => { @@ -262,7 +261,7 @@ export const Background = forwardRef((props, ref) => { {potentialPoint && } + r={graphModel.getPointRadius('hover-drag')} fill={graphModel.getEditablePointsColor()}/> } ); }); diff --git a/src/plugins/graph/components/graph-toolbar-registration.tsx b/src/plugins/graph/components/graph-toolbar-registration.tsx index 99ff8514c6..e88aadfa55 100644 --- a/src/plugins/graph/components/graph-toolbar-registration.tsx +++ b/src/plugins/graph/components/graph-toolbar-registration.tsx @@ -7,7 +7,6 @@ import { IToolbarButtonComponentProps, registerTileToolbarButtons } from "../../../components/toolbar/toolbar-button-manager"; import { GraphControllerContext } from "../models/graph-controller"; import { useGraphModelContext } from "../hooks/use-graph-model-context"; -import { useGraphEditingContext } from "../hooks/use-graph-editing-context"; import LinkTableIcon from "../../../clue/assets/icons/geometry/link-table-icon.svg"; import AddIcon from "../../../assets/icons/add-data-graph-icon.svg"; @@ -94,7 +93,6 @@ const ToggleLockAxesButton = observer(function ToggleLockAxesButton({name}: IToo const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IToolbarButtonComponentProps) { const graph = useGraphModelContext(); - const graphEditMode = useGraphEditingContext(); const hasEditableLayers = graph.getEditableLayers().length > 0; // Enable button if axes are numeric or undefined. @@ -105,7 +103,7 @@ const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IT function handleClick() { graph.createEditableLayer(); - graphEditMode.setEditMode("add"); + graph.setEditingMode("add"); } return ( @@ -123,13 +121,13 @@ const AddPointsByHandButton = observer(function AddPointsByHandButton({name}: IT const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProps) { const graph = useGraphModelContext(); - const graphEditModeContext = useGraphEditingContext(); + const currentMode = graph.editingMode; const editableLayers = graph.getEditableLayers(); const hasEditableLayers = editableLayers.length > 0; function handleClick() { - graphEditModeContext.setEditMode(graphEditModeContext.editPointsMode ? "none" : "edit"); + graph.setEditingMode(currentMode==="edit" ? "none" : "edit"); } return ( @@ -137,7 +135,7 @@ const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProp name={name} title="Select/Move point" onClick={handleClick} - selected={graphEditModeContext.editPointsMode} + selected={currentMode === "edit"} disabled={!hasEditableLayers} > @@ -147,16 +145,15 @@ const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProp const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) { const graph = useGraphModelContext(); - const graphEditModeContext = useGraphEditingContext(); + const currentMode = graph.editingMode; const editableLayers = graph.getEditableLayers(); const hasEditableLayers = editableLayers.length > 0; - const iconStyle = { fill: graphEditModeContext.getEditablePointsColor() }; + const iconStyle = { fill: graph.getEditablePointsColor() }; function handleClick() { - // Toggle the mode - graphEditModeContext.setEditMode(graphEditModeContext.addPointsMode ? "none" : "add"); + graph.setEditingMode(currentMode==="add" ? "none" : "add"); } return ( @@ -164,7 +161,7 @@ const AddPointsButton = observer(function({name}: IToolbarButtonComponentProps) name={name} title="Add point" onClick={handleClick} - selected={graphEditModeContext.addPointsMode} + selected={currentMode==="add"} disabled={!hasEditableLayers} > diff --git a/src/plugins/graph/components/graph-wrapper-component.tsx b/src/plugins/graph/components/graph-wrapper-component.tsx index c2c147e66c..5c0b80842c 100644 --- a/src/plugins/graph/components/graph-wrapper-component.tsx +++ b/src/plugins/graph/components/graph-wrapper-component.tsx @@ -18,8 +18,7 @@ import { IGraphModel } from "../models/graph-model"; import { decipherDotId } from "../utilities/graph-utils"; import { GraphComponent } from "./graph-component"; import { isNumericAxisModel } from "../imports/components/axis/models/axis-model"; -import { GraphEditMode, Point } from "../graph-types"; -import { GraphEditingContext, IGraphEditModeContext } from "../hooks/use-graph-editing-context"; +import { Point } from "../graph-types"; import "./graph-toolbar-registration"; @@ -37,36 +36,6 @@ export const GraphWrapperComponent: React.FC = observer(function(pro const xAttrType = content.config.attributeType("x"); const yAttrType = content.config.attributeType("y"); - function addPoint(x: number, y: number) { - const layer = content.editingLayer; - if (layer) { - layer.addPoint(0, x, y); // Point is added in layer 0 -- we only support editing one trace so far. - } - } - function setEditMode(string: GraphEditMode) { - content.setEditingMode(string); - } - - function getEditablePointsColor() { - let color = "#000000"; - const editableLayers = content.getEditableLayers(); - if (editableLayers.length > 0) { - const yAttributes = editableLayers[0].config.yAttributeIDs; - if (yAttributes.length > 0) { - color = content.getColorForId(yAttributes[0]); - } - } - return color; - } - - const graphEditMode: IGraphEditModeContext = { - addPointsMode: content.editingMode==="add", - editPointsMode: content.editingMode==="edit", - setEditMode, - addPoint, - getEditablePointsColor - }; - // This is used for locating Sparrow endpoints. const getDotCenter = useCallback((dotId: string) => { // FIXME Currently, getScreenX and getScreenY only handle numeric axes, so just bail if they are a different type. @@ -184,18 +153,16 @@ export const GraphWrapperComponent: React.FC = observer(function(pro return ( - -
- - -
- +
+ + +
); }); diff --git a/src/plugins/graph/components/scatterdots.tsx b/src/plugins/graph/components/scatterdots.tsx index 781d2bb0ed..2c7cd4ca5f 100644 --- a/src/plugins/graph/components/scatterdots.tsx +++ b/src/plugins/graph/components/scatterdots.tsx @@ -15,14 +15,12 @@ import { setPointSelection } from "../utilities/graph-utils"; import { useGraphLayerContext } from "../hooks/use-graph-layer-context"; -import { useGraphEditingContext } from "../hooks/use-graph-editing-context"; export const ScatterDots = function ScatterDots(props: PlotProps) { const {dotsRef, enableAnimation} = props, graphModel = useGraphModelContext(), layer = useGraphLayerContext(), dataConfiguration = useDataConfigurationContext(), - graphEditingContext = useGraphEditingContext(), dataset = dataConfiguration?.dataset, secondaryAttrIDsRef = useRef([]), pointRadiusRef = useRef(0), @@ -43,7 +41,7 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { yScaleRef.current = layout.getAxisScale("left") as ScaleNumericBaseType; const onDragStart = useCallback((event: MouseEvent) => { - if (graphEditingContext.editPointsMode || graphEditingContext.addPointsMode) { + if (graphModel.editingMode !== "none") { const targetDot = event.target && inGraphDot(event.target as SVGSVGElement); if (!targetDot) return; target.current = select(targetDot as SVGSVGElement); @@ -66,8 +64,7 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { handleClickOnDot(event, aCaseData, dataConfiguration); } - }, [dataConfiguration, dataset, enableAnimation, graphEditingContext.addPointsMode, - graphEditingContext.editPointsMode, graphModel]), + }, [dataConfiguration, dataset, enableAnimation, graphModel]), updatePositions = useCallback((event: MouseEvent, forceUpdate: boolean) => { if (dragID !== '') { diff --git a/src/plugins/graph/hooks/use-graph-editing-context.ts b/src/plugins/graph/hooks/use-graph-editing-context.ts deleted file mode 100644 index 64edf62f1a..0000000000 --- a/src/plugins/graph/hooks/use-graph-editing-context.ts +++ /dev/null @@ -1,26 +0,0 @@ -import { createContext, useContext } from "react"; -import { GraphEditMode } from "../graph-types"; - -/** - * This context holds state relevant to editing points on the Graph tile. - */ - -export interface IGraphEditModeContext { - editPointsMode: boolean; - addPointsMode: boolean; - setEditMode: (mode: GraphEditMode) => void; - addPoint: (x: number, y: number) => void; - getEditablePointsColor: () => string; -} - -const kDefaultGraphEditModeContext: IGraphEditModeContext = { - editPointsMode: false, - addPointsMode: false, - setEditMode: mode => { }, - addPoint: (x, y) => { }, - getEditablePointsColor: () => "#000000" -}; - -export const GraphEditingContext = createContext(kDefaultGraphEditModeContext); - -export const useGraphEditingContext = () => useContext(GraphEditingContext); diff --git a/src/plugins/graph/models/graph-layer-model.ts b/src/plugins/graph/models/graph-layer-model.ts index a4e4bdcfc2..68f4eaa08a 100644 --- a/src/plugins/graph/models/graph-layer-model.ts +++ b/src/plugins/graph/models/graph-layer-model.ts @@ -65,12 +65,14 @@ export const GraphLayerModel = types self.autoAssignedAttributes = []; }, /** - * Add a point in the given plotNum with the given x and y values. - * @param plotNum + * Add a point to this layer with the given x and y values. + * A plot number can be provided; it defaults to 0 since currently + * only a single trace of manually created points can be created in the graph. * @param x * @param y + * @param plotNum optional, default 0 */ - addPoint(plotNum: number, x: number, y: number) { + addPoint(x: number, y: number, plotNum: number=0) { const dataset = self.config.dataset; const xAttr = self.config.attributeID("x"); const yAttr = self.config.yAttributeIDs[plotNum]; diff --git a/src/plugins/graph/models/graph-model.ts b/src/plugins/graph/models/graph-model.ts index e1b3048677..c9db305713 100644 --- a/src/plugins/graph/models/graph-model.ts +++ b/src/plugins/graph/models/graph-model.ts @@ -207,7 +207,8 @@ export const GraphModel = TileContentModel attributeType(role: GraphAttrRole) { return self.layers[0].config.attributeType(role); }, - getLayerById(layerId: string) { + getLayerById(layerId: string): IGraphLayerModel|undefined { + if (!layerId) return undefined; return self.layers.find(layer => layer.id === layerId); }, layerForDataConfigurationId(dataConfID: string) { @@ -238,8 +239,9 @@ export const GraphModel = TileContentModel /** * Return the layer currently being edited, or undefined if none. */ - get editingLayer() { - return self.editingLayerId && this.getLayerById(self.editingLayerId); + get editingLayer(): IGraphLayerModel|undefined { + if (!self.editingLayerId) return undefined; + return this.getLayerById(self.editingLayerId); }, /** * Find all tooltip-related attributes from all layers. @@ -542,6 +544,17 @@ export const GraphModel = TileContentModel const colorIndex = self._idColors.get(id); if (colorIndex === undefined) return "black"; return clueGraphColors[colorIndex % clueGraphColors.length].name; + }, + getEditablePointsColor() { + let color = "#000000"; + const layer = self.editingLayer; + if (layer) { + const yAttributes = layer.config.yAttributeIDs; + if (yAttributes.length > 0) { + color = this.getColorForId(yAttributes[0]); + } + } + return color; } })) .actions(self => ({ diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index 67818de3b2..8cd5885241 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -146,7 +146,7 @@ export function handleClickOnDot(event: MouseEvent, caseData: CaseData, dataConf const x = dataset?.getNumeric(caseData.caseID, xAttributeId); const y = dataset?.getNumeric(caseData.caseID, yAttributeId); if (x !== undefined && y !== undefined) { - graphModel.editingLayer.addPoint(0, x, y); + graphModel.editingLayer.addPoint(x, y); } } } else { From b065a385ff6c1d61dabecd102a674c159953009e Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Mon, 11 Mar 2024 16:14:58 -0400 Subject: [PATCH 25/29] Refactor how drags are organized. Cypress test was failing due to drags not getting updated. --- .../tile_tests/xy_plot_tool_spec.js | 4 +- src/plugins/graph/components/background.tsx | 61 ++++++++++++------- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js index 514300e9f9..dc391a5b1e 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -501,10 +501,10 @@ context('XYPlot Tool Tile', function () { // Create manual layer clueCanvas.clickToolbarButton('graph', 'add-points-by-hand'); - clueCanvas.toolbarButtonIsDisabled('graph', 'add-points-by-hand'); // only one manual set + clueCanvas.toolbarButtonIsDisabled('graph', 'add-points-by-hand'); // only one manual set allowed clueCanvas.toolbarButtonIsEnabled('graph', 'move-points'); clueCanvas.toolbarButtonIsEnabled('graph', 'add-points'); - clueCanvas.toolbarButtonIsSelected('graph', 'add-points'); // automatically turns on add mode + clueCanvas.toolbarButtonIsSelected('graph', 'add-points'); // automatically turns on "add" mode xyTile.getXAttributesLabel().should('have.length', 1).should('contain.text', 'X Variable'); xyTile.getYAttributesLabel().should('have.length', 1).should('contain.text', 'Y Variable 1'); xyTile.getLayerName().should('have.length', 1).should('contain.text', 'Added by hand'); diff --git a/src/plugins/graph/components/background.tsx b/src/plugins/graph/components/background.tsx index f1f4a9e426..e7e0259d8f 100644 --- a/src/plugins/graph/components/background.tsx +++ b/src/plugins/graph/components/background.tsx @@ -153,7 +153,9 @@ export const Background = forwardRef((props, ref) => { } }, [graphModel, pointCoordinates]); - const selectModeDragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { + // Define the dragging behaviors for "edit" mode and for "add" mode, then assemble into one "drag" object. + const + dragStartEditMode = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { const {computedBounds} = layout, plotBounds = computedBounds.plot; selectionTree.current = prepareTree(`.${instanceId}`, graphDotSelector); @@ -167,7 +169,7 @@ export const Background = forwardRef((props, ref) => { marqueeState.setMarqueeRect({x: startX.current, y: startY.current, width: 0, height: 0}); }, [graphModel, instanceId, layout, marqueeState]), - selectModeDrag = useCallback((event: { dx: number; dy: number }) => { + dragMoveEditMode = useCallback((event: { dx: number; dy: number }) => { if (event.dx !== 0 || event.dy !== 0) { previousMarqueeRect.current = rectNormalize( {x: startX.current, y: startY.current, w: width.current, h: height.current}); @@ -188,42 +190,55 @@ export const Background = forwardRef((props, ref) => { } }, [graphModel, marqueeState]), - selectModeDragEnd = useCallback(() => { + dragEndEditMode = useCallback((event: { x: number; y: number; }) => { marqueeState.setMarqueeRect({x: 0, y: 0, width: 0, height: 0}); selectionTree.current = null; - }, [marqueeState]); + }, [marqueeState]), - const - createModeDragStart = useCallback((event: { x: number; y: number; }) => { + dragStartAddMode = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { setPotentialPoint(event); }, []), - createModeDrag = useCallback((event: { x: number; y: number; }) => { + dragMoveAddMode = useCallback((event: { x: number; y: number; dx: number; dy: number }) => { setPotentialPoint(event); }, []), - createModeDragEnd = useCallback((event: { x: number; y: number; }) => { + dragEndAddMode = useCallback((event: { x: number; y: number; }) => { const point = pointCoordinates(event.x, event.y); graphModel.editingLayer?.addPoint(point.x, point.y); setPotentialPoint(undefined); - }, [graphModel.editingLayer, pointCoordinates]); + }, [graphModel.editingLayer, pointCoordinates]), + + dragStart = useCallback((event: { x: number; y: number; sourceEvent: { shiftKey: boolean } }) => { + if (graphModel.editingMode === "add") { + dragStartAddMode(event); + } else { + dragStartEditMode(event); + } + }, [dragStartAddMode, graphModel.editingMode, dragStartEditMode]), + + dragMove = useCallback((event: { x: number; y: number; dx: number; dy: number }) => { + if (graphModel.editingMode === "add") { + dragMoveAddMode(event); + } else { + dragMoveEditMode(event); + } + }, [dragMoveAddMode, graphModel.editingMode, dragMoveEditMode]), + dragEnd = useCallback((event: { x: number; y: number; }) => { + if (graphModel.editingMode === "add") { + dragEndAddMode(event); + } else { + dragEndEditMode(event); + } + }, [dragEndAddMode, graphModel.editingMode, dragEndEditMode]); const dragBehavior = useMemo(() => { - if (graphModel.editingMode==="add") { - return drag() - .on("start", createModeDragStart) - .on("drag", createModeDrag) - .on("end", createModeDragEnd); - } else { - return drag() - .on("start", selectModeDragStart) - .on("drag", selectModeDrag) - .on("end", selectModeDragEnd); - } - }, [createModeDrag, createModeDragEnd, createModeDragStart, - graphModel.editingMode, - selectModeDrag, selectModeDragEnd, selectModeDragStart]); + return drag() + .on("start", dragStart) + .on("drag", dragMove) + .on("end", dragEnd); + }, [dragMove, dragEnd, dragStart]); useEffect(() => { return autorun(() => { From 28f86aa16d3aeb68c9935476269d736716b23d0e Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Tue, 12 Mar 2024 16:31:28 -0400 Subject: [PATCH 26/29] De-nest component definitions --- .../utilities/editable-label-with-button.tsx | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/components/utilities/editable-label-with-button.tsx b/src/components/utilities/editable-label-with-button.tsx index 90365ee861..b2508fe285 100644 --- a/src/components/utilities/editable-label-with-button.tsx +++ b/src/components/utilities/editable-label-with-button.tsx @@ -19,19 +19,6 @@ interface IProps { */ export const EditableLabelWithButton = observer(function EditableDataSetName({defaultValue, onSubmit}: IProps) { - function EditButton() { - const { isEditing, getEditButtonProps } = useEditableControls(); - if (!isEditing) { - return ( - - ); - } else { - return null; - } - } - return ( ); }); + +function EditButton() { + const { isEditing, getEditButtonProps } = useEditableControls(); + if (!isEditing) { + return ( + + ); + } else { + return null; + } +} From 3b317e092e50ae3b2cb95629a245aa3cb3db2c6e Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Tue, 12 Mar 2024 16:45:18 -0400 Subject: [PATCH 27/29] Don't select attribute when adding manual dataset --- .../graph/models/data-configuration-model.ts | 19 +++++++++++-------- src/plugins/graph/models/graph-model.ts | 4 ++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/plugins/graph/models/data-configuration-model.ts b/src/plugins/graph/models/data-configuration-model.ts index 95be8dff56..f017054f3a 100644 --- a/src/plugins/graph/models/data-configuration-model.ts +++ b/src/plugins/graph/models/data-configuration-model.ts @@ -716,7 +716,14 @@ export const DataConfigurationModel = types removeAttributeFromRole(role: GraphAttrRole) { self._setAttributeDescription(role); }, - setAttributeForRole(role: GraphAttrRole, desc?: IAttributeDescriptionSnapshot) { + /** + * Assign the Attribute to the given graph role. + * By default will also select the attribute. + * @param role graph role. + * @param desc attribute description, including the attribute ID and optionally a type. + * @param select boolean default true to select the attribute. + */ + setAttributeForRole(role: GraphAttrRole, desc?: IAttributeDescriptionSnapshot, select: boolean=true) { if (role === 'y') { // Setting "Y" role implies that user only wants one, or no Y attributes. while (self._yAttributeDescriptions.length) { @@ -724,20 +731,16 @@ export const DataConfigurationModel = types } if (desc && desc.attributeID !== '') { self._yAttributeDescriptions.push(desc); - self.dataset?.setSelectedAttributes([desc.attributeID]); } } else if (role === 'yPlus' && desc && desc.attributeID !== '') { self._yAttributeDescriptions.push(desc); } else if (role === 'rightNumeric') { this.setY2Attribute(desc); - if (desc) { - self.dataset?.setSelectedAttributes([desc.attributeID]); - } } else { self._setAttributeDescription(role, desc); - if (desc) { - self.dataset?.setSelectedAttributes([desc.attributeID]); - } + } + if (desc && select) { + self.dataset?.setSelectedAttributes([desc.attributeID]); } this.syncFilteredCasesCount(true); if (role === 'legend') { diff --git a/src/plugins/graph/models/graph-model.ts b/src/plugins/graph/models/graph-model.ts index c9db305713..1fa5aa82e8 100644 --- a/src/plugins/graph/models/graph-model.ts +++ b/src/plugins/graph/models/graph-model.ts @@ -375,8 +375,8 @@ export const GraphModel = TileContentModel const dataConfiguration = DataConfigurationModel.create(); layer.setDataConfiguration(dataConfiguration); dataConfiguration.setDataset(dataset, metadata); - dataConfiguration.setAttributeForRole("x", { attributeID: xAttr.id, type: "numeric" }); - dataConfiguration.setAttributeForRole("y", { attributeID: yAttr.id, type: "numeric" }); + dataConfiguration.setAttributeForRole("x", { attributeID: xAttr.id, type: "numeric" }, false); + dataConfiguration.setAttributeForRole("y", { attributeID: yAttr.id, type: "numeric" }, false); } }, setXAttributeLabel(label: string) { From bd785eebd6e9697146e283a3eaa854c159d56391 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Tue, 12 Mar 2024 17:02:38 -0400 Subject: [PATCH 28/29] Fix color of toolbar dot --- src/plugins/graph/models/graph-model.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/plugins/graph/models/graph-model.ts b/src/plugins/graph/models/graph-model.ts index 1fa5aa82e8..4c0a281af7 100644 --- a/src/plugins/graph/models/graph-model.ts +++ b/src/plugins/graph/models/graph-model.ts @@ -547,7 +547,11 @@ export const GraphModel = TileContentModel }, getEditablePointsColor() { let color = "#000000"; - const layer = self.editingLayer; + let layer = self.editingLayer; + if (!layer) { + // Even if no layer is currently being edited, show the color of the one that would be. + layer = self.getEditableLayers()?.[0]; + } if (layer) { const yAttributes = layer.config.yAttributeIDs; if (yAttributes.length > 0) { From 98280479ff21c5a415316f702dadec90335785f7 Mon Sep 17 00:00:00 2001 From: Boris Goldowsky Date: Wed, 13 Mar 2024 15:02:14 -0400 Subject: [PATCH 29/29] Make edit button active whenever there's a dataset --- src/plugins/graph/components/graph-toolbar-registration.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/plugins/graph/components/graph-toolbar-registration.tsx b/src/plugins/graph/components/graph-toolbar-registration.tsx index e88aadfa55..881a4f2da9 100644 --- a/src/plugins/graph/components/graph-toolbar-registration.tsx +++ b/src/plugins/graph/components/graph-toolbar-registration.tsx @@ -123,8 +123,7 @@ const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProp const graph = useGraphModelContext(); const currentMode = graph.editingMode; - const editableLayers = graph.getEditableLayers(); - const hasEditableLayers = editableLayers.length > 0; + const linked = graph.isLinkedToDataSet; function handleClick() { graph.setEditingMode(currentMode==="edit" ? "none" : "edit"); @@ -136,7 +135,7 @@ const SelectPointsButton = observer(function({name}: IToolbarButtonComponentProp title="Select/Move point" onClick={handleClick} selected={currentMode === "edit"} - disabled={!hasEditableLayers} + disabled={!linked} >