Skip to content

Add Cmd-drag ripple insert for clips already on the timeline.#340

Open
mricopeng wants to merge 1 commit into
mainfrom
fix/ripple-insert-timeline-clip-drag
Open

Add Cmd-drag ripple insert for clips already on the timeline.#340
mricopeng wants to merge 1 commit into
mainfrom
fix/ripple-insert-timeline-clip-drag

Conversation

@mricopeng

Copy link
Copy Markdown
Contributor

Ripple insert only worked when dragging media onto the timeline but not if the clips were already on the timeline.

Now you can ripple insert clips on the timeline but holding cmd and dragging

Screen.Recording.2026-07-16.at.10.47.54.PM.mov

Mirror media-panel ripple insert for internal moves: insert at the drop frame, push downstream, and cover it with tests.

Co-authored-by: Cursor <cursoragent@cursor.com>
@cursor

cursor Bot commented Jul 17, 2026

Copy link
Copy Markdown

Current version of PR was reviewed by /review-bugbot on Jul 16, 22:42 PDT. It flagged 0 findings.

Bugbot on commit 0c5c86e is skipped.

@htin1

htin1 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Big issues:

  1. Cmd-drag ripple doesn't work on audio clips. addVolumeKeyframeOnClick (TimelineInputController.swift:771) claims Cmd+mousedown anywhere on a clip's body (the guard is just body.contains(point)), adds a volume keyframe, and sets dragState = .idle — so a Cmd-drag on an audio clip never starts a move. The advertised "Cmd + Drag = Ripple Insert" shortcut is broken for a whole media type; the gestures need disambiguation (e.g. only add the keyframe on mouseUp-without-drag).
  2. Linked-partner right halves can overlap downstream clips. In rippleMoveClips, when a split straddler's linked partner lives on a non-push track (unlocked, not a destination), its right half is shifted by pushAmount but downstream clips on that same track are not — the shifted half can land on top of them. linkedPartnerOfSplitStraddlerRidesThePush doesn't cover this (no downstream clip on the partner track). Either push the full downstream chain on those tracks, or leave the partner half in place to match unlockedOtherTrackStaysPut semantics.

Duplication / refactor:

  • Extract a shared gap-opening helper. rippleMoveClips duplicates the core of rippleInsertClips(specs:trackIndex:atFrame:) (EditorViewModel+Ripple.swift:455-465): compute push tracks, split straddlers at the insert frame, then computeRipplePush + applyShifts per track. An openRippleGap(atFrame:pushAmount:trackIds:) -> [String] (returning split right-half IDs) used by both would shrink this PR's new code to remove → guard → open gap → re-place, let it reuse rippleInsertShiftingTrackIds for the push-track set, and resolve issue 2 in one place for both paths (the specs path has the same linked-partner divergence today). Note the two operations can't merge outright: move must remove the clip before computing the push, and pushes by span (parallel multi-track landing) while insert pushes by summed duration (sequential placement) — the helper is the right seam.

Decisions to confirm (not bugs):

  • First-responder change is broader than the feature. acceptsFirstResponder = true + makeFirstResponder(self) on every mouseDown changes focus behavior app-wide (e.g. clicking the timeline ends editing in inspector text fields). Probably fine, but worth a manual pass over keyboard-driven flows.
  • Source gap is left open (moveBeyondFollowerLeavesSourceGap codifies it). Insert-only ripple is defensible, but most NLEs' ripple-move also closes the source gap — confirm this is the intended UX before the test locks it in.

Minor:

  • TimelineView.flagsChanged should call super.flagsChanged(with:) — nothing else overrides it today, but it silently eats modifier events for the rest of the responder chain.
  • guard pushAmount > 0 is dead code (non-empty infos guarantees it).
  • Tests never pass more than one move to rippleMoveClips even though it accepts many; a linked video+audio pair move would be worth covering.
  • rippleMoveInsertFrame() re-runs resolvedMoveParticipants on every draw during a drag; caching the insert frame on the drag state in mouseDragged would drop the redundant per-frame work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants