Skip to content

feat(timeline): add track/clip management with drag-and-drop#140

Open
fatHFISH wants to merge 1 commit into
jbilcke-hf:mainfrom
fatHFISH:feat/timeline-track-clip-management
Open

feat(timeline): add track/clip management with drag-and-drop#140
fatHFISH wants to merge 1 commit into
jbilcke-hf:mainfrom
fatHFISH:feat/timeline-track-clip-management

Conversation

@fatHFISH
Copy link
Copy Markdown

  • Add new tracks via '+ Add Track' button
  • Cycle track type by clicking the type badge
  • Double-click empty track area to create clips
  • Drag segments horizontally on the timeline
  • Cross-track drag with type validation
  • Real-time segment position updates during drag

Closes #10

- Add new tracks via '+ Add Track' button
- Cycle track type by clicking the type badge
- Double-click empty track area to create clips
- Drag segments horizontally on the timeline
- Cross-track drag with type validation
- Real-time segment position updates during drag

Closes jbilcke-hf#10
@MyTH-zyxeon
Copy link
Copy Markdown

Thanks, this looks like a useful first pass for #10 and it stays close to the timeline-native authoring scope.

Quick review/acceptance pass from the issue checklist:

  • Track creation: covered by the + Add Track control.
  • Track typing: covered by the type badge cycling.
  • Clip creation: covered by double-clicking empty track space.
  • Horizontal drag: covered with snapped timeline updates.
  • Cross-track move with type validation: covered through dragHoveredTrackId + category checks.

A couple of things I would double-check before merge:

  1. TRACK_CATEGORIES includes SOUND twice. If that is not intentional, it should probably be deduped so cycling does not repeat the same category.
  2. I did not see an explicit overlap/collision rejection before finalizing a dragged or newly created segment. If Timeline - Create new Tracks and Clips #10 expects clips not to overlap on a track, this probably needs a guard before the move/create is committed.
  3. The visibility toggle uses emoji glyphs inside Text; if the timeline renderer or target font has issues with those glyphs, a plain text/icon fallback may be safer.

I avoided opening a competing PR because there are already several #10 implementations in flight. If maintainers want a narrow companion patch, I can focus on the category dedupe and collision-policy guard.

@MyTH-zyxeon
Copy link
Copy Markdown

Follow-up review-assist note for #10 / PR #140. I am not claiming this bounty here; this is another focused acceptance pass on the existing implementation.

After rechecking the current diff, a few additional edge cases look worth tightening before this is treated as complete timeline-native authoring:

  1. Track type changes can invalidate existing clips. setTrackType(trackId, category) updates the track type even if the track already contains segments with a different category. Since the issue requires cross-track moves only when the type is the same, the implementation should also prevent changing a populated track to an incompatible type, or explicitly revalidate/migrate existing clips.

  2. Negative horizontal drag can create invalid timing. During drag, segment.startTimeInMs is clamped with Math.max(0, ...), but segment.endTimeInMs is still set to dragSegmentOriginalEndTimeMs + snappedDelta. If a clip is dragged left past zero, the start can clamp to 0 while the end can become earlier than the start or even negative. It would be safer to preserve duration when clamping the start time and add a regression test.

  3. Empty tracks can produce generic clips. createClipOnTrack maps a track named "(empty)" to GENERIC, but the issue says track type should be set from the dropdown and clips should be created based on the track type. It may be better to block clip creation on empty/untyped tracks, or automatically require/select an explicit type before creating the clip.

  4. The new track ID is currently tracks.length. That is fine while tracks are append-only, but if deletion/reordering lands later, it can collide with existing track IDs. A monotonic ID source or max-id-plus-one guard would make the feature easier to extend safely.

These are small, testable guards and they fit the original scope without adding entity/content editing 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.

Timeline - Create new Tracks and Clips

2 participants