Skip to content

Commit d393dc1

Browse files
committed
fix(studio): give edge-stretch the gesture contract the other four follow
Seven review findings against this branch, five of which were one defect: edge-stretch was added as a fifth mutually-exclusive gesture on the lane without joining the threshold / live-preview / revert-on-cancel contract the point drag, curve bend, range drag and double-click all obey. Patching them one at a time would have been more code and less coherent, so this makes the stretch structurally parallel to its sibling range drag instead, and extracts it to useAutomationEdgeStretch on the way out — the gestures file had ~60 lines of headroom under the 600-line studio cap, and shaving comments to fit a refactor in is not a plan. - Threshold. A press within the 8px halo of either edge used to persist a no-op commit and push an undo entry that changed nothing (commitDataAttribute has no unchanged-value short-circuit). Worse, it made the pre-existing "click the background to clear the selection" escape unreachable anywhere near an edge. Below 3px of travel — the same threshold the range drag uses — the press now clears the selection and writes nothing at all. - Live preview. moveEdge never fired onRangeSelect and the hook discarded the drag's live position, so the highlight rect and both edge lines stayed pinned at the pre-drag bounds for the whole gesture and snapped into place on release: the user dragged an invisible handle. It now reports bounds on every move, exactly as the marquee drag does and for the same reason. - Revert on cancel. pointercancel means the browser abandoned the gesture; it was routed to the same handler as pointerup, which persisted whatever partial retime it had reached. It now restores the arm-time snapshot through the preview channel — there is nothing persisted to undo — and puts the selection back. A new cancelDrag handler owns that, so a release and an abandonment are no longer the same event. - Lost capture. capturePointer took the capture on e.target, i.e. whichever child the press landed on. A child that unmounts mid-drag takes the capture with it, silently, with no pointercancel — after which edgeDrag stayed non-null and every later button-less pointermove kept retiming and writing. Capture is now taken on the svg, which outlives every gesture on it, and a move reporting no buttons held ends the drag as a cancel. - Hit priority. A breakpoint sitting exactly on the selection's edge used to win the press. Since replaceRange pins an anchor at the union bound and finishEdgeDrag leaves the selection edge at that same time, EVERY range operation — stretch, delete, shape insert — leaves a point exactly on the edge it just created: the second stretch of the same edge resolved to a point-drag, at the one height (on the envelope) where a user naturally grabs it. The feature was not repeatable. An active selection's edge now outranks a point on it; clearing the selection reaches the point again, which is tested. - Clamp order. The dragged edge was bounded against its partner AFTER the 0-floor, so a selection thinner than the minimum width yielded a negative t0, which core's cleanPoint then collapses onto a duplicate t=0 on the serialize round-trip — silent envelope corruption. The floor is now applied last. The minimum width is its own MIN_SELECTION_SEC rather than a borrowed POINT_MERGE_SEC: when two breakpoints are the same breakpoint is a different question from how thin a time selection may get. One finding does not survive: edgeAt's `d0 <= d1` tiebreak was reported as making the t1 edge ungrabbable on a narrow selection, but that comparison IS nearest-wins, and a press right of the midpoint already resolved to t1. The midpoint split here is the same rule written so it is legible rather than inferred, and the test for it is labelled as characterizing behaviour, not fixing it. What was genuinely unreachable inside a narrow halo — starting a fresh range, or clearing the old one without Escape — the threshold above fixes. Also settles what retimeRange does with a breakpoint sitting ON a dragged edge, which was never decided: pointsIn is endpoint-inclusive, so it is interior and travels with the stretch. It has to be, because the commonest stretch of all is grabbing an edge to drag exactly that point outward, and anchoring it would delete it and flatten the span instead. The price is that the retimed point lands on the union's own boundary where a preservation anchor would go, and anchor() stands down within a merge radius — one time cannot hold two values — so the segment leaving the union reshapes. That is the one place replaceRange's outside-never-moves invariant bends, and both halves are now pinned: the exact points and the sampled slope for the on-edge case, and the full two-sided invariant for a selection whose edges are off any breakpoint. The earlier right-side probe at t=5.1 that caught this was deleted during development as inherent; it was reporting the real behaviour.
1 parent 157fbd5 commit d393dc1

8 files changed

Lines changed: 617 additions & 191 deletions

‎packages/studio/src/player/components/TimelineAutomationLane.test.tsx‎

Lines changed: 193 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ function fire(
8585
clientX?: number;
8686
clientY?: number;
8787
button?: number;
88+
/** Buttons still held. A move reporting none is how a lost capture shows up. */
89+
buttons?: number;
8890
altKey?: boolean;
8991
shiftKey?: boolean;
9092
} = {},
@@ -485,6 +487,27 @@ const mount = (automation: HfAutomation, over: Record<string, unknown> = {}) =>
485487
return { container, svg, props };
486488
};
487489

490+
/** `mount`, plus the re-render a real store update causes — the persisted
491+
* automation and the new selection coming back down as props. */
492+
const mountRerenderable = (automation: HfAutomation, over: Record<string, unknown> = {}) => {
493+
const base = laneProps({ automation, ...over });
494+
const props = {
495+
...base,
496+
onPreview: base.onPreview as ReturnType<typeof vi.fn>,
497+
onCommit: base.onCommit as ReturnType<typeof vi.fn>,
498+
};
499+
const { container, rerender } = renderRerenderable(<TimelineAutomationLane {...props} />);
500+
const svg = container.querySelector("svg")!;
501+
stubBox(svg, BOX);
502+
return {
503+
container,
504+
svg,
505+
props,
506+
rerender: (next: Record<string, unknown>) =>
507+
rerender(<TimelineAutomationLane {...props} {...next} />),
508+
};
509+
};
510+
488511
describe("TimelineAutomationLane modifiers", () => {
489512
it("bends a segment when it is Alt-dragged, and leaves the points where they were", () => {
490513
// `curve` was honoured everywhere it is read — drawn, sampled in preview,
@@ -688,10 +711,11 @@ describe("TimelineAutomationLane selection menu", () => {
688711
});
689712

690713
describe("TimelineAutomationLane stretch", () => {
691-
// Edges deliberately off any existing point: the lane's hit-priority rule
692-
// (a point always wins) means a selection edge sitting exactly on a
693-
// breakpoint would resolve to a point-drag, never a stretch — see the
694-
// dedicated priority test below for that case instead.
714+
// Most edges here are off any existing point, which is the EASY case. The
715+
// normal state after any range operation is the opposite — delete, shape
716+
// insert and stretch all leave a breakpoint exactly on the edge they created
717+
// — so the priority test below is the one that decides whether the feature is
718+
// repeatable, not an edge case.
695719

696720
/** Press, drag and release the right edge of a stretchable selection — the
697721
* shape most of this block's tests share, differing only in where the
@@ -732,10 +756,99 @@ describe("TimelineAutomationLane stretch", () => {
732756
expect(points.some((p) => Math.abs(p.t - 1.2) < 0.01 && p.v === 0.5)).toBe(true);
733757
expect(points.some((p) => Math.abs(p.t - 2.6) < 0.01 && p.v === 0.8)).toBe(true);
734758

735-
expect(onRangeSelect).toHaveBeenCalledTimes(1);
736759
expect(onRangeSelect).toHaveBeenLastCalledWith(0.5, expect.closeTo(3.3, 1));
737760
});
738761

762+
it("moves the selection with the pointer instead of snapping it on release", () => {
763+
// The highlight and both edge lines render from the rangeSelection prop, so
764+
// a stretch that only reported its bounds on release dragged an invisible
765+
// handle: the rect stayed pinned at the pre-drag bounds for the whole
766+
// gesture. The marquee drag fires live for exactly this reason.
767+
const onRangeSelect = vi.fn();
768+
const { svg } = mount(stretchable, {
769+
rangeSelection: { t0: 0.5, t1: 2.5 },
770+
onRangeSelect,
771+
});
772+
fire(svg, "pointerdown", at(2.5, 0.5));
773+
fire(svg, "pointermove", at(3, 0.5));
774+
expect(onRangeSelect).toHaveBeenLastCalledWith(0.5, expect.closeTo(3, 1));
775+
fire(svg, "pointermove", at(3.3, 0.5));
776+
expect(onRangeSelect).toHaveBeenLastCalledWith(0.5, expect.closeTo(3.3, 1));
777+
});
778+
779+
it("a bare click on an edge clears the selection instead of committing a no-op", () => {
780+
// Without a movement threshold this pushed an undo entry that changed
781+
// nothing, and — because the halo covers both edges — it also made the
782+
// "click the background to clear" escape unreachable near either one.
783+
const onRangeSelect = vi.fn();
784+
const onRangeClear = vi.fn();
785+
const { svg, props } = mount(stretchable, {
786+
rangeSelection: { t0: 0.5, t1: 2.5 },
787+
onRangeSelect,
788+
onRangeClear,
789+
});
790+
fire(svg, "pointerdown", at(2.5, 0.5));
791+
fire(svg, "pointerup", at(2.5, 0.5));
792+
expect(props.onCommit).not.toHaveBeenCalled();
793+
expect(props.onPreview).not.toHaveBeenCalled();
794+
expect(onRangeSelect).not.toHaveBeenCalled();
795+
expect(onRangeClear).toHaveBeenCalledTimes(1);
796+
});
797+
798+
it("a press that jitters under the threshold still clears rather than retiming", () => {
799+
const onRangeClear = vi.fn();
800+
const { svg, props } = mount(stretchable, {
801+
rangeSelection: { t0: 0.5, t1: 2.5 },
802+
onRangeClear,
803+
});
804+
fire(svg, "pointerdown", at(2.5, 0.5));
805+
// ~2px at 100 px/s: a hand resting on the button, not a drag.
806+
fire(svg, "pointermove", at(2.52, 0.5));
807+
fire(svg, "pointerup", at(2.52, 0.5));
808+
expect(props.onPreview).not.toHaveBeenCalled();
809+
expect(props.onCommit).not.toHaveBeenCalled();
810+
expect(onRangeClear).toHaveBeenCalledTimes(1);
811+
});
812+
813+
it("reverts an interrupted stretch rather than persisting the partial retime", () => {
814+
// pointercancel means the browser abandoned the gesture. Routing it to the
815+
// same handler as pointerup persisted whatever half-drag it had reached,
816+
// with no way back other than undo.
817+
const onRangeSelect = vi.fn();
818+
const { svg, props } = mount(stretchable, {
819+
rangeSelection: { t0: 0.5, t1: 2.5 },
820+
onRangeSelect,
821+
});
822+
fire(svg, "pointerdown", at(2.5, 0.5));
823+
fire(svg, "pointermove", at(3.3, 0.5));
824+
fire(svg, "pointercancel", at(3.3, 0.5));
825+
826+
expect(props.onCommit).not.toHaveBeenCalled();
827+
// The envelope goes back through the preview channel — nothing to undo —
828+
// and the selection returns to the bounds the drag started from.
829+
const reverted = (props.onPreview.mock.calls.at(-1)?.[0] as HfAutomation | undefined)?.lanes[0]
830+
?.points;
831+
expect(reverted).toEqual(stretchable.lanes[0]?.points);
832+
expect(onRangeSelect).toHaveBeenLastCalledWith(0.5, 2.5);
833+
});
834+
835+
it("gives up a stretch whose pointer capture vanished without a cancel", () => {
836+
// A capture taken on a child that unmounts mid-drag is lost silently: no
837+
// pointercancel, no pointerup. Every later hover kept retiming and writing.
838+
const { svg, props } = mount(stretchable, {
839+
rangeSelection: { t0: 0.5, t1: 2.5 },
840+
});
841+
fire(svg, "pointerdown", at(2.5, 0.5));
842+
fire(svg, "pointermove", { ...at(3.3, 0.5), buttons: 1 });
843+
fire(svg, "pointermove", { ...at(3.4, 0.5), buttons: 0 });
844+
const writes = props.onPreview.mock.calls.length;
845+
846+
fire(svg, "pointermove", { ...at(3.8, 0.5), buttons: 0 });
847+
fire(svg, "pointermove", { ...at(1, 0.5), buttons: 0 });
848+
expect(props.onPreview.mock.calls.length).toBe(writes);
849+
expect(props.onCommit).not.toHaveBeenCalled();
850+
});
851+
739852
it("previews the stretch on move without persisting, then commits once on release", () => {
740853
const onPreview = vi.fn();
741854
const onCommit = vi.fn();
@@ -753,36 +866,92 @@ describe("TimelineAutomationLane stretch", () => {
753866
expect(onCommit).toHaveBeenCalledTimes(1);
754867
});
755868

756-
it("a point sitting on the selection's edge wins over the edge-stretch gesture", () => {
757-
const sel: HfAutomation = {
758-
version: 1,
759-
lanes: [
760-
{
761-
target: "volume",
762-
points: [
763-
{ t: 0, v: 1 },
764-
{ t: 1.5, v: 0.5 },
765-
{ t: 2, v: 0.8 },
766-
{ t: 4, v: 0 },
767-
],
768-
},
769-
],
770-
};
869+
/** A breakpoint sitting exactly on the selection's right edge — the state every
870+
* range operation leaves behind, and the one a stretch has to be able to grab
871+
* a second time. */
872+
const pointOnEdge: HfAutomation = {
873+
version: 1,
874+
lanes: [
875+
{
876+
target: "volume",
877+
points: [
878+
{ t: 0, v: 1 },
879+
{ t: 1.5, v: 0.5 },
880+
{ t: 2, v: 0.8 },
881+
{ t: 4, v: 0 },
882+
],
883+
},
884+
],
885+
};
886+
887+
it("the selection's edge wins over a point sitting exactly on it", () => {
888+
// The regression this pins is the feature not being repeatable: stretching
889+
// pins a breakpoint on the edge it just created, so a point-first rule made
890+
// the SECOND stretch of that edge a point-drag every time — at the one
891+
// height (on the envelope) where the user naturally grabs it.
771892
const onRangeSelect = vi.fn();
772-
const { svg, props } = mount(sel, {
893+
const { svg } = mount(pointOnEdge, {
773894
rangeSelection: { t0: 1, t1: 2 },
774895
onRangeSelect,
775896
});
776-
fire(svg, "pointerdown", at(2, 0.8)); // exactly the point at t=2, which is also the right edge
897+
fire(svg, "pointerdown", at(2, 0.8)); // the point at t=2, which is also the right edge
898+
fire(svg, "pointermove", at(3, 0.8));
899+
fire(svg, "pointerup", at(3, 0.8));
900+
expect(onRangeSelect).toHaveBeenLastCalledWith(1, expect.closeTo(3, 1));
901+
});
902+
903+
it("leaves that point reachable once the selection is gone", () => {
904+
// The escape hatch the rule above depends on: no selection, no handle, so
905+
// the point is an ordinary point again.
906+
const onRangeSelect = vi.fn();
907+
const { svg, props } = mount(pointOnEdge, { rangeSelection: null, onRangeSelect });
908+
fire(svg, "pointerdown", at(2, 0.8));
777909
fire(svg, "pointermove", at(3, 0.8));
778910
fire(svg, "pointerup", at(3, 0.8));
779-
// A point-drag moved just that point; the selection itself was untouched.
780-
expect(onRangeSelect).not.toHaveBeenCalled();
781911
const written = props.onCommit.mock.calls.at(-1)?.[0] as HfAutomation;
782912
const times = (written.lanes[0]?.points ?? []).map((p) => p.t);
783913
expect(times).toContain(3);
784914
});
785915

916+
it("stretches the same edge twice in a row", () => {
917+
// The whole point of the priority rule, end to end: two stretches of the
918+
// right edge, the second grabbing the breakpoint the first one left there.
919+
const onRangeSelect = vi.fn();
920+
const { svg, props, rerender } = mountRerenderable(stretchable, {
921+
rangeSelection: { t0: 0.5, t1: 2 },
922+
onRangeSelect,
923+
});
924+
dragRightEdge(svg, 2, 2.6);
925+
expect(onRangeSelect).toHaveBeenLastCalledWith(0.5, expect.closeTo(2.6, 1));
926+
const afterFirst = props.onCommit.mock.calls.at(-1)?.[0] as HfAutomation;
927+
// A breakpoint landed on the new edge, which is what used to disarm it.
928+
expect((afterFirst.lanes[0]?.points ?? []).some((p) => Math.abs(p.t - 2.6) < 0.05)).toBe(true);
929+
930+
// The store comes back with the persisted envelope and the new selection.
931+
rerender({ automation: afterFirst, rangeSelection: { t0: 0.5, t1: 2.6 } });
932+
dragRightEdge(svg, 2.6, 3.4);
933+
expect(onRangeSelect).toHaveBeenLastCalledWith(0.5, expect.closeTo(3.4, 1));
934+
expect(props.onCommit).toHaveBeenCalledTimes(2);
935+
});
936+
937+
it("keeps the far edge grabbable on a selection narrower than its own halos", () => {
938+
// A selection this thin — a pasted short span, or any span at low zoom — has
939+
// both handles under one press, so which edge a press takes is decided
940+
// entirely by the midpoint split. Characterizing it here because nothing
941+
// else does: every other test in this block has halos far enough apart that
942+
// the rule never comes up.
943+
const onRangeSelect = vi.fn();
944+
const { svg } = mount(stretchable, {
945+
rangeSelection: { t0: 2, t1: 2.08 }, // 8px wide at 100 px/s: both halos overlap
946+
onRangeSelect,
947+
});
948+
fire(svg, "pointerdown", at(2.07, 0.5)); // nearer t1, inside t0's halo too
949+
fire(svg, "pointermove", at(3, 0.5));
950+
fire(svg, "pointerup", at(3, 0.5));
951+
// t0 stayed put and t1 moved out: the press resolved to the right edge.
952+
expect(onRangeSelect).toHaveBeenLastCalledWith(2, expect.closeTo(3, 1));
953+
});
954+
786955
it("clamps the dragged edge so it cannot cross its partner", () => {
787956
const onRangeSelect = vi.fn();
788957
const { svg } = mount(stretchable, {

‎packages/studio/src/player/components/TimelineAutomationLane.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -301,7 +301,7 @@ export function TimelineAutomationLane({
301301
onPointerDown={gestures.onPointerDown}
302302
onPointerMove={gestures.onPointerMove}
303303
onPointerUp={gestures.endDrag}
304-
onPointerCancel={gestures.endDrag}
304+
onPointerCancel={gestures.cancelDrag}
305305
onDoubleClick={gestures.onDoubleClick}
306306
onContextMenu={onSvgContextMenu}
307307
role="group"
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* Pointer capture for a lane gesture. Its own module only because both gesture
3+
* hooks need it and `automationLaneGeometry` is deliberately DOM-free.
4+
*/
5+
6+
/**
7+
* Keep the rest of a gesture even if the pointer leaves the lane. Without it a
8+
* drag that strays outside the svg stops sending moves and the point sticks.
9+
*
10+
* Captured on the svg the handler is bound to, NOT on `e.target`: the target is
11+
* whatever child the press happened to land on — a breakpoint circle, a
12+
* selection line — and a child that unmounts mid-drag takes the capture with it,
13+
* silently and with no `pointercancel` to notice it by. The svg outlives every
14+
* gesture on it.
15+
*
16+
* Structurally typed so this stays testable without React's event types.
17+
*/
18+
export function capturePointer(e: { currentTarget: Element; pointerId: number }): void {
19+
e.currentTarget.setPointerCapture?.(e.pointerId);
20+
}

‎packages/studio/src/player/components/automationLaneSelection.test.ts‎

Lines changed: 55 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -116,15 +116,65 @@ describe("retimeRange", () => {
116116
5,
117117
);
118118
}
119-
// The next real breakpoint past the edited region keeps its own exact
120-
// value — growing past it reshapes the transition INTO it, not the point
121-
// itself. (Sampling inside that transition, e.g. at t=5.1, is expected to
122-
// differ: one of that segment's endpoints moved from t=3 to t=5, even
123-
// though this point at t=6 did not move at all.)
119+
// The next real breakpoint past the edited region keeps its own exact value.
124120
const farPoint = after.points.find((p) => p.t === 6);
125121
expect(farPoint).toEqual({ t: 6, v: 0 });
126122
});
127123

124+
it("preserves the envelope on BOTH sides when no breakpoint sits on the moved edge", () => {
125+
// The right side is where the invariant is worth asserting — `newT0 === t0`
126+
// makes the left side of the test above trivially true, and an earlier
127+
// right-side probe at t=5.1 was DELETED as inherent when it was reporting
128+
// the real behaviour below. With the selection's edges off any breakpoint,
129+
// the guarantee holds exactly, in both directions.
130+
const before: HfAutomationLane = { target: "volume", points: ramp.points };
131+
const after: HfAutomationLane = {
132+
target: "volume",
133+
points: retimeRange({
134+
lane: ramp,
135+
range: VOLUME_RANGE,
136+
t0: 2.2,
137+
t1: 2.9,
138+
newT0: 2.2,
139+
newT1: 4,
140+
}),
141+
};
142+
for (const t of [0, 1, 2, 4.5, 5, 5.5, 6]) {
143+
expect(sampleAutomationLane(after, t, "linear")).toBeCloseTo(
144+
sampleAutomationLane(before, t, "linear"),
145+
5,
146+
);
147+
}
148+
});
149+
150+
it("moves a breakpoint sitting exactly on the dragged edge, reshaping the segment past it", () => {
151+
// The design decision this pins, because it is not free either way.
152+
// `pointsIn` is endpoint-inclusive, so a breakpoint ON the edge is interior
153+
// and travels with the stretch. It has to: every range operation leaves a
154+
// breakpoint exactly on the edge it created, so treating that point as an
155+
// anchor instead would make the commonest stretch — grabbing the edge to
156+
// drag that very point outward — delete it and flatten the span.
157+
//
158+
// The cost is that the retimed point lands ON the union's boundary, where a
159+
// preservation anchor would also go, and `anchor()` stands down within a
160+
// merge radius. Two different values cannot occupy one time; the segment
161+
// leaving the union reshapes, which is the "envelope outside the selection
162+
// never moves" invariant bending exactly here and nowhere else.
163+
const pts = retimeRange({ lane: ramp, range: VOLUME_RANGE, t0: 2, t1: 3, newT0: 2, newT1: 5 });
164+
expect(pts).toEqual([
165+
{ t: 0, v: 1 },
166+
{ t: 2, v: 0.6 },
167+
{ t: 5, v: 0.4 }, // the t=3 point, retimed onto the new edge
168+
{ t: 6, v: 0 },
169+
]);
170+
// The old 3→6 segment sloped -0.133/s; the new 5→6 slopes -0.4/s, so the
171+
// envelope past the union genuinely moves. Asserted, not tolerated: if this
172+
// number changes, the decision above changed with it.
173+
const after: HfAutomationLane = { target: "volume", points: pts };
174+
expect(sampleAutomationLane(after, 5.5, "linear")).toBeCloseTo(0.2, 5);
175+
expect(sampleAutomationLane(ramp, 5.5, "linear")).toBeCloseTo(0.0667, 4);
176+
});
177+
128178
it("rejects a degenerate span", () => {
129179
expect(
130180
retimeRange({ lane: ramp, range: VOLUME_RANGE, t0: 2, t1: 3, newT0: 4, newT1: 4 }),

‎packages/studio/src/player/components/automationLaneSelection.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,16 @@ export function replaceRange(input: {
8080
* Retime a selection: interior points scale proportionally into the new span,
8181
* then replaceRange runs over the UNION of old and new spans — growing eats
8282
* whatever it covers, shrinking pins anchors where the envelope re-enters.
83+
*
84+
* Interior is `pointsIn`, so a breakpoint sitting exactly ON an edge travels
85+
* with the stretch. Deliberate: every range operation leaves a breakpoint on the
86+
* edge it created, so treating that point as a fixed anchor would make the
87+
* commonest stretch of all — grabbing the edge to drag that point outward —
88+
* delete it instead. The price is that such a point lands on the union's own
89+
* boundary, where `anchor` then stands down (one time cannot hold two values),
90+
* so the segment leaving the union reshapes. That is the ONE place
91+
* `replaceRange`'s outside-never-moves invariant bends, and it is pinned by name
92+
* in automationLaneSelection.test.ts.
8393
*/
8494
export function retimeRange(input: {
8595
lane: HfAutomationLane;

0 commit comments

Comments
 (0)