From 3694505ed4716143e0ae5bd5fa5121c88f59ca54 Mon Sep 17 00:00:00 2001 From: zitann2020 Date: Fri, 30 Oct 2026 12:39:53 +0900 Subject: [PATCH] ui: a drop that holds the target composition lands the rest of the selection (#2589) Co-Authored-By: DeepSeek V4.1 Flash --- crates/ui-egui/src/panels.rs | 9 ++- crates/ui-egui/src/panels/project.rs | 11 +++- crates/ui-egui/src/panels/timeline.rs | 4 +- crates/ui-egui/src/panels/viewer/frame.rs | 2 +- crates/ui-egui/src/panels/viewer_drop.rs | 6 +- crates/ui-egui/tests/it/main.rs | 1 + crates/ui-egui/tests/it/ui_self_drop.rs | 75 +++++++++++++++++++++++ 7 files changed, 99 insertions(+), 9 deletions(-) create mode 100644 crates/ui-egui/tests/it/ui_self_drop.rs diff --git a/crates/ui-egui/src/panels.rs b/crates/ui-egui/src/panels.rs index f6a5b5321..c4a6f4d3a 100644 --- a/crates/ui-egui/src/panels.rs +++ b/crates/ui-egui/src/panels.rs @@ -124,8 +124,10 @@ impl PresetRef { /// Layers carried by a Project item drag. Dragging a selected item uses the /// complete selection, while dragging an unselected item uses only that item. -/// Folders and data files are not composition layers. -pub(crate) fn project_drop_items(app: &EffectcraftApp, dragged: u64) -> Vec { +/// Folders and data files are not composition layers, and the composition the +/// drop lands in is left out of a selection: it cannot contain itself, and one +/// such item would make the engine refuse the whole drop (#2589). +pub(crate) fn project_drop_items(app: &EffectcraftApp, dragged: u64, target: Option) -> Vec { use effectcraft_engine::project::{FootageKind, ItemId, ItemKind}; let selected = &app.session.state.project_selection; @@ -139,6 +141,9 @@ pub(crate) fn project_drop_items(app: &EffectcraftApp, dragged: u64) -> Vec candidates .into_iter() .filter(|id| { + if Some(ItemId(*id)) == target { + return false; + } app.session.project.item(ItemId(*id)).is_some_and(|item| match &item.kind { ItemKind::Comp(_) | ItemKind::Solid(_) => true, ItemKind::Footage(f) => f.kind != FootageKind::Data, diff --git a/crates/ui-egui/src/panels/project.rs b/crates/ui-egui/src/panels/project.rs index cdf57de27..05657ee07 100644 --- a/crates/ui-egui/src/panels/project.rs +++ b/crates/ui-egui/src/panels/project.rs @@ -618,9 +618,14 @@ fn item_menu( actions.push(("project.move".into(), json!({"items": targets(app, id, selected), "folder": null}))); ui.close(); } - if !it.is_folder() && ui.button(crate::i18n::tr("Add to Composition")).clicked() { - actions.push(("layer.addItem".into(), json!({"item": id.0}))); - ui.close(); + // A composition cannot contain itself, so its own row offers nothing to add to (#2589). + if !it.is_folder() { + let add = ui.add_enabled(app.session.state.active_comp != Some(id), egui::Button::new(crate::i18n::tr("Add to Composition"))); + app.auto.add("project.context.addToComposition", add.rect, "Add to Composition"); + if add.clicked() { + actions.push(("layer.addItem".into(), json!({"item": id.0}))); + ui.close(); + } } // File ▸ New Comp from Selection on the selection (the right-clicked row joins it // above): several items ask how in its dialog, as in After Effects (#360). diff --git a/crates/ui-egui/src/panels/timeline.rs b/crates/ui-egui/src/panels/timeline.rs index cb61e6f08..5fccd39c2 100644 --- a/crates/ui-egui/src/panels/timeline.rs +++ b/crates/ui-egui/src/panels/timeline.rs @@ -1778,7 +1778,7 @@ fn empty_timeline(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { if let Some(crate::panels::DragPayload::Item(dragged)) = egui::DragAndDrop::payload::(&ctx).as_deref() && ui.rect_contains_pointer(rect) { - let items: Vec = crate::panels::project_drop_items(app, *dragged) + let items: Vec = crate::panels::project_drop_items(app, *dragged, None) .into_iter() .map(ItemId) .filter(|id| { @@ -4945,7 +4945,7 @@ fn drop_insert_layers(app: &mut EffectcraftApp, g: &mut Rw, payload: &std::sync: match payload.as_ref() { crate::panels::DragPayload::Item(id) => { // Insert each selected item at the drop point, in selection order. - for (offset, item) in crate::panels::project_drop_items(app, *id).into_iter().enumerate() { + for (offset, item) in crate::panels::project_drop_items(app, *id, Some(g.tl.cid)).into_iter().enumerate() { actions.push(("layer.addItem".into(), json!({"item": item, "index": index.saturating_add(offset), "time": time.clone()}))); } } diff --git a/crates/ui-egui/src/panels/viewer/frame.rs b/crates/ui-egui/src/panels/viewer/frame.rs index 65095b399..e0d2a3bc8 100644 --- a/crates/ui-egui/src/panels/viewer/frame.rs +++ b/crates/ui-egui/src/panels/viewer/frame.rs @@ -25,7 +25,7 @@ pub(super) fn empty_state(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rec if let Some(crate::panels::DragPayload::Item(dragged)) = egui::DragAndDrop::payload::(&ctx).as_deref() && ui.rect_contains_pointer(rect) { - let items: Vec = crate::panels::project_drop_items(app, *dragged) + let items: Vec = crate::panels::project_drop_items(app, *dragged, None) .into_iter() .map(effectcraft_engine::project::ItemId) .filter(|id| { diff --git a/crates/ui-egui/src/panels/viewer_drop.rs b/crates/ui-egui/src/panels/viewer_drop.rs index ac56d9acb..aadd2c122 100644 --- a/crates/ui-egui/src/panels/viewer_drop.rs +++ b/crates/ui-egui/src/panels/viewer_drop.rs @@ -54,7 +54,11 @@ pub(crate) fn show(app: &mut EffectcraftApp, ui: &egui::Ui, painter: &egui::Pain } // New layers go above the current selection, so insert in reverse // to preserve the order of the dragged Project items. - crate::panels::project_drop_items(app, *item).into_iter().rev().map(|item| ("layer.addItem", json!({"item": item, "position": at}))).collect() + crate::panels::project_drop_items(app, *item, app.session.state.active_comp) + .into_iter() + .rev() + .map(|item| ("layer.addItem", json!({"item": item, "position": at}))) + .collect() } DragPayload::Files(paths) => vec![("mediaBrowser.import", json!({"paths": paths, "addToComp": true, "position": at}))], DragPayload::Property { .. } => vec![], diff --git a/crates/ui-egui/tests/it/main.rs b/crates/ui-egui/tests/it/main.rs index 29456b2a1..00533f15a 100644 --- a/crates/ui-egui/tests/it/main.rs +++ b/crates/ui-egui/tests/it/main.rs @@ -68,6 +68,7 @@ mod ui_render_queue; mod ui_script_console; mod ui_scriptui; mod ui_scrolling; +mod ui_self_drop; mod ui_settings_info; mod ui_tabs; mod ui_tail; diff --git a/crates/ui-egui/tests/it/ui_self_drop.rs b/crates/ui-egui/tests/it/ui_self_drop.rs new file mode 100644 index 000000000..a371cc80b --- /dev/null +++ b/crates/ui-egui/tests/it/ui_self_drop.rs @@ -0,0 +1,75 @@ +//! #2589: a Project selection that contains the composition being dropped into must not fail the +//! whole drop. Rendering writes a file named after the composition, so the Project panel holds +//! both and selects both; the composition itself is not a layer it can hold. + +use effectcraft_engine::Session; +use effectcraft_ui_egui::EffectcraftApp; +use egui::{Event, Modifiers, Pos2, Rect, pos2, vec2}; +use egui_kittest::Harness; +use serde_json::json; + +/// Compositions "Main" (active, 4 s at 30 fps, one solid) and "Clip". +fn harness() -> (Harness<'static, EffectcraftApp>, u64, u64) { + let mut s = Session::default(); + let clip = s.execute("comp.new", json!({"name": "Clip", "width": 160, "height": 90, "frameRate": 30, "duration": 1})).unwrap()["comp"].as_u64().unwrap(); + let main = s.execute("comp.new", json!({"name": "Main", "width": 320, "height": 180, "frameRate": 30, "duration": 4})).unwrap()["comp"].as_u64().unwrap(); + s.execute("layer.newSolid", json!({"name": "Bottom", "color": "#406080"})).unwrap(); + let mut h = Harness::builder().with_size(vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(s)); + h.run_steps(3); + (h, main, clip) +} + +fn rect(h: &Harness<'_, EffectcraftApp>, id: &str) -> Rect { + let e = h.state().auto.find(id).unwrap_or_else(|| panic!("no {id}")).clone(); + Rect::from_min_size(pos2(e.rect[0], e.rect[1]), vec2(e.rect[2], e.rect[3])) +} + +/// Press on `from`, move to `to` in steps, release there. +fn drag(h: &mut Harness<'_, EffectcraftApp>, from: Pos2, to: Pos2) { + h.event(Event::PointerMoved(from)); + h.step(); + h.event(Event::PointerButton { pos: from, button: egui::PointerButton::Primary, pressed: true, modifiers: Modifiers::NONE }); + h.step(); + for k in 1..=10 { + h.event(Event::PointerMoved(from + (to - from) * (k as f32 / 10.0))); + h.step(); + } + h.event(Event::PointerButton { pos: to, button: egui::PointerButton::Primary, pressed: false, modifiers: Modifiers::NONE }); + h.run_steps(3); +} + +fn stack(h: &Harness<'_, EffectcraftApp>) -> Vec { + h.state().session.active_comp().unwrap().layers.iter().map(|l| l.name.clone()).collect() +} + +fn layer_id(h: &Harness<'_, EffectcraftApp>, name: &str) -> u64 { + h.state().session.active_comp().unwrap().layers.iter().find(|l| l.name == name).unwrap().id.0 +} + +/// Both the composition and the render output are selected: dragging the file onto the Timeline +/// still lands the file, and the composition is not carried along into itself. +#[test] +fn a_selection_holding_the_drop_target_does_not_fail_the_drop() { + let (mut h, main, clip) = harness(); + h.state_mut().session.execute("project.select", json!({"items": [main, clip]})).unwrap(); + h.run_steps(2); + let from = rect(&h, &format!("project.item.{clip}.name")).center(); + let row = rect(&h, &format!("timeline.layer.{}.row", layer_id(&h, "Bottom"))); + drag(&mut h, from, row.center()); + assert_eq!(stack(&h), ["Bottom", "Clip"], "the composition the drop lands in is left out"); + assert!(!h.state().ui.status.contains("contain itself"), "the drop did not fail: {}", h.state().ui.status); +} + +/// Dragging the composition's own row onto its Timeline adds nothing and reports nothing. +#[test] +fn the_composition_dropped_on_itself_adds_nothing() { + let (mut h, main, _clip) = harness(); + h.state_mut().session.execute("project.select", json!({"items": [main]})).unwrap(); + h.run_steps(2); + let before = stack(&h); + let from = rect(&h, &format!("project.item.{main}.name")).center(); + let row = rect(&h, &format!("timeline.layer.{}.row", layer_id(&h, "Bottom"))); + drag(&mut h, from, row.center()); + assert_eq!(stack(&h), before, "nothing is added"); + assert!(!h.state().ui.status.contains("contain itself"), "no error: {}", h.state().ui.status); +}