feat(editor): the piano roll stretches, compacts and scrolls - #330
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5fa1515 to
9ac57e3
Compare
STACKED ON #327 (fix/editor-lane-vertical-scroll) — it needs the shared lane-scroll model that PR introduces. Merge #327 first; this diff shows only its own changes against that branch. The roll never overflowed because it SQUASHED instead. Lane height was derived and nothing could touch it — max(4, min(14, 350 / range)) — packing any pitch range into ~350px. A wide range collapsed to 4px per semitone: passable for reading, useless for editing, with no way out. That is exactly why #327 left the roll alone and fixed only the drum grid's reachability. That derived value is now the DEFAULT. S.rollLaneH is a user override, and the vertical scrolling shared with the drum grid is what makes a taller-than- viewport roll usable — the two halves only work together, which is why they land together. Alt+wheel is Live's binding for key-track zoom (Live 12 manual p.153, p.240); Logic exposes the same as a Vertical Zoom slider (p.297). Checked BEFORE the ctrl branch so Alt+Ctrl can't land on horizontal zoom while the user means vertical, and gated on isKeysMode() so every other view keeps today's gestures untouched. midiToY/yToMidi take the scroll term, mirroring _drumLaneIdxToY's pair — every painter and hit-test funnels through them, so one edit reaches all of them. _beatBarTopY() follows the roll's scroll: the anchor, handshape and beat-bar strips annotate the chart, so they ride with the note content rather than floating over it. The string view has no vertical scroll, so laneScrollY() is 0 there and that expression is unchanged. The lane-band clip is pushed only when the view actually scrolls, so the string view's pixels are untouched — clipping it unconditionally would be a silent behaviour change for the common case. It is released in the `finally`, not after the last painter: a throw in between would otherwise leave the context one save deep and the outer restore would pop the wrong level. Song load resets both the override and the scroll offset, before updatePianoRange so it re-derives the auto height instead of honouring a stale override from the previous arrangement's range. Verified in a browser on a real song, roll view: the scrollbar goes from ABSENT to 158px of thumb after stretching — and it only ever paints when content exceeds the viewport, so its appearance is proof the lanes grew. Plain wheel then scrolls. Waveform and ruler stay clean above the clip. Status reads "Roll lane height 23.6px — Alt+scroll to stretch/compact". 249 JS tests, 346 pytest, lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix
591a26c to
32b74c7
Compare
1 participant
The problem
The roll never overflowed because it squashed instead:
Derived, untouchable, and it packs any pitch range into ~350px. A wide range collapses to four pixels per semitone — passable for reading, useless for editing, and with no way out.
That's exactly why #327 fixed only the drum grid's reachability and deliberately left the roll alone.
The fix
That derived value is now the default.
S.rollLaneHis a user override, and the vertical scrolling shared with the drum grid is what makes a taller-than-viewport roll usable. The two halves only work together, which is why they land together — stretching without scrolling would just move content off-screen.Nothing changes until you reach for it. The derived height is still the default, a roll that fits still shows no scrollbar, and a plain wheel still pans horizontally.
Alt is Live's binding for key-track zoom (Live 12 manual p.153, p.240); Logic exposes the same as a Vertical Zoom slider (Logic Pro guide, p.297). It's checked before the ctrl branch so Alt+Ctrl can't land on horizontal zoom while the user means vertical, and gated on
isKeysMode()so every other view keeps today's gestures.Decisions worth reviewing
_beatBarTopY()follows the scroll. The anchor, handshape and beat-bar strips annotate the chart, so they ride with the note content rather than floating over it. The string view has no vertical scroll —laneScrollY()is 0 there and the expression is unchanged.The clip is conditional. Pushed only when the view actually scrolls, so the string view's pixels are untouched; clipping unconditionally would be a silent behaviour change for the common case. Released in the
finally, not after the last painter — a throw in between would leave the context one save deep and the outerrestore()would pop the wrong level.Song load resets both the override and the offset, before
updatePianoRangeso it re-derives the auto height rather than honouring a stale override from the previous arrangement's range.Verification
Browser, real song, roll view. The scrollbar goes from absent (0px) to 158px of thumb after stretching — and it only ever paints when content exceeds the viewport, so its appearance is the proof the lanes grew. Plain wheel then scrolls (content shifts). Waveform and ruler stay clean above the clip. Status reads
Roll lane height 23.6px — Alt+scroll to stretch/compact. No page errors.tests/roll_vertical_zoom.test.mjs(16 cases) against the realkeys.js: auto-fit unchanged for narrow and wide ranges (the old pathology is preserved as the default); override wins, clamps both ends, and degrades to auto-fit on garbage; stretch→compact round-trips; clamping is silent at both stops; a nonsense factor is a no-op; reset restores auto; the override survives a range recompute (updatePianoRangeruns on every extent-changing edit and must not stomp it); unscrolled geometry is byte-identical; scroll shifts by exactly the offset; scrolled y→midi→y round-trips; and a fully stretched roll genuinely exceeds a real canvas — the test that ties the two halves together.249 JS, 346 pytest, lint clean.
This closes the navigation ask
Horizontal (#325) → drum vertical (#327) → roll vertical + stretch/compact (here). Track list already scrolled and resized; the tempo map needs no vertical, as Christian called.
🤖 Generated with Claude Code
https://claude.ai/code/session_017xGPjDBF8NTwTK7VQvizix