From a37699ae677aaf0ca54d4f520c74beb844b070ce Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 28 Feb 2026 18:08:37 +0000 Subject: [PATCH] Refactor anti-patterns: zoom constant, setRange(), and recordTrackPoint() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3.2 – Duplicate zoom factor constant Add GRAPH_ZOOM_FACTOR = 1.1 to TrackLabConstants.ts and import it in AxisGestureHandler and ZoomGestureHandler, removing the two local `private readonly zoomFactor = 1.1` fields. 3.1 + 4.2 – Shotgun surgery / direct chartTransform mutations Add GraphDataManager.setRange(xRange, yRange) that atomically sets isManuallyZoomed, applies both ranges to chartTransform, and updates tick spacing. All gesture handlers (Axis, Pan, Zoom) now call this single method instead of individually calling setModelXRange/YRange, updateTickSpacing, and setManuallyZoomed(true). The seven scattered setManuallyZoomed(true) call-sites and the duplicate updateTickSpacing calls are removed. 3.3 – Feature envy: DigitizingOverlayNode reaching into sub-models Add SimModel.recordTrackPoint(trackId, pixelPoint) to encapsulate the frame-from-time and model-coordinate computations that previously lived in DigitizingOverlayNode's FireListener. The view now passes raw inputs (track id + pixel position) and the model coordinates all sub-model interactions. DigitizingOverlayNode gains a `recordPoint` callback in its options type; VideoPlayerNode wires it to model.recordTrackPoint(). https://claude.ai/code/session_015RyaayM2oq4T59EQKjycRW --- src/TrackLabConstants.ts | 5 ++++ src/screen-name/graph/AxisGestureHandler.ts | 18 ++++-------- src/screen-name/graph/GraphDataManager.ts | 15 ++++++++++ src/screen-name/graph/PanGestureHandler.ts | 11 ++----- src/screen-name/graph/ZoomGestureHandler.ts | 29 +++++-------------- src/screen-name/model/SimModel.ts | 14 +++++++++ src/screen-name/view/DigitizingOverlayNode.ts | 9 ++---- src/screen-name/view/VideoPlayerNode.ts | 1 + 8 files changed, 54 insertions(+), 48 deletions(-) diff --git a/src/TrackLabConstants.ts b/src/TrackLabConstants.ts index 8294c04..1e958d4 100644 --- a/src/TrackLabConstants.ts +++ b/src/TrackLabConstants.ts @@ -97,3 +97,8 @@ export const CONTROL_ICON_SIZE = 20; export const CONTROL_PANEL_ROWS_SPACING = 12; export const CONTROL_PANEL_X_MARGIN = 12; export const CONTROL_PANEL_Y_MARGIN = 12; + +// ── Graph zoom ───────────────────────────────────────────────────────────── +// Multiplicative zoom step applied by wheel ticks and the zoom-in/zoom-out +// buttons on the configurable graph. A value of 1.1 zooms by 10% per step. +export const GRAPH_ZOOM_FACTOR = 1.1; diff --git a/src/screen-name/graph/AxisGestureHandler.ts b/src/screen-name/graph/AxisGestureHandler.ts index 50541ef..08bb094 100644 --- a/src/screen-name/graph/AxisGestureHandler.ts +++ b/src/screen-name/graph/AxisGestureHandler.ts @@ -17,6 +17,7 @@ import type { ChartRectangle, ChartTransform } from "scenerystack/bamboo"; import { Range, Vector2 } from "scenerystack/dot"; import { DragListener, type Pointer, type Rectangle } from "scenerystack/scenery"; +import { GRAPH_ZOOM_FACTOR } from "../../TrackLabConstants.js"; import trackLab from "../../TrackLabNamespace.js"; import type GraphDataManager from "./GraphDataManager.js"; import type { ChartConfig, GraphDimensions } from "./GraphInteractionHandler.js"; @@ -39,8 +40,6 @@ export default class AxisGestureHandler { private graphWidth: number; private graphHeight: number; - private readonly zoomFactor: number = 1.1; - public constructor(chartConfig: ChartConfig, regions: AxisInteractionRegions, dimensions: GraphDimensions) { this.chartTransform = chartConfig.chartTransform; this.chartRectangle = chartConfig.chartRectangle; @@ -77,14 +76,13 @@ export default class AxisGestureHandler { // Read the current model range for this axis. const getRange = (): Range => (isX ? this.chartTransform.modelXRange : this.chartTransform.modelYRange); - // Apply a new range for this axis and update tick spacing. + // Apply a new range for this axis via the data manager (sets isManuallyZoomed, + // updates chartTransform, and updates tick spacing atomically). const setRange = (range: Range): void => { if (isX) { - this.chartTransform.setModelXRange(range); - this.dataManager.updateTickSpacing(range, this.chartTransform.modelYRange); + this.dataManager.setRange(range, this.chartTransform.modelYRange); } else { - this.chartTransform.setModelYRange(range); - this.dataManager.updateTickSpacing(this.chartTransform.modelXRange, range); + this.dataManager.setRange(this.chartTransform.modelXRange, range); } }; @@ -125,7 +123,6 @@ export default class AxisGestureHandler { if (activePointers.size === 1) { singleTouchStart = coord(pt); initialRange = getRange().copy(); - this.dataManager.setManuallyZoomed(true); } else if (activePointers.size === 2) { const points = Array.from(activePointers.values()); const p0 = points[0]; @@ -135,7 +132,6 @@ export default class AxisGestureHandler { initialPinchMidpoint = (coord(p0) + coord(p1)) / 2; initialRange = getRange().copy(); singleTouchStart = null; - this.dataManager.setManuallyZoomed(true); } } }, @@ -229,7 +225,6 @@ export default class AxisGestureHandler { start: (event) => { mouseDragStart = coord(event.pointer.point); mouseDragInitialRange = getRange().copy(); - this.dataManager.setManuallyZoomed(true); }, drag: (event) => { @@ -278,7 +273,7 @@ export default class AxisGestureHandler { const modelCenter = isX ? modelPos.x : modelPos.y; const currentRange = getRange(); - const zoomFactor = delta < 0 ? this.zoomFactor : 1 / this.zoomFactor; + const zoomFactor = delta < 0 ? GRAPH_ZOOM_FACTOR : 1 / GRAPH_ZOOM_FACTOR; setRange( new Range( @@ -286,7 +281,6 @@ export default class AxisGestureHandler { modelCenter + (currentRange.max - modelCenter) / zoomFactor, ), ); - this.dataManager.setManuallyZoomed(true); }, }); } diff --git a/src/screen-name/graph/GraphDataManager.ts b/src/screen-name/graph/GraphDataManager.ts index b8bf1c3..fe2fea7 100644 --- a/src/screen-name/graph/GraphDataManager.ts +++ b/src/screen-name/graph/GraphDataManager.ts @@ -319,6 +319,21 @@ export default class GraphDataManager { return Math.max(spacing, rangeLength / 20); } + /** + * Apply a new x/y range to the chart, mark the view as manually zoomed, and + * update tick spacing — all in one atomic call. + * + * All gesture handlers should use this method instead of mutating + * chartTransform directly, so that range validation and the manually-zoomed + * flag are always applied together. + */ + public setRange(xRange: Range, yRange: Range): void { + this.isManuallyZoomed = true; + this.chartTransform.setModelXRange(xRange); + this.chartTransform.setModelYRange(yRange); + this.updateTickSpacing(xRange, yRange); + } + /** * Set the manually zoomed flag (called by interaction handlers) */ diff --git a/src/screen-name/graph/PanGestureHandler.ts b/src/screen-name/graph/PanGestureHandler.ts index 8c98993..7c026a1 100644 --- a/src/screen-name/graph/PanGestureHandler.ts +++ b/src/screen-name/graph/PanGestureHandler.ts @@ -61,9 +61,7 @@ export default class PanGestureHandler { break; } - this.chartTransform.setModelXRange(newXRange); - this.chartTransform.setModelYRange(newYRange); - this.dataManager.updateTickSpacing(newXRange, newYRange); + this.dataManager.setRange(newXRange, newYRange); } // ── Private setup helpers ────────────────────────────────────────────────── @@ -79,9 +77,6 @@ export default class PanGestureHandler { dragStartModelPoint = this.chartTransform.viewToModelPosition(viewPoint); dragStartXRange = this.chartTransform.modelXRange.copy(); dragStartYRange = this.chartTransform.modelYRange.copy(); - - // Suppress auto-scaling while the user is panning - this.dataManager.setManuallyZoomed(true); }, drag: (event) => { @@ -98,9 +93,7 @@ export default class PanGestureHandler { const newXRange = new Range(dragStartXRange.min + deltaX, dragStartXRange.max + deltaX); const newYRange = new Range(dragStartYRange.min + deltaY, dragStartYRange.max + deltaY); - this.chartTransform.setModelXRange(newXRange); - this.chartTransform.setModelYRange(newYRange); - this.dataManager.updateTickSpacing(newXRange, newYRange); + this.dataManager.setRange(newXRange, newYRange); }, end: () => { diff --git a/src/screen-name/graph/ZoomGestureHandler.ts b/src/screen-name/graph/ZoomGestureHandler.ts index 20c41ca..996452c 100644 --- a/src/screen-name/graph/ZoomGestureHandler.ts +++ b/src/screen-name/graph/ZoomGestureHandler.ts @@ -9,6 +9,7 @@ import type { ChartRectangle, ChartTransform } from "scenerystack/bamboo"; import { Range, Vector2 } from "scenerystack/dot"; import type { Pointer } from "scenerystack/scenery"; +import { GRAPH_ZOOM_FACTOR } from "../../TrackLabConstants.js"; import trackLab from "../../TrackLabNamespace.js"; import type GraphDataManager from "./GraphDataManager.js"; import type { ChartConfig, GraphDimensions } from "./GraphInteractionHandler.js"; @@ -17,7 +18,6 @@ export default class ZoomGestureHandler { private readonly chartTransform: ChartTransform; private readonly chartRectangle: ChartRectangle; private readonly dataManager: GraphDataManager; - private readonly zoomFactor: number = 1.1; // 10% zoom per wheel tick private graphWidth: number; private graphHeight: number; @@ -58,7 +58,7 @@ export default class ZoomGestureHandler { */ public zoomIn(): void { const center = new Vector2(this.graphWidth / 2, this.graphHeight / 2); - this.zoom(this.zoomFactor, center, true); + this.zoom(GRAPH_ZOOM_FACTOR, center); } /** @@ -66,7 +66,7 @@ export default class ZoomGestureHandler { */ public zoomOut(): void { const center = new Vector2(this.graphWidth / 2, this.graphHeight / 2); - this.zoom(1 / this.zoomFactor, center, true); + this.zoom(1 / GRAPH_ZOOM_FACTOR, center); } // ── Private setup helpers ────────────────────────────────────────────────── @@ -79,9 +79,9 @@ export default class ZoomGestureHandler { const pointerPoint = this.chartRectangle.globalToLocalPoint(event.pointer.point); if (delta < 0) { - this.zoom(this.zoomFactor, pointerPoint); + this.zoom(GRAPH_ZOOM_FACTOR, pointerPoint); } else { - this.zoom(1 / this.zoomFactor, pointerPoint); + this.zoom(1 / GRAPH_ZOOM_FACTOR, pointerPoint); } }, }); @@ -122,7 +122,6 @@ export default class ZoomGestureHandler { initialMidpoint = point0.average(point1); initialXRange = this.chartTransform.modelXRange.copy(); initialYRange = this.chartTransform.modelYRange.copy(); - this.dataManager.setManuallyZoomed(true); } } }, @@ -152,9 +151,7 @@ export default class ZoomGestureHandler { const yMin = initialModelCenter.y - (initialModelCenter.y - initialYRange.min) * zoomFactor; const yMax = initialModelCenter.y + (initialYRange.max - initialModelCenter.y) * zoomFactor; - this.chartTransform.setModelXRange(new Range(xMin, xMax)); - this.chartTransform.setModelYRange(new Range(yMin, yMax)); - this.dataManager.updateTickSpacing(this.chartTransform.modelXRange, this.chartTransform.modelYRange); + this.dataManager.setRange(new Range(xMin, xMax), new Range(yMin, yMax)); } }, @@ -193,13 +190,8 @@ export default class ZoomGestureHandler { * * @param factor - >1 zooms in, <1 zooms out * @param centerPoint - Zoom anchor in local view coordinates - * @param setManualFlag - When true, suppresses subsequent auto-rescaling */ - public zoom(factor: number, centerPoint: Vector2, setManualFlag: boolean = true): void { - if (setManualFlag) { - this.dataManager.setManuallyZoomed(true); - } - + public zoom(factor: number, centerPoint: Vector2): void { const currentXRange = this.chartTransform.modelXRange; const currentYRange = this.chartTransform.modelYRange; const modelCenter = this.chartTransform.viewToModelPosition(centerPoint); @@ -209,12 +201,7 @@ export default class ZoomGestureHandler { const yMin = modelCenter.y - (modelCenter.y - currentYRange.min) / factor; const yMax = modelCenter.y + (currentYRange.max - modelCenter.y) / factor; - const newXRange = new Range(xMin, xMax); - const newYRange = new Range(yMin, yMax); - - this.chartTransform.setModelXRange(newXRange); - this.chartTransform.setModelYRange(newYRange); - this.dataManager.updateTickSpacing(newXRange, newYRange); + this.dataManager.setRange(new Range(xMin, xMax), new Range(yMin, yMax)); } private resetZoom(): void { diff --git a/src/screen-name/model/SimModel.ts b/src/screen-name/model/SimModel.ts index 89ee080..3594d4b 100644 --- a/src/screen-name/model/SimModel.ts +++ b/src/screen-name/model/SimModel.ts @@ -56,6 +56,20 @@ export class SimModel { return this.overlayTools.modelViewTransformProperty.value.inversePosition2(pixelPoint); } + /** + * Record a digitized point on the given track at the current playback position. + * + * The view passes raw inputs (track id + pixel position); this method + * coordinates all sub-model interactions so the view does not need to reach + * into playback or overlayTools directly. + */ + public recordTrackPoint(trackId: string, pixelPoint: Vector2): void { + const time = this.playback.currentTimeProperty.value; + const frame = Math.round(time * this.playback.frameRateProperty.value); + const modelPt = this.overlayTools.modelViewTransformProperty.value.inversePosition2(pixelPoint); + this.tracking.addPointToTrack(trackId, frame, time, modelPt.x, modelPt.y); + } + // ── Video source activation ───────────────────────────────────────────── // Each method sets all affected sub-model properties atomically so that no // intermediate state is visible to subscribers. diff --git a/src/screen-name/view/DigitizingOverlayNode.ts b/src/screen-name/view/DigitizingOverlayNode.ts index 1c5462d..b24f897 100644 --- a/src/screen-name/view/DigitizingOverlayNode.ts +++ b/src/screen-name/view/DigitizingOverlayNode.ts @@ -23,6 +23,8 @@ type DigitizingOverlayNodeOptions = { playback: VideoPlaybackModel; magnifyVideoProperty: TReadOnlyProperty; modelViewTransformProperty: TReadOnlyProperty; + /** Record a digitized point on the given track at the given pixel position. */ + recordPoint: (trackId: string, pixelPoint: Vector2) => void; }; const OUTER_R = 20; @@ -390,12 +392,7 @@ export class DigitizingOverlayNode extends Node { const rawLocalPt = digitizingOverlay.globalToLocalPoint(event.pointer.point); const localPt = event.pointer.type === "touch" ? rawLocalPt.plusXY(0, -TOUCH_CURSOR_OFFSET_Y) : rawLocalPt; - const time = playback.currentTimeProperty.value; - const frame = Math.round(time * playback.frameRateProperty.value); - - const modelPt = modelViewTransformProperty.value.inversePosition2(localPt); - - tracking.addPointToTrack(activeId, frame, time, modelPt.x, modelPt.y); + options.recordPoint(activeId, localPt); onPointAdded(); }, tandem: Tandem.OPT_OUT, diff --git a/src/screen-name/view/VideoPlayerNode.ts b/src/screen-name/view/VideoPlayerNode.ts index 8987af1..861cd97 100644 --- a/src/screen-name/view/VideoPlayerNode.ts +++ b/src/screen-name/view/VideoPlayerNode.ts @@ -144,6 +144,7 @@ export class VideoPlayerNode extends Node { playback: model.playback, magnifyVideoProperty: model.overlayTools.magnifyVideoProperty, modelViewTransformProperty: model.overlayTools.modelViewTransformProperty, + recordPoint: (trackId, pixelPoint) => model.recordTrackPoint(trackId, pixelPoint), }, () => this.stepForward(), );