From 948bb018f32bfd5cc959b63ea2ca0ed1beb32870 Mon Sep 17 00:00:00 2001 From: Tadiwa Mbuwayesango Date: Thu, 21 May 2026 12:40:40 -0500 Subject: [PATCH] Fix changed clip ID merge contract --- crates/vedit-core/src/merge.rs | 151 +++++++++++------ crates/vedit-core/src/repo.rs | 11 +- crates/vedit-core/tests/merge_workflow.rs | 193 ++++++++++++++++++++++ 3 files changed, 307 insertions(+), 48 deletions(-) diff --git a/crates/vedit-core/src/merge.rs b/crates/vedit-core/src/merge.rs index 3f54c38..42f2ade 100644 --- a/crates/vedit-core/src/merge.rs +++ b/crates/vedit-core/src/merge.rs @@ -91,17 +91,21 @@ pub enum Side { } pub fn changed_clip_ids(base: Option<&Timeline>, after: &Timeline) -> Vec { - let after_clips = clips_by_id(after); + let after_clips = clip_infos_by_id(after); let mut changed = BTreeSet::new(); let Some(base) = base else { return after_clips.keys().cloned().collect(); }; - let base_clips = clips_by_id(base); - for (id, after_clip) in &after_clips { + let base_clips = clip_infos_by_id(base); + for (id, after_info) in &after_clips { match base_clips.get(id) { - Some(base_clip) if *base_clip == *after_clip => {} + Some(base_info) + if base_info.clip == after_info.clip + && base_info.track_name == after_info.track_name + && base_info.track_kind == after_info.track_kind + && base_info.clip_index == after_info.clip_index => {} _ => { changed.insert(id.clone()); } @@ -321,14 +325,31 @@ fn diff_track(before: &Track, after: &Track) -> Vec { diff(&before_tl, &after_tl) } -fn clips_by_id(timeline: &Timeline) -> BTreeMap { +struct ClipInfo<'a> { + clip: &'a Clip, + track_name: String, + track_kind: TrackKind, + clip_index: usize, +} + +fn clip_infos_by_id(timeline: &Timeline) -> BTreeMap> { let mut out = BTreeMap::new(); for track in &timeline.tracks { + let mut clip_index = 0; for child in &track.children { if let TrackChild::Clip(clip) = child && let Some(id) = &clip.clip_id { - out.insert(id.clone(), clip); + out.insert( + id.clone(), + ClipInfo { + clip, + track_name: track.name.clone(), + track_kind: track.kind, + clip_index, + }, + ); + clip_index += 1; } } } @@ -341,25 +362,20 @@ fn overlay_clip_id_changes( source_changed: &[String], ) -> Timeline { let source_changed: BTreeSet = source_changed.iter().cloned().collect(); - let source_clips = clips_by_id(source); let mut merged = target.clone(); - let mut applied = BTreeSet::new(); for track in &mut merged.tracks { - for child in &mut track.children { - if let TrackChild::Clip(target_clip) = child - && let Some(id) = target_clip.clip_id.clone() - && source_changed.contains(&id) - && let Some(source_clip) = source_clips.get(&id) - { - *target_clip = (*source_clip).clone(); - applied.insert(id); - } - } + track.children.retain(|child| match child { + TrackChild::Clip(clip) => clip + .clip_id + .as_ref() + .is_none_or(|id| !source_changed.contains(id)), + _ => true, + }); } for source_track in &source.tracks { - let source_additions: Vec = source_track + let source_changes: Vec = source_track .children .iter() .filter_map(|child| match child { @@ -367,47 +383,90 @@ fn overlay_clip_id_changes( if clip .clip_id .as_ref() - .is_some_and(|id| source_changed.contains(id) && !applied.contains(id)) => + .is_some_and(|id| source_changed.contains(id)) => { - Some(TrackChild::Clip(clip.clone())) + Some(clip.clone()) } _ => None, }) .collect(); - if source_additions.is_empty() { + if source_changes.is_empty() { continue; } - if let Some(target_track) = merged - .tracks - .iter_mut() - .find(|track| track.name == source_track.name && track.kind == source_track.kind) - { - for child in source_additions { - if let TrackChild::Clip(clip) = &child - && let Some(id) = &clip.clip_id - { - applied.insert(id.clone()); - } - target_track.children.push(child); - } - } else { - let mut track = source_track.clone(); - track.children = source_additions; - for child in &track.children { - if let TrackChild::Clip(clip) = child - && let Some(id) = &clip.clip_id - { - applied.insert(id.clone()); - } - } - merged.tracks.push(track); + let target_track = ensure_track(&mut merged, source_track); + for clip in source_changes { + let insert_at = + source_insertion_index(source_track, target_track, &source_changed, &clip) + .unwrap_or(target_track.children.len()); + target_track + .children + .insert(insert_at, TrackChild::Clip(clip)); } } merged } +fn ensure_track<'a>(timeline: &'a mut Timeline, source_track: &Track) -> &'a mut Track { + if let Some(index) = timeline + .tracks + .iter() + .position(|track| track.name == source_track.name && track.kind == source_track.kind) + { + return &mut timeline.tracks[index]; + } + + timeline.tracks.push(Track { + name: source_track.name.clone(), + kind: source_track.kind, + children: Vec::new(), + }); + timeline.tracks.last_mut().unwrap() +} + +fn source_insertion_index( + source_track: &Track, + target_track: &Track, + source_changed: &BTreeSet, + clip: &Clip, +) -> Option { + let source_pos = source_track.children.iter().position( + |child| matches!(child, TrackChild::Clip(candidate) if candidate.clip_id == clip.clip_id), + )?; + + for child in source_track.children.iter().skip(source_pos + 1) { + if let TrackChild::Clip(anchor) = child + && let Some(anchor_id) = &anchor.clip_id + && !source_changed.contains(anchor_id) + && let Some(index) = target_clip_index(target_track, anchor_id) + { + return Some(index); + } + } + + for child in source_track.children[..source_pos].iter().rev() { + if let TrackChild::Clip(anchor) = child + && let Some(anchor_id) = &anchor.clip_id + && !source_changed.contains(anchor_id) + && let Some(index) = target_clip_index(target_track, anchor_id) + { + return Some(index + 1); + } + } + + None +} + +fn target_clip_index(track: &Track, clip_id: &str) -> Option { + track.children.iter().position(|child| { + matches!( + child, + TrackChild::Clip(clip) if clip.clip_id.as_deref() == Some(clip_id) + ) + }) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/vedit-core/src/repo.rs b/crates/vedit-core/src/repo.rs index 111d7b3..2078390 100644 --- a/crates/vedit-core/src/repo.rs +++ b/crates/vedit-core/src/repo.rs @@ -491,8 +491,15 @@ impl Repo { message.to_string(), ); let commit_hash = self.write_timeline(&serde_json::to_value(&commit)?)?; - if self.branch_target(target_ref)?.is_some() { - self.set_branch_target(target_ref, &commit_hash)?; + let target_branch = if target_ref == "HEAD" { + self.current_branch()? + } else if self.branch_target(target_ref)?.is_some() { + Some(target_ref.to_string()) + } else { + None + }; + if let Some(branch) = target_branch { + self.set_branch_target(&branch, &commit_hash)?; } Ok(ChangedClipIdMergeOutcome::Clean(ChangedClipIdMergeClean { diff --git a/crates/vedit-core/tests/merge_workflow.rs b/crates/vedit-core/tests/merge_workflow.rs index 0c419b0..f91b299 100644 --- a/crates/vedit-core/tests/merge_workflow.rs +++ b/crates/vedit-core/tests/merge_workflow.rs @@ -423,3 +423,196 @@ fn changed_clip_id_merge_returns_typed_conflict_for_overlap() { Some(target_c.as_str()) ); } + +#[test] +fn changed_clip_id_merge_head_target_advances_current_branch() { + let dir = tempdir().unwrap(); + let repo = Repo::init(dir.path()).unwrap(); + let base_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-a", 24.0), + identified_clip("clip-b", 24.0), + ], + )])) + .unwrap(); + repo.commit(&base_v, author(), "base").unwrap(); + + repo.create_branch("source", "HEAD").unwrap(); + + let target_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-a", 12.0), + identified_clip("clip-b", 24.0), + ], + )])) + .unwrap(); + repo.commit(&target_v, author(), "trim a").unwrap(); + let before_merge_main = repo.branch_target("main").unwrap().unwrap(); + + repo.switch_branch("source").unwrap(); + let source_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-a", 24.0), + identified_clip("clip-b", 12.0), + ], + )])) + .unwrap(); + repo.commit(&source_v, author(), "trim b").unwrap(); + + repo.switch_branch("main").unwrap(); + let outcome = repo + .merge_changed_clip_ids("source", "HEAD", author(), "merge source into head") + .unwrap(); + let clean = match outcome { + ChangedClipIdMergeOutcome::Clean(clean) => clean, + other => panic!("expected clean changed-clip-id merge, got {other:?}"), + }; + + assert_eq!(clean.target_ref, "HEAD"); + assert_ne!(clean.commit_hash, before_merge_main); + assert_eq!( + repo.branch_target("main").unwrap().as_deref(), + Some(clean.commit_hash.as_str()) + ); +} + +#[test] +fn changed_clip_id_merge_applies_source_clip_deletions() { + let dir = tempdir().unwrap(); + let repo = Repo::init(dir.path()).unwrap(); + let base_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-a", 24.0), + identified_clip("clip-b", 24.0), + ], + )])) + .unwrap(); + repo.commit(&base_v, author(), "base").unwrap(); + + repo.create_branch("source", "HEAD").unwrap(); + + let target_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-a", 12.0), + identified_clip("clip-b", 24.0), + ], + )])) + .unwrap(); + repo.commit(&target_v, author(), "trim a").unwrap(); + + repo.switch_branch("source").unwrap(); + let source_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![identified_clip("clip-a", 24.0)], + )])) + .unwrap(); + repo.commit(&source_v, author(), "delete b").unwrap(); + + let outcome = repo + .merge_changed_clip_ids("source", "main", author(), "merge source into main") + .unwrap(); + let clean = match outcome { + ChangedClipIdMergeOutcome::Clean(clean) => clean, + other => panic!("expected clean changed-clip-id merge, got {other:?}"), + }; + + assert_eq!(clean.source_changed_clip_ids, vec!["clip-b".to_string()]); + let merge_commit = repo.read_commit(&clean.commit_hash).unwrap(); + let merged_value = repo.read_timeline(&merge_commit.timeline).unwrap(); + let merged = otio::parse_timeline(&merged_value).unwrap(); + let clip_ids: Vec<_> = merged.tracks[0] + .children + .iter() + .filter_map(|child| match child { + vedit_core::model::TrackChild::Clip(clip) => clip.clip_id.as_deref(), + _ => None, + }) + .collect(); + assert_eq!(clip_ids, vec!["clip-a"]); +} + +#[test] +fn changed_clip_id_merge_applies_source_clip_reorders() { + let dir = tempdir().unwrap(); + let repo = Repo::init(dir.path()).unwrap(); + let base_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-a", 24.0), + identified_clip("clip-b", 24.0), + identified_clip("clip-c", 24.0), + ], + )])) + .unwrap(); + repo.commit(&base_v, author(), "base").unwrap(); + + repo.create_branch("source", "HEAD").unwrap(); + + let target_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-a", 24.0), + identified_clip("clip-b", 24.0), + identified_clip("clip-c", 12.0), + ], + )])) + .unwrap(); + repo.commit(&target_v, author(), "trim c").unwrap(); + + repo.switch_branch("source").unwrap(); + let source_v = repo + .write_timeline(&timeline_with_tracks(vec![identified_video_track( + "V1", + vec![ + identified_clip("clip-b", 24.0), + identified_clip("clip-a", 24.0), + identified_clip("clip-c", 24.0), + ], + )])) + .unwrap(); + repo.commit(&source_v, author(), "move b before a").unwrap(); + + let outcome = repo + .merge_changed_clip_ids("source", "main", author(), "merge source into main") + .unwrap(); + let clean = match outcome { + ChangedClipIdMergeOutcome::Clean(clean) => clean, + other => panic!("expected clean changed-clip-id merge, got {other:?}"), + }; + + assert_eq!( + clean.source_changed_clip_ids, + vec!["clip-a".to_string(), "clip-b".to_string()] + ); + let merge_commit = repo.read_commit(&clean.commit_hash).unwrap(); + let merged_value = repo.read_timeline(&merge_commit.timeline).unwrap(); + let merged = otio::parse_timeline(&merged_value).unwrap(); + let clips: Vec<_> = merged.tracks[0] + .children + .iter() + .filter_map(|child| match child { + vedit_core::model::TrackChild::Clip(clip) => Some(( + clip.clip_id.as_deref().unwrap(), + clip.source_range.unwrap().duration.value, + )), + _ => None, + }) + .collect(); + assert_eq!( + clips, + vec![("clip-b", 24.0), ("clip-a", 24.0), ("clip-c", 12.0)] + ); +}