Repository navigation
feat(studio): zoom the timeline to the selected clips - #5824
miguel-heygen merged 4 commits into
Conversation
626b77b to
4a3f5b3
Compare
|
@miguel-heygen could I trouble you for a review? :) |
4a3f5b3 to
bdce2a0
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for this, and for the demo video, it made the behavior easy to check. The keyboard part is right: meta and control events are routed to the modifier dispatcher before the plain key dispatcher runs, so bare Z cannot swallow undo on any platform, and the typing guard already runs first. The percent math also matches the existing range zoom exactly, so the zoom button and pinch paths are unchanged. I checked that algebraically rather than trusting it.
The blocker is the scroll half. zoomToSelectedClips computes scrollLeft, contentWidth and maxLeft, with the same bare 2 pixel fudge, but nothing in the app reads any of them: easeToRange takes only zoomPercent, and zoomTimelineToSelectedClips takes only start and end. The real clamp is the one request() already owns, which has done this since before the change. So the PR body's claim about selections at the very start or end of the timeline is true in the app but is not what this code does, and the four new tests in timelineZoom.test.ts assert against the copy, through a place() helper that re-implements the production formula inside the test file. If the timeline's track width or margin ever changes in request(), those tests keep passing while the behavior drifts. Please drop scrollLeft and the fields that feed it, keep start, end and zoomPercent, and let the mounted timeline test in the toolbar spec carry the proof. That one is the good test here.
Two smaller things. The button's enabled state asks whether anything is selected, but the action asks whether the selection resolves to a timeline element, so selecting an expanded sub composition child, which selects as file#id, leaves the button live and the click does nothing with no feedback. The same happens when every selected clip has zero duration. Driving the disabled state from the same resolution the action uses would close that. Separately, the new not greater than guard on the viewport width changes zoomTimelineToRange, which is exported from the package index, from proceeding to resolving cancelled when the timeline is collapsed. That is probably the better behavior, but it is an unflagged change to a public entry point and no test pins either side.
Worth flagging before more work goes in: there is another open pull request, 5597, on the same issue with the same title, the same button and the same Z key. It is a draft and has not moved since the day it was opened, so this one looks like the live attempt, but someone should decide which survives rather than landing both halves of the same screen.
Last note on checks. Only the mergeability and WIP jobs have run at this head, so the suite is unproven here no matter what the checks tab suggests. The branch is also a few hundred commits behind main and will need a rebase.
bdce2a0 to
f735daf
Compare
|
Addressed the review on f735daf, rebased onto current main. Scroll is no longer computed beside the zoom percent. The button and
New demo, from Fit: disabled tooltip, zoom-selection-demo.mp4Draft #5597 is still open on the same issue with the same button and key. This is the live attempt. I have not closed that draft. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, and for rebasing. All three points from the last review are addressed:
zoomToSelectedClipsnow returns only the span and the zoom percent, the copied-formula tests are gone, and the mounted toolbar tests click the real button and read the real scroll clamp. That's the right place for the proof.- The button and
Znow use the same check as the action, and the tests cover thefile#idand zero-duration selections. - The
"cancelled"change tozoomTimelineToRangeis pinned and called out in the description.
I ran the touched studio tests at this head and they pass. Removing the resolvable-clip check from the Z handler makes the new hotkey test fail, so it's testing the real thing.
Small things, none blocking:
- The new collapsed-timeline guard lives in
zoomToSelectedClips, whicheaseToRangenow calls for every zoom, so it also reacheszoomTimelineStep. With the timeline no wider than its track headers, the zoom in/out buttons used to step the zoom; now they do nothing. I checked this both ways: main steps to 2x, this branch stays at fit. It's a narrow case, but it's a behavior change outside the new feature. LettingzoomTimelineStepfall back torequestTimelineZoom(nextPercent), like its no-viewport branch already does, would keep it. ZcallspreventDefault()beforezoomTimelineToSelectedClips()gets a chance to return false (no viewport yet, or the collapsed case). Using the boolean it already returns would keep it from becoming a silent no-op.- Nit:
zoomTimelineToSelectedClipsbuilds the selection spans three times and computes a zoom percent it then discards, sincezoomTimelineToRangecomputes its own.positiveClipSpan(selectedClipSpans(state))followed byzoomTimelineToRange(span.start, span.end)would be enough.
The full CI suite still hasn't run at this head (only the mergeability and WIP checks), so a maintainer needs to approve the workflow run. Draft #5597 is still open on the same issue, and maintainers should decide which one stays open.
Verdict: COMMENT
Reasoning: The earlier blocker and both smaller points are fixed and the touched tests pass locally, but the required CI suite has not run at this head; approve once it is green.
|
The three follow-ups are in 59c6529.
Draft #5597 is still the maintainers' call. I can't approve the workflow run on this repo. |
|
Demo after the follow-ups. Fit, disabled tooltip, last-clip zoom, zoom-in stepping to 200%, then Z on the clip at time 0. zoom-selection-followup.mp4 |
Toolbar action and Z fit the selected clips to the visible width, with the range margin, and stay disabled until something is selected.
The scroll clamp already lives in the timeline zoom request. Drop the unused copy, and disable the button and Z unless the selection matches a timeline clip with duration. A collapsed timeline cancels zoomTimelineToRange instead of laying out a percent the view cannot show.
A collapsed timeline has no track for a range zoom, so zoom in and out step the percent directly. Z is consumed only when that zoom actually starts, and zoom-to-selection no longer builds the span three times.
59c6529 to
2abcdac
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the follow-ups. All three points from my last review are fixed at 2abcdacb:
- On a timeline no wider than its track headers, zoom in/out falls back to
requestTimelineZoom, so the step still changes the scale. The new test pins it. Zis only consumed when the zoom actually starts. The hotkey test now covers the no-viewport and collapsed cases as well.- Zoom-to-selection builds the span once and hands it to
zoomTimelineToRange.
I ran the touched studio tests at this head on Linux (hotkeys, zoom input, zoom math and the toolbar), and they pass three times in a row. Two checks to confirm the new tests are real: removing the collapsed fallback from zoomTimelineStep fails the new step test, and moving preventDefault() back in front of the zoom call fails the Z test.
The full CI suite still hasn't run here, since fork workflows need a maintainer to start them, so that has to go green before this merges.
Verdict: APPROVE
Reasoning: Every earlier point is fixed and covered by tests that fail without the fix; the only open item is starting CI for the fork.
The approved run was cancelled during checkout, so the required checks never executed.
|
@miguel-heygen thanks so much for your feedback. looks like a maintainer needs to run the ci again. it passed locally |
|
Looks good, we want it. Your last push came after Miguel's approval, so it needs a fresh one at the new head. I've approved the CI runs. |
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approved. The source at this head matches what Miguel approved at 2abcdac, and the earlier scroll-math, selection and Z-key fixes all still hold.
What
A timeline toolbar action (next to Fit) and a
Zshortcut that zoom and scroll so the selected clips span the visible width, with the same 24px margin the range zoom uses. With nothing the zoom can land on, the button is disabled and its tooltip says why.Demo
Nothing selected, the button stays disabled and the tooltip says to select a clip. Selecting the last clip, hovering the toolbar action (the tooltip shows
Z), and clicking it: the eased zoom goes from Fit to that clip spanning the visible width, with its right edge on the margin. Zoom in still steps the scale. Then Z on the clip at time 0, which stays on screen without scrolling past zero.zoom-selection-followup.mp4
Before
Fit shows the whole span. The last clip is a small block at the right.
https://github.com/user-attachments/assets/ba5415bb-69bd-47d5-b19e-71e388a1d3a8
After
Zoom to that clip. It fills the visible width, and the zoom readout leaves Fit.
https://github.com/user-attachments/assets/47daaba3-9d09-43fa-a782-a3d937efbe0e
Why
Fixes #5581. There was no way to land on the cut you are tightening in a long composition; Fit always shows the whole span.
Related work
Fixes #5581. Draft #5597 is the same button and the same
Zkey, and it has not moved since the day it opened. This is the live attempt. That draft should not land as well.How
zoomToSelectedClipsintimelineZoom.tsowns the span and the zoom percent: union of the selected clips'[start, end], fitted intoviewportWidth - contentOrigin - 2×margin, clamped throughclampTimelineZoomPercent. It returnsstart,end, andzoomPercent. Scroll stays in the timeline zoomrequest, which already clamps a selection at the start or the end of the content. The margin constant is shared withrangeBox(TIMELINE_RANGE_MARGIN_PX), andeaseToRangederives its zoom percent from this function so the range zoom and zoom-to-selection cannot drift apart.Zuse the same resolution as the action. A selection has to match a timeline element (key ?? id) with a positive duration. An expanded sub-composition child, which selects asfile#id, and a selection whose clips all have zero duration leave the button disabled.Zis not consumed in those cases, so it is not a silent no-op. Shift+Z keeps redo.zoomTimelineToRange, exported from the package index, resolves"cancelled"when the timeline viewport is not wider than its headers. A collapsed timeline has no track to fit. That is pinned intimelineZoomInput.test.ts.Test plan
How was this tested?
timelineZoom.test.tscovers the span, the zoom percent, a zero-length entry, and a collapsed viewport.TimelineToolbar.test.tsxdisables the button for no selection, afile#idselection, and a zero-duration clip, and the mounted timeline test asserts the real scroll for a mid-span clip, a clip at time 0, and a clip at the end of the fit span.timelineZoomInput.test.tspinszoomTimelineToRangeresolving"cancelled"when the timeline is no wider than its headers.useAppHotkeys.test.tscoversZconsuming the key for a resolvable clip and leaving it alone otherwise.Ztooltip, the last-clip zoom, and the clip at time 0 staying at scroll 0.