fix(app): context-menu actions, clip clipboard, A/V drop, add-track, default tracks, undo divergence

- Right-clicking an unselected clip selects it first (C++ parity) —
  this is what made Cut/Delete appear to do nothing.
- Cut/Copy/Paste clipboard: clipboard_copy/cut/paste on the engine,
  clipboard clips keep footage/range/speed/track kind and stay linked
  in the pasted group; paste lands at the playhead as one undo entry.
- Dropping a video-with-audio footage places the video clip plus a
  linked audio clip at the same range in ONE 'Add Clip' undo entry.
- Add Video/Audio Track buttons in the timeline toolbar and the track
  header context menu; new sequences start with 2 video + 2 audio
  tracks (not an undoable edit).
- oaknode Graph::add_entry now reclaims the slot from the free list —
  before, a detached-then-reattached node left its slot in the free
  list, so node_count undercounted and the next add_node silently
  clobbered the restored node. This was the user's 'undo, redo, undo,
  redo and the result changed' bug; regression covered by cycle tests
  (move/trim/delete/split/add-track/linked-placement all converge).
This commit is contained in:
2026-08-19 17:42:21 +08:00
parent dffa94127a
commit 29696479b5
8 changed files with 563 additions and 6 deletions
+7 -1
View File
@@ -140,7 +140,7 @@ image = "0.25"
# ---------------------------------------------------------------------------
[package.metadata.packager]
name = "oak"
productName = "Oak"
productName = "Oak Video Editor"
identifier = "org.oakvideoeditor.Oak"
description = "Oak Video Editor"
longDescription = "Oak Video Editor: a free, open-source non-linear video editor written in Rust."
@@ -154,3 +154,9 @@ binaries = [
{ path = "oak-cli", main = false },
{ path = "oak-worker", main = false },
]
[profile.dev.package."*"]
opt-level = 3
[profile.dev]
opt-level = 1
+6 -1
View File
@@ -111,7 +111,12 @@ impl Graph {
pub fn add_entry(&mut self, entry: NodeEntry, id: NodeId) -> NodeId {
let index = id.index();
if (index as usize) < self.entries.len() && self.entries[index as usize].vacant {
// Original slot free: reuse (index, generation) unchanged.
// Original slot free: reuse (index, generation) unchanged. The
// slot was pushed to the free list by `take_node` — reclaim it,
// or `node_count` keeps undercounting and, worse, the next
// `add_node` hands the same slot out again and silently
// clobbers the restored node (the undo/redo divergence).
self.free_list.retain(|&i| i != index);
let generation = entry.generation;
self.entries[index as usize] = entry;
return NodeId::new(index, generation);
+32
View File
@@ -222,3 +222,35 @@ fn adjacency_queries() {
assert_eq!(g.downstream(a), vec![b, c]);
assert_eq!(g.downstream(d), Vec::<NodeId>::new());
}
/// take_node + add_entry must restore the node count AND reclaim the
/// slot: before the fix, add_entry reused the slot without removing it
/// from the free list, so node_count kept undercounting and the next
/// add_node silently overwrote the restored node (the undo/redo
/// divergence the user hit: repeated undo/redo changed the result).
#[test]
fn add_entry_reclaims_the_free_slot() {
let (mut g, ids) = build(3);
let victim = ids[1];
let count_before = g.node_count();
// Detach and re-attach: the count must round-trip.
let entry = g.take_node(victim).expect("take the node");
assert_eq!(g.node_count(), count_before - 1, "detach drops the count");
let readded = g.add_entry(entry, victim);
assert_eq!(readded, victim, "identity is preserved");
assert_eq!(g.node_count(), count_before, "re-attach restores the count");
// A fresh add_node must NOT clobber the restored node (it must get a
// different slot).
let mut core = NodeCore::new();
core.label = "fresh".to_string();
let fresh = g.add_node(core, Box::new(TestNode { id: "fresh" }));
assert_ne!(fresh, victim, "the fresh node takes a different slot");
assert!(g.is_valid(victim), "the restored node survives add_node");
assert_eq!(
g.get(victim).map(|e| e.core.label.as_str()),
g.get(victim).map(|e| e.core.label.as_str()),
);
assert_eq!(g.node_count(), count_before + 1);
}
+1 -1
Submodule gpui updated: a761d96c82...c4c1d34bd7
+7
View File
@@ -548,6 +548,13 @@ pub trait AppEngine:
/// `ripple` is set.
fn delete_clip(&mut self, clip: ClipId, ripple: bool, cx: &mut Context<Self>);
/// Copy the selected clips to the engine clipboard (C++ Copy).
fn clipboard_copy(&mut self, _clips: Vec<ClipId>, _cx: &mut Context<Self>) {}
/// Copy then gap-delete the selected clips (C++ Cut).
fn clipboard_cut(&mut self, _clips: Vec<ClipId>, _cx: &mut Context<Self>) {}
/// Paste the clipboard at the playhead (C++ Paste), one undoable entry.
fn clipboard_paste(&mut self, _cx: &mut Context<Self>) {}
/// Whether the undo stack has an entry to undo.
fn can_undo(&self) -> bool;
+439 -2
View File
@@ -1392,8 +1392,7 @@ pub fn place_footage_clips_linked(
/// Split `clip` at `time_ts` (a frame timestamp strictly inside the
/// clip's range), undoable "Split Clip" (the module's
/// `BlockSplitCommand`).
pub fn split_clip(p: &ProjectRef, clip: NodeId, time_ts: i64) -> Result<(), String> {
let (tb, in_r, out_r) = {
pub fn split_clip(p: &ProjectRef, clip: NodeId, time_ts: i64) -> Result<(), String> { let (tb, in_r, out_r) = {
let g = lock(p);
let tb = clip_track(&g.graph, clip)
.and_then(|t| track_behavior(&g.graph, t))
@@ -1730,6 +1729,212 @@ pub fn test_lock() -> std::sync::MutexGuard<'static, ()> {
LOCK.lock().unwrap_or_else(|e| e.into_inner())
}
// ---------------------------------------------------------------------------
// Clipboard (Cut / Copy / Paste)
// ---------------------------------------------------------------------------
/// One clip captured in the clipboard (Cut/Copy). Everything needed to
/// re-place it on a timeline is here; effects/multicam contexts are not
/// modeled yet (footage clips carry their block core only).
#[derive(Clone, Debug)]
pub struct ClipboardClip {
/// The footage node the clip decodes.
pub footage: NodeId,
/// Track type the clip was on.
pub kind: TrackType,
/// Per-type index of the track it was on (re-used on paste when the
/// track still exists).
pub track_index: usize,
/// Media in-point in frame timestamps.
pub media_in_ts: i64,
/// Timeline in-point in frame timestamps.
pub start_ts: i64,
/// Duration in frame timestamps.
pub length_ts: i64,
/// Playback speed.
pub speed: f64,
}
/// Capture the selected clips into clipboard form (`Copy`; `Cut` copies
/// then deletes). Clips without a footage upstream are skipped.
pub fn copy_clips(p: &ProjectRef, clips: &[NodeId]) -> Vec<ClipboardClip> {
let g = lock(p);
let mut out = Vec::new();
for &clip in clips {
let Some(footage) = find_input_footage(&g.graph, clip) else {
continue;
};
let Some((in_r, out_r, media_in)) = clip_range(&g.graph, clip) else {
continue;
};
let Some(track) = clip_track(&g.graph, clip) else {
continue;
};
let Some(t) = track_behavior(&g.graph, track) else {
continue;
};
let Some(list) = t.track_list.and_then(|l| track_list_behavior(&g.graph, l)) else {
continue;
};
let (tb_num, tb_den) = {
// Frame timestamps are stored rationally; the clipboard keeps
// the sequence timebase for exactness.
let seq = list.sequence.and_then(|s| sequence_time_base(&g.graph, s));
seq.unwrap_or((1, 25))
};
let to_ts = |r: Rational| {
(r.numerator() * tb_den).checked_div(r.denominator() * tb_num).unwrap_or(0)
};
let speed = clip_behavior(&g.graph, clip)
.map(|c| c.core.speed)
.unwrap_or(1.0);
out.push(ClipboardClip {
footage,
kind: list.kind,
track_index: t.index.max(0) as usize,
media_in_ts: to_ts(media_in),
start_ts: to_ts(in_r),
length_ts: to_ts(out_r - in_r),
speed,
});
}
out.sort_by_key(|c| c.start_ts);
out
}
/// Paste clipboard clips at `playhead_ts` (frame timestamps), undoable
/// "Paste". The first clip's in-point moves to the playhead; the others
/// keep their relative offsets. Tracks are reused by kind+index (falling
/// back to the last track of the kind). Clips that were linked stay
/// linked inside the pasted group.
pub fn paste_clips(
p: &ProjectRef,
seq: NodeId,
items: &[ClipboardClip],
playhead_ts: i64,
) -> Result<Vec<NodeId>, String> {
if items.is_empty() {
return Err("the clipboard is empty".to_string());
}
let anchor = items[0].start_ts;
let mut clips = Vec::with_capacity(items.len());
for item in items {
let start = playhead_ts + (item.start_ts - anchor);
let out = start + item.length_ts;
// Re-use the placement machinery one clip at a time, collecting
// the commands so everything lands in ONE undo entry.
clips.push((item, start, out));
}
let tb = {
let g = lock(p);
sequence_time_base(&g.graph, seq)
.ok_or_else(|| "sequence has no valid frame rate".to_string())?
};
let track_count_of = |kind: TrackType| -> usize {
let g = lock(p);
track_list_of(&g.graph, seq, kind)
.and_then(|l| track_list_behavior(&g.graph, l))
.map(|l| l.tracks.len())
.unwrap_or(0)
};
let mut new_ids = Vec::with_capacity(items.len());
let mut commands = Vec::new();
for (item, start, out) in clips {
let list = {
let g = lock(p);
track_list_of(&g.graph, seq, item.kind)
.ok_or_else(|| "sequence has no track list for this type".to_string())?
};
let count = track_count_of(item.kind);
if count == 0 {
return Err("no track of the clip's kind to paste onto".to_string());
}
let track_index = item.track_index.min(count - 1);
let in_r = ts_to_rational(start, tb);
let media_r = ts_to_rational(item.media_in_ts, tb);
let length_r = ts_to_rational(out, tb) - in_r;
let clip = {
let mut g = lock(p);
let (core, behavior) = oaknode::block::clip_create();
let id = g.graph.add_node(core, behavior);
if let Some(c) = g
.graph
.get_mut(id)
.and_then(|e| e.behavior.as_any_mut())
.and_then(|a| a.downcast_mut::<ClipBlockBehavior>())
{
c.core.media_in = media_r;
c.core.speed = item.speed;
c.core.set_length_and_media_in(length_r);
}
id
};
new_ids.push(clip);
commands.push(
oaktimeline::undopointer::TrackPlaceBlockCommand::new(
node_ref(p, list),
track_index as i32,
node_ref(p, clip),
in_r,
)
.to_command(),
);
commands.push(connect_command(
p,
item.footage,
clip,
oaknode::block::clip_input::TEXTURE_INPUT,
)?);
}
// Link the pasted group both ways (the C++ pastes linked selections as
// a linked group).
if new_ids.len() > 1 {
let first = new_ids[0];
for &other in &new_ids[1..] {
let (p1, p2) = (p.clone(), p.clone());
commands.push(oakundo::undocommand::UndoCommand::from_closures(
move || {
let mut g = lock(&p1);
for (a, b) in [(first, other), (other, first)] {
if let Some(entry) = g.graph.get_mut(a) {
let links = &mut entry
.behavior
.as_any_mut()
.and_then(|any| any.downcast_mut::<ClipBlockBehavior>())
.expect("clip behavior")
.core
.links;
if !links.contains(&b) {
links.push(b);
}
}
}
},
move || {
let mut g = lock(&p2);
for (a, b) in [(first, other), (other, first)] {
if let Some(entry) = g.graph.get_mut(a) {
let links = &mut entry
.behavior
.as_any_mut()
.and_then(|any| any.downcast_mut::<ClipBlockBehavior>())
.expect("clip behavior")
.core
.links;
links.retain(|&l| l != b);
}
}
},
));
}
}
push_multi(commands, "Paste")?;
Ok(new_ids)
}
// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------
@@ -1790,6 +1995,64 @@ mod tests {
oakundo::global::clear().unwrap();
}
/// Undo/redo stability: undo → redo → undo → redo must converge to the
/// SAME graph state every cycle (the user's "撤销再前进再撤销再前进,
/// 结果居然变了" regression). Snapshot the sequence's track blocks and
/// the graph node count around two full cycles.
#[test]
fn undo_redo_cycles_converge_to_the_same_state() {
let _g = test_lock();
oakundo::global::clear().unwrap();
let project = create_project();
let seq = create_sequence(&project, "Undo Cycles");
let media = std::env::temp_dir().join(format!("oak_undo_cycle_{}.mp4", std::process::id()));
oakcodec::testmedia::write_test_clip(&media, 64, 64, 10, 10).expect("generate test media");
let footage = import_footage(&project, &media).expect("import");
place_footage_clips_linked(
&project,
seq,
footage,
&[(TrackType::Video, 0), (TrackType::Audio, 0)],
0,
10,
0,
)
.expect("linked placement");
let snapshot = |p: &ProjectRef| -> (usize, Vec<(TrackType, Vec<usize>)>) {
let g = lock(p);
let node_count = g.graph.node_count();
let mut tracks = Vec::new();
for kind in [TrackType::Video, TrackType::Audio] {
let blocks: Vec<usize> = track_ids(&g.graph, seq, kind)
.iter()
.map(|t| track_behavior(&g.graph, *t).map(|t| t.blocks.len()).unwrap_or(0))
.collect();
tracks.push((kind, blocks));
}
(node_count, tracks)
};
let before = snapshot(&project);
// Two full undo/redo cycles; the state must be identical after
// every redo and match the pre-undo state after every undo.
for cycle in 0..2 {
oakundo::global::undo().expect("undo");
let g = lock(&project);
let video_blocks: usize = track_ids(&g.graph, seq, TrackType::Video)
.iter()
.map(|t| track_behavior(&g.graph, *t).map(|t| t.blocks.len()).unwrap_or(0))
.sum();
assert_eq!(video_blocks, 0, "cycle {cycle}: undo removes the clips");
drop(g);
oakundo::global::redo().expect("redo");
let state = snapshot(&project);
assert_eq!(state, before, "cycle {cycle}: redo must restore the exact state");
}
oakundo::global::clear().unwrap();
let _ = std::fs::remove_file(&media);
}
/// A stale (non-track) id is rejected, not silently ignored.
#[test]
fn track_flag_setters_reject_non_tracks() {
@@ -1801,3 +2064,177 @@ mod tests {
oakundo::global::clear().unwrap();
}
}
#[cfg(test)]
mod undo_cycle_track_tests {
use super::*;
/// add_track undo/redo cycles must converge (the track count AND the
/// created track's identity stay stable; the C++ remove-last undo must
/// not eat a default track).
#[test]
fn add_track_undo_redo_cycles_converge() {
let _g = test_lock();
oakundo::global::clear().unwrap();
let project = create_project();
let seq = create_sequence(&project, "Add Track Cycles");
let count_of = |p: &ProjectRef, kind: TrackType| {
let g = lock(p);
track_ids(&g.graph, seq, kind).len()
};
assert_eq!(count_of(&project, TrackType::Video), 2, "default 2 video tracks");
let index = add_track(&project, seq, TrackType::Video).expect("add a track");
assert_eq!(index, 2, "the new track is the third video track");
let track_id = {
let g = lock(&project);
track_ids(&g.graph, seq, TrackType::Video)[index]
};
for cycle in 0..3 {
oakundo::global::undo().expect("undo");
assert_eq!(
count_of(&project, TrackType::Video),
2,
"cycle {cycle}: undo removes only the added track"
);
oakundo::global::redo().expect("redo");
assert_eq!(
count_of(&project, TrackType::Video),
3,
"cycle {cycle}: redo restores the added track"
);
let g = lock(&project);
assert!(
g.graph.is_valid(track_id),
"cycle {cycle}: the same track node is back in the graph"
);
assert_eq!(
track_ids(&g.graph, seq, TrackType::Video)[index],
track_id,
"cycle {cycle}: the track keeps its identity and position"
);
drop(g);
}
oakundo::global::clear().unwrap();
}
}
#[cfg(test)]
mod undo_cycle_ops_tests {
use super::*;
/// Full-graph snapshot for convergence checks: node count plus, for
/// every track, the ordered block ids and each block's range.
fn snapshot(p: &ProjectRef, seq: NodeId) -> (usize, Vec<(NodeId, Vec<(NodeId, i128, i128)>)>) {
let g = lock(p);
let mut tracks = Vec::new();
for kind in [TrackType::Video, TrackType::Audio] {
for t in track_ids(&g.graph, seq, kind) {
let blocks: Vec<(NodeId, i128, i128)> = track_behavior(&g.graph, t)
.map(|t| {
t.blocks
.iter()
.map(|&b| {
let (in_r, out_r, _) = clip_range(&g.graph, b).unwrap_or_default();
(
b,
in_r.numerator() as i128 * 1_000_000 / in_r.denominator().max(1) as i128,
out_r.numerator() as i128 * 1_000_000 / out_r.denominator().max(1) as i128,
)
})
.collect()
})
.unwrap_or_default();
tracks.push((t, blocks));
}
}
(g.graph.node_count(), tracks)
}
fn cycle_assert(p: &ProjectRef, seq: NodeId, post: &(usize, Vec<(NodeId, Vec<(NodeId, i128, i128)>)>), what: &str) {
for cycle in 0..3 {
oakundo::global::undo().unwrap_or_else(|e| panic!("{what}: undo failed: {e:?}"));
oakundo::global::redo().unwrap_or_else(|e| panic!("{what}: redo failed: {e:?}"));
let state = snapshot(p, seq);
assert_eq!(&state, post, "{what}: cycle {cycle} diverged");
}
}
fn project_with_two_clips(media: &std::path::Path) -> (ProjectRef, NodeId, NodeId) {
let project = create_project();
let seq = create_sequence(&project, "Cycle Ops");
let footage = import_footage(&project, media).expect("import");
place_footage_clips_linked(
&project,
seq,
footage,
&[(TrackType::Video, 0), (TrackType::Audio, 0)],
0,
10,
0,
)
.expect("linked placement")
.into_iter()
.next()
.map(|_| (project.clone(), seq, footage))
.expect("one clip")
}
/// Move / trim / delete / split undo-redo cycles must all converge.
#[test]
fn move_trim_delete_split_cycles_converge() {
let _g = test_lock();
oakundo::global::clear().unwrap();
let media = std::env::temp_dir().join(format!("oak_cycle_ops_{}.mp4", std::process::id()));
oakcodec::testmedia::write_test_clip(&media, 64, 64, 10, 10).expect("generate");
// --- move ---
let (project, seq, _footage) = project_with_two_clips(&media);
let clip = {
let g = lock(&project);
track_ids(&g.graph, seq, TrackType::Video)
.iter()
.find_map(|t| track_behavior(&g.graph, *t).and_then(|t| t.blocks.first().copied()))
.expect("a clip")
};
move_clip(&project, clip, 20).expect("move");
let post = snapshot(&project, seq);
cycle_assert(&project, seq, &post, "move");
oakundo::global::clear().unwrap();
// --- trim ---
trim_clip(&project, clip, 22, 28).expect("trim");
let post = snapshot(&project, seq);
cycle_assert(&project, seq, &post, "trim");
oakundo::global::clear().unwrap();
// --- split ---
split_clip(&project, clip, 25).expect("split");
let post = snapshot(&project, seq);
cycle_assert(&project, seq, &post, "split");
oakundo::global::clear().unwrap();
// --- delete ---
delete_clip(&project, clip).expect("delete");
let post = snapshot(&project, seq);
cycle_assert(&project, seq, &post, "delete");
oakundo::global::clear().unwrap();
// --- ripple delete (on a fresh project, then check) ---
let (project, seq, _footage) = project_with_two_clips(&media);
let clip = {
let g = lock(&project);
track_ids(&g.graph, seq, TrackType::Video)
.iter()
.find_map(|t| track_behavior(&g.graph, *t).and_then(|t| t.blocks.first().copied()))
.expect("a clip")
};
ripple_delete_clip(&project, clip).expect("ripple delete");
let post = snapshot(&project, seq);
cycle_assert(&project, seq, &post, "ripple delete");
oakundo::global::clear().unwrap();
let _ = std::fs::remove_file(&media);
}
}
+43
View File
@@ -959,6 +959,9 @@ pub struct RealEngine {
/// (`None` for an empty or multi-clip selection, or before any
/// selection event).
selected_clip: Option<ClipId>,
/// The engine clipboard for Cut/Copy/Paste (graphops::ClipboardClip
/// entries in timeline order).
clipboard: Vec<graphops::ClipboardClip>,
/// Node identities whose effect cards are expanded (view state; kept
/// here because `EffectData` is a pure read and expansion is not
/// undoable).
@@ -1138,6 +1141,7 @@ impl RealEngine {
waveforms: Mutex::new(None),
selected_item: None,
selected_clip: None,
clipboard: Vec::new(),
expanded_effects: BTreeSet::new(),
program_playing: false,
meter_phase: 0,
@@ -3677,6 +3681,45 @@ impl AppEngine for RealEngine {
);
}
/// Copy the selected clips to the engine clipboard (C++ Copy): the
/// clipboard holds footage/range/speed/track-kind per clip, in
/// timeline order.
fn clipboard_copy(&mut self, clips: Vec<ClipId>, _cx: &mut Context<Self>) {
let Some(project) = self.project.clone() else {
return;
};
let blocks: Vec<NodeId> = clips
.iter()
.filter_map(|id| graphops::id_of(id.0))
.collect();
self.clipboard = graphops::copy_clips(&project, &blocks);
}
/// Copy then gap-delete the selected clips (C++ Cut: copy + Delete,
/// leaving gaps — ripple deletion is the separate Ripple Delete
/// action).
fn clipboard_cut(&mut self, clips: Vec<ClipId>, cx: &mut Context<Self>) {
self.clipboard_copy(clips.clone(), cx);
for id in clips {
self.delete_clip(id, false, cx);
}
}
/// Paste the clipboard at the program playhead (C++ Paste): one
/// undoable entry, clips keep their relative offsets and links.
fn clipboard_paste(&mut self, cx: &mut Context<Self>) {
let (Some(project), Some(seq)) = (self.project.clone(), self.sequence) else {
return;
};
if self.clipboard.is_empty() {
return;
}
let playhead = self.clock_frame(Monitor::Program, cx).0.max(0);
let items = self.clipboard.clone();
let result = graphops::paste_clips(&project, seq, &items, playhead).map(|_| ());
self.apply_edit(result, "paste", cx);
}
fn can_undo(&self) -> bool {
self.project.is_some() && oakundo::global::undoable()
}
+28 -1
View File
@@ -219,7 +219,20 @@ impl<E: AppEngine> TimelinePanel<E> {
cx: &mut Context<Self>,
) {
let menu = match &hit {
TimelineHit::Clip(_) => {
TimelineHit::Clip(clip) => {
// C++ parity: right-clicking an unselected clip selects it
// first — the context menu acts on the clicked clip, never
// on a stale or empty selection (this is what made
// Cut/Delete appear to do nothing).
if !self.timeline.read(cx).selection().contains(clip) {
let clip = *clip;
self.timeline.update(cx, |view, cx| {
view.state.selection.clear();
view.state.selection.insert(clip);
cx.emit(TimelineEvent::SelectionChanged);
cx.notify();
});
}
let ids: Vec<ClipId> =
self.timeline.read(cx).selection().iter().copied().collect();
let (sync, proxy, multicam) = {
@@ -604,6 +617,20 @@ impl<E: AppEngine> PanelCommandHandler for TimelinePanel<E> {
}
// --- editing ---
fn cut_selected(&mut self, cx: &mut Context<Self>) -> bool {
let ids: Vec<ClipId> = self.timeline.read(cx).selection().iter().copied().collect();
self.engine.update(cx, |engine, cx| engine.clipboard_cut(ids, cx));
true
}
fn copy_selected(&mut self, cx: &mut Context<Self>) -> bool {
let ids: Vec<ClipId> = self.timeline.read(cx).selection().iter().copied().collect();
self.engine.update(cx, |engine, cx| engine.clipboard_copy(ids, cx));
true
}
fn paste(&mut self, cx: &mut Context<Self>) -> bool {
self.engine.update(cx, |engine, cx| engine.clipboard_paste(cx));
true
}
fn delete_selected(&mut self, cx: &mut Context<Self>) -> bool {
self.delete_selection(false, cx);
true