From 21dbde8435e570d83646a6104458653507bb9439 Mon Sep 17 00:00:00 2001 From: Mike Solar Date: Thu, 24 Sep 2026 14:45:06 +0800 Subject: [PATCH] fix(timeline): place a clip from empty space on top of what it covers Dropping a new clip whose in-point landed in empty track space (the stored-range model allows holes between blocks; the C++ layout is contiguous) left the overlapped clip untouched AND inserted the new clip before it in track order, so it slid UNDER the clip it covered. Starting on a clip already overwrote correctly, so the behavior depended on where the in-point happened to fall. TrackRippleRemoveAreaCommand::prepare now handles the hole case (no block spans the range start): nothing is trimmed on the left, the insertion anchor is the last block ending at/before the range, and the shared trailing scan removes/head-trims the blocks the range covers. Regression tests: domain_test (command level) and graphops (the app's place_footage_clip path). --- crates/oak-app/src/oakui/graphops.rs | 57 +++++++++++++ crates/oak-timeline/src/undoripple.rs | 100 +++++++++++++++-------- crates/oak-timeline/tests/domain_test.rs | 55 +++++++++++++ docs/zh/plans/test-coverage-90-80.md | 5 ++ 4 files changed, 181 insertions(+), 36 deletions(-) diff --git a/crates/oak-app/src/oakui/graphops.rs b/crates/oak-app/src/oakui/graphops.rs index da3f3b06a..3a8719a09 100644 --- a/crates/oak-app/src/oakui/graphops.rs +++ b/crates/oak-app/src/oakui/graphops.rs @@ -6660,4 +6660,61 @@ mod gap_coverage_tests { let _ = std::fs::remove_file(&media); oak_undo::global::clear().unwrap(); } + + /// Dropping a new clip whose in-point lands in EMPTY track space still + /// overwrites the clip its tail covers (the start-on-a-clip behavior): + /// the covered head is removed and the placed clip lands before the + /// overlapped clip in track order. The stored model allows holes between + /// blocks; the place command used to leave the overlapped clip untouched + /// and insert the new one before it, sliding it UNDER the clip it + /// covered. + #[test] + fn place_clip_from_empty_space_overwrites_the_overlapped_clip() { + let _g = test_lock(); + oak_undo::global::clear().unwrap(); + let (project, seq, footage, media) = project_with_footage("place_from_empty"); + let track = video_track_of(&project, seq); + let head = place_clip(&project, seq, footage, 0, 40); + let tail = place_clip(&project, seq, footage, 100, 200); + + // Drop a 100-frame clip at 50: its tail covers the tail clip's head. + let placed = place_clip(&project, seq, footage, 50, 150); + + let tb = { + let g = lock(&project); + sequence_time_base(&g.graph, seq).expect("time base") + }; + let span = |id: NodeId| { + let g = lock(&project); + clip_range(&g.graph, id).map(|r| (r.0, r.1)) + }; + assert_eq!( + clip_ids(&lock(&project).graph, track), + vec![head, placed, tail], + "the placed clip lands before the clip it covers" + ); + assert_eq!( + span(head), + Some((Rational::new(0, 1), ts_to_rational(40, tb))) + ); + assert_eq!( + span(placed), + Some((ts_to_rational(50, tb), ts_to_rational(150, tb))) + ); + assert_eq!( + span(tail), + Some((ts_to_rational(150, tb), ts_to_rational(200, tb))), + "the covered head is removed" + ); + + oak_undo::global::undo().unwrap(); + assert_eq!( + span(tail), + Some((ts_to_rational(100, tb), ts_to_rational(200, tb))) + ); + assert_eq!(clip_ids(&lock(&project).graph, track), vec![head, tail]); + + let _ = std::fs::remove_file(&media); + oak_undo::global::clear().unwrap(); + } } diff --git a/crates/oak-timeline/src/undoripple.rs b/crates/oak-timeline/src/undoripple.rs index e86234227..ebf287a81 100644 --- a/crates/oak-timeline/src/undoripple.rs +++ b/crates/oak-timeline/src/undoripple.rs @@ -42,7 +42,7 @@ use crate::util::{ block_add_to_graph, block_gap_create, block_in, block_kind, block_length, block_next, block_out, block_previous, block_remove_from_graph, block_set_in, block_set_length_and_media_in, block_set_length_and_media_out, block_set_length_keeping_out, - block_track, sequence_all_tracks, sequence_track_list, + block_track, sequence_all_tracks, sequence_track_list, track_block_at, track_insert_block_after, track_locked, track_nearest_block_after_or_at, track_nearest_block_before_or_at, track_prepend_block, track_ripple_remove_block, tracklist_track_at, tracklist_track_count, BlockKind, NodeRef, @@ -229,11 +229,35 @@ impl TrackRippleRemoveAreaCommand { let in_ = self.range.in_(); let out = self.range.out(); - // CPP-PARITY timelineundoripple.cpp:57-64 - let Some(first_block) = nearest_block_before_or_at(&track, in_) else { - return; + // C++'s track layout is contiguous (gaps fill the holes), so the + // nearest block before-or-at `in_` always spans it. The stored-range + // model allows genuine holes BETWEEN blocks: the helper can instead + // return the last block that merely ENDS at/before `in_` (or + // nothing at all), with blocks after the hole still overlapping the + // range. There is nothing to trim on the left in that case: the + // replacement goes after that block and the trailing scan starts at + // its successor. Missing this branch left the overlapped clips + // untouched AND inserted the replacement before them, so a drop + // whose in-point landed in empty space slid UNDER the clip it + // covered instead of overwriting it. + let first = nearest_block_before_or_at(&track, in_); + let starts_in_a_hole = match &first { + Some(fb) => block_out(fb) <= in_, + None => true, }; - let fb = first_block; + if starts_in_a_hole { + self.insert_previous_ = first.clone(); + let start = match &first { + Some(fb) => block_next(fb), + None => track_block_at(&track, 0), + }; + self.scan_successors(start, out); + self.prepared = true; + return; + } + + // CPP-PARITY timelineundoripple.cpp:57-64 + let fb = first.expect("the hole case returned early"); // Determine if this first block is getting trimmed or removed let first_block_is_out_trimmed = block_in(&fb) < in_; @@ -285,43 +309,47 @@ impl TrackRippleRemoveAreaCommand { // If the first block is getting in trimmed, we're already at the // end of our range if !first_block_is_in_trimmed { - let mut next = block_next(&fb); - while let Some(nx) = next { - // The module world allows gaps BETWEEN blocks (their - // in/out points are stored, unlike Olive's contiguous - // track ordering), so a successor starting at/after the - // region does not overlap it and must not be touched. - if block_in(&nx) >= out { - break; - } - let trimming = block_out(&nx) > out; - - if trimming { - self.trim_in_ = Some(TrimOperation { - block: nx.clone(), - old_length: block_length(&nx), - new_length: block_length(&nx) - (out - block_in(&nx)), - }); - break; - } else { - self.removals_.push(RemoveOperation { - block: nx.clone(), - before: block_previous(&nx), - }); - - if block_out(&nx) == out { - break; - } - } - - next = block_next(&nx); - } + self.scan_successors(block_next(&fb), out); } } self.prepared = true; } + /// Remove the blocks fully inside `[_, out)` and head-trim the first one + /// reaching past it — the shared trailing scan of [`Self::prepare`]. + fn scan_successors(&mut self, mut next: Option, out: Rational) { + while let Some(nx) = next { + // The module world allows gaps BETWEEN blocks (their in/out + // points are stored, unlike Olive's contiguous track ordering), + // so a successor starting at/after the region does not overlap + // it and must not be touched. + if block_in(&nx) >= out { + break; + } + let trimming = block_out(&nx) > out; + + if trimming { + self.trim_in_ = Some(TrimOperation { + block: nx.clone(), + old_length: block_length(&nx), + new_length: block_length(&nx) - (out - block_in(&nx)), + }); + break; + } + self.removals_.push(RemoveOperation { + block: nx.clone(), + before: block_previous(&nx), + }); + + if block_out(&nx) == out { + break; + } + + next = block_next(&nx); + } + } + /// `redo`: apply the ripple removal. pub fn redo(&mut self) { // The oakundo command path never invokes `prepare()` (the oakundo diff --git a/crates/oak-timeline/tests/domain_test.rs b/crates/oak-timeline/tests/domain_test.rs index 35f8f97fe..130ab530d 100644 --- a/crates/oak-timeline/tests/domain_test.rs +++ b/crates/oak-timeline/tests/domain_test.rs @@ -483,6 +483,61 @@ fn place_block_splice_round_trip() { ); } +/// `TrackPlaceBlockCommand` starting in EMPTY SPACE between blocks (the +/// stored model allows holes; the C++ layout is contiguous) must still +/// overwrite what it covers: the following clip is head-trimmed at the +/// placed block's out point and the placed block lands BEFORE it in track +/// order. Missing this used to leave the overlapped clip untouched and +/// insert the placed block before it, so the drop slid UNDER the clip it +/// covered instead of overwriting it. +#[test] +fn place_block_from_empty_space_overwrites_the_overlapped_clip() { + let project = make_project(); + let (_seq, list) = sequence_and_list(&project); + let track = TimelineAddTrackCommand::run_immediately(list.clone()); + let head = add_clip(&track, Rational::new(0, 1), Rational::new(40, 1)); + let tail = add_clip(&track, Rational::new(100, 1), Rational::new(200, 1)); + let placed = add_clip(&track, Rational::new(0, 1), Rational::new(100, 1)); + track_ripple_remove_block(&track, &placed); + + let mut cmd = oak_timeline::undopointer::TrackPlaceBlockCommand::new( + list.clone(), + 0, + placed.clone(), + Rational::new(50, 1), + ); + cmd.redo(); + assert_eq!(track_block_count(&track), 3); + assert_eq!(track_block_at(&track, 0).unwrap().id, head.id); + assert_eq!( + track_block_at(&track, 1).unwrap().id, + placed.id, + "the placed block lands before the clip it covers" + ); + assert_eq!( + span_of(&placed), + Some((Rational::new(50, 1), Rational::new(150, 1))) + ); + assert_eq!( + span_of(&tail), + Some((Rational::new(150, 1), Rational::new(200, 1))), + "the covered head is removed" + ); + assert_eq!( + span_of(&head), + Some((Rational::new(0, 1), Rational::new(40, 1))), + "the block before the hole is untouched" + ); + + cmd.undo(); + assert_eq!(track_block_count(&track), 2); + assert_eq!(span_of(&head), Some((Rational::new(0, 1), Rational::new(40, 1)))); + assert_eq!( + span_of(&tail), + Some((Rational::new(100, 1), Rational::new(200, 1))) + ); +} + /// `TrackListInsertGaps` splits a block at the insertion point and /// inserts a gap of the requested length after it; undo removes the gap /// and re-joins the block. diff --git a/docs/zh/plans/test-coverage-90-80.md b/docs/zh/plans/test-coverage-90-80.md index a066598ab..3b7e1383f 100644 --- a/docs/zh/plans/test-coverage-90-80.md +++ b/docs/zh/plans/test-coverage-90-80.md @@ -798,6 +798,11 @@ python3 tooling/coverage_report.py --json target/coverage.json （gpui 子模块提交见 `oak-gpui`。） - **OCIO 参数面板**：Vec4 评分参数（对比度/偏移/曝光）建 4 个 SpinBox， `base` 步长锚定范围（pivot 不再一拖就飞出 ±10000）。 +- **拖放覆盖修复**：`TrackRippleRemoveAreaCommand::prepare` 补"范围起点落在 + 块间空洞"分支（C++ 布局连续、无此情形）——此前该情形下既不裁剪后面的 + 重叠块、又把新块插到它前面，导致"起点在空白、终点压在别的 clip 上"时 + 新 clip 被压在下层而不是覆盖。现在统一与起点在 clip 上时一致：重叠部分 + 被移除、新块在前。`domain_test` 与 `graphops` 各补一条回归断言。 ## 8. 风险与对策