feat(loop): adjust a loop region instead of redrawing it - #539
Merged
Conversation
Reported in discussion #507: changing the size of a selection meant clicking and dragging again from scratch, which restarts the whole thing and loses the precision you had. The numeric fields are the only exact route, and finding a seamless loop through them is guesswork. The region could not be touched at all -- .loop-region was pointer-events: none, so every pointerdown reached the create-drag underneath, which resets both edges to the click position. Adjusting one edge necessarily destroyed the other. Three gestures now: - a handle at either edge moves that edge alone, so a loop can be tightened one side at a time; - the body moves both edges together, preserving length, so a loop found by ear can be slid without being re-measured; - a press that does not travel still seeks, which is what clicking inside the selection did before the region took pointer events. Losing that would have been a regression for anyone who just wants to move the playhead. The handles are 12px and overhang the 2px border on both sides, because an edge you cannot reliably grab is the finnicky behaviour this is meant to replace. They only render on hover or while dragging, so the selection looks the same at rest. create-drag already guarded with `e.target.closest(".loop-region")` -- dead code until now, since the element could not receive events. It is a real guard again, alongside the stopPropagation on the region's own handler. The geometry lives in its own module rather than in transport.js, which cannot be imported outside a browser: it pulls in state.js, which touches document at module load. Same reason playbackStems.js is separate. That makes the clamping testable, which is the half that matters -- an edge crossing its partner, or a region pushed against either end of the track. MIN_LOOP_SEC moves with it and now has one definition. Pointer events throughout, so this works with touch and pen. Closes #538.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #538. From discussion #507 (@digidirt). Branches off
0.16.1.The problem
Changing a selection's size meant re-dragging from scratch:
The region could not be touched at all --
.loop-regionwaspointer-events: none, so every pointerdown reached the create-drag underneath, which resets both edges to the click position. Adjusting one edge necessarily destroyed the other.Three gestures
That third one matters: making the body draggable would otherwise break click-to-seek over the selection. It uses the same movement threshold the create-drag already uses to tell a click from a drag.
Handles are 12px and overhang the 2px border on both sides -- an edge you cannot reliably grab is the finnicky behaviour this replaces. They render only on hover or while dragging, so the selection looks unchanged at rest.
startDragalready guarded withe.target.closest(".loop-region"). That was dead code, since the element could not receive events; it is a real guard again now, alongside astopPropagationon the region's own handler.Structure
The geometry went into
static/js/loopRegion.jsrather than staying intransport.js, which cannot be imported outside a browser -- it pulls instate.js, which touchesdocumentat module load. Same reasonplaybackStems.jsis its own module.That makes the clamping testable, which is the half worth testing: an edge crossing its partner, or a region pushed against either end of the track. The event plumbing is not something a node test can say anything useful about.
MIN_LOOP_SECmoved with it and now has a single definition.Verification
The e2e run matters here: it includes
a loop survives zooming in and out untouchedanda loop can be marked while zoomed in, so the region becoming interactive has not broken existing loop behaviour.Checked by hand that nothing else is blocked by the region now taking pointer events:
.waves-columnis its parent and has no lane- or canvas-level pointer handlers, and wheel events still bubble, so wheel-zoom over a selection is unaffected.Two notes
Targeting
0.16.1. That branch has the transport accessor from #536 which this builds on, andmaindoes not yet. If you would rather keep 0.16.1 to fixes only, say so and I will retarget tomainonce 0.16.1 lands -- it should be a clean rebase.Not implemented: keyboard nudging. Arrow keys to move a selected edge by a frame would complement this for the "seamless loop" goal, and is probably the natural follow-up. Left out to keep this reviewable.