From 6877f38c71c824857f628cc08c4bd76ee351b5c5 Mon Sep 17 00:00:00 2001 From: Kirk Swenson Date: Wed, 11 Oct 2023 21:28:36 -0700 Subject: [PATCH] fix: undo slider creation (#938) --- v3/cypress/e2e/slider.spec.ts | 10 +-- .../components/axis/hooks/use-axis.test.tsx | 29 ++++--- v3/src/components/axis/hooks/use-axis.ts | 11 +-- v3/src/components/axis/hooks/use-sub-axis.ts | 17 ++-- v3/src/components/slider/slider-component.tsx | 16 ++-- v3/src/components/slider/slider-model.ts | 17 ++-- v3/src/components/slider/slider-thumb.tsx | 15 ++-- v3/src/components/slider/slider-title-bar.tsx | 26 ++++-- v3/src/components/slider/slider-types.ts | 6 ++ .../slider/use-slider-animation.tsx | 81 ++++++++++--------- v3/src/components/tool-shelf/tool-shelf.tsx | 10 ++- v3/src/models/document/document-content.ts | 11 --- v3/src/utilities/mobx-autorun.ts | 27 +++++++ v3/src/utilities/mst-utils.ts | 4 + 14 files changed, 176 insertions(+), 104 deletions(-) create mode 100644 v3/src/utilities/mobx-autorun.ts diff --git a/v3/cypress/e2e/slider.spec.ts b/v3/cypress/e2e/slider.spec.ts index ebe12f44a1..a341959fa9 100644 --- a/v3/cypress/e2e/slider.spec.ts +++ b/v3/cypress/e2e/slider.spec.ts @@ -198,13 +198,13 @@ context("Slider UI", () => { slider.getVariableValue(2).should("contain", initialSliderValue) slider.checkPlayButtonIsPaused(2) }) - it("creates sliders with incrementing names when existing ones are closed", () => { + it("reuses slider names after existing ones are closed", () => { c.closeComponent("slider") c.checkComponentDoesNotExist("slider") c.createFromToolshelf("slider") - c.getComponentTitle("slider").should("contain", "v2") - slider.getVariableName().should("have.text", "v2") + c.getComponentTitle("slider").should("contain", "v1") + slider.getVariableName().should("have.text", "v1") slider.getVariableValue().should("contain", initialSliderValue) slider.checkPlayButtonIsPaused() @@ -212,8 +212,8 @@ context("Slider UI", () => { c.checkComponentDoesNotExist("slider") c.createFromToolshelf("slider") - c.getComponentTitle("slider").should("contain", "v3") - slider.getVariableName().should("have.text", "v3") + c.getComponentTitle("slider").should("contain", "v1") + slider.getVariableName().should("have.text", "v1") slider.getVariableValue().should("contain", initialSliderValue) slider.checkPlayButtonIsPaused() diff --git a/v3/src/components/axis/hooks/use-axis.test.tsx b/v3/src/components/axis/hooks/use-axis.test.tsx index 8b5d303b0b..695e7d96c8 100644 --- a/v3/src/components/axis/hooks/use-axis.test.tsx +++ b/v3/src/components/axis/hooks/use-axis.test.tsx @@ -1,27 +1,38 @@ /* eslint-disable testing-library/no-node-access */ import { renderHook } from "@testing-library/react" +import { Instance, types } from "mobx-state-tree" import React from "react" import { SliderAxisLayout } from "../../slider/slider-layout" import { AxisLayoutContext } from "../models/axis-layout-context" import { INumericAxisModel, NumericAxisModel } from "../models/axis-model" import {IUseAxis, useAxis} from "./use-axis" -import { AxisProviderContext, IAxisProvider } from "./use-axis-provider-context" +import { AxisProviderContext } from "./use-axis-provider-context" -describe("useNumericAxis", () => { +const TestAxisProvider = types.model("TestAxisProvider", { + axis: NumericAxisModel +}) +.views(self => ({ + getAxis() { + return self.axis + }, + getNumericAxis() { + return self.axis + } +})) +interface ITestAxisProvider extends Instance {} - let provider: IAxisProvider - let layout: SliderAxisLayout +describe("useAxis", () => { + + let provider: ITestAxisProvider let axisModel: INumericAxisModel + let layout: SliderAxisLayout let axisElt: SVGGElement let useAxisOptions: IUseAxis beforeEach(() => { - provider = { - getAxis: () => axisModel, - getNumericAxis: () => axisModel - } + provider = TestAxisProvider.create({ axis: { place: "bottom", min: 0, max: 10 }}) + axisModel = provider.axis layout = new SliderAxisLayout() - axisModel = NumericAxisModel.create({ place: "bottom", min: 0, max: 10 }) axisElt = document.createElementNS("http://www.w3.org/2000/svg", "g") useAxisOptions = { axisPlace: "bottom", centerCategoryLabels: true } }) diff --git a/v3/src/components/axis/hooks/use-axis.ts b/v3/src/components/axis/hooks/use-axis.ts index f18021dc9e..ca23d4a230 100644 --- a/v3/src/components/axis/hooks/use-axis.ts +++ b/v3/src/components/axis/hooks/use-axis.ts @@ -1,7 +1,8 @@ import {ScaleBand, ScaleLinear, scaleLinear, scaleOrdinal} from "d3" -import {autorun, reaction} from "mobx" +import {reaction} from "mobx" import {isAlive} from "mobx-state-tree" import {useCallback, useEffect, useRef} from "react" +import {MobXAutorun} from "../../../utilities/mobx-autorun" import {AxisPlace, axisGap} from "../axis-types" import {useAxisLayoutContext} from "../models/axis-layout-context" import {IAxisModel, isNumericAxisModel} from "../models/axis-model" @@ -101,16 +102,16 @@ export const useAxis = ({ axisPlace, axisTitle = "", centerCategoryLabels }: IUs // update d3 scale and axis when axis domain changes useEffect(function installDomainSync() { if (isNumeric) { - const disposer = autorun(() => { - const _axisModel = axisProvider.getNumericAxis?.(axisPlace) + const mobXAutorun = new MobXAutorun(() => { + const _axisModel = axisProvider?.getNumericAxis?.(axisPlace) if (_axisModel && !isAlive(_axisModel)) { console.warn("useAxis.installDomainSync skipping sync of defunct axis model") return } _axisModel?.domain && multiScale?.setNumericDomain(_axisModel?.domain) layout.setDesiredExtent(axisPlace, computeDesiredExtent()) - }, { name: "useAxis.installDomainSync" }) - return () => disposer() + }, { name: "useAxis.installDomainSync" }, axisProvider) + return () => mobXAutorun.dispose() } // Note axisModelChanged as a dependent. Shouldn't be necessary. }, [axisModelChanged, isNumeric, multiScale, axisPlace, layout, computeDesiredExtent, axisProvider]) diff --git a/v3/src/components/axis/hooks/use-sub-axis.ts b/v3/src/components/axis/hooks/use-sub-axis.ts index 3e33258f14..adacbde068 100644 --- a/v3/src/components/axis/hooks/use-sub-axis.ts +++ b/v3/src/components/axis/hooks/use-sub-axis.ts @@ -1,6 +1,5 @@ import {BaseType, drag, format, ScaleLinear, select, Selection} from "d3" -import {autorun, reaction} from "mobx" -import {isAlive} from "mobx-state-tree" +import {reaction} from "mobx" import {MutableRefObject, useCallback, useEffect, useMemo, useRef} from "react" import {transitionDuration} from "../../data-display/data-display-types" import {AxisBounds, AxisPlace, axisPlaceToAxisFn, AxisScaleType, otherPlace} from "../axis-types" @@ -8,6 +7,8 @@ import {useAxisLayoutContext} from "../models/axis-layout-context" import {isCategoricalAxisModel, isNumericAxisModel} from "../models/axis-model" import {isVertical} from "../../axis-graph-shared" import {between} from "../../../utilities/math-utils" +import {MobXAutorun} from "../../../utilities/mobx-autorun" +import {isAliveSafe} from "../../../utilities/mst-utils" import {kAxisTickLength} from "../../graph/graphing-types" import {DragInfo, collisionExists, computeBestNumberOfTicks, getCategoricalLabelPlacement, getCoordFunctions, IGetCoordFunctionsProps} from "../axis-utils" @@ -55,7 +56,7 @@ export const useSubAxis = ({ renderSubAxis = useCallback(() => { const _axisModel = axisProvider.getAxis?.(axisPlace) - if (_axisModel && !isAlive(_axisModel)) { + if (!isAliveSafe(_axisModel)) { console.warn("useSubAxis.renderSubAxis skipping rendering of defunct axis model") return } @@ -350,9 +351,9 @@ export const useSubAxis = ({ // update d3 scale and axis when axis domain changes useEffect(function installDomainSync() { - const disposer = autorun(() => { - const _axisModel = axisProvider.getAxis?.(axisPlace) - if (_axisModel && isAlive(_axisModel)) { + const mobXAutorun = new MobXAutorun(() => { + const _axisModel = axisProvider?.getAxis?.(axisPlace) + if (isAliveSafe(_axisModel)) { if (isNumericAxisModel(_axisModel)) { const {domain} = _axisModel || {} layout.getAxisMultiScale(axisPlace)?.setNumericDomain(domain) @@ -362,8 +363,8 @@ export const useSubAxis = ({ else if (_axisModel) { console.warn("useSubAxis.installDomainSync skipping sync of defunct axis model") } - }, { name: "useSubAxis.installDomainSync" }) - return () => disposer() + }, { name: "useSubAxis.installDomainSync" }, axisProvider) + return () => mobXAutorun.dispose() }, [axisPlace, axisProvider, layout, renderSubAxis]) // Refresh when category set, if any, changes diff --git a/v3/src/components/slider/slider-component.tsx b/v3/src/components/slider/slider-component.tsx index e0460259c5..db08df69ae 100644 --- a/v3/src/components/slider/slider-component.tsx +++ b/v3/src/components/slider/slider-component.tsx @@ -1,7 +1,7 @@ -import React, { CSSProperties, useEffect, useMemo, useRef, useState } from "react" -import { useResizeDetector } from "react-resize-detector" import { Flex, Editable, EditablePreview, EditableInput, Button } from "@chakra-ui/react" import { observer } from "mobx-react-lite" +import React, { CSSProperties, useEffect, useMemo, useRef, useState } from "react" +import { useResizeDetector } from "react-resize-detector" import PlayIcon from "../../assets/icons/icon-play.svg" import PauseIcon from "../../assets/icons/icon-pause.svg" import { SliderAxisLayout } from "./slider-layout" @@ -11,6 +11,7 @@ import { Axis } from "../axis/components/axis" import { AxisProviderContext } from "../axis/hooks/use-axis-provider-context" import { AxisLayoutContext } from "../axis/models/axis-layout-context" import { InstanceIdContext, useNextInstanceId } from "../../hooks/use-instance-id-context" +import { isAliveSafe } from "../../utilities/mst-utils" import { ITileBaseProps } from "../tiles/tile-base-props" import { CodapSliderThumb } from "./slider-thumb" import { EditableSliderValue } from "./editable-slider-value" @@ -20,7 +21,7 @@ import './slider.scss' const kAxisMargin = 30 export const SliderComponent = observer(function SliderComponent({ tile } : ITileBaseProps) { - const sliderModel = isSliderModel(tile?.content) ? tile?.content : undefined + const sliderModel = isAliveSafe(tile?.content) && isSliderModel(tile?.content) ? tile?.content : undefined const instanceId = useNextInstanceId("slider") const layout = useMemo(() => new SliderAxisLayout(), []) const {width, height, ref: sliderRef} = useResizeDetector() @@ -28,11 +29,6 @@ export const SliderComponent = observer(function SliderComponent({ tile } : ITil const animationRef = useRef(false) const multiScale = layout.getAxisMultiScale("bottom") - const axisProvider = useMemo(() => ({ - getAxis: () => sliderModel?.axis, - getNumericAxis: () => sliderModel?.axis - }), [sliderModel?.axis]) - // width and positioning useEffect(() => { if ((width != null) && (height != null)) { @@ -53,7 +49,7 @@ export const SliderComponent = observer(function SliderComponent({ tile } : ITil appRef.current = document.querySelector(".app") }, []) - if (!isSliderModel(sliderModel)) return null + if (!sliderModel) return null const handleSliderNameInput = (name: string) => { sliderModel.setName(name) @@ -61,7 +57,7 @@ export const SliderComponent = observer(function SliderComponent({ tile } : ITil return ( - +
diff --git a/v3/src/components/slider/slider-model.ts b/v3/src/components/slider/slider-model.ts index 8f107fb585..3f73c289c3 100644 --- a/v3/src/components/slider/slider-model.ts +++ b/v3/src/components/slider/slider-model.ts @@ -1,6 +1,6 @@ import { reaction } from "mobx" import { addDisposer, Instance, types} from "mobx-state-tree" -import { NumericAxisModel } from "../axis/models/axis-model" +import { INumericAxisModel, NumericAxisModel } from "../axis/models/axis-model" import { GlobalValue } from "../../models/global/global-value" import { IGlobalValueManager, kGlobalValueManagerType } from "../../models/global/global-value-manager" import { applyUndoableAction } from "../../models/history/apply-undoable-action" @@ -32,6 +32,12 @@ export const SliderModel = TileContentModel get value() { return self.globalValue.value }, + getAxis(): INumericAxisModel { + return self.axis + }, + getNumericAxis(): INumericAxisModel { + return self.axis + }, get domain() { return self.axis.domain }, @@ -118,12 +124,9 @@ export const SliderModel = TileContentModel }, { name: "SliderModel [sharedModelManager]", fireImmediately: true } )) }, - beforeDestroy() { - // destroying the slider component removes the underlying global value - // unfortunately, removing the global value here invalidates the reference and leads to MST warnings. - // TODO: figure out a mechanism to remove the slider model and its global value safely - // for now, the global value stays in the registry when the slider model is removed - // self.globalValue && self.globalValueManager?.removeValue(self.globalValue) + destroyGlobalValue() { + // the underlying global value should be removed when the slider model is destroyed + self.globalValue && self.globalValueManager?.removeValue(self.globalValue) }, updateAfterSharedModelChanges(sharedModel?: ISharedModel) { // nothing to do diff --git a/v3/src/components/slider/slider-thumb.tsx b/v3/src/components/slider/slider-thumb.tsx index 4ae2dafb16..56fd02bb1b 100644 --- a/v3/src/components/slider/slider-thumb.tsx +++ b/v3/src/components/slider/slider-thumb.tsx @@ -2,6 +2,7 @@ import { clsx } from "clsx" import { observer } from "mobx-react-lite" import React, {CSSProperties, useEffect, useState, useRef} from "react" import { ISliderModel } from "./slider-model" +import { isAliveSafe } from "../../utilities/mst-utils" import { useAxisLayoutContext } from "../axis/models/axis-layout-context" import { useSliderAnimation } from "./use-slider-animation" import ThumbIcon from "../../assets/icons/icon-thumb.svg" @@ -18,20 +19,22 @@ interface IProps { // offset from left edge of thumb to center of thumb const kThumbOffset = 10 -export const CodapSliderThumb = observer(function CodapSliderThumb({sliderContainer, sliderModel, - running, setRunning} : IProps) { +export const CodapSliderThumb = observer(function CodapSliderThumb({ + sliderContainer, sliderModel: _sliderModel, running, setRunning +} : IProps) { + const sliderModel = isAliveSafe(_sliderModel) ? _sliderModel : undefined const layout = useAxisLayoutContext() const scale = layout.getAxisMultiScale("bottom") const [isDragging, setIsDragging] = useState(false) // offset from center of thumb to pointerDown const downOffset = useRef(0) - const thumbPos = (scale?.getScreenCoordinate({cell: 0, data: sliderModel.value}) ?? 0) - kThumbOffset + const thumbPos = (scale?.getScreenCoordinate({cell: 0, data: sliderModel?.value ?? 0}) ?? 0) - kThumbOffset const thumbStyle: CSSProperties = { left: thumbPos } // forces thumbnail to rerender when axis bounds change - sliderModel.axis.domain // eslint-disable-line no-unused-expressions + sliderModel?.axis.domain // eslint-disable-line no-unused-expressions useEffect(() => { const containerX = sliderContainer?.getBoundingClientRect().x @@ -46,7 +49,7 @@ export const CodapSliderThumb = observer(function CodapSliderThumb({sliderContai const handlePointerMove = (e: PointerEvent) => { const sliderValue = getSliderValueFromEvent(e) if (sliderValue != null) { - sliderModel.setDynamicValue(sliderValue) + sliderModel?.setDynamicValue(sliderValue) } e.preventDefault() e.stopImmediatePropagation() @@ -55,7 +58,7 @@ export const CodapSliderThumb = observer(function CodapSliderThumb({sliderContai const handlePointerUp = (e: PointerEvent) => { const sliderValue = getSliderValueFromEvent(e) if (sliderValue != null) { - sliderModel.applyUndoableAction( + sliderModel?.applyUndoableAction( () => sliderModel.setValue(sliderValue), "DG.Undo.slider.change", "DG.Redo.slider.change") } diff --git a/v3/src/components/slider/slider-title-bar.tsx b/v3/src/components/slider/slider-title-bar.tsx index 530c41c7e6..b50b303b37 100644 --- a/v3/src/components/slider/slider-title-bar.tsx +++ b/v3/src/components/slider/slider-title-bar.tsx @@ -1,14 +1,30 @@ -import React from "react" +import { addDisposer } from "mobx-state-tree" +import React, { useCallback } from "react" import { observer } from "mobx-react-lite" import { ComponentTitleBar } from "../component-title-bar" +import { isAliveSafe } from "../../utilities/mst-utils" import t from "../../utilities/translation/translate" import { ITileTitleBarProps } from "../tiles/tile-base-props" import { isSliderModel } from "./slider-model" -export const SliderTitleBar = observer(function SliderTitleBar({ tile, ...others }: ITileTitleBarProps) { - const sliderModel = isSliderModel(tile?.content) ? tile?.content : undefined - const getTitle = () => tile?.title || sliderModel?.name || t("DG.DocumentController.sliderTitle") +export const SliderTitleBar = observer(function SliderTitleBar({ tile, onCloseTile, ...others }: ITileTitleBarProps) { + const { content } = tile || {} + + const getTitle = useCallback(() => { + const { title } = tile || {} + const sliderModel = isAliveSafe(content) && isSliderModel(content) ? content : undefined + const { name } = sliderModel || {} + return title || name || t("DG.DocumentController.sliderTitle") + }, [content, tile]) + + const handleCloseTile = useCallback((tileId: string) => { + const sliderModel = isAliveSafe(content) && isSliderModel(content) ? content : undefined + // when tile is closed by user, destroy the underlying global value as well + sliderModel && addDisposer(sliderModel, () => sliderModel.destroyGlobalValue()) + onCloseTile?.(tileId) + }, [content, onCloseTile]) + return ( - + ) }) diff --git a/v3/src/components/slider/slider-types.ts b/v3/src/components/slider/slider-types.ts index 8f9fcb867e..6b784f20d1 100644 --- a/v3/src/components/slider/slider-types.ts +++ b/v3/src/components/slider/slider-types.ts @@ -21,4 +21,10 @@ export const kDefaultAnimationMode = "onceOnly" export const kDefaultAnimationRate = 20 // frames/second +export const kAnimationDefaults = { + animationMode: kDefaultAnimationMode, + animationDirection: kDefaultAnimationDirection, + animationRate: kDefaultAnimationRate +} + export type FixValueFn = (value: number) => number diff --git a/v3/src/components/slider/use-slider-animation.tsx b/v3/src/components/slider/use-slider-animation.tsx index 3b91c07bba..97a7fe4b62 100644 --- a/v3/src/components/slider/use-slider-animation.tsx +++ b/v3/src/components/slider/use-slider-animation.tsx @@ -1,27 +1,35 @@ +import { useInterval } from "@chakra-ui/react" import { useCallback, useEffect, useRef } from "react" import { ISliderModel } from "./slider-model" -import { useInterval } from "@chakra-ui/react" +import { kAnimationDefaults } from "./slider-types" interface IUseSliderAnimationProps { - sliderModel: ISliderModel + sliderModel?: ISliderModel running: boolean setRunning: (running: boolean) => void } export const useSliderAnimation = ({sliderModel, running, setRunning}: IUseSliderAnimationProps) => { - const tickTime = 1000/sliderModel.animationRate - const direction = sliderModel.animationDirection - const mode = sliderModel.animationMode + const { animationRate, animationDirection, animationMode } = sliderModel || kAnimationDefaults + const tickTime = 1000 / animationRate + const direction = animationDirection + const mode = animationMode const prevDirectionRef = useRef("") const maxMinHitsRef = useRef(0) + const getAxisDomain = useCallback(function getAxisDomain(): readonly [number, number] { + const { axis: { domain } } = sliderModel || { axis: { domain: [0, 10] } } + return domain + }, [sliderModel]) + const resetSlider = useCallback((val?: number) => { - const dir = sliderModel.animationDirection + if (!sliderModel) return 0 + const [axisMin, axisMax] = getAxisDomain() const testValue = val || sliderModel.value - if (dir === "lowToHigh" && testValue >= sliderModel.axis.max) sliderModel.setValue(sliderModel.axis.min) - if (dir === "highToLow" && testValue <= sliderModel.axis.min) sliderModel.setValue(sliderModel.axis.max) + if (animationDirection === "lowToHigh" && testValue >= axisMax) sliderModel.setValue(axisMin) + if (animationDirection === "highToLow" && testValue <= axisMin) sliderModel.setValue(axisMax) return sliderModel.value - }, [sliderModel]) + }, [animationDirection, getAxisDomain, sliderModel]) useEffect(()=> { running && resetSlider() @@ -30,14 +38,15 @@ export const useSliderAnimation = ({sliderModel, running, setRunning}: IUseSlide // Reset the prevDirectionRef to blank when user changes animation direction. // Otherwise the increment modifier in moveSlider() stays to -1 because the logic check will always return true useEffect(()=> { - if (sliderModel.animationDirection === "lowToHigh" || sliderModel.animationDirection === "highToLow") { + if (animationDirection === "lowToHigh" || animationDirection === "highToLow") { prevDirectionRef.current = "" } - }, [sliderModel.animationDirection]) + }, [animationDirection]) useInterval(() => { - if (running) { - const newValue = moveSlider() + if (running && sliderModel) { + const incrementModifier = direction === 'highToLow' || prevDirectionRef.current === 'highToLow' ? -1 : 1 + const newValue = sliderModel.value + sliderModel.increment * incrementModifier switch (direction) { case "lowToHigh": @@ -53,55 +62,53 @@ export const useSliderAnimation = ({sliderModel, running, setRunning}: IUseSlide } }, tickTime) - const moveSlider = () => { - const incrementModifier = direction === 'highToLow' || prevDirectionRef.current === 'highToLow' ? -1 : 1 - return sliderModel.value + sliderModel.increment * incrementModifier - } - const handleLowToHighAnimation = (newValue: number) => { + const [axisMin, axisMax] = getAxisDomain() const aboveMax = (val: number) => { if (mode === "onceOnly") { setRunning(false) - return sliderModel.axis.max + return axisMax } else { return resetSlider(newValue) } } const belowMin = (val: number) => { - return sliderModel.axis.min + return axisMin } - sliderModel.setValue(sliderModel.validateValue(newValue, belowMin, aboveMax)) + sliderModel?.setValue(sliderModel.validateValue(newValue, belowMin, aboveMax)) } const handleHighToLowAnimation = (newValue: number) => { - const aboveMax = (val: number) => { - return sliderModel.axis.max + const [axisMin, axisMax] = getAxisDomain() + const aboveMax = (val: number) => { + return axisMax + } + const belowMin = (val: number) => { + if (mode === "onceOnly") { + setRunning(false) + return axisMin } - const belowMin = (val: number) => { - if (mode === "onceOnly") { - setRunning(false) - return sliderModel.axis.min - } - else { - return resetSlider(newValue) - } + else { + return resetSlider(newValue) } - sliderModel.setValue(sliderModel.validateValue(newValue, belowMin, aboveMax)) + } + sliderModel?.setValue(sliderModel.validateValue(newValue, belowMin, aboveMax)) } const handleBackAndForthAnimation = (newValue: number) => { - const reachedLimit = (prevDirectionRef.current === 'lowToHigh' && newValue >= sliderModel.axis.max) || - (prevDirectionRef.current === 'highToLow' && newValue <= sliderModel.axis.min) + const [axisMin, axisMax] = getAxisDomain() + const reachedLimit = (prevDirectionRef.current === 'lowToHigh' && newValue >= axisMax) || + (prevDirectionRef.current === 'highToLow' && newValue <= axisMin) const aboveMax = (val: number) => { prevDirectionRef.current = 'highToLow' maxMinHitsRef.current += 1 - return sliderModel.axis.max + return axisMax } const belowMin = (val: number) => { prevDirectionRef.current = 'lowToHigh' maxMinHitsRef.current += 1 - return sliderModel.axis.min + return axisMin } if (prevDirectionRef.current === "") { @@ -113,6 +120,6 @@ export const useSliderAnimation = ({sliderModel, running, setRunning}: IUseSlide setRunning(false) maxMinHitsRef.current = 0 } - sliderModel.setValue(sliderModel.validateValue(newValue, belowMin, aboveMax)) + sliderModel?.setValue(sliderModel.validateValue(newValue, belowMin, aboveMax)) } } diff --git a/v3/src/components/tool-shelf/tool-shelf.tsx b/v3/src/components/tool-shelf/tool-shelf.tsx index d4751b8b38..0c91ecc03d 100644 --- a/v3/src/components/tool-shelf/tool-shelf.tsx +++ b/v3/src/components/tool-shelf/tool-shelf.tsx @@ -105,7 +105,15 @@ export const ToolShelf = observer(function ToolShelf({ document }: IProps) { entries.sort((a, b) => a.shelf.position - b.shelf.position) function handleTileButtonClick(tileType: string) { - document?.content?.createOrShowTile?.(tileType) + const undoRedoStringKeysMap: Record = { + Calculator: ["DG.Undo.toggleComponent.add.calcView", "DG.Redo.toggleComponent.add.calcView"], + CodapSlider: ["DG.Undo.sliderComponent.create", "DG.Redo.sliderComponent.create"], + Graph: ["DG.Undo.graphComponent.create", "DG.Redo.graphComponent.create"] + } + const [undoStringKey = "", redoStringKey = ""] = undoRedoStringKeysMap[tileType] + document?.content?.applyUndoableAction(() => { + document?.content?.createOrShowTile?.(tileType) + }, undoStringKey, redoStringKey) } function handleRightButtonClick(entry: IRightButtonEntry) { diff --git a/v3/src/models/document/document-content.ts b/v3/src/models/document/document-content.ts index 8df69ec6ee..c4a241545e 100644 --- a/v3/src/models/document/document-content.ts +++ b/v3/src/models/document/document-content.ts @@ -12,7 +12,6 @@ import { getPositionOfNewComponent } from "../../utilities/view-utils" import { DataSet, IDataSet, toCanonical } from "../data/data-set" import { gDataBroker } from "../data/data-broker" import { applyUndoableAction } from "../history/apply-undoable-action" -import { withUndoRedoStrings } from "../history/codap-undo-types" import { linkTileToDataSet } from "../shared/shared-data-utils" import t from "../../utilities/translation/translate" @@ -99,16 +98,6 @@ export const DocumentContentModel = BaseDocumentContentModel } else { return self.createTile(tileType) } - - const undoRedoStringKeysMap: Record = { - Calculator: ["DG.Undo.toggleComponent.add.calcView", "DG.Redo.toggleComponent.add.calcView"], - CodapSlider: ["DG.Undo.sliderComponent.create", "DG.Redo.sliderComponent.create"], - Graph: ["DG.Undo.graphComponent.create", "DG.Redo.graphComponent.create"] - } - const undoRedoStringKeys = undoRedoStringKeysMap[tileType] - if (undoRedoStringKeys) { - withUndoRedoStrings(...undoRedoStringKeys) - } } } })) diff --git a/v3/src/utilities/mobx-autorun.ts b/v3/src/utilities/mobx-autorun.ts new file mode 100644 index 0000000000..256b2a943a --- /dev/null +++ b/v3/src/utilities/mobx-autorun.ts @@ -0,0 +1,27 @@ +import { IAutorunOptions, autorun } from "mobx" +import { IAnyStateTreeNode, addDisposer } from "mobx-state-tree" +import { SetRequired } from "type-fest" + +/* + MobXAutorun is a utility class which manages a MobX autorun that should be disposed + when an MST model it is observing is destroyed. This is accomplished by calling + `addDisposer()` on the MST model. The rest of the arguments are identical to the + MobX `autorun` API, except that passing a name is required. + */ +type IAutorunOptionsWithName = SetRequired + +export class MobXAutorun { + private disposer: (() => void) | undefined + + constructor(fn: () => void, options: IAutorunOptionsWithName, model: IAnyStateTreeNode) { + // install autorun + this.disposer = autorun(fn, options) + // dispose of autorun if the model it depends on is destroyed + addDisposer(model, () => this.dispose()) + } + + dispose() { + this.disposer?.() + this.disposer = undefined + } +} diff --git a/v3/src/utilities/mst-utils.ts b/v3/src/utilities/mst-utils.ts index ebf68738c2..d054aa301c 100644 --- a/v3/src/utilities/mst-utils.ts +++ b/v3/src/utilities/mst-utils.ts @@ -31,6 +31,10 @@ export function getParentWithTypeName(target: IAnyStateTreeNode, typeName: strin return undefined } +export function isAliveSafe(target: IAnyStateTreeNode | undefined) { + return !!target && isAlive(target) +} + /** * A short circuit isAlive check. It is intended to be used in observing * components. If the observing component is working with a MST object that