From 95213d73536dfa29eae931672a8c875a22f5ff3e Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Sun, 26 Jul 2026 00:54:23 +0200 Subject: [PATCH] refactor(studio): give resolveTimelineMove a row-based vertical axis Rows stopped sharing one pixel height when lanes gained expansion, so the only production caller was passing cumulative row coordinates with trackHeight 1 and both scrollTops zeroed. The parameter names described units the values no longer carried. The vertical axis is now a row index and the caller keeps ownership of folding scroll and per-row heights into it. --- .../components/timelineClipDragPreview.ts | 10 ++-- .../player/components/timelineEditing.test.ts | 46 ++++++++----------- .../src/player/components/timelineEditing.ts | 15 +++--- 3 files changed, 29 insertions(+), 42 deletions(-) diff --git a/packages/studio/src/player/components/timelineClipDragPreview.ts b/packages/studio/src/player/components/timelineClipDragPreview.ts index e5c054c03..982f42fbf 100644 --- a/packages/studio/src/player/components/timelineClipDragPreview.ts +++ b/packages/studio/src/player/components/timelineClipDragPreview.ts @@ -139,22 +139,18 @@ export function computeDragPreview( ctx.rowHeights, ); const currentRow = getTimelineRowFromY(clientY - scrollRectTop + scrollTop, ctx.rowHeights); - // resolveTimelineMove's vertical axis is expressed in track-height units. - // Feeding cumulative row coordinates with a unit height preserves its existing - // threshold/create-track behavior while supporting variable pixel heights. + // resolveTimelineMove's vertical axis is row indices, which is why the pointer + // and scroll pixels are folded into originRow/currentRow above. const nextMove = resolveTimelineMove( { start: drag.element.start, track: drag.element.track, duration: drag.element.duration, originClientX: drag.originClientX, - originClientY: originRow, + originRow, originScrollLeft: drag.originScrollLeft, - originScrollTop: 0, currentScrollLeft: scroll?.scrollLeft ?? drag.originScrollLeft, - currentScrollTop: 0, pixelsPerSecond: pps, - trackHeight: 1, maxStart: dragMaxStart, trackOrder, }, diff --git a/packages/studio/src/player/components/timelineEditing.test.ts b/packages/studio/src/player/components/timelineEditing.test.ts index f2c2913ea..fc5aa9553 100644 --- a/packages/studio/src/player/components/timelineEditing.test.ts +++ b/packages/studio/src/player/components/timelineEditing.test.ts @@ -27,14 +27,13 @@ describe("resolveTimelineMove", () => { track: 2, duration: 2, originClientX: 100, - originClientY: 200, + originRow: 0, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 8, trackOrder: [0, 1, 2, 3, 4], }, 245, - 200, + 0, ), ).toEqual({ start: 2.7, track: 2 }); }); @@ -47,14 +46,13 @@ describe("resolveTimelineMove", () => { track: 1, duration: 3, originClientX: 200, - originClientY: 200, + originRow: 0, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 10, trackOrder: [0, 1, 5, 9], }, 150, - 390, + 190 / 72, ), ).toEqual({ start: 1.5, track: 9 }); }); @@ -67,14 +65,13 @@ describe("resolveTimelineMove", () => { track: 0, duration: 4, originClientX: 300, - originClientY: 200, + originRow: 0, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 6, trackOrder: [0, 10, 20], }, -100, - -200, + -400 / 72, ), ).toEqual({ start: 0, track: -1 }); @@ -85,14 +82,13 @@ describe("resolveTimelineMove", () => { track: 10, duration: 4, originClientX: 300, - originClientY: 200, + originRow: 0, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 6, trackOrder: [0, 10, 20], }, 500, - 200, + 0, ), ).toEqual({ start: 6, track: 10 }); }); @@ -105,14 +101,13 @@ describe("resolveTimelineMove", () => { track: 0, duration: 2, originClientX: 100, - originClientY: 200, + originRow: 0, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 8, trackOrder: [0, 10, 20], }, 100, - 150, + -50 / 72, ), ).toEqual({ start: 1, track: -1 }); }); @@ -125,19 +120,18 @@ describe("resolveTimelineMove", () => { track: 20, duration: 2, originClientX: 100, - originClientY: 200, + originRow: 0, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 8, trackOrder: [0, 10, 20], }, 100, - 250, + 50 / 72, ), ).toEqual({ start: 1, track: 21 }); }); - it("accounts for scroll displacement while dragging", () => { + it("accounts for horizontal scroll displacement while dragging", () => { expect( resolveTimelineMove( { @@ -145,18 +139,17 @@ describe("resolveTimelineMove", () => { track: 0, duration: 2, originClientX: 100, - originClientY: 200, + originRow: 0, originScrollLeft: 0, - originScrollTop: 0, currentScrollLeft: 100, - currentScrollTop: 144, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 8, + // Vertical scroll never reaches here: the drag preview folds it into the + // row index it passes, because rows no longer share one pixel height. trackOrder: [0, 1, 2, 3], }, 100, - 200, + 2, ), ).toEqual({ start: 2, track: 2 }); }); @@ -195,9 +188,8 @@ describe("resolveTimelineMove", () => { track: 1, duration: 2, originClientX: 0, - originClientY: 0, + originRow: 0, pixelsPerSecond: 100, - trackHeight: 72, maxStart: 8, trackOrder: [0, 1], layerOrder: layers.map((layer) => layer.id), @@ -206,7 +198,7 @@ describe("resolveTimelineMove", () => { stackingElements, }, 0, - -72, + -1, ); expect(result).toEqual({ diff --git a/packages/studio/src/player/components/timelineEditing.ts b/packages/studio/src/player/components/timelineEditing.ts index 9b08a67ce..f183807b0 100644 --- a/packages/studio/src/player/components/timelineEditing.ts +++ b/packages/studio/src/player/components/timelineEditing.ts @@ -46,13 +46,11 @@ export interface TimelineMoveInput { track: number; duration: number; originClientX: number; - originClientY: number; + /** Vertical position as a track-row index, not pixels: rows vary in height. */ + originRow: number; originScrollLeft?: number; - originScrollTop?: number; currentScrollLeft?: number; - currentScrollTop?: number; pixelsPerSecond: number; - trackHeight: number; maxStart: number; trackOrder: number[]; layerOrder?: TimelineLayerId[]; @@ -108,7 +106,7 @@ export function resolveTimelineAutoScroll( export function resolveTimelineMove( input: TimelineMoveInput, clientX: number, - clientY: number, + currentRow: number, ): { start: number; track: number; @@ -117,11 +115,12 @@ export function resolveTimelineMove( stackingReorder?: TimelineStackingReorderIntent | null; } { const scrollDeltaX = (input.currentScrollLeft ?? 0) - (input.originScrollLeft ?? 0); - const scrollDeltaY = (input.currentScrollTop ?? 0) - (input.originScrollTop ?? 0); const deltaTime = (clientX - input.originClientX + scrollDeltaX) / Math.max(input.pixelsPerSecond, 1); - const trackDeltaRaw = - (clientY - input.originClientY + scrollDeltaY) / Math.max(input.trackHeight, 1); + // Rows, so vertical scroll and per-row heights are the caller's problem: rows + // have varied in height since lanes expand, and a single trackHeight can't + // describe them. + const trackDeltaRaw = currentRow - input.originRow; const deltaTrack = Math.round(trackDeltaRaw); const nextStart = clamp( roundToCentiseconds(input.start + deltaTime),