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 d3cdbe632e..edc6f74ff4 100644 --- a/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js +++ b/cypress/e2e/functional/tile_tests/xy_plot_tool_spec.js @@ -117,14 +117,17 @@ context('XYPlot Tool Tile', function () { tableToolTile.getTableCell().eq(6).should('contain', '6'); }); - cy.log("verify graph dot is updated"); + cy.log("verify graph dot is added"); xyTile.getGraphDot().should('have.length', 2); // X axis should have scaled to fit 5 and 7. xyTile.getEditableAxisBox("bottom", "min").invoke('text').then(parseFloat).should("be.within", -1, 5); xyTile.getEditableAxisBox("bottom", "max").invoke('text').then(parseFloat).should("be.within", 7, 12); - cy.log("add another data point"); + cy.log("add another data point with axes locked"); + xyTile.getTile().click(); + clueCanvas.clickToolbarButton("graph", "toggle-lock"); + clueCanvas.toolbarButtonIsSelected("graph", "toggle-lock"); cy.get(".primary-workspace").within((workspace) => { tableToolTile.typeInTableCell(9, '15'); tableToolTile.getTableCell(8).should('contain', '15'); @@ -150,6 +153,10 @@ context('XYPlot Tool Tile', function () { xyTile.getEditableAxisBox("bottom", "min").invoke('text').then(parseFloat).should("be.within", -1, 5); xyTile.getEditableAxisBox("bottom", "max").invoke('text').then(parseFloat).should("be.within", 15, 20); + // Turn Lock axes off + clueCanvas.clickToolbarButton("graph", "toggle-lock"); + clueCanvas.toolbarButtonIsNotSelected("graph", "toggle-lock"); + cy.log("add y2 column to table and show it"); tableToolTile.getTableTile().click(); tableToolTile.getAddColumnButton().click(); @@ -560,14 +567,21 @@ 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 }) - .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); + clueCanvas.clickToolbarButton('graph', 'toggle-lock'); // so that we can test position without rescale happening + + xyTile.getGraphDot().eq(0).then(elt => { + const currentPos = elt[0].getBoundingClientRect(); + cy.window().then(win => { + xyTile.getGraphDot().eq(0).children('circle').eq(1) + .trigger("mousedown", { force: true, view: win }) + .trigger("mousemove", { force: true, view: win, clientX: currentPos.x+25, clientY: currentPos.y+25 }) + .trigger("mouseup", { force: true, view: win, clientX: currentPos.x+25, clientY: currentPos.y+25 }); + }); + cy.wait(500); // animation + xAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 175, 10); + yAttributeOfTransform(xyTile.getGraphDot().eq(0)).should("be.closeTo", 75, 10); + clueCanvas.clickToolbarButton('graph', 'toggle-lock'); // unlock + }); // Click toolbar button again to leave edit mode clueCanvas.clickToolbarButton('graph', 'move-points'); diff --git a/src/plugins/graph/components/graph-axis.tsx b/src/plugins/graph/components/graph-axis.tsx index 6d83921121..5ac60ec487 100644 --- a/src/plugins/graph/components/graph-axis.tsx +++ b/src/plugins/graph/components/graph-axis.tsx @@ -17,7 +17,6 @@ import {axisPlaceToAttrRole, kGraphClassSelector} from "../graph-types"; 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 { DroppableAxis } from "./droppable-axis"; import { useGraphSettingsContext } from "../hooks/use-graph-settings-context"; import { GraphController } from "../models/graph-controller"; @@ -25,7 +24,6 @@ import { GraphController } from "../models/graph-controller"; interface IProps { place: AxisPlace; enableAnimation: MutableRefObject; - autoAdjust?: React.MutableRefObject; controller: GraphController; onDropAttribute?: (place: GraphPlace, dataSet: IDataSet, attrId: string) => void; onRemoveAttribute?: (place: GraphPlace, attrId: string) => void; @@ -33,7 +31,7 @@ interface IProps { } export const GraphAxis = observer(function GraphAxis({ - place, enableAnimation, autoAdjust, controller, onDropAttribute, onRemoveAttribute, onTreatAttributeAs + place, enableAnimation, controller, onDropAttribute, onRemoveAttribute, onTreatAttributeAs }: IProps) { const dataConfig = useDataConfigurationContext(), // FIXME mult-dataset. isDropAllowed = dataConfig?.graphPlaceCanAcceptAttributeIDDrop ?? (() => true), @@ -87,24 +85,6 @@ export const GraphAxis = observer(function GraphAxis({ }); }, [layout, place, wrapperElt]); - useEffect(() => { - if (autoAdjust?.current) { - // 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 - }, [autoAdjust]); - useEffect(function cleanup () { return () => { // This gets called when the component is unmounted, which happens when the graph is closed. diff --git a/src/plugins/graph/components/graph-component.tsx b/src/plugins/graph/components/graph-component.tsx index b8e77b80ed..79acb7814b 100644 --- a/src/plugins/graph/components/graph-component.tsx +++ b/src/plugins/graph/components/graph-component.tsx @@ -29,9 +29,8 @@ export const GraphComponent = observer( const graphRef = useRef(null); const {width, height} = useResizeDetector({ targetRef: graphRef }); const enableAnimation = useRef(true); - const autoAdjustAxes = useRef(true); const graphController = useMemo( - () => new GraphController({layout, enableAnimation, instanceId, autoAdjustAxes}), + () => new GraphController({layout, enableAnimation, instanceId}), [layout, instanceId] ); diff --git a/src/plugins/graph/components/graph.tsx b/src/plugins/graph/components/graph.tsx index 4a94675785..9390b9dd61 100644 --- a/src/plugins/graph/components/graph.tsx +++ b/src/plugins/graph/components/graph.tsx @@ -47,7 +47,7 @@ export const Graph = observer( function Graph({ graphController, readOnly, graphRef, onRequestRowHeight }: IProps) { const graphModel = useGraphModelContext(), - {autoAdjustAxes, enableAnimation} = graphController, + {enableAnimation} = graphController, {plotType} = graphModel, instanceId = useInstanceIdContext(), marqueeState = useMemo(() => new MarqueeState(), []), @@ -185,7 +185,6 @@ export const Graph = observer( place={place} controller={graphController} enableAnimation={enableAnimation} - autoAdjust={autoAdjustAxes} onDropAttribute={handleChangeAttribute} onRemoveAttribute={handleRemoveAttribute} onTreatAttributeAs={handleTreatAttrAs} diff --git a/src/plugins/graph/components/scatterdots.tsx b/src/plugins/graph/components/scatterdots.tsx index 2c7cd4ca5f..409f6be183 100644 --- a/src/plugins/graph/components/scatterdots.tsx +++ b/src/plugins/graph/components/scatterdots.tsx @@ -1,15 +1,14 @@ -import {ScaleBand, ScaleLinear, select} from "d3"; -import React, {useCallback, useRef, useState} from "react"; +import {D3DragEvent, drag, ScaleBand, ScaleLinear, select} from "d3"; +import React, {useCallback, useRef} from "react"; import {ScaleNumericBaseType} from "../imports/components/axis/axis-types"; -import {CaseData, inGraphDot} from "../d3-types"; -import {PlotProps} from "../graph-types"; -import {useDragHandlers, usePlotResponders} from "../hooks/use-plot"; +import {CaseData, inGraphDot, selectGraphDots} from "../d3-types"; +import {PlotProps, Point} from "../graph-types"; +import { usePlotResponders} from "../hooks/use-plot"; import {useDataConfigurationContext} from "../hooks/use-data-configuration-context"; import {useGraphLayoutContext} from "../models/graph-layout"; import {ICase} from "../../../models/data/data-set-types"; import { useGraphModelContext } from "../hooks/use-graph-model-context"; import { - // getScreenCoord, handleClickOnDot, setPointCoordinates, setPointSelection @@ -29,8 +28,6 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { layout = useGraphLayoutContext(), legendAttrID = dataConfiguration?.attributeID('legend') as string, yScaleRef = useRef(), - [dragID, setDragID] = useState(''), - currPos = useRef({x: 0, y: 0}), target = useRef(), plotNumRef = useRef(0); @@ -40,85 +37,67 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { dragPointRadiusRef.current = graphModel.getPointRadius('hover-drag'); yScaleRef.current = layout.getAxisScale("left") as ScaleNumericBaseType; - const onDragStart = useCallback((event: MouseEvent) => { - if (graphModel.editingMode !== "none") { - const targetDot = event.target && inGraphDot(event.target as SVGSVGElement); + const + onDragStart = useCallback((event: D3DragEvent, datum) => { + const targetDot = event.sourceEvent.target && inGraphDot(event.sourceEvent.target as SVGSVGElement); if (!targetDot) return; target.current = select(targetDot as SVGSVGElement); - const aCaseData: CaseData = target.current.node().__data__; - if (!aCaseData || !dataConfiguration || aCaseData.dataConfigID !== dataConfiguration.id) return; - event.stopPropagation(); + if (!datum || !dataConfiguration || datum.dataConfigID !== dataConfiguration.id) return; + handleClickOnDot(event.sourceEvent, datum, dataConfiguration); graphModel.setInteractionInProgress(true); dataset?.beginCaching(); secondaryAttrIDsRef.current = dataConfiguration.yAttributeIDs || []; enableAnimation.current = false; // We don't want to animate points until end of drag - const tItsID = aCaseData.caseID; - plotNumRef.current = target.current.datum()?.plotNum ?? 0; - // 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, graphModel]), - - 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}, - dx = newPos.x - currPos.current.x, - dy = newPos.y - currPos.current.y; - currPos.current = newPos; - 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[] = []; - 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__: cell.caseId, - [xAttrID]: currX + deltaX, - [cell.attributeId]: currY + deltaY - }); - } + plotNumRef.current = datum.plotNum ?? 0; + }, [dataConfiguration, dataset, enableAnimation, graphModel]), + + updatePositions = useCallback((dx: number, dy: number) => { + const + xAxisScale = layout.getAxisScale('bottom') as ScaleLinear, + xAttrID = dataConfiguration?.attributeID('x') ?? '', + deltaX = Number(xAxisScale.invert(dx)) - Number(xAxisScale.invert(0)), + deltaY = Number(yScaleRef.current?.invert(dy)) - Number(yScaleRef.current?.invert(0)), + caseValues: ICase[] = [], + 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__: cell.caseId, + [xAttrID]: currX + deltaX, + [cell.attributeId]: currY + deltaY }); - caseValues.length && - dataset?.setCanonicalCaseValues(caseValues); } - } - }, [layout, dataConfiguration, dataset, dragID]), + }); + caseValues.length && + dataset?.setCanonicalCaseValues(caseValues); + }, [layout, dataConfiguration, dataset]), - onDrag = useCallback((event: MouseEvent) => { - updatePositions(event, false); + onDrag = useCallback((event: D3DragEvent) => { + if (event.dx !== 0 || event.dy !== 0) { + updatePositions(event.dx, event.dy); + } }, [updatePositions]), - onDragEnd = useCallback((event: MouseEvent) => { - if (dragID !== '') { + onDragEnd = useCallback((event: D3DragEvent) => { 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) - .property('isDragging', false) - .transition() - .attr('r', selectedPointRadiusRef.current); - setDragID(() => ''); + updatePositions(event.dx, event.dy); target.current = null; - } - }, [dragID, graphModel, updatePositions]); - - useDragHandlers(window, {start: onDragStart, drag: onDrag, end: onDragEnd}); + }, [graphModel, updatePositions]), + + refreshDragHandlers = useCallback(() => { + selectGraphDots(dotsRef.current) + ?.call(drag() + .filter(() => graphModel.editingMode !== "none") + .subject((event) => ({ x: event.x, y: event.y })) + .on('start', onDragStart) + .on('drag', onDrag) + .on('end', onDragEnd) + ); + }, [dotsRef, graphModel, onDrag, onDragEnd, onDragStart]); const refreshPointSelection = useCallback(() => { const { pointColor, pointStrokeColor } = graphModel; @@ -169,8 +148,9 @@ export const ScatterDots = function ScatterDots(props: PlotProps) { selectedOnly, getScreenX, getScreenY, getLegendColor, getColorForId: graphModel.getColorForId, enableAnimation, pointColor, pointStrokeColor }); + refreshDragHandlers(); }, [dataConfiguration, dataset, dotsRef, layout, legendAttrID, - enableAnimation, graphModel, yScaleRef]); + enableAnimation, graphModel, yScaleRef, refreshDragHandlers]); // const refreshPointPositionsSVG = useCallback((selectedOnly: boolean) => { // const xAttrID = dataConfiguration?.attributeID('x') ?? '', diff --git a/src/plugins/graph/d3-types.ts b/src/plugins/graph/d3-types.ts index 4f6b0c931d..e393ff0d17 100644 --- a/src/plugins/graph/d3-types.ts +++ b/src/plugins/graph/d3-types.ts @@ -12,7 +12,7 @@ export type DotsElt = SVGSVGElement | null; // CaseData: type of data attached to selected element // SVGSVGElement: type of parent element selected by initial/global select // unknown: type of data attached to parent element (none in this case) -export type DotSelection = Selection; +export type DotSelection = Selection; export const graphDotSelector = "g.graph-dot"; diff --git a/src/plugins/graph/hooks/use-graph-model.ts b/src/plugins/graph/hooks/use-graph-model.ts index 29979f4f14..86479a56b3 100644 --- a/src/plugins/graph/hooks/use-graph-model.ts +++ b/src/plugins/graph/hooks/use-graph-model.ts @@ -12,39 +12,12 @@ 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 callMatchAllCirclesToData = useCallback(() => { matchAllCirclesToData({ graphModel, enableAnimation, instanceId }); }, [graphModel, enableAnimation, instanceId]); - // respond to added/removed cases - // 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() { const disposer = onAnyAction(graphModel, action => { diff --git a/src/plugins/graph/hooks/use-plot.ts b/src/plugins/graph/hooks/use-plot.ts index b02dc5a745..25d213752e 100644 --- a/src/plugins/graph/hooks/use-plot.ts +++ b/src/plugins/graph/hooks/use-plot.ts @@ -1,4 +1,4 @@ -import React, {useCallback, useEffect, useRef} from "react"; +import React, {useCallback, useContext, useEffect, useRef} from "react"; import {autorun, reaction} from "mobx"; import { isAddCasesAction, isRemoveAttributeAction, isRemoveCasesAction, isSetCaseValuesAction } from "../../../models/data/data-set-actions"; @@ -13,6 +13,8 @@ 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"; +import { GraphControllerContext } from "../models/graph-controller"; +import { useGraphSettingsContext } from "./use-graph-settings-context"; interface IDragHandlers { start: (event: MouseEvent) => void @@ -52,6 +54,8 @@ export const usePlotResponders = (props: IPlotResponderProps) => { graphModel = useGraphModelContext(), layout = useGraphLayoutContext(), instanceId = useInstanceIdContext(), + controller = useContext(GraphControllerContext), + graphSettings = useGraphSettingsContext(), refreshPointPositionsRef = useCurrent(refreshPointPositions); useEffect(function respondToLayerDotsEltCreation() { @@ -104,6 +108,14 @@ export const usePlotResponders = (props: IPlotResponderProps) => { }; }, []); + const callRescaleIfNeeded = useCallback((growOnly: boolean = false) => { + if (graphSettings.scalePlotOnValueChange && + !graphModel.lockAxes && + !graphModel.interactionInProgress) { + controller!.autoscaleAllAxes(growOnly); + } + }, [controller, graphModel, graphSettings]); + // respond to numeric axis domain changes (e.g. axis dragging) useEffect(() => { const disposer = reaction( @@ -161,6 +173,7 @@ export const usePlotResponders = (props: IPlotResponderProps) => { if (dataset) { const disposer = onAnyAction(dataset, action => { if (isSetCaseValuesAction(action)) { + callRescaleIfNeeded(); // assumes that if we're caching then only selected cases are being updated callRefreshPointPositions(dataset.isCaching); // TODO: handling of add/remove cases was added specifically for the case plot. @@ -173,7 +186,7 @@ export const usePlotResponders = (props: IPlotResponderProps) => { }); return () => disposer(); } - }, [dataset, callRefreshPointPositions]); + }, [dataset, callRefreshPointPositions, callRescaleIfNeeded]); // respond to color changes useEffect(() => { @@ -210,11 +223,14 @@ export const usePlotResponders = (props: IPlotResponderProps) => { dotsElement: dotsRef.current, enableAnimation, instanceId }); + const growOnly = isAddCasesAction(action); + callRescaleIfNeeded(growOnly); callRefreshPointPositions(false); } }) || (() => true); return () => disposer(); - }, [dataset, dataConfiguration, enableAnimation, graphModel, callRefreshPointPositions, dotsRef, instanceId]); + }, [controller, dataset, dataConfiguration, enableAnimation, graphModel, + callRefreshPointPositions, dotsRef, instanceId, callRescaleIfNeeded]); // respond to pointsNeedUpdating becoming false; that is when the points have been updated // Happens when the number of plots has changed for now. Possibly other situations in the future. diff --git a/src/plugins/graph/imports/components/axis/models/axis-model.ts b/src/plugins/graph/imports/components/axis/models/axis-model.ts index ecfebd72fd..e5f50de094 100644 --- a/src/plugins/graph/imports/components/axis/models/axis-model.ts +++ b/src/plugins/graph/imports/components/axis/models/axis-model.ts @@ -100,8 +100,9 @@ export const NumericAxisModel = AxisModel } else if ((min < 0) && (Math.abs(max) < Math.abs(min / snapFactor))) { max = 0; } - self.min = parseFloat(min.toFixed(2)); - self.max = parseFloat(max.toFixed(2)); + // Truncate to 2 decimal digits, but without making the range smaller + self.min = Math.floor(min*100)/100; + self.max = Math.ceil(max*100)/100; } })); export interface INumericAxisModel extends Instance {} diff --git a/src/plugins/graph/models/graph-controller.ts b/src/plugins/graph/models/graph-controller.ts index 897603f1d6..35a42312b8 100644 --- a/src/plugins/graph/models/graph-controller.ts +++ b/src/plugins/graph/models/graph-controller.ts @@ -27,7 +27,6 @@ interface IGraphControllerConstructorProps { layout: GraphLayout enableAnimation: React.MutableRefObject instanceId: string - autoAdjustAxes: React.MutableRefObject } interface IGraphControllerProps { @@ -39,13 +38,11 @@ export class GraphController { layout: GraphLayout; enableAnimation: React.MutableRefObject; instanceId: string; - autoAdjustAxes: React.MutableRefObject; - constructor({layout, enableAnimation, instanceId, autoAdjustAxes}: IGraphControllerConstructorProps) { + constructor({layout, enableAnimation, instanceId}: IGraphControllerConstructorProps) { this.layout = layout; this.instanceId = instanceId; this.enableAnimation = enableAnimation; - this.autoAdjustAxes = autoAdjustAxes; } setProperties(props: IGraphControllerProps) { @@ -197,14 +194,14 @@ export class GraphController { /** * Set the domains of all axes to fit all of the data points. */ - autoscaleAllAxes() { + autoscaleAllAxes(growOnly: boolean = false) { if (!this.graphModel) return; startAnimation(this.enableAnimation); for (const place of AxisPlaces) { const role = graphPlaceToAttrRole[place]; const axisModel = this.graphModel.getAxis(place); if (isNumericAxisModel(axisModel)) { - setNiceDomain(this.graphModel.numericValuesForAttrRole(role), axisModel); + setNiceDomain(this.graphModel.numericValuesForAttrRole(role), axisModel, growOnly); } } } diff --git a/src/plugins/graph/models/graph-layer-model.ts b/src/plugins/graph/models/graph-layer-model.ts index 0d508fa3fe..3eeb903c22 100644 --- a/src/plugins/graph/models/graph-layer-model.ts +++ b/src/plugins/graph/models/graph-layer-model.ts @@ -81,9 +81,6 @@ export const GraphLayerModel = types 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 caseAdded[0]; } }, diff --git a/src/plugins/graph/utilities/graph-utils.ts b/src/plugins/graph/utilities/graph-utils.ts index 8cd5885241..f8bf253c03 100644 --- a/src/plugins/graph/utilities/graph-utils.ts +++ b/src/plugins/graph/utilities/graph-utils.ts @@ -10,7 +10,7 @@ import { } from "../d3-types"; import { IDotsRef, kGraphFont, Point, outerCircleSelectedRadius, outerCircleUnselectedRadius, - Rect,rTreeRect, transitionDuration + Rect,rTreeRect, transitionDuration, kDefaultNumericAxisBounds } from "../graph-types"; import {between} from "./math-utils"; import {IAxisModel, isNumericAxisModel} from "../imports/components/axis/models/axis-model"; @@ -51,7 +51,8 @@ export function ptInRect(pt: Point, iRect: Rect) { /** * This function closely follows V2's CellLinearAxisModel:_computeBoundsAndTickGap */ -export function computeNiceNumericBounds(min: number, max: number): { min: number, max: number } { +export function computeNiceNumericBounds( + min: number|undefined, max: number|undefined): { min: number, max: number } { function computeTickGap(iMin: number, iMax: number) { const range = (iMin >= iMax) ? Math.abs(iMin) : iMax - iMin, @@ -74,37 +75,64 @@ export function computeNiceNumericBounds(min: number, max: number): { min: numbe return Math.max(power * base, Number.MIN_VALUE); } + if (min === undefined || max === undefined || (min === max && min === 0)) { + return { min: kDefaultNumericAxisBounds[0], max: kDefaultNumericAxisBounds[1] }; + } + const kAddend = 5, // amount to extend scale kFactor = 2.5, bounds = {min, max}; - if (min === max && min === 0) { - bounds.min = -10; - bounds.max = 10; - } else if (min === max && isInteger(min)) { + + if (min === max && isInteger(min)) { bounds.min -= kAddend; bounds.max += kAddend; } else if (min === max) { - bounds.min = bounds.min + 0.1 * Math.abs(bounds.min); - bounds.max = bounds.max - 0.1 * Math.abs(bounds.max); - } else if (min > 0 && max > 0 && min <= max / kFactor) { // Snap to zero - bounds.min = 0; - } else if (min < 0 && max < 0 && max >= min / kFactor) { // Snap to zero - bounds.max = 0; + // Make a range around a single point by moving limits 10% in each direction + bounds.min = bounds.min - 0.1 * Math.abs(bounds.min); + bounds.max = bounds.max + 0.1 * Math.abs(bounds.max); } + + // Find location of tick marks, move limits to a tick mark + // making sure there is at least 1/2 of a tick mark space beyond the last data point const tickGap = computeTickGap(bounds.min, bounds.max); + if (tickGap !== 0) { - bounds.min = (Math.floor(bounds.min / tickGap) - 0.5) * tickGap; - bounds.max = (Math.floor(bounds.max / tickGap) + 1.5) * tickGap; + bounds.min = Math.floor(bounds.min / tickGap - 0.5) * tickGap; + bounds.max = Math.ceil(bounds.max / tickGap + 0.5) * tickGap; } else { bounds.min -= 1; bounds.max += 1; } + + // Snap to zero if close to 0. + if (min > 0 && max > 0 && min <= max / kFactor) { + bounds.min = 0; + } else if (min < 0 && max < 0 && max >= min / kFactor) { + bounds.max = 0; + } return bounds; } -export function setNiceDomain(values: number[], axisModel: IAxisModel) { +/** + * Set the domain of the axis so that it fits all the given data points in a + * nice way. The values can be any numbers in any order; only the min and max in + * the array matter. The actual min and max set on the axis are derived from + * this considering factors such as making sure there is a non-zero range, + * starting and ending on tick marks, and snapping an end to 0 if it is close to 0. + * + * @param values data points to be fit. + * @param axisModel axis to update. + * @param growOnly if true, the range will not be reduced from its current limits, only expanded if necessary. + */ +export function setNiceDomain(values: number[], axisModel: IAxisModel, growOnly: boolean = false) { if (isNumericAxisModel(axisModel)) { const [minValue, maxValue] = extent(values, d => d) as [number, number]; + if (growOnly) { + const [currentMin, currentMax] = axisModel.domain; + if (minValue >= currentMin && maxValue <= currentMax) { + return; + } + } const {min: niceMin, max: niceMax} = computeNiceNumericBounds(minValue, maxValue); axisModel.setDomain(niceMin, niceMax); } diff --git a/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-component.tsx b/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-component.tsx index 9ff716f306..747313574f 100644 --- a/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-component.tsx +++ b/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-component.tsx @@ -1,5 +1,6 @@ -import React, { useCallback, useEffect, useRef } from "react"; +import React, { useCallback, useContext, useEffect, useMemo, useRef } from "react"; import { drag, select, Selection } from "d3"; +import { comparer } from "mobx"; import { observer } from "mobx-react-lite"; import { VariableType } from "@concord-consortium/diagram-view"; @@ -7,17 +8,17 @@ import { useTileModelContext } from "../../../../components/tiles/hooks/use-tile import { getSharedModelManager } from "../../../../models/tiles/tile-environment"; import { mstAutorun } from "../../../../utilities/mst-autorun"; import { mstReaction } from "../../../../utilities/mst-reaction"; -import { useDataConfigurationContext } from "../../../graph/hooks/use-data-configuration-context"; import { useGraphModelContext } from "../../../graph/hooks/use-graph-model-context"; import { ScaleNumericBaseType } from "../../../graph/imports/components/axis/axis-types"; import { useAxisLayoutContext } from "../../../graph/imports/components/axis/models/axis-layout-context"; -import { IAxisModel, INumericAxisModel } from "../../../graph/imports/components/axis/models/axis-model"; -import { curveBasis, setNiceDomain } from "../../../graph/utilities/graph-utils"; +import { INumericAxisModel } from "../../../graph/imports/components/axis/models/axis-model"; +import { curveBasis } from "../../../graph/utilities/graph-utils"; import { SharedVariables } from "../../shared-variables"; import { IPlottedVariablesAdornmentModel } from "./plotted-variables-adornment-model"; import { useReadOnlyContext } from "../../../../components/document/read-only-context"; import { isFiniteNumber } from "../../../../utilities/math-utils"; -import { useUIStore } from "../../../../hooks/use-stores"; +import { useGraphSettingsContext } from "../../../graph/hooks/use-graph-settings-context"; +import { GraphControllerContext } from "../../../graph/models/graph-controller"; import "../../../graph/adornments/plotted-function/plotted-function-adornment-component.scss"; import "./plotted-variables.scss"; @@ -36,23 +37,10 @@ export const PlottedVariablesAdornmentComponent = observer(function PlottedVaria const {model, cellKey = {}, plotWidth, plotHeight, xAxis, yAxis} = props; const { tile } = useTileModelContext(); const graphModel = useGraphModelContext(); - const dataConfig = useDataConfigurationContext(); const readOnly = useReadOnlyContext(); const layout = useAxisLayoutContext(); - const ui = useUIStore(); - const isTileSelected = !!tile && ui.isSelectedTile(tile); - const xScale = layout.getAxisScale("bottom") as ScaleNumericBaseType; - const yScale = layout.getAxisScale("left") as ScaleNumericBaseType; - const xAttrType = dataConfig?.attributeType("x"); - const yAttrType = dataConfig?.attributeType("y"); - const xSubAxesCount = layout.getAxisMultiScale("bottom")?.repetitions ?? 1; - const ySubAxesCount = layout.getAxisMultiScale("left")?.repetitions ?? 1; - const xCatSet = layout.getAxisMultiScale("bottom")?.categorySet; - const xCats = xAttrType === "categorical" && xCatSet ? Array.from(xCatSet.values) : [""]; - const yCatSet = layout.getAxisMultiScale("left")?.categorySet; - const yCats = yAttrType === "categorical" && yCatSet ? Array.from(yCatSet.values) : [""]; - const xCellCount = xCats.length * xSubAxesCount; - const yCellCount = yCats.length * ySubAxesCount; + const settings = useGraphSettingsContext(); + const controller = useContext(GraphControllerContext); const classFromKey = model.classNameFromKey(cellKey); const plottedFunctionRef = useRef(null); const smm = getSharedModelManager(graphModel); @@ -62,7 +50,8 @@ export const PlottedVariablesAdornmentComponent = observer(function PlottedVaria const offsetFromPoint = 14; const highlightStrokeWidth = 5; const labelRectHeight = textHeight + 2 * padding; - const numberFormatter = Intl.NumberFormat(undefined, { maximumFractionDigits: 4, useGrouping: false}); + const numberFormatter + = useMemo(() => Intl.NumberFormat(undefined, { maximumFractionDigits: 4, useGrouping: false}), []); // Set the positions of the point-related SVG objects and the contents of the label when the variable value changes. const positionPointMarkers = useCallback((xValue: number, yValue: number, @@ -91,15 +80,21 @@ export const PlottedVariablesAdornmentComponent = observer(function PlottedVaria // Assign a new value to the Variable based on the given pixel position const setVariableValue = useCallback((variable: VariableType, position: number) => { + const xScale = layout.getAxisScale("bottom") as ScaleNumericBaseType; + const xCellCount = layout.getAxisMultiScale("bottom")?.repetitions ?? 1; const newValue = model.valueForPosition(position, xScale, xCellCount); if (isFiniteNumber(newValue)) { // Truncate extra decimals to match the value that is displayed. variable.setValue(+numberFormatter.format(newValue)); } - }, [model, numberFormatter, xCellCount, xScale]); + }, [layout, model, numberFormatter]); // Draw the variable traces const addPath = useCallback(() => { + const xScale = layout.getAxisScale("bottom") as ScaleNumericBaseType; + const xCellCount = layout.getAxisMultiScale("bottom")?.repetitions ?? 1; + const yScale = layout.getAxisScale("left") as ScaleNumericBaseType; + const yCellCount = layout.getAxisMultiScale("left")?.repetitions ?? 1; const xMin = xScale.domain()[0]; const xMax = xScale.domain()[1]; const tPixelMin = xScale(xMin); @@ -172,7 +167,7 @@ export const PlottedVariablesAdornmentComponent = observer(function PlottedVaria pointHighlight .call(drag() .container(() => { return plottedFunctionRef.current!; }) - .filter((e) => { return !e.ctrlKey && !e.button && isTileSelected; }) + .filter((e) => { return !e.ctrlKey && !e.button; }) .on('start', (e) => traceGroup.classed('dragging', true)) .on('drag', (e) => { const newX = Math.round(e.x); @@ -194,8 +189,7 @@ export const PlottedVariablesAdornmentComponent = observer(function PlottedVaria } } } - }, [xScale, model, xCellCount, yCellCount, yScale, graphModel, - labelRectHeight, positionPointMarkers, readOnly, sharedVariables, isTileSelected, setVariableValue]); + }, [layout, model, sharedVariables, readOnly, graphModel, setVariableValue, labelRectHeight, positionPointMarkers]); // Add the lines and their associated covers and labels const refreshValues = useCallback(() => { @@ -215,26 +209,68 @@ export const PlottedVariablesAdornmentComponent = observer(function PlottedVaria return mstAutorun(() => { model.updateCategories(graphModel.layers[0].getUpdateCategoriesOptions(false)); }, { name: "PlottedVariablesAdornmentComponent.refreshExpressionChange" }, model); - }, [graphModel, model, xScale, xSubAxesCount, yScale]); + }, [graphModel, model]); - // Refresh values on axis or expression change + // Draw when becoming visible + useEffect(function refreshOnVisibility() { + return mstReaction( + () => { return model.isVisible; }, + (visible) => { + if (visible) { + refreshValues(); + } + }, + { name: "PlottedVariablesAdornment.refreshOnVisibility" }, + model); + }, [model, refreshValues]); + + // Refresh display when axis domains change useEffect(function refreshAxisChange() { - return mstAutorun(() => { - // We observe changes to the axis domains within the autorun by extracting them from the axes below. - // We do this instead of including domains in the useEffect dependency array to prevent domain changes - // from triggering a reinstall of the autorun. - if (xAxis && yAxis) { - const { domain: xDomain } = xAxis; // eslint-disable-line unused-imports/no-unused-vars - const { domain: yDomain } = yAxis; // eslint-disable-line unused-imports/no-unused-vars - } - // Trigger an autorun if any inputs or the expression of y change, or if the x variable changes - Array.from(model.plottedVariables.values()).forEach(plottedVariables => { - plottedVariables.yVariable?.computedValueIncludingMessageAndError; // eslint-disable-line no-unused-expressions - plottedVariables.xVariable; // eslint-disable-line no-unused-expressions - }); - refreshValues(); - }, { name: "PlottedVariablesAdornmentComponent.refreshAxisChange" }, model); - }, [dataConfig, model, plotWidth, plotHeight, sharedVariables, xAxis, yAxis, refreshValues]); + return mstReaction( + () => { + if (xAxis && yAxis) { + const { domain: xDomain } = xAxis; + const { domain: yDomain } = yAxis; + // Return a primitive value (string, not array) so that equality will work + // and effect will only be called when these numbers actually change. + return `${xDomain[0]}, ${xDomain[1]}, ${yDomain[0]}, ${yDomain[1]}`; + } + return ''; + }, + (limits) => { + refreshValues(); + }, + { fireImmediately: true, name: "PlottedVariablesAdornmentComponent.refreshAxisChange" }, + model); + }, [model, plotWidth, plotHeight, xAxis, yAxis, refreshValues, controller]); + + useEffect(function updateOnValuesChange() { + return mstReaction( + () => { + let hasAnyVars = false; + const varValues = Array.from(model.plottedVariables.values()).map(plottedVariables => { + if (plottedVariables.xVariable || plottedVariables.yVariable) { + hasAnyVars = true; + } + return { + x: plottedVariables.xVariable, + y: plottedVariables.yVariable?.computedValueIncludingMessageAndError + }; + }); + return {hasAnyVars, varValues}; + }, + (args: {hasAnyVars: boolean, varValues: any}) => { + if (args.hasAnyVars) { + if (settings.scalePlotOnValueChange && !graphModel.lockAxes) { + controller?.autoscaleAllAxes(); + } + refreshValues(); + } + }, + { name: "PlottedVariablesAdornmentComponent.rescaleOnValuesChange", + equals: comparer.structural }, + model); + }, [controller, graphModel, model, refreshValues, settings]); // Scale graph when a new X or Y variable is selected useEffect(function scaleOnVariableChange() { @@ -242,23 +278,13 @@ export const PlottedVariablesAdornmentComponent = observer(function PlottedVaria return Array.from(model.plottedVariables.values()).map((pvi) => [pvi.xVariableId, pvi.yVariableId]); }, (varlist) => { - if (graphModel.lockAxes) return; - // Set a range that includes 0 to 2x for all the given values. - function fitValues(values: number[], axis: IAxisModel) { - if (values.length) { - setNiceDomain([0, ...values.map(x => 2 * x)], axis); - } - } - - const variableValues = model.variableValues; - if (xAxis && yAxis) { - fitValues(variableValues.x, xAxis); - fitValues(variableValues.y, yAxis); + if (!graphModel.lockAxes) { + controller?.autoscaleAllAxes(); } }, { name: "PlottedVariablesAdornmentComponent.scaleOnVariableChange" }, model); - }, [graphModel.lockAxes, model, xAxis, yAxis]); + }, [graphModel, model, xAxis, yAxis, controller]); return ( diff --git a/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-model.ts b/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-model.ts index a3026b509e..6d106fac70 100644 --- a/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-model.ts +++ b/src/plugins/shared-variables/graph/plotted-variables-adornment/plotted-variables-adornment-model.ts @@ -1,4 +1,4 @@ -import { destroy, getSnapshot, IAnyStateTreeNode, Instance, types } from "mobx-state-tree"; +import { destroy, getParentOfType, getSnapshot, IAnyStateTreeNode, Instance, types } from "mobx-state-tree"; import { Variable, VariableType } from "@concord-consortium/diagram-view"; import { getTileModel } from "../../../../models/document/shared-model-document-manager"; @@ -10,8 +10,10 @@ import { } from "../../../graph/adornments/plotted-function/plotted-function-adornment-model"; import { SharedVariables, SharedVariablesType } from "../../shared-variables"; import { kPlottedVariablesType } from "./plotted-variables-adornment-types"; -import { GraphAttrRole, Point } from "../../../graph/graph-types"; +import { GraphAttrRole, Point, PrimaryAttrRole } from "../../../graph/graph-types"; import { IClueObject } from "../../../../models/annotations/clue-object"; +import { GraphModel } from "../../../graph/models/graph-model"; +import { isNumericAxisModel } from "../../../graph/imports/components/axis/models/axis-model"; function getSharedVariables(node: IAnyStateTreeNode) { const sharedModelManager = getSharedModelManager(node); @@ -39,6 +41,9 @@ function decipherAnnotationId(id: string) { return {}; } +/** + * A single X,Y pair of variables to be plotted on the graph. + */ export const PlottedVariables = types.model("PlottedVariables", {}) .props({ xVariableId: types.maybe(types.string), @@ -108,6 +113,9 @@ export const PlottedVariables = types.model("PlottedVariables", {}) } })); +/** + * An Adornment that holds one or more PlottedVariables to be shown on the graph. + */ export const PlottedVariablesAdornmentModel = PlottedFunctionAdornmentModel .named("PlottedVariablesAdornmentModel") .props({ @@ -162,16 +170,43 @@ export const PlottedVariablesAdornmentModel = PlottedFunctionAdornmentModel } })) .views(self => ({ - numericValuesForAttrRole(role: GraphAttrRole) { - const values = self.variableValues; - if (role in values) { - // We don't return the actual variable values, but rather 0 and 2 times each value. - // This is because of how autoscale is defined for variables - not just the current-value point - // has to fit in the graph, but a range of values around it so the function line can be seen. - return [0, ...values[role as 'x'|'y'].map(x => 2*x)]; - } else { - return [] as number[]; + numericValuesForAttrRole(_role: GraphAttrRole) { + // We only have any values for X and Y + if (!['x', 'y'].includes(_role)) return []; + + const role = _role as PrimaryAttrRole; + const result = [] as number[]; + for (const pvi of self.plottedVariables.values()) { + if (pvi.xVariableId && pvi.yVariableId) { + const vals = pvi.variableValues; + if (vals && role in vals) { + // We return 2 times each value because of how autoscale is defined for + // variables. Not just the current-value point has to fit in the graph, + // but a range of values around it so the function line can be seen clearly. + result.push(2 * vals[role]); + } else { + // If the variable exists but does not have a value, we pretend that + // the 'X' value is in the middle of the graph's X axis, and return + // a 'Y' range that will bring that part of the variable trace into view. + if (role === 'y') { + const graph = getParentOfType(self, GraphModel); + const bottomAxis = graph.getAxis("bottom"); + const fakeX = isNumericAxisModel(bottomAxis) ? (bottomAxis.min + bottomAxis.max) / 2 : undefined; + if (fakeX) { + const { computeY, dispose } = pvi.setupCompute(); + const fakeY = computeY(fakeX); + result.push(2 * fakeY); + dispose(); + } + } + } + } } + // The region we want to be visible is from 0 to twice the value(s) + if (result.length > 0) { + result.push(0); + } + return result; } })) .actions(self => ({ @@ -184,6 +219,9 @@ export const PlottedVariablesAdornmentModel = PlottedFunctionAdornmentModel removePlottedVariables(key: string) { self.plottedVariables.delete(key); }, + clearPlottedVariables() { + self.plottedVariables.clear(); + }, setupCompute(instanceKey: string) { return self.plottedVariables.get(instanceKey)?.setupCompute(); } diff --git a/src/plugins/shared-variables/shared-variables-registration.ts b/src/plugins/shared-variables/shared-variables-registration.ts index 2ca5bb14f6..789559ec4c 100644 --- a/src/plugins/shared-variables/shared-variables-registration.ts +++ b/src/plugins/shared-variables/shared-variables-registration.ts @@ -7,7 +7,7 @@ import { IGraphModel, registerGraphSharedModelUpdateFunction } from "../graph/mo import { EditVariableButton, InsertVariableButton, NewVariableButton, VariableChipComponent, VariableChipObject } from "./drawing/variable-object"; -import { PlottedVariablesAdornmentModel +import { isPlottedVariablesAdornment, PlottedVariablesAdornmentModel } from "./graph/plotted-variables-adornment/plotted-variables-adornment-model"; import "./graph/plotted-variables-adornment/plotted-variables-adornment-registration"; import { @@ -74,16 +74,26 @@ registerGraphSharedModelUpdateFunction( // Display a plotted variables adornment when this is linked to a shared variables model const sharedVariableModels = smm.getTileSharedModelsByType(graphModel, SharedVariables); if (sharedVariableModels && sharedVariableModels.length > 0) { - // We're connected to variables; show adornment or create if not there already. - if (graphModel.getAdornmentOfType(kPlottedVariablesType)) { + // We're connected to variables; make sure adornment is showing, or create if not there already. + let adornment = graphModel.getAdornmentOfType(kPlottedVariablesType); + if (adornment) { graphModel.showAdornment(kPlottedVariablesType); } else { - const plottedVariablesAdornment = PlottedVariablesAdornmentModel.create(); - plottedVariablesAdornment.addPlottedVariables(); - graphModel.addAdornment(plottedVariablesAdornment); + adornment = PlottedVariablesAdornmentModel.create(); + graphModel.addAdornment(adornment); + } + // Make sure there's at least one PlottedVariables in it. + if (adornment && isPlottedVariablesAdornment(adornment)) { + if (adornment.plottedVariables.size === 0) { + adornment.addPlottedVariables(); + } } } else { // Disconnected + const adornment = graphModel.getAdornmentOfType(kPlottedVariablesType); + if (adornment && isPlottedVariablesAdornment(adornment)) { + adornment.clearPlottedVariables(); + } graphModel.hideAdornment(kPlottedVariablesType); } }