From 975108dfcf28c63180d727d2da61ecf87d536bac Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 27 Feb 2026 11:09:50 +0000 Subject: [PATCH] Apply class design suggestions from review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SimModel: remove prevModelViewTransform private field; lazyLink already provides the previous value as its second argument, so the manual cache was redundant and mutation-prone. - DigitizingAwareOverlayNode: extract the shared "hide until video loaded + dim/lock during digitizing" pattern into an abstract base class. CoordinateSystemNode and CalibrationToolNode both extend it, removing ~25 lines of identical listener/dispose boilerplate from each. - isWebcamVideoProperty → isUserVideoProperty: the old name was misleading because the flag is also true for uploaded files (not just webcam streams). The new name accurately reflects "user-provided vs bundled sample". - DataTableNode: move the pan-drag listener inside the node itself. SimScreenView is a layout file and should not own interaction logic that belongs to the node being dragged. https://claude.ai/code/session_016hiUhJvruvQ8463TWCPYdf --- src/screen-name/model/SimModel.ts | 19 +++----- src/screen-name/model/VideoSourceModel.ts | 8 ++-- src/screen-name/view/CalibrationToolNode.ts | 26 ++-------- src/screen-name/view/CoordinateSystemNode.ts | 25 ++-------- src/screen-name/view/DataTableNode.ts | 27 ++++++++++- .../view/DigitizingAwareOverlayNode.ts | 48 +++++++++++++++++++ src/screen-name/view/SimScreenView.ts | 29 +---------- .../view/VideoSourceControlNode.ts | 2 +- 8 files changed, 93 insertions(+), 91 deletions(-) create mode 100644 src/screen-name/view/DigitizingAwareOverlayNode.ts diff --git a/src/screen-name/model/SimModel.ts b/src/screen-name/model/SimModel.ts index 46a9d88..4bd2b56 100644 --- a/src/screen-name/model/SimModel.ts +++ b/src/screen-name/model/SimModel.ts @@ -14,7 +14,7 @@ * - re-expressing track points when the model-view transform changes */ -import type { Transform3, Vector2 } from "scenerystack/dot"; +import type { Vector2 } from "scenerystack/dot"; import trackLab from "../../TrackLabNamespace.js"; import { OverlayToolsModel } from "./OverlayToolsModel.js"; import { TrackingModel } from "./TrackingModel.js"; @@ -38,18 +38,12 @@ export class SimModel { public readonly sources = new VideoSourceModel(); public readonly tracking = new TrackingModel(); - // Cache the previous MVT so we can compute retransforms when it changes. - private prevModelViewTransform: Transform3 | null = null; - public constructor() { // Whenever the model-view transform changes, re-express all stored track // points in the new coordinate system so they remain visually anchored to // the same pixel on the video. - this.overlayTools.modelViewTransformProperty.lazyLink((newMvt) => { - if (this.prevModelViewTransform !== null) { - this.tracking.retransformTrackPoints(this.prevModelViewTransform, newMvt); - } - this.prevModelViewTransform = newMvt; + this.overlayTools.modelViewTransformProperty.lazyLink((newMvt, oldMvt) => { + this.tracking.retransformTrackPoints(oldMvt, newMvt); }); } @@ -67,7 +61,7 @@ export class SimModel { /** Activate a webcam recording as the current video source. */ public activateRecording(recording: WebcamRecording): void { - this.sources.isWebcamVideoProperty.value = true; + this.sources.isUserVideoProperty.value = true; this.playback.frameRateProperty.value = recording.fps; this.playback.totalFrameCountProperty.value = 0; this.sources.currentWebcamBlobProperty.value = recording.blob; @@ -75,7 +69,7 @@ export class SimModel { /** Activate an uploaded video as the current video source. */ public activateUpload(upload: UploadedVideo): void { - this.sources.isWebcamVideoProperty.value = true; + this.sources.isUserVideoProperty.value = true; this.playback.frameRateProperty.value = upload.fps; this.playback.totalFrameCountProperty.value = upload.frameCount ?? 0; this.sources.currentWebcamBlobProperty.value = upload.blob; @@ -83,14 +77,13 @@ export class SimModel { /** Activate a bundled (sample) video as the current video source. */ public activateBundledVideo(frameCount: number, fps: number): void { - this.sources.isWebcamVideoProperty.value = false; + this.sources.isUserVideoProperty.value = false; this.sources.currentWebcamBlobProperty.value = null; this.playback.totalFrameCountProperty.value = frameCount; this.playback.frameRateProperty.value = fps; } public reset(): void { - this.prevModelViewTransform = null; this.playback.reset(); this.sources.reset(); this.tracking.reset(); diff --git a/src/screen-name/model/VideoSourceModel.ts b/src/screen-name/model/VideoSourceModel.ts index f517a71..bb187af 100644 --- a/src/screen-name/model/VideoSourceModel.ts +++ b/src/screen-name/model/VideoSourceModel.ts @@ -40,9 +40,9 @@ export type UploadedVideo = { * the flag that indicates whether the current video is user-provided. */ export class VideoSourceModel { - // Track whether the current video is from webcam/upload (enables FPS editing - // and shows the download button). - public readonly isWebcamVideoProperty = new BooleanProperty(false); + // True when the active video is user-provided (webcam recording or upload); + // false for bundled sample videos. Controls FPS editing and download button. + public readonly isUserVideoProperty = new BooleanProperty(false); // ── Webcam recordings storage ────────────────────────────────────────── public readonly webcamRecordingsProperty = new Property([]); @@ -92,7 +92,7 @@ export class VideoSourceModel { } public reset(): void { - this.isWebcamVideoProperty.reset(); + this.isUserVideoProperty.reset(); this.webcamRecordingsProperty.value = []; this.currentWebcamBlobProperty.value = null; this.nextRecordingNumber = 1; diff --git a/src/screen-name/view/CalibrationToolNode.ts b/src/screen-name/view/CalibrationToolNode.ts index e7163af..e0be1b1 100644 --- a/src/screen-name/view/CalibrationToolNode.ts +++ b/src/screen-name/view/CalibrationToolNode.ts @@ -8,7 +8,7 @@ import type { TReadOnlyProperty } from "scenerystack/axon"; import { DerivedProperty, Multilink } from "scenerystack/axon"; import { Shape } from "scenerystack/kite"; -import { Circle, HBox, Line, Node, RichDragListener, Text } from "scenerystack/scenery"; +import { Circle, HBox, Line, type Node, RichDragListener, Text } from "scenerystack/scenery"; import { Keypad, PhetFont } from "scenerystack/scenery-phet"; import { KeypadDialog } from "scenerystack/sim"; import { ButtonNode, ComboBox, type ComboBoxItem, Panel, TextPushButton } from "scenerystack/sun"; @@ -18,12 +18,12 @@ import TrackLabColors from "../../TrackLabColors.js"; import { BUTTON_X_MARGIN, BUTTON_Y_MARGIN, - DIGITIZING_DIM_OPACITY, OVERLAY_DRAG_SPEED, OVERLAY_SHIFT_DRAG_SPEED, } from "../../TrackLabConstants.js"; import trackLab from "../../TrackLabNamespace.js"; import { CALIBRATION_UNITS, type OverlayToolsModel } from "../model/OverlayToolsModel.js"; +import { DigitizingAwareOverlayNode } from "./DigitizingAwareOverlayNode.js"; const FONT = new PhetFont(14); const WARNING_FONT = new PhetFont({ size: 11, weight: "bold" }); @@ -55,7 +55,7 @@ const CALIBRATION_DECIMAL_PLACES = 2; * are too close together to produce a valid calibration. Hidden until a video * is loaded. */ -export class CalibrationToolNode extends Node { +export class CalibrationToolNode extends DigitizingAwareOverlayNode { private readonly disposeCalibrationToolNode: () => void; /** @@ -70,7 +70,7 @@ export class CalibrationToolNode extends Node { overlayTools: OverlayToolsModel, activeTrackIdProperty: TReadOnlyProperty, ) { - super(); + super(videoLoadedProperty, activeTrackIdProperty); const calibrationStrings = StringManager.getInstance().getCalibration(); @@ -280,26 +280,8 @@ export class CalibrationToolNode extends Node { }), ); - // ── Visibility ───────────────────────────────────────────────────────── - const onVideoLoaded = (loaded: boolean) => { - this.visible = loaded; - }; - videoLoadedProperty.link(onVideoLoaded); - - // ── Lock out interaction while the user is manually digitizing ───────── - // Dimming + pickable:false signals that the tool is temporarily inactive - // so the user cannot accidentally move calibration points mid-session. - const onActiveTrackChange = (activeId: string | null) => { - const isDigitizing = activeId !== null; - this.pickable = !isDigitizing; - this.opacity = isDigitizing ? DIGITIZING_DIM_OPACITY : 1; - }; - activeTrackIdProperty.link(onActiveTrackChange); - this.disposeCalibrationToolNode = () => { calibMultilink.dispose(); - videoLoadedProperty.unlink(onVideoLoaded); - activeTrackIdProperty.unlink(onActiveTrackChange); rangePatternProperty.dispose(); buttonLabelProperty.dispose(); }; diff --git a/src/screen-name/view/CoordinateSystemNode.ts b/src/screen-name/view/CoordinateSystemNode.ts index 066839a..79e6625 100644 --- a/src/screen-name/view/CoordinateSystemNode.ts +++ b/src/screen-name/view/CoordinateSystemNode.ts @@ -13,9 +13,9 @@ import { ArrowNode, PhetFont } from "scenerystack/scenery-phet"; import { Tandem } from "scenerystack/tandem"; import { StringManager } from "../../i18n/StringManager.js"; import TrackLabColors from "../../TrackLabColors.js"; -import { DIGITIZING_DIM_OPACITY } from "../../TrackLabConstants.js"; import trackLab from "../../TrackLabNamespace.js"; import type { OverlayToolsModel } from "../model/OverlayToolsModel.js"; +import { DigitizingAwareOverlayNode } from "./DigitizingAwareOverlayNode.js"; const ARROW_LENGTH = 120; const HANDLE_FRACTION = 1 / 3; @@ -56,7 +56,7 @@ const DEG_TO_RAD = Math.PI / 180; * model-view transform in sync. Keyboard drag is supported via RichDragListener. * Hidden until a video is loaded. */ -export class CoordinateSystemNode extends Node { +export class CoordinateSystemNode extends DigitizingAwareOverlayNode { private readonly disposeCoordinateSystemNode: () => void; /** @@ -69,7 +69,7 @@ export class CoordinateSystemNode extends Node { overlayTools: OverlayToolsModel, activeTrackIdProperty: TReadOnlyProperty, ) { - super(); + super(videoLoadedProperty, activeTrackIdProperty); const coordStrings = StringManager.getInstance().getCoordSystem(); @@ -255,28 +255,9 @@ export class CoordinateSystemNode extends Node { }), ); - // ── Visibility: only shown once a video with a finite duration is loaded - const onVideoLoaded = (loaded: boolean) => { - this.visible = loaded; - }; - videoLoadedProperty.link(onVideoLoaded); - - // ── Lock out interaction while the user is manually digitizing ───────── - // Dimming + pickable:false signals that the coordinate system is temporarily - // inactive so the user cannot accidentally move or rotate the axes while - // placing track points. - const onActiveTrackChange = (activeId: string | null) => { - const isDigitizing = activeId !== null; - this.pickable = !isDigitizing; - this.opacity = isDigitizing ? DIGITIZING_DIM_OPACITY : 1; - }; - activeTrackIdProperty.link(onActiveTrackChange); - this.disposeCoordinateSystemNode = () => { overlayTools.coordOriginProperty.unlink(onOriginChange); overlayTools.coordAngleProperty.unlink(onAngleChange); - videoLoadedProperty.unlink(onVideoLoaded); - activeTrackIdProperty.unlink(onActiveTrackChange); }; } diff --git a/src/screen-name/view/DataTableNode.ts b/src/screen-name/view/DataTableNode.ts index ac9e7c9..82cc694 100644 --- a/src/screen-name/view/DataTableNode.ts +++ b/src/screen-name/view/DataTableNode.ts @@ -12,7 +12,7 @@ */ import { BooleanProperty, type TReadOnlyProperty } from "scenerystack/axon"; -import type { Vector2 } from "scenerystack/dot"; +import { Vector2 } from "scenerystack/dot"; import { DOM, DragListener, HBox, Rectangle, Text, VBox } from "scenerystack/scenery"; import { PhetFont } from "scenerystack/scenery-phet"; import { Panel } from "scenerystack/sun"; @@ -681,6 +681,31 @@ export class DataTableNode extends Panel { }; this.isResizingProperty.link(isResizingListener); + // ── Pan drag: lets the user freely reposition the panel ────────────────── + let panStartPosition: Vector2 | null = null; + let panStartPointerPoint: Vector2 | null = null; + this.cursor = "grab"; + this.addInputListener( + new DragListener({ + start: (event) => { + panStartPosition = new Vector2(this.x, this.y); + panStartPointerPoint = event.pointer.point.copy(); + }, + drag: (event) => { + if (!(panStartPosition && panStartPointerPoint)) { + return; + } + const delta = event.pointer.point.minus(panStartPointerPoint); + this.x = panStartPosition.x + delta.x; + this.y = panStartPosition.y + delta.y; + }, + end: () => { + panStartPosition = null; + panStartPointerPoint = null; + }, + }), + ); + // Store cleanup function this.disposeDataTable = () => { resizeObserver.disconnect(); diff --git a/src/screen-name/view/DigitizingAwareOverlayNode.ts b/src/screen-name/view/DigitizingAwareOverlayNode.ts new file mode 100644 index 0000000..aa496a1 --- /dev/null +++ b/src/screen-name/view/DigitizingAwareOverlayNode.ts @@ -0,0 +1,48 @@ +/** + * DigitizingAwareOverlayNode.ts + * + * Abstract base for overlay tool nodes that share two behaviours: + * - Hidden until a video is loaded (videoLoadedProperty drives visibility). + * - Dimmed and non-interactive while the user is manually digitizing a track + * (activeTrackIdProperty non-null → pickable:false + reduced opacity). + * + * CoordinateSystemNode and CalibrationToolNode both use this pattern identically, + * so it is factored here to avoid duplication. + */ + +import type { TReadOnlyProperty } from "scenerystack/axon"; +import { Node } from "scenerystack/scenery"; +import { DIGITIZING_DIM_OPACITY } from "../../TrackLabConstants.js"; + +export abstract class DigitizingAwareOverlayNode extends Node { + private readonly disposeDigitizingAwareOverlayNode: () => void; + + protected constructor( + videoLoadedProperty: TReadOnlyProperty, + activeTrackIdProperty: TReadOnlyProperty, + ) { + super(); + + const onVideoLoaded = (loaded: boolean) => { + this.visible = loaded; + }; + videoLoadedProperty.link(onVideoLoaded); + + const onActiveTrackChange = (activeId: string | null) => { + const isDigitizing = activeId !== null; + this.pickable = !isDigitizing; + this.opacity = isDigitizing ? DIGITIZING_DIM_OPACITY : 1; + }; + activeTrackIdProperty.link(onActiveTrackChange); + + this.disposeDigitizingAwareOverlayNode = () => { + videoLoadedProperty.unlink(onVideoLoaded); + activeTrackIdProperty.unlink(onActiveTrackChange); + }; + } + + public override dispose(): void { + this.disposeDigitizingAwareOverlayNode(); + super.dispose(); + } +} diff --git a/src/screen-name/view/SimScreenView.ts b/src/screen-name/view/SimScreenView.ts index c6562eb..6aa62a8 100644 --- a/src/screen-name/view/SimScreenView.ts +++ b/src/screen-name/view/SimScreenView.ts @@ -6,8 +6,7 @@ */ import { DerivedProperty } from "scenerystack/axon"; -import { Vector2 } from "scenerystack/dot"; -import { DragListener, Node } from "scenerystack/scenery"; +import { Node } from "scenerystack/scenery"; import { InfoButton, ResetAllButton } from "scenerystack/scenery-phet"; import { ScreenView, type ScreenViewOptions } from "scenerystack/sim"; import { Tandem } from "scenerystack/tandem"; @@ -242,32 +241,6 @@ export class SimScreenView extends ScreenView { ], }), ); - - // ── Data table drag ─────────────────────────────────────────────────── - // Lets the user freely reposition the data table panel by dragging it. - let dragStartPosition: Vector2 | null = null; - let dragStartPointerPoint: Vector2 | null = null; - dataTableNode.cursor = "grab"; - dataTableNode.addInputListener( - new DragListener({ - start: (event) => { - dragStartPosition = new Vector2(dataTableNode.x, dataTableNode.y); - dragStartPointerPoint = event.pointer.point.copy(); - }, - drag: (event) => { - if (!(dragStartPosition && dragStartPointerPoint)) { - return; - } - const delta = event.pointer.point.minus(dragStartPointerPoint); - dataTableNode.x = dragStartPosition.x + delta.x; - dataTableNode.y = dragStartPosition.y + delta.y; - }, - end: () => { - dragStartPosition = null; - dragStartPointerPoint = null; - }, - }), - ); } } diff --git a/src/screen-name/view/VideoSourceControlNode.ts b/src/screen-name/view/VideoSourceControlNode.ts index 0f34748..d2a9431 100644 --- a/src/screen-name/view/VideoSourceControlNode.ts +++ b/src/screen-name/view/VideoSourceControlNode.ts @@ -378,7 +378,7 @@ export class VideoSourceControlNode extends HBox { }, }); downloadButton.visible = false; - sources.isWebcamVideoProperty.link((isUserVideo) => { + sources.isUserVideoProperty.link((isUserVideo) => { downloadButton.visible = isUserVideo; });