From d88e2d6ec17b47cab871aa707b1a3cb0a78c149c Mon Sep 17 00:00:00 2001 From: Mike Solar Date: Thu, 24 Sep 2026 12:41:29 +0800 Subject: [PATCH] fix(timeline): restore the C++ block length anchors and repair pointer edits The two BlockCore length setters swapped their anchors relative to the C++ semantics they document, so the ported edit commands produced wrong geometry on the live UI paths: roll edits kept the seam still, slides left negative in-points, and trims wrote the timeline in-point into media_in (playing the wrong media content). Adopt three stored-range primitives in block.rs: - set_length_and_media_out: in fixed, out moves, media untouched (resize, trim-out, gaps growing rightward). - set_length_and_media_in: in fixed, out moves, media_in += old-new (resize-with-media-in, splice right half, ripple trim-in). - set_length_keeping_out (new): out fixed, in moves, media_in += old-new (trim-in body and out-neighbour, slide out-neighbour, ripple trim-in of the trailing block). Point every command at the primitive matching its intent (undopointer, undogeneral, undoripple, undosplit, graphops, cli, nodeops) and fix the two real defects the swap hid: - TrackReplaceBlockWithGapCommand grew a following gap rightward, swallowing whatever followed it: the "dragging one clip moves unrelated clips" regression. The gap now grows leftward over the removed block's span; regression test in domain_test. - The ripple/splice trims now advance media_in instead of rewriting it, and BlockSplitCommand writes both halves' ranges and media explicitly (the second half continues from the split point). Rewrite the KNOWN-SWAP expectations to the correct geometry (roll moves the seam, slide has no negative in-point, insert-gaps grows rightward, resize-with-media-in yields media_in = 20) and add the missing media assertions. TrackSlideCommand documents that the caller positions the sliding blocks (the stored model has no track layout). --- crates/oak-app/src/oakui/graphops.rs | 69 ++++--- crates/oak-cli/src/engine.rs | 4 +- crates/oak-node/src/block.rs | 33 ++- crates/oak-node/tests/phase2_units_test.rs | 21 +- crates/oak-task/src/nodeops.rs | 3 +- crates/oak-timeline/src/undogeneral.rs | 34 +++- crates/oak-timeline/src/undopointer.rs | 62 +++--- crates/oak-timeline/src/undoripple.rs | 42 ++-- crates/oak-timeline/src/undosplit.rs | 42 ++-- crates/oak-timeline/src/util.rs | 22 +- crates/oak-timeline/tests/domain_test.rs | 27 ++- .../oak-timeline/tests/undocommands_test.rs | 191 ++++++++---------- docs/zh/plans/test-coverage-90-80-review.md | 52 ++++- docs/zh/plans/test-coverage-90-80.md | 24 +++ 14 files changed, 374 insertions(+), 252 deletions(-) diff --git a/crates/oak-app/src/oakui/graphops.rs b/crates/oak-app/src/oakui/graphops.rs index 897852f15..da3f3b06a 100644 --- a/crates/oak-app/src/oakui/graphops.rs +++ b/crates/oak-app/src/oakui/graphops.rs @@ -44,8 +44,8 @@ use oak_node::track::{TrackBehavior, TrackListBehavior, TrackType}; use oak_node::value::VideoParams; use oak_timeline::handle::CHandle; use oak_timeline::util::{ - block_in, block_length, block_out, block_set_in, block_set_length_and_media_in, - block_set_length_and_media_out, clip_set_media_in, NodeRef, + block_in, block_length, block_out, block_set_in, block_set_length_and_media_out, + block_set_length_keeping_out, clip_set_media_in, NodeRef, }; use oak_storage::backend::StorageBackend; @@ -1565,7 +1565,8 @@ fn footage_display_name(g: &Graph, footage: NodeId) -> String { /// Create a footage clip block in `g`: the shared span/state setup plus /// the per-clip color and footage-name label, both fixed at creation /// time (C++ `oaknode_clip_set_media_in` + -/// `oaknode_block_set_length_and_media_in`). +/// `oaknode_block_set_length_and_media_out`; the place command writes the +/// in point). fn create_footage_clip( g: &mut Graph, footage: NodeId, @@ -1584,7 +1585,7 @@ fn create_footage_clip( .and_then(|a| a.downcast_mut::()) { c.core.media_in = media_in; - c.core.set_length_and_media_in(length); + c.core.set_length_and_media_out(length); } } id @@ -2363,7 +2364,7 @@ pub fn place_generator_clip( .as_any_mut() .and_then(|a| a.downcast_mut::()) { - c.core.set_length_and_media_in(length); + c.core.set_length_and_media_out(length); } } @@ -2545,11 +2546,6 @@ pub fn split_clips_preserving_links( ) } -/// Trim `clip`'s timeline range to `[new_in_ts, new_out_ts)` (undoable -/// "Trim Clip"; the facade's `oakengine_clip_trim` semantics: one end at -/// a time, trim-in anchors the OUT, trim-out anchors the IN — the -/// module's own `BlockTrimCommand` applies its length setters with -/// inverted semantics, so the closures carry the correct mapping). /// An undoable length change for `clip` (trim semantics: `out_anchored` /// keeps the out point — trim-in — and shifts the in; otherwise the in /// stays — trim-out). Shared by [`trim_clip`] and [`ripple_trim_clip`]; @@ -2567,9 +2563,9 @@ pub(crate) fn trim_command( let mut guard = lock(&p1); with_block_core_mut(&mut guard.graph, clip, |core| { if out_anchored { - core.set_length_and_media_out(new); + core.set_length_keeping_out(new); } else { - core.set_length_and_media_in(new); + core.set_length_and_media_out(new); } }); }, @@ -2577,9 +2573,9 @@ pub(crate) fn trim_command( let mut guard = lock(&p2); with_block_core_mut(&mut guard.graph, clip, |core| { if out_anchored { - core.set_length_and_media_out(old); + core.set_length_keeping_out(old); } else { - core.set_length_and_media_in(old); + core.set_length_and_media_out(old); } }); }, @@ -2588,9 +2584,9 @@ pub(crate) fn trim_command( /// Trim `clip`'s timeline range to `[new_in_ts, new_out_ts)` (undoable /// "Trim Clip"; the facade's `oakengine_clip_trim` semantics: one end at -/// a time, trim-in anchors the OUT, trim-out anchors the IN — the -/// module's own `BlockTrimCommand` applies its length setters with -/// inverted semantics, so the closures carry the correct mapping). +/// a time, trim-in anchors the OUT (its in moves and the media window +/// follows), trim-out anchors the IN (its out moves, media untouched) — +/// the same net anchors as the module's own `BlockTrimCommand`). pub fn trim_clip( p: &ProjectRef, clip: NodeId, @@ -2811,22 +2807,22 @@ pub fn slide_clip(p: &ProjectRef, clip: NodeId, new_start: i64) -> Result<(), St if new > block_in(&l_ref) { let (r1, r2) = (l_ref.clone(), l_ref); children.push(oak_undo::undocommand::UndoCommand::from_closures( - move || block_set_length_and_media_in(&r1, l_len + delta), - move || block_set_length_and_media_in(&r2, l_len), + move || block_set_length_and_media_out(&r1, l_len + delta), + move || block_set_length_and_media_out(&r2, l_len), )); } } - // Right neighbor: its in follows the clip's new out (out anchored, - // media untouched); skipped when it would collapse to a negative - // length. + // Right neighbor: its in follows the clip's new out while its content + // end stays anchored (out anchored; the media in advances over the + // consumed head); skipped when it would collapse to a negative length. if let Some(r) = right { let r_ref = node_ref(p, r); let r_len = block_length(&r_ref); if new + clip_len < block_out(&r_ref) { let (r1, r2) = (r_ref.clone(), r_ref); children.push(oak_undo::undocommand::UndoCommand::from_closures( - move || block_set_length_and_media_out(&r1, r_len - delta), - move || block_set_length_and_media_out(&r2, r_len), + move || block_set_length_keeping_out(&r1, r_len - delta), + move || block_set_length_keeping_out(&r2, r_len), )); } } @@ -6053,21 +6049,28 @@ mod gap_coverage_tests { assert!(roll_edit(&project, track, a, b, 10).is_ok(), "unmoved boundary"); roll_edit(&project, track, a, b, 12).expect("roll"); { - // NOTE: the module's TrimOut roll mapping anchors the left clip's - // OUT (its IN moves) and the follower's IN (its OUT moves); the - // shared seam itself does not move. Assert the actual outcome so - // a future semantic fix has to update this expectation - // deliberately (review §3.1 of test-coverage-90-80-review.md). + // The roll moves the shared seam to 12: the left clip keeps its + // in point and grows at its out (media window untouched), the + // follower keeps its out point and its head is consumed (its + // media in advances by the rolled amount). let g = lock(&project); assert_eq!( clip_range(&g.graph, a).unwrap(), ( - ts_to_rational(-2, tb), - ts_to_rational(10, tb), - ts_to_rational(-2, tb) + Rational::new(0, 1), + ts_to_rational(12, tb), + Rational::new(0, 1) ) ); - assert_eq!(clip_range(&g.graph, b).unwrap().1, ts_to_rational(18, tb)); + assert_eq!( + clip_range(&g.graph, b).unwrap(), + ( + ts_to_rational(12, tb), + ts_to_rational(20, tb), + ts_to_rational(2, tb) + ), + "the follower's head loses the 2 rolled frames from its media" + ); } oak_undo::global::undo().unwrap(); diff --git a/crates/oak-cli/src/engine.rs b/crates/oak-cli/src/engine.rs index 9144ecf39..a147e697f 100644 --- a/crates/oak-cli/src/engine.rs +++ b/crates/oak-cli/src/engine.rs @@ -411,14 +411,14 @@ pub fn place_footage_clip( }; // The clip block, positioned by media-in + length (the facade's - // `oaknode_clip_set_media_in` + `oaknode_block_set_length_and_media_in`). + // `oaknode_clip_set_media_in` + `oaknode_block_set_length_and_media_out`). let clip_id = { let mut guard = lock(project); let (core, behavior) = block::clip_create(); let id = guard.graph.add_node(core, behavior); if let Some(c) = clip_behavior_mut(&mut guard.graph, id) { c.core.media_in = media_r; - c.core.set_length_and_media_in(length); + c.core.set_length_and_media_out(length); } id }; diff --git a/crates/oak-node/src/block.rs b/crates/oak-node/src/block.rs index 4d656a571..49e304c0b 100644 --- a/crates/oak-node/src/block.rs +++ b/crates/oak-node/src/block.rs @@ -90,20 +90,35 @@ impl BlockCore { self.range = TimeRange::new(self.in_(), out); } - /// Set the length, keeping the media out anchored (C++ - /// `Block::set_length_and_media_out`): the timeline in-point shifts - /// so the out-point stays put, and the media in follows it. + /// Set the length, keeping the timeline in-point and the media in-point + /// anchored (C++ `Block::set_length_and_media_out`): the out-point + /// shifts (and the media out follows it). The stored-range model has no + /// track layout to derive the in-point from, so the caller arranges the + /// neighbouring blocks explicitly when a different anchor is wanted. pub fn set_length_and_media_out(&mut self, length: Rational) { - let out = self.in_() + self.length(); - self.range = TimeRange::new(out - length, out); - self.media_in = self.range.in_(); + self.range = TimeRange::new(self.in_(), self.in_() + length); } - /// Set the length, keeping the media in anchored (C++ - /// `Block::set_length_and_media_in`): the in-point stays, the - /// out-point shifts. + /// Set the length, keeping the timeline in-point and the MEDIA OUT + /// anchored (C++ `Block::set_length_and_media_in`): the media in-point + /// moves by `old_length - length` so the media content end stays put + /// while the timeline out-point follows the length. pub fn set_length_and_media_in(&mut self, length: Rational) { + let delta = self.length() - length; self.range = TimeRange::new(self.in_(), self.in_() + length); + self.media_in = self.media_in + delta; + } + + /// Set the length, keeping the timeline OUT-point and the media content + /// between the two ends anchored: the in-point shifts by the length + /// delta and the media in-point follows it, so the media out stays put + /// too (the stored-range model's net for a head trim, where the C++ + /// derives the new in-point from the resized previous block). + pub fn set_length_keeping_out(&mut self, length: Rational) { + let delta = self.length() - length; + let out = self.out(); + self.range = TimeRange::new(out - length, out); + self.media_in = self.media_in + delta; } /// Media out (in + length; C++ `Block::media_out`). diff --git a/crates/oak-node/tests/phase2_units_test.rs b/crates/oak-node/tests/phase2_units_test.rs index 1ae29b5c3..7b4c2c092 100644 --- a/crates/oak-node/tests/phase2_units_test.rs +++ b/crates/oak-node/tests/phase2_units_test.rs @@ -46,16 +46,27 @@ fn block_core_ranges() { core.set_out(Rational::new(9, 1)); assert_eq!(core.length(), Rational::new(4, 1)); - // media_out anchored: in shifts so out stays. + // `set_length_and_media_out`: the in point (and the media in point) + // stays, the out point follows the length. core.media_in = Rational::new(2, 1); core.set_length_and_media_out(Rational::new(3, 1)); - assert_eq!(core.out(), Rational::new(9, 1), "out stays put"); - assert_eq!(core.length(), Rational::new(3, 1)); + assert_eq!(core.in_(), Rational::new(5, 1), "in stays put"); + assert_eq!(core.out(), Rational::new(8, 1), "out follows the length"); + assert_eq!(core.media_in, Rational::new(2, 1), "media in untouched"); - // media_in anchored: in stays, out shifts. + // `set_length_and_media_in`: the in point stays while the media in + // moves with the length (the media out is what stays anchored). core.set_length_and_media_in(Rational::new(4, 1)); - assert_eq!(core.in_(), Rational::new(6, 1), "in stays put"); + assert_eq!(core.in_(), Rational::new(5, 1), "in stays put"); assert_eq!(core.length(), Rational::new(4, 1)); + assert_eq!(core.media_in, Rational::new(1, 1), "media in follows the length"); + + // `set_length_keeping_out`: the out point (and the media content end) + // stays, the in point shifts. + core.set_length_keeping_out(Rational::new(2, 1)); + assert_eq!(core.out(), Rational::new(9, 1), "out stays put"); + assert_eq!(core.in_(), Rational::new(7, 1), "in shifts"); + assert_eq!(core.media_in, Rational::new(3, 1), "media in follows the in point"); } /// block.rs: behavior constructors + node inputs. diff --git a/crates/oak-task/src/nodeops.rs b/crates/oak-task/src/nodeops.rs index b123f8b6f..649310104 100644 --- a/crates/oak-task/src/nodeops.rs +++ b/crates/oak-task/src/nodeops.rs @@ -774,7 +774,8 @@ pub fn block_media_in(project: &ProjectRef, block: NodeId) -> Rational { .unwrap_or_else(|| Rational::new(0, 1)) } -/// Set the block's length keeping the media out anchored +/// Set the block's length keeping the in point and the media in point +/// anchored — the out point (and the media out) shift /// (`oaknode_block_set_length_and_media_out`). pub fn block_set_length_and_media_out(project: &ProjectRef, block: NodeId, n: i64, d: i64) { let mut guard = lock_project(project); diff --git a/crates/oak-timeline/src/undogeneral.rs b/crates/oak-timeline/src/undogeneral.rs index 043a88295..6ed00fa3a 100644 --- a/crates/oak-timeline/src/undogeneral.rs +++ b/crates/oak-timeline/src/undogeneral.rs @@ -37,8 +37,9 @@ use crate::util::{ block_add_to_graph, block_connect, block_connected_input, block_disconnect_input, block_enabled, block_gap_create, block_in, block_kind, block_length, block_next, block_out, block_previous, block_range, block_remove_from_graph, block_set_enabled, block_set_in, - block_set_length_and_media_in, block_set_length_and_media_out, block_set_range, block_track, - clip_media_in, clip_set_media_in, same_block, track_append_block, track_create, + block_set_length_and_media_in, block_set_length_and_media_out, block_set_length_keeping_out, + block_set_range, block_track, clip_media_in, clip_set_media_in, same_block, track_append_block, + track_create, track_insert_block_after, track_insert_block_before, track_replace_block, track_ripple_remove_block, tracklist_append, tracklist_remove, tracklist_track_at, tracklist_track_count, tracklist_type, transition_offsets, @@ -865,11 +866,17 @@ impl TrackReplaceBlockWithGapCommand { if let Some(gap) = &self.existing_gap { // Extend an existing gap. In the module world the block's in/out // points are stored on the block (Olive derives them from the track - // order), so the extension must keep the gap's in point anchored — - // an out-anchored `set_length_and_media_out` would push the in point - // negative. + // order), so the extension must grow towards the block's span: + // a gap that PRECEDES the block grows rightward (in anchored), + // while a gap that merely FOLLOWS it must grow leftward to start + // at the block's in point (out anchored) — growing it at the out + // would swallow whatever follows the gap. new_gap_length = new_gap_length + block_length(gap); - block_set_length_and_media_in(gap, new_gap_length); + if previous_is_a_gap { + block_set_length_and_media_out(gap, new_gap_length); + } else { + block_set_length_keeping_out(gap, new_gap_length); + } track_ripple_remove_block(&self.track, &self.block); self.existing_gap_precedes = previous.as_ref().map(|p| same_block(gap, p)).unwrap_or(false); @@ -882,7 +889,7 @@ impl TrackReplaceBlockWithGapCommand { // (the module stores positions on the block; the gap // fills the block's stored span). block_set_in(gap, self.block_in_point); - block_set_length_and_media_in(gap, new_gap_length); + block_set_length_and_media_out(gap, new_gap_length); } } if let Some(gap) = &self.our_gap { @@ -935,9 +942,14 @@ impl TrackReplaceBlockWithGapCommand { track_insert_block_before(&self.track, &self.block, gap); } - // Restore the gap's original length (in-anchored: its in point was - // untouched by the redo extension). - block_set_length_and_media_in(gap, original_gap_length); + // Restore the gap's original length on the same anchor the redo + // grew it from (the in point for a preceding gap, the out point + // for a following one). + if self.existing_gap_precedes { + block_set_length_and_media_out(gap, original_gap_length); + } else { + block_set_length_keeping_out(gap, original_gap_length); + } self.existing_gap = None; } } else { @@ -1191,7 +1203,7 @@ impl TrackListInsertGaps { // explicitly. let gap_in = g.before.as_ref().map(block_out).unwrap_or(self.point); block_set_in(&g.gap, gap_in); - block_set_length_and_media_in(&g.gap, self.length); + block_set_length_and_media_out(&g.gap, self.length); block_add_to_graph(&g.gap, g.entry.take()); track_insert_block_after(&g.track, &g.gap, g.before.as_ref()); } diff --git a/crates/oak-timeline/src/undopointer.rs b/crates/oak-timeline/src/undopointer.rs index dc63f3691..ca192dbb2 100644 --- a/crates/oak-timeline/src/undopointer.rs +++ b/crates/oak-timeline/src/undopointer.rs @@ -34,7 +34,7 @@ use crate::undocommon::{ 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_track, + block_set_length_and_media_out, block_set_length_keeping_out, block_track, track_append_block, track_insert_block_after, track_insert_block_before, track_length, track_ripple_remove_block, tracklist_track_at, tracklist_track_count, BlockKind, NodeRef, }; @@ -159,18 +159,20 @@ impl BlockTrimCommand { if self.we_created_adjacent_ { // We shortened but don't have a viable adjacent to lengthen, so create - // one filling exactly the space the trim freed: for a trim-in it - // spans [in - diff, in), for a trim-out [out, out + diff). The - // module stores positions on the block, so both the in point and - // the length are written explicitly. + // one filling exactly the space the trim freed. The module stores + // positions on the block (Olive derives the inserted gap's position + // from the track order), so the in point is the freed space's start + // and the length closes the gap up to the block's new edge: + // trim-in frees [in, in + diff) (the gap is inserted before the + // block), trim-out frees [out - diff, out) (inserted after it). self.adjacent_ = Some(block_gap_create(&self.track.project)); if let Some(gap) = &self.adjacent_ { if self.mode == MovementMode::TrimIn { - block_set_in(gap, block_in(&self.block) - self.trim_diff_); + block_set_in(gap, block_in(&self.block)); } else { - block_set_in(gap, block_out(&self.block)); + block_set_in(gap, block_out(&self.block) - self.trim_diff_); } - block_set_length_and_media_in(gap, self.trim_diff_); + block_set_length_and_media_out(gap, self.trim_diff_); } } else if let Some(adjacent) = &self.adjacent_ { // Determine if we're removing the adjacent @@ -187,8 +189,10 @@ impl BlockTrimCommand { } if self.mode == MovementMode::TrimIn { - block_set_length_and_media_in(&self.block, self.new_length); + // Trim-in keeps the out fixed and moves the in (media follows). + block_set_length_keeping_out(&self.block, self.new_length); } else { + // Trim-out keeps the in fixed and moves the out (media untouched). block_set_length_and_media_out(&self.block, self.new_length); } @@ -221,9 +225,13 @@ impl BlockTrimCommand { let adjacent_length = block_length(adjacent) + self.trim_diff_; if self.mode == MovementMode::TrimIn { + // The previous neighbour grows/shrinks at its out edge + // to meet the trimmed in point. block_set_length_and_media_out(adjacent, adjacent_length); } else { - block_set_length_and_media_in(adjacent, adjacent_length); + // The next neighbour grows/shrinks at its in edge to + // meet the trimmed out point (its content end anchored). + block_set_length_keeping_out(adjacent, adjacent_length); } } } @@ -264,14 +272,14 @@ impl BlockTrimCommand { if self.mode == MovementMode::TrimIn { block_set_length_and_media_out(adjacent, adjacent_length); } else { - block_set_length_and_media_in(adjacent, adjacent_length); + block_set_length_keeping_out(adjacent, adjacent_length); } } } } if self.mode == MovementMode::TrimIn { - block_set_length_and_media_in(&self.block, self.old_length_); + block_set_length_keeping_out(&self.block, self.old_length_); } else { block_set_length_and_media_out(&self.block, self.old_length_); } @@ -365,7 +373,7 @@ impl TrackSlideCommand { // of the first block. Positions are stored on the block in // the module model, so both ends are written explicitly. block_set_in(gap, block_in(&self.blocks[0]) + self.movement); - block_set_length_and_media_in(gap, Rational::new(0, 1) - self.movement); + block_set_length_and_media_out(gap, Rational::new(0, 1) - self.movement); } self.we_created_in_adjacent_ = true; } else { @@ -381,7 +389,7 @@ impl TrackSlideCommand { // (movement > 0): the gap fills [last_out, last_out + movement) // after the last block. block_set_in(gap, block_out(self.blocks.last().expect("non-empty blocks"))); - block_set_length_and_media_in(gap, self.movement); + block_set_length_and_media_out(gap, self.movement); } self.we_created_out_adjacent_ = true; } else { @@ -390,6 +398,12 @@ impl TrackSlideCommand { } /// `redo`: apply the slide. + /// + /// The C++ derives the sliding blocks' new positions from the track + /// layout (the resized neighbours shift them implicitly); the + /// stored-range model has no layout, so the CALLER positions the + /// `blocks` by `movement` before running this command — it only owns + /// the adjacent resize/insert/remove half of the slide. pub fn redo(&mut self) { // We will always have an in adjacent if there was a valid slide if self.we_created_in_adjacent_ { @@ -452,16 +466,15 @@ impl TrackSlideCommand { self.we_removed_out_adjacent_ = true; } else { - // Simply resize the adjacent - block_set_length_and_media_in( - adjacent, - block_length(adjacent) - self.movement, - ); + // Simply resize the adjacent: its in edge follows the last + // sliding block's out, so the content end stays anchored. + block_set_length_keeping_out(adjacent, block_length(adjacent) - self.movement); } } } - /// `undo`: revert the slide. + /// `undo`: revert the slide (the caller restores the moved blocks + /// alongside this, mirroring how it positioned them for the redo). pub fn undo(&mut self) { if self.we_created_in_adjacent_ { // We created this, so we can remove it now @@ -508,11 +521,8 @@ impl TrackSlideCommand { Some(self.blocks.last().expect("non-empty blocks")), ); } else { - // Simply resize the adjacent - block_set_length_and_media_in( - adjacent, - block_length(adjacent) + self.movement, - ); + // Simply resize the adjacent (mirror of the redo). + block_set_length_keeping_out(adjacent, block_length(adjacent) + self.movement); } } } @@ -625,7 +635,7 @@ impl TrackPlaceBlockCommand { self.gap_ = Some(block_gap_create(&self.timeline.project)); if let Some(gap) = &self.gap_ { block_set_in(gap, track_length(&track)); - block_set_length_and_media_in(gap, in_ - track_length(&track)); + block_set_length_and_media_out(gap, in_ - track_length(&track)); } } if let Some(gap) = &self.gap_ { diff --git a/crates/oak-timeline/src/undoripple.rs b/crates/oak-timeline/src/undoripple.rs index 5d05516af..e86234227 100644 --- a/crates/oak-timeline/src/undoripple.rs +++ b/crates/oak-timeline/src/undoripple.rs @@ -41,8 +41,8 @@ use crate::undocommon::{ 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_track, - sequence_all_tracks, sequence_track_list, + 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, 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, @@ -335,10 +335,11 @@ impl TrackRippleRemoveAreaCommand { let cmd = self.splice_split_command_.as_mut().unwrap(); cmd.redo(); - // Trim the in of the split (the second half produced by the - // split keeps the original out point; the in-end trim shifts - // its stored in point to the range out — the module equivalent - // of the C++ ripple shifting the remainder earlier). + // Trim the in of the split: the second half produced by the + // split keeps the original out point, so removing the first + // `range.length()` of it keeps the timeline in fixed at the + // range out and advances the media window past the removed + // content (in-anchored, media in moves with the length). if let Some(split) = cmd.new_block() { let new_len = block_length(&split) - (self.range.out() - block_in(&split)); @@ -346,15 +347,15 @@ impl TrackRippleRemoveAreaCommand { } } else { if let Some(t) = &self.trim_out_ { - // An out-end trim keeps the in point (in-anchored; the - // C++ setter name is kept, but the module's in/out are - // stored values, see the splice trim above). - block_set_length_and_media_in(&t.block, t.new_length); + // An out-end trim keeps the in point and the media window + // (in-anchored). + block_set_length_and_media_out(&t.block, t.new_length); } if let Some(t) = &self.trim_in_ { - // An in-end trim keeps the out point (out-anchored). - block_set_length_and_media_out(&t.block, t.new_length); + // An in-end trim keeps the out point and advances the media + // window past the removed head. + block_set_length_keeping_out(&t.block, t.new_length); } // Perform removals @@ -389,12 +390,12 @@ impl TrackRippleRemoveAreaCommand { } else { if let Some(t) = &self.trim_out_ { // In-anchored, matching the redo (see above). - block_set_length_and_media_in(&t.block, t.old_length); + block_set_length_and_media_out(&t.block, t.old_length); } if let Some(t) = &self.trim_in_ { // Out-anchored, matching the redo (see above). - block_set_length_and_media_out(&t.block, t.old_length); + block_set_length_keeping_out(&t.block, t.old_length); } // Un-remove any blocks @@ -699,14 +700,15 @@ impl TrackListRippleToolCommand { if redo { if wd.created_gap.is_none() { let gap = block_gap_create(&track_copy.project); - // The gap takes the ripple movement's span ahead of - // `b`; positions are stored on the block in the + // The gap takes the ripple movement's span at `b`'s + // in point; positions are stored on the block in the // module model, so both ends are written explicitly - // (approximation: the C++ ripple additionally shifts - // the successors, which the stored positions render - // as an overlap-free gap right before `b`). + // (approximation: the C++ layout additionally shifts + // `b` and its successors right by that span — the + // stored-range command leaves that shift to the + // caller). block_set_in(&gap, block_in(&b)); - block_set_length_and_media_in( + block_set_length_and_media_out( &gap, if self.ripple_movement < Rational::new(0, 1) { rat_neg(self.ripple_movement) diff --git a/crates/oak-timeline/src/undosplit.rs b/crates/oak-timeline/src/undosplit.rs index 751775414..8268bc000 100644 --- a/crates/oak-timeline/src/undosplit.rs +++ b/crates/oak-timeline/src/undosplit.rs @@ -35,9 +35,9 @@ use oak_node::id::NodeId; use oak_undo::undocommand::UndoCommand; use crate::util::{ - block_in, block_length, block_out, block_set_length_and_media_in, - block_set_length_and_media_out, block_track, track_insert_block_after, - track_ripple_remove_block, GraphBlockRange, NodeRef, + block_in, block_length, block_out, block_set_length_and_media_out, block_set_range, block_track, + clip_media_in, clip_set_media_in, track_insert_block_after, track_ripple_remove_block, + GraphBlockRange, NodeRef, }; /// `BlockSplitCommand` — split one block at a point @@ -177,10 +177,11 @@ impl BlockSplitCommand { /// second half, and insert it after `block`. /// /// The split keeps the ORIGINAL block anchored at its in-point (the - /// C++ `set_length_and_media_in` on the original) and anchors the - /// cloned `new_block` at its out-point (the C++ - /// `set_length_and_media_out` on the copy — the copy carried the - /// original span, so the out point is preserved exactly). + /// C++ `set_length_and_media_out` on the original) and anchors the + /// cloned `new_block` between the split point and the original out + /// point (the C++ `set_length_and_media_in` on the copy — the copy + /// carried the original span, so the out point is preserved exactly), + /// with its media window continuing where the first half stops. pub fn redo(&mut self) { // Create the second half if redo is invoked without a preceding // prepare() (the oakundo command path may call redo directly). @@ -199,14 +200,19 @@ impl BlockSplitCommand { let first_half_length = self.point - block_in; let second_half_length = block_out - self.point; + // The second half continues the media exactly where the first half + // stops (C++ `ClipBlock::set_length_and_media_in` on the copy). + let media_in = clip_media_in(&self.block); if let Some(new_block) = &self.new_block { - // In-anchored length for the first half keeps the original's in - // point (the C++ `set_length_and_media_in`); the out-anchored - // length for the second half keeps the copy's out point (the - // C++ `set_length_and_media_out`). - block_set_length_and_media_in(&self.block, first_half_length); - block_set_length_and_media_out(new_block, second_half_length); + // First half: the original's in point stays and its out lands on + // the split point (in-anchored, media unchanged). + block_set_length_and_media_out(&self.block, first_half_length); + // Second half: the split point up to the original out point. The + // stored-range model has no track layout to derive these from, so + // both ends are written explicitly along with the media window. + block_set_range(new_block, oak_core::TimeRange::new(self.point, self.point + second_half_length)); + clip_set_media_in(new_block, media_in + first_half_length); if let Some(track) = block_track(&self.block) { track_insert_block_after(&track, new_block, Some(&self.block)); @@ -223,12 +229,10 @@ impl BlockSplitCommand { /// and detach the whole copied subgraph from the project graph. pub fn undo(&mut self) { if let Some(track) = block_track(&self.block) { - // The redo shrank the original from its in point (it became the - // first half, in-anchored), so the restore grows it in-anchored - // too — the module stores the span on the block (the C++ - // `set_length_and_media_out` derives positions from the track - // order and does not apply here). - block_set_length_and_media_in(&self.block, self.old_length); + // The redo shrank the original from its out point (it became the + // first half, in-anchored), so the restore grows it at the out + // again; the media window was never touched. + block_set_length_and_media_out(&self.block, self.old_length); if let Some(new_block) = &self.new_block { track_ripple_remove_block(&track, new_block); } diff --git a/crates/oak-timeline/src/util.rs b/crates/oak-timeline/src/util.rs index 766244e7c..db45cbd55 100644 --- a/crates/oak-timeline/src/util.rs +++ b/crates/oak-timeline/src/util.rs @@ -311,8 +311,8 @@ pub fn block_set_enabled(b: &NodeRef, enabled: bool) { } } -/// `block_set_length_and_media_out`: keep the out point anchored (the -/// in point shifts). +/// `block_set_length_and_media_out`: keep the in point anchored (the out +/// point shifts; the media in point does not move). pub fn block_set_length_and_media_out(b: &NodeRef, len: Rational) { let mut p = b.lock(); if let Some(core) = block_core_of_mut(&mut p, b.id) { @@ -320,8 +320,9 @@ pub fn block_set_length_and_media_out(b: &NodeRef, len: Rational) { } } -/// `block_set_length_and_media_in`: keep the in point anchored (the -/// out point shifts). +/// `block_set_length_and_media_in`: keep the in point anchored (the out +/// point shifts) while the media in point follows the length change, so +/// the media out stays anchored. pub fn block_set_length_and_media_in(b: &NodeRef, len: Rational) { let mut p = b.lock(); if let Some(core) = block_core_of_mut(&mut p, b.id) { @@ -329,6 +330,16 @@ pub fn block_set_length_and_media_in(b: &NodeRef, len: Rational) { } } +/// Keep the OUT point anchored (the in point shifts) and move the media +/// in point with it: the net of a head trim in the stored-range model +/// (the C++ derives the shifted in-point from the resized previous block). +pub fn block_set_length_keeping_out(b: &NodeRef, len: Rational) { + let mut p = b.lock(); + if let Some(core) = block_core_of_mut(&mut p, b.id) { + core.set_length_keeping_out(len); + } +} + /// `block_set_in`: set the in point, keeping the length (the out follows). pub fn block_set_in(b: &NodeRef, in_: Rational) { let mut p = b.lock(); @@ -364,7 +375,8 @@ pub fn block_range(b: &NodeRef) -> Option { /// Set the block's stored timeline range (a no-op for a stale/non-block /// node). The range is written whole, so the caller can move the in/out /// pair together without a media-in side effect (unlike -/// [`block_set_length_and_media_out`] / [`block_set_length_and_media_in`]). +/// [`block_set_length_and_media_in`] / [`block_set_length_keeping_out`], +/// which adjust the media in point with the length). pub fn block_set_range(b: &NodeRef, range: oak_core::TimeRange) { let mut p = b.lock(); if let Some(core) = block_core_of_mut(&mut p, b.id) { diff --git a/crates/oak-timeline/tests/domain_test.rs b/crates/oak-timeline/tests/domain_test.rs index 7a3f571cf..35f8f97fe 100644 --- a/crates/oak-timeline/tests/domain_test.rs +++ b/crates/oak-timeline/tests/domain_test.rs @@ -254,7 +254,10 @@ fn split_command_redo_undo() { } /// `TrackReplaceBlockWithGapCommand` merges a clip into a following gap on -/// redo and restores both blocks on undo. +/// redo and restores both blocks on undo. The gap grows LEFTWARD over the +/// clip's span (out anchored): growing it at the out instead would swallow +/// the clip that follows the gap — the "dragging one clip moved unrelated +/// clips" regression. #[test] fn replace_block_with_gap_round_trip() { let project = make_project(); @@ -262,19 +265,23 @@ fn replace_block_with_gap_round_trip() { let track = TimelineAddTrackCommand::run_immediately(list); let clip = add_clip(&track, Rational::new(0, 1), Rational::new(50, 1)); let gap = add_gap(&track, Rational::new(50, 1), Rational::new(100, 1)); + let tail = add_clip(&track, Rational::new(100, 1), Rational::new(150, 1)); let mut cmd = TrackReplaceBlockWithGapCommand::new(track.clone(), clip.clone(), false); cmd.redo(); - // The clip is gone; the gap absorbed its length (in-anchored at 50). - assert_eq!(track_block_count(&track), 1); + // The clip is gone; the FOLLOWING gap grew leftward over its span. + assert_eq!(track_block_count(&track), 2); let remaining = track_block_at(&track, 0).unwrap(); assert_eq!(remaining.id, gap.id); - assert_eq!(block_in(&remaining), Rational::new(50, 1)); + assert_eq!(block_in(&remaining), Rational::new(0, 1)); assert_eq!(block_length(&remaining), Rational::new(100, 1)); assert!(block_track(&clip).is_none()); + assert_eq!(track_block_at(&track, 1).unwrap().id, tail.id); + assert_eq!(block_in(&tail), Rational::new(100, 1)); + assert_eq!(block_out(&tail), Rational::new(150, 1), "the clip after the gap is untouched"); cmd.undo(); - assert_eq!(track_block_count(&track), 2); + assert_eq!(track_block_count(&track), 3); let first = track_block_at(&track, 0).unwrap(); assert_eq!(first.id, clip.id); assert_eq!(block_in(&first), Rational::new(0, 1)); @@ -283,6 +290,9 @@ fn replace_block_with_gap_round_trip() { assert_eq!(second.id, gap.id); assert_eq!(block_in(&second), Rational::new(50, 1)); assert_eq!(block_length(&second), Rational::new(50, 1)); + assert_eq!(track_block_at(&track, 2).unwrap().id, tail.id); + assert_eq!(block_in(&tail), Rational::new(100, 1)); + assert_eq!(block_out(&tail), Rational::new(150, 1)); } /// `TrackReplaceBlockWithGapCommand` creates its own gap when no @@ -358,7 +368,7 @@ fn ripple_remove_area_round_trip() { ); } -/// `BlockResizeCommand` changes a block's length out-anchored and restores +/// `BlockResizeCommand` changes a block's length in-anchored and restores /// it on undo. #[test] fn block_resize_round_trip() { @@ -370,8 +380,9 @@ fn block_resize_round_trip() { let mut cmd = BlockResizeCommand::new(clip.clone(), Rational::new(30, 1)); cmd.redo(); assert_eq!(block_length(&clip), Rational::new(30, 1)); - // Out-anchored: the in point shifts so the out stays at 50. - assert_eq!(block_out(&clip), Rational::new(50, 1)); + // In-anchored: the in point stays so the out follows the length. + assert_eq!(block_in(&clip), Rational::new(0, 1)); + assert_eq!(block_out(&clip), Rational::new(30, 1)); cmd.undo(); assert_eq!(block_length(&clip), Rational::new(50, 1)); assert_eq!(block_in(&clip), Rational::new(0, 1)); diff --git a/crates/oak-timeline/tests/undocommands_test.rs b/crates/oak-timeline/tests/undocommands_test.rs index 18404c061..eea38cda1 100644 --- a/crates/oak-timeline/tests/undocommands_test.rs +++ b/crates/oak-timeline/tests/undocommands_test.rs @@ -478,9 +478,12 @@ fn timeline_ripple_delete_gaps_resizes_longer_gap() { assert!(cmd.has_commands()); cmd.redo(); - // The gap keeps its out point and loses the 20-frame region length. + // The gap keeps its in point and loses the 20-frame region length. In + // the C++ the following clip also ripples left by the removed length + // (the layout derives it); the stored-range command trims the gap only, + // so callers that need the tail shifted do it explicitly. assert_eq!(track_block_count(&track), 3); - assert_eq!(span_of(&gap), Some((Rational::new(70, 1), Rational::new(150, 1)))); + assert_eq!(span_of(&gap), Some((Rational::new(50, 1), Rational::new(130, 1)))); cmd.undo(); assert_eq!(span_of(&gap), Some((Rational::new(50, 1), Rational::new(150, 1)))); @@ -556,14 +559,9 @@ fn track_list_ripple_tool_empty_info_is_noop() { } /// `TrackListRippleToolCommand` with real per-track info resizes the block -/// and restores it on undo. -/// -/// KNOWN-SWAP: the expectations below pin the CURRENT (swapped) -/// `set_length_and_media_out` behavior documented in -/// `docs/zh/plans/test-coverage-90-80-review.md` §3.1/§9 — the trim ripples the -/// in-point and writes the timeline in into `media_in` instead of moving the -/// out-point. The semantic fix must rewrite these values deliberately -/// (C++ `BlockTrimCommand::redo` kTrimOut keeps the in-point and moves out). +/// and restores it on undo. Trim-out is in-anchored: the clip keeps its in +/// point and its out follows the movement, while the media window (which +/// the ripple does not consume here) stays put. #[test] fn track_list_ripple_tool_resizes_with_real_info() { let project = make_project(); @@ -583,10 +581,10 @@ fn track_list_ripple_tool_resizes_with_real_info() { cmd.redo(); assert_eq!( span_of(&clip), - Some((Rational::new(-10, 1), Rational::new(100, 1))), - "current (swapped) behavior: the in-point shifts, the out-point stays" + Some((Rational::new(0, 1), Rational::new(110, 1))), + "the in-point stays and the out-point follows the movement" ); - assert_eq!(clip_media_in(&clip), Rational::new(-10, 1)); + assert_eq!(clip_media_in(&clip), Rational::new(0, 1)); cmd.undo(); assert_eq!(span_of(&clip), Some((Rational::new(0, 1), Rational::new(100, 1)))); @@ -610,8 +608,9 @@ fn track_list_ripple_tool_append_gap_creates_a_gap() { ); cmd.redo(); assert_eq!(track_block_count(&track), 2, "a gap was inserted"); - // KNOWN-SWAP: the inserted gap overlaps the clip (its in is the clip's - // in); §3.1/§9's fix will move it to the clip's out. + // The gap takes the block's in point; the stored-range command leaves + // the block in place (the C++ layout would push it right by the gap + // length — the caller owns that shift, see the insert site's note). let gap = track_block_at(&track, 0).expect("gap at the front"); assert_eq!(block_kind(&gap), BlockKind::Gap); assert_eq!(span_of(&gap), Some((Rational::new(0, 1), Rational::new(10, 1)))); @@ -629,12 +628,9 @@ fn track_list_ripple_tool_append_gap_creates_a_gap() { /// `BlockTrimCommand` shortens a clip and lengthens its adjacent gap so /// the rest of the track keeps its position; undo restores both. /// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): -/// the expectations below pin the current (swapped) -/// `set_length_and_media_out` behavior — the trim moves the clip's in-point -/// instead of its out-point. C++ `BlockTrimCommand` TrimOut is in-anchored -/// (`common.rs` `k_trim_out`: trim the out point); the semantic fix must -/// re-derive this geometry. +/// Trim-out is in-anchored (`common.rs` `k_trim_out`: trim the out point): +/// the clip keeps its in and the following gap grows leftward to meet the +/// new out, so everything after the gap stays put. #[test] fn block_trim_with_gap_adjacent_round_trip() { let project = make_project(); @@ -651,27 +647,22 @@ fn block_trim_with_gap_adjacent_round_trip() { ); cmd.prepare(); cmd.redo(); - // Current (swapped) behavior: the clip's in shifts to 25 and the gap - // keeps its in at 50, growing its out by the 25 trimmed away. The - // in-anchored C++ TrimOut would keep the in and move the out instead - // (see the KNOWN-SWAP note). - assert_eq!(span_of(&clip), Some((Rational::new(25, 1), Rational::new(50, 1)))); - assert_eq!(span_of(&gap), Some((Rational::new(50, 1), Rational::new(125, 1)))); + assert_eq!(span_of(&clip), Some((Rational::new(0, 1), Rational::new(25, 1)))); + assert_eq!(span_of(&gap), Some((Rational::new(25, 1), Rational::new(100, 1)))); + assert_eq!( + clip_media_in(&clip), + Rational::new(0, 1), + "a trim-out does not move the media window" + ); cmd.undo(); assert_eq!(span_of(&clip), Some((Rational::new(0, 1), Rational::new(50, 1)))); assert_eq!(span_of(&gap), Some((Rational::new(50, 1), Rational::new(100, 1)))); + assert_eq!(clip_media_in(&clip), Rational::new(0, 1)); } /// `BlockTrimCommand` inserts a gap filling the space the trim freed when /// the adjacent block is a clip and the trim is not a roll edit. -/// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): -/// the geometry below pins the swapped `set_length_and_media_out` behavior -/// — the swapped trim moved `first`'s in to 25 and inserted the -/// compensating gap at [50,75), sharing its in with `second` (the true -/// in-anchored TrimOut keeps `first` at [0,25); the semantic fix must -/// rewrite this geometry). #[test] fn block_trim_creates_gap_next_to_clip() { let project = make_project(); @@ -692,11 +683,10 @@ fn block_trim_creates_gap_next_to_clip() { assert_eq!(track_block_at(&track, 0).unwrap().id, first.id); let created = track_block_at(&track, 1).unwrap(); assert_eq!(block_kind(&created), BlockKind::Gap); - // KNOWN-SWAP: the swapped trim shrinks `first` from the in side, so the - // "compensating" gap lands at [50,75) and overlaps `second` instead of - // filling the freed [25,50) space (§3.1/§9). - assert_eq!(span_of(&first), Some((Rational::new(25, 1), Rational::new(50, 1)))); - assert_eq!(span_of(&created), Some((Rational::new(50, 1), Rational::new(75, 1)))); + // The trim keeps `first`'s in fixed and moves its out to 25; the fresh + // gap fills the freed [25,50) space so `second` does not move. + assert_eq!(span_of(&first), Some((Rational::new(0, 1), Rational::new(25, 1)))); + assert_eq!(span_of(&created), Some((Rational::new(25, 1), Rational::new(50, 1)))); assert_eq!(track_block_at(&track, 2).unwrap().id, second.id); cmd.undo(); @@ -706,13 +696,8 @@ fn block_trim_creates_gap_next_to_clip() { } /// `BlockTrimCommand::set_trim_is_a_roll_edit` trims into the adjacent -/// clip instead of creating a compensating gap. -/// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): the -/// expectations below characterize the swapped `set_length_and_media_out` -/// behavior — the seam does NOT move; the left clip's IN shifts and the -/// follower's OUT grows. A true roll edit is first=[0,25) / second=[25,100) -/// with the seam at 25; the semantic fix must rewrite these values. +/// clip instead of creating a compensating gap: the shared seam moves, the +/// left clip keeps its in, the follower keeps its out. #[test] fn block_trim_roll_edit_resizes_adjacent_clip() { let project = make_project(); @@ -731,23 +716,29 @@ fn block_trim_roll_edit_resizes_adjacent_clip() { cmd.prepare(); cmd.redo(); assert_eq!(track_block_count(&track), 2); - assert_eq!(span_of(&first), Some((Rational::new(25, 1), Rational::new(50, 1)))); - assert_eq!(span_of(&second), Some((Rational::new(50, 1), Rational::new(125, 1)))); - // The seam stays at 50 under the current mapping (see the KNOWN-SWAP - // note); `media_in` pins the coordinate-space confusion: the left clip's - // timeline in (25) is written into `media_in`, and the follower's - // media-in is left untouched. + assert_eq!(span_of(&first), Some((Rational::new(0, 1), Rational::new(25, 1)))); + assert_eq!(span_of(&second), Some((Rational::new(25, 1), Rational::new(100, 1)))); assert_eq!( span_of(&first).map(|(_, out)| out), span_of(&second).map(|(in_, _)| in_), - "the shared seam does not move under the current mapping" + "the two clips keep sharing the rolled seam" + ); + assert_eq!( + clip_media_in(&first), + Rational::new(0, 1), + "the left clip's media window is untouched (trim-out)" + ); + assert_eq!( + clip_media_in(&second), + Rational::new(-25, 1), + "the follower grows at its head, revealing earlier media" ); - assert_eq!(clip_media_in(&first), Rational::new(25, 1)); - assert_eq!(clip_media_in(&second), Rational::new(0, 1)); cmd.undo(); assert_eq!(span_of(&first), Some((Rational::new(0, 1), Rational::new(50, 1)))); assert_eq!(span_of(&second), Some((Rational::new(50, 1), Rational::new(100, 1)))); + assert_eq!(clip_media_in(&first), Rational::new(0, 1)); + assert_eq!(clip_media_in(&second), Rational::new(0, 1)); } /// `BlockTrimCommand` (default `set_remove_zero_length_from_graph`) removes @@ -834,14 +825,12 @@ fn block_trim_same_length_is_noop() { } /// `TrackSlideCommand` resizes both adjacent blocks by the movement; -/// undo restores their original lengths. -/// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): -/// under the current setters the slid block itself does not move; the -/// previous block grows leftward into the slide's start (reaching a negative -/// timeline in-point) while the next block's out shrinks by the movement. -/// These expectations characterize that state; the semantic fix must rewrite -/// them to a true slide. +/// undo restores their original lengths. The previous neighbour grows at +/// its out edge (in anchored; no negative in-point) and the next shrinks at +/// its in edge (out anchored, media advancing over the consumed head). The +/// command does not position the sliding blocks itself — the stored-range +/// model has no track layout, so the caller places them by `movement` (see +/// `graphops::slide_clip`). #[test] fn track_slide_resizes_adjacent_blocks() { let project = make_project(); @@ -860,22 +849,30 @@ fn track_slide_resizes_adjacent_blocks() { ); cmd.prepare(); cmd.redo(); - assert_eq!(span_of(&previous), Some((Rational::new(-20, 1), Rational::new(50, 1)))); + assert_eq!(span_of(&previous), Some((Rational::new(0, 1), Rational::new(70, 1)))); assert_eq!(span_of(&slide), Some((Rational::new(50, 1), Rational::new(100, 1)))); - assert_eq!(span_of(&next), Some((Rational::new(100, 1), Rational::new(130, 1)))); + assert_eq!(span_of(&next), Some((Rational::new(120, 1), Rational::new(150, 1)))); + assert_eq!( + clip_media_in(&previous), + Rational::new(0, 1), + "the in-anchored growth leaves the media window alone" + ); + assert_eq!( + clip_media_in(&next), + Rational::new(20, 1), + "the next block's head is consumed, advancing its media in" + ); cmd.undo(); assert_eq!(span_of(&previous), Some((Rational::new(0, 1), Rational::new(50, 1)))); assert_eq!(span_of(&next), Some((Rational::new(100, 1), Rational::new(150, 1)))); + assert_eq!(clip_media_in(&previous), Rational::new(0, 1)); + assert_eq!(clip_media_in(&next), Rational::new(0, 1)); } /// `TrackSlideCommand` removes the out adjacent when the movement exactly -/// consumes it; undo re-attaches it after the moving block. -/// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): -/// the swapped setters leave the slid clip in place and grow `in_gap`'s in -/// leftward (a negative timeline in-point) instead of resizing it on the -/// slide's trailing side; the semantic fix must rewrite this geometry. +/// consumes it; undo re-attaches it after the moving block. The in adjacent +/// grows at its out edge (in anchored) while the out adjacent disappears. #[test] fn track_slide_removes_out_adjacent_at_boundary() { let project = make_project(); @@ -897,9 +894,7 @@ fn track_slide_removes_out_adjacent_at_boundary() { assert_eq!(track_block_count(&track), 2); assert!(block_track(&out_gap).is_none()); assert!(!node_in_graph(&project, &out_gap)); - // KNOWN-SWAP: the gap grew leftward past zero because the slide did not - // move the clip (§3.1/§9). - assert_eq!(span_of(&in_gap), Some((Rational::new(-20, 1), Rational::new(20, 1)))); + assert_eq!(span_of(&in_gap), Some((Rational::new(0, 1), Rational::new(40, 1)))); cmd.undo(); assert_eq!(track_block_count(&track), 3); @@ -910,12 +905,8 @@ fn track_slide_removes_out_adjacent_at_boundary() { } /// `TrackSlideCommand` removes the in adjacent when a leftward movement -/// exactly consumes it; undo re-attaches it before the moving block. -/// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): -/// the swapped setters leave the slid clip in place and grow `out_gap`'s out -/// rightward instead of resizing it on the slide's leading side; the -/// semantic fix must rewrite this geometry. +/// exactly consumes it; undo re-attaches it before the moving block. The +/// out adjacent grows at its in edge (out anchored) as the clip slides left. #[test] fn track_slide_removes_in_adjacent_at_boundary() { let project = make_project(); @@ -937,9 +928,7 @@ fn track_slide_removes_in_adjacent_at_boundary() { assert_eq!(track_block_count(&track), 2); assert!(block_track(&in_gap).is_none()); assert!(!node_in_graph(&project, &in_gap)); - // KNOWN-SWAP: the gap grew rightward because the slide did not move the - // clip (§3.1/§9). - assert_eq!(span_of(&out_gap), Some((Rational::new(70, 1), Rational::new(110, 1)))); + assert_eq!(span_of(&out_gap), Some((Rational::new(50, 1), Rational::new(90, 1)))); cmd.undo(); assert_eq!(track_block_count(&track), 3); @@ -1230,15 +1219,9 @@ fn block_split_preserving_links_noop_time() { // --------------------------------------------------------------------------- /// `BlockResizeWithMediaInCommand` resizes keeping the timeline in-point -/// fixed (C++ `ClipBlock::set_length_and_media_in` adjusts `media_in` so the -/// media out stays put); undo restores the original length. -/// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): -/// today neither setter adjusts `media_in`, so the media -/// out is silently shortened with the block (media_in stays 0 here). The -/// assertion below pins that current state so the semantic fix must change -/// it deliberately (the fixed behavior yields `media_in = 20` for this -/// resize, keeping `media_out = 50`). +/// fixed while `media_in` follows the length change, so the media content +/// end stays anchored (C++ `ClipBlock::set_length_and_media_in`); undo +/// restores the original length and media window. #[test] fn block_resize_with_media_in_round_trip() { let project = make_project(); @@ -1249,7 +1232,11 @@ fn block_resize_with_media_in_round_trip() { let mut cmd = BlockResizeWithMediaInCommand::new(clip.clone(), Rational::new(30, 1)); cmd.redo(); assert_eq!(span_of(&clip), Some((Rational::new(0, 1), Rational::new(30, 1)))); - assert_eq!(clip_media_in(&clip), Rational::new(0, 1)); + assert_eq!( + clip_media_in(&clip), + Rational::new(20, 1), + "media_in advances by the trimmed 20 frames (media_out stays at 50)" + ); cmd.undo(); assert_eq!(span_of(&clip), Some((Rational::new(0, 1), Rational::new(50, 1)))); assert_eq!(clip_media_in(&clip), Rational::new(0, 1)); @@ -1302,12 +1289,10 @@ fn block_enable_disable_round_trip() { } /// `TrackListInsertGaps` extends an existing gap crossed by the insertion -/// point instead of adding a second one; undo restores its length. -/// -/// KNOWN-SWAP (§3.1/§9 of `docs/zh/plans/test-coverage-90-80-review.md`): -/// the gap grows leftwards across the insertion point -/// and overlaps the preceding block; the fixed `_media_out`/`_media_in` -/// semantics must re-derive this geometry. +/// point instead of adding a second one; undo restores its length. The +/// extension is in-anchored (the insertion pushes the following content +/// rightward; the C++ derives that shift from the track layout, while this +/// caller-facing command leaves it to whoever places the followers). #[test] fn track_list_insert_gaps_extends_existing_gap() { let project = make_project(); @@ -1321,9 +1306,9 @@ fn track_list_insert_gaps_extends_existing_gap() { TrackListInsertGaps::new(list, Rational::new(75, 1), Rational::new(20, 1)); cmd.prepare(); cmd.redo(); - // The gap keeps its out point and grows leftward by the gap length. + // The gap keeps its in point and grows rightward by the gap length. assert_eq!(track_block_count(&track), 3); - assert_eq!(span_of(&gap), Some((Rational::new(30, 1), Rational::new(100, 1)))); + assert_eq!(span_of(&gap), Some((Rational::new(50, 1), Rational::new(120, 1)))); cmd.undo(); assert_eq!(track_block_count(&track), 3); @@ -1603,8 +1588,8 @@ fn boxed_prepared_commands_round_trip() { cmd.prepare(); let mut boxed = cmd.to_command(); boxed.redo_now(); - assert_eq!(span_of(&previous), Some((Rational::new(-20, 1), Rational::new(50, 1)))); - assert_eq!(span_of(&next), Some((Rational::new(100, 1), Rational::new(130, 1)))); + assert_eq!(span_of(&previous), Some((Rational::new(0, 1), Rational::new(70, 1)))); + assert_eq!(span_of(&next), Some((Rational::new(120, 1), Rational::new(150, 1)))); boxed.undo_now(); assert_eq!(span_of(&previous), Some((Rational::new(0, 1), Rational::new(50, 1)))); assert_eq!(span_of(&next), Some((Rational::new(100, 1), Rational::new(150, 1)))); diff --git a/docs/zh/plans/test-coverage-90-80-review.md b/docs/zh/plans/test-coverage-90-80-review.md index 0b7812109..d7ee9bfd2 100644 --- a/docs/zh/plans/test-coverage-90-80-review.md +++ b/docs/zh/plans/test-coverage-90-80-review.md @@ -148,6 +148,10 @@ disturbing anything else")。附带缺陷:`media_in := range.in_()` 把媒 统一裁决 block.rs:93-107 的语义互换,届时上述期望值按 graphops NOTE 的 意图 "deliberately" 改写。 +**裁决(2026-09-24,已修复)**:准备项落地后按 §9 的三原语方案完成语义 +修复,全部 KNOWN-SWAP 期望已按正确几何改写,并补上 `media_in` 与 +"缺口后的块不受影响"回归断言(详见 §9 §3.1)。 + ### 3.2 🔴 `oak-task/tests/render_test.rs:932-951`:全批次唯一"永不失败"的测试 [实证] `private_dispatcher_renders_frames_and_audio_through_workers` 对两次真实 @@ -328,10 +332,10 @@ M4 的平台/真机 job。 ## 7. 建议行动(按优先级) -1. **裁决 `block.rs:93-107` setter 互换**(产品缺陷,§3.1):修复时按 - graphops NOTE 的意图 "deliberately" 改写约 12 个期望值;先给 - `undocommands_test.rs` 的无标注固化处补偏差 NOTE、给全文件补 - media_in 断言维度、给 `RippleInfo` 加公开构造器(M5)。 +1. ✅ **裁决 `block.rs:93-107` setter 互换**(产品缺陷,§3.1):2026-09-24 + 按 §9 的三原语方案修复,约 12 个期望值按 graphops NOTE 的意图 + "deliberately" 改写;`RippleInfo` 公开构造器与 media_in 断言维度已先行 + 落地(M5)。 2. **消灭"双向接受"**(§3.2):`render_test.rs:932` 的 Err 分支按错误 内容分流,产品性错误必须失败。 3. **修回放验收测试的跳过判别**(§3.3):source-window 补槽位检查; @@ -357,10 +361,10 @@ M4 的平台/真机 job。 ## 9. 处理记录(2026-09-22) -本报告的高/中危与 §5 条目已按下列状态处理;仅 §3.1 的语义裁决按报告 -建议延后(准备项已落地,修复清单已细化)。 +本报告的高/中危与 §5 条目已按下列状态处理;§3.1 的语义裁决首先按报告 +建议延后(准备项先行落地),随后于 2026-09-24 完成修复。 -### §3.1(准备项已落地,语义裁决延后) +### §3.1 ✅(2026-09-24 裁决并修复) **已用仓库内 C++ 上游源码核准语义**(此前报告 §8 的局限已解除): @@ -387,10 +391,38 @@ M4 的平台/真机 job。 | `BlockTrimCommand`(TrimOut) | in 固定、out 移动 | 无 | 另有 `TrackSlide`、`TrackListInsertGaps`、`TrackListRippleToolCommand`、multicam 等 -调用点与 undosplit/undoripple/undogeneral 的补偿性选边需要一并裁决;应作为独立 -PR 处理(先按 §7 第 7 条把 M5 与覆盖率测试分开提交)。 +调用点与 undosplit/undoripple/undogeneral 的补偿性选边需要一并裁决(2026-09-24 +已随修复落地,见下;`TrackSlide`/`TrackListInsertGaps` 的“调用方摆放/ripple” +契约同时记入模块文档)。 -**本批已落地**: +**修复落地(2026-09-24)**。存储 range 模型定为三个原语 +(`crates/oak-node/src/block.rs`),各调用点按上表逐点选择: + +- `set_length_and_media_out`:**in 固定、out 移动,media 不动** + (Resize / TrimOut / 缺口向右生长); +- `set_length_and_media_in`:**in 固定、out 移动,`media_in += old−new`** + (ResizeWithMediaIn、splice 的右半、ripple TrimIn); +- 新增 `set_length_keeping_out`:**out 固定、in 移动,`media_in += old−new`** + (TrimIn 本体与 out 邻块、滑块右侧邻块、ripple trim_in)。 + +关键点: + +- `TrackReplaceBlockWithGapCommand` 的“仅后随缺口”分支改为 out 锚定: + 原实现把缺口向右生长,会吞掉缺口之后的块——这正是“拖动一个素材影响到 + 其他无关素材”的实证缺陷。回归断言已补(`domain_test`)。 +- `undoripple` 的 trim_out 改为 in 锚定(不再误写 media_in),splice 与 + trim_in 现在同步推进 `media_in`(内容损坏修复)。 +- `undosplit` 显式写两半的 range 与 media:第二半从切点继续,不再依赖 + `range.in_()` 的坐标空间巧合。 +- `TrackSlideCommand` 保持“调用方摆放滑动块,命令只处理邻块”的存储模型 + 契约(文档已注明);`TrackListInsertGaps`/`timeline_ripple_delete_gaps` + 的后续块 ripple 仍由调用方负责(C++ 布局的显式替代,未在本次范围)。 +- 测试:`domain_test`、`undocommands_test` 的全部 KNOWN-SWAP 期望按上表 + “故意”改写(roll 真正移动接缝、slide 无负入点、insert-gaps 向右生长、 + ResizeWithMediaIn `media_in=20`),`graphops` 内联 roll 期望同步改写; + `phase2_units_test` 按新语义修正并补 `set_length_keeping_out` 断言。 + +**本批已落地(准备项)**: - `RippleInfo::new(block, append_gap)` + `block()`/`append_gap()` 访问器; `undocommands_test.rs` 新增 2 个真实 info 测试(resize 往返、append_gap diff --git a/docs/zh/plans/test-coverage-90-80.md b/docs/zh/plans/test-coverage-90-80.md index fad2e1f45..a066598ab 100644 --- a/docs/zh/plans/test-coverage-90-80.md +++ b/docs/zh/plans/test-coverage-90-80.md @@ -775,6 +775,30 @@ python3 tooling/coverage_report.py --json target/coverage.json oak-task 89.5% / 62.8%;oak-storage 82.9% / 66.6%。 - 下一步仍为分支 80%(缺口 ~8.4pp);热点见上。 +### 第八批(§3.1 语义修复与两处 UI 回归,2026-09-24) + +- **§3.1 修复**(详见 review §3.1/§9):`block.rs` 的两个长度 setter 按其 + C++ 契约重写为三个存储模型原语: + - `set_length_and_media_out` = in 固定、out 移动、media 不动; + - `set_length_and_media_in` = in 固定、out 移动、`media_in += old−new`; + - 新增 `set_length_keeping_out` = out 固定、in 移动、`media_in += old−new`。 + undopointer/undogeneral/undoripple/undosplit/graphops/cli/nodeops 的调用点 + 逐点对齐,修掉两处实证缺陷: + 1. `TrackReplaceBlockWithGapCommand` 在“块后紧跟缺口”时把缺口向右生长, + 吞掉缺口之后的块——即用户报告的“拖动一个素材影响到其他无关素材”; + 现在缺口向左覆盖被移除块的跨度,后续块不动(domain_test 回归断言)。 + 2. roll 不滚动、slide 出现负入点、trim-in/ripple splice 把时间线 in 写进 + `media_in`(播放错误内容);全部 KNOWN-SWAP 期望按正确几何改写,并补 + `media_in` 维度断言(roll 接缝移动、follower 媒体推进、slide 无负坐标、 + insert-gaps 向右生长、ResizeWithMediaIn `media_in=20`)。 +- **i18n 刷新**:dock 的 `PanelHandle` 在注册时快照 `DockPanel::title`, + 语言切换后页签停留在旧语言(截图的“英文界面 + 中文页签”)。gpui 侧新增 + `DockArea::refresh_panel_titles`(经类型擦除 provider 重读标题 + 通知面板), + shell 在菜单与首选项两条切换路径调用;首选项对话框自身同步重绘。 + (gpui 子模块提交见 `oak-gpui`。) +- **OCIO 参数面板**:Vec4 评分参数(对比度/偏移/曝光)建 4 个 SpinBox, + `base` 步长锚定范围(pivot 不再一拖就飞出 ±10000)。 + ## 8. 风险与对策 1. **oak-app UI 覆盖成本最高、最易 flaky**:只用 test-support 的确定性