collective
Browse / github.com/storytold/effectcraft / patch
Fix timeline layer drops after scrolling (#2492)
[Bug report] using scroll wheel while moving layers When having a large amount of layers in a comp and then trying to move a layer, but then also using my scroll wheel to go down/up. It kind of just glitches out but the blue line still appears when going over the layers. spamming your keys gets most of the time rid of it or just clicking everywhere. only really visible if i could show a video Windows 11 using most recent version
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- WillPastor (imported from pull request #2519, open)
- Base
bc4b48f67deef2458e9dd3e3b0e592d5f75768a1- Size
- +237 −18 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
3650a2b1d7fde402189206b2
How it went
No other operator has reported using it yet.
Diff
raw · crates/ui-egui/src/panels/timeline.rs crates/ui-egui/tests/it/ui_viewer.rs
@@ -1287,6 +1287,13 @@ fn layer_icon(l: &Layer, project: &effectcraft_engine::project::Project) -> Icon } /// Layers being dragged to a new place in the stack (timeline rows).+#[derive(Clone)]+struct LayerDrag {+ comp: ItemId,+ owner: egui::Id,+ layers: Vec<u64>,+}+ fn layer_drag_id() -> egui::Id { egui::Id::unique("tl-layer-drag") }@@ -1490,12 +1497,6 @@ fn insertion(layer_rows: &[(Rect, LayerId)], py: f32, top: f32) -> (Option<usize (at, y) } -/// The `egui::Id` of the flag the row drag's release sets, telling the same frame's reorder-/// drawing code to apply the drop.-fn layer_drop_id() -> egui::Id {- egui::Id::unique("tl-layer-drop")-}- /// Whether a Project item can replace a layer's source (the same set `layer.replaceSource` /// accepts): compositions, footage and solids; not folders, data files or 3D models. fn replaceable_item(it: &effectcraft_engine::project::Item) -> bool {@@ -2274,14 +2275,11 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { } // Locked layers can't be selected or moved (unlock them with the lock switch). let locked = layer.switches.locked;- if row_resp.drag_started() && !locked {+ if row_resp.drag_started_by(egui::PointerButton::Primary) && !locked { // The selected layers move together; an unselected row moves alone. let moving: Vec<u64> = if is_sel { comp.layers.iter().filter(|l| selected.contains(&l.id)).map(|l| l.id.0).collect() } else { vec![layer.id.0] };- ctx.data_mut(|d| d.insert_temp(layer_drag_id(), moving));- }- if row_resp.drag_stopped() {- ctx.data_mut(|d| d.insert_temp(layer_drop_id(), true));+ ctx.data_mut(|d| d.insert_temp(layer_drag_id(), LayerDrag { comp: cid, owner: row_resp.id, layers: moving })); } app.auto.add(&format!("timeline.layer.{}.row", layer.id.0), left, &layer.name); if row_resp.clicked() && !locked {@@ -3335,17 +3333,25 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { gp.rect_stroke(br, 0.0, Stroke::new(1.0, t.accent), StrokeKind::Middle); ctx.data_mut(|d| d.insert_temp(egui::Id::unique("tl-box"), br)); }- // Layer reorder drag: a line where the layers will land; the release moves them there.- if let Some(moving) = ctx.data(|d| d.get_temp::<Vec<u64>>(layer_drag_id())) {- let dropped = ctx.data_mut(|d| d.remove_temp::<bool>(layer_drop_id())).unwrap_or(false);+ // The origin row can scroll out of view, so finish the gesture here, independently of+ // its Response. Disabled UI, Escape or lost ownership cancels; only a primary release drops layers.+ if let Some(drag) = ctx.data(|d| d.get_temp::<LayerDrag>(layer_drag_id())) {+ let (down, released, escape) =+ ctx.input(|i| (i.pointer.primary_down(), i.pointer.button_released(egui::PointerButton::Primary), i.key_pressed(egui::Key::Escape)));+ let dropped = ctx.drag_stopped_id() == Some(drag.owner) && released;+ let valid = ui.is_enabled() && drag.comp == cid && !escape && (dropped || (down && ctx.dragged_id() == Some(drag.owner)));+ if dropped || !valid {+ ctx.data_mut(|d| d.remove::<LayerDrag>(layer_drag_id()));+ }+ let moving = &drag.layers; let layer_rows: Vec<(Rect, LayerId)> = hit_rows.iter().filter(|(_, r)| matches!(r.kind, RowKind::Layer)).map(|(rr, r)| (*rr, r.layer)).collect(); if let Some(py) = ui.input(|i| i.pointer.interact_pos().or(i.pointer.latest_pos())).map(|p| p.y)+ && valid && !layer_rows.is_empty() { let (at, line_y) = insertion(&layer_rows, py, rows_rect.min.y); let target = at.and_then(|i| layer_rows[i..].iter().find(|(_, l)| !moving.contains(&l.0)).map(|(_, l)| *l)); if dropped {- ctx.data_mut(|d| d.remove::<Vec<u64>>(layer_drag_id())); // Only when the order changes (no empty undo step for a drop in place). let mut order: Vec<u64> = comp.layers.iter().map(|l| l.id.0).filter(|id| !moving.contains(id)).collect(); let at = target.and_then(|t| order.iter().position(|id| *id == t.0)).unwrap_or(order.len());@@ -3361,8 +3367,6 @@ pub fn show(app: &mut EffectcraftApp, ui: &mut egui::Ui, rect: Rect) { ctx.set_cursor_icon(egui::CursorIcon::Grabbing); p.with_clip_rect(rows_rect).line_segment([pos2(rect.min.x, line_y), pos2(graph_x0, line_y)], Stroke::new(2.0, t.accent)); }- } else if dropped {- ctx.data_mut(|d| d.remove::<Vec<u64>>(layer_drag_id())); } } // Quick Offset of layer bars: the last selected layer moves by whole frames with the pointer@@ -1718,7 +1718,7 @@ fn timeline_rows_drag_to_reorder_layers() { assert_eq!(h.state().session.history.undo.len(), steps + 1); // Dropping a layer where it already is changes nothing. let b = row(&h, "Box").center();- drag(&mut h, b, b + vec2(0.0, 3.0));+ drag(&mut h, b, b + vec2(20.0, 0.0)); assert_eq!(names(&h), ["Plate", "Box"]); assert_eq!(h.state().session.history.undo.len(), steps + 1); // And back to the top.@@ -1727,6 +1727,221 @@ fn timeline_rows_drag_to_reorder_layers() { assert_eq!(names(&h), ["Box", "Plate"]); } +fn timeline_layer_order(h: &Harness<'_, EffectcraftApp>) -> Vec<u64> {+ h.state().session.active_comp().unwrap().layers.iter().map(|l| l.id.0).collect()+}++fn timeline_layer_row(h: &Harness<'_, EffectcraftApp>, layer: u64) -> Rect {+ rect(h, &format!("timeline.layer.{layer}.row"))+}++fn start_timeline_row_drag(h: &mut Harness<'_, EffectcraftApp>, from: Pos2) {+ h.input_mut().events.push(Event::PointerMoved(from));+ h.input_mut().events.push(Event::PointerButton { pos: from, button: egui::PointerButton::Primary, pressed: true, modifiers: Default::default() });+ h.step();+ h.input_mut().events.push(Event::PointerMoved(from + vec2(20.0, 0.0)));+ h.step();+ assert_eq!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing, "the row drag started");+}++fn release_timeline_row_drag(h: &mut Harness<'_, EffectcraftApp>, at: Pos2) {+ h.input_mut().events.push(Event::PointerMoved(at));+ h.step();+ h.input_mut().events.push(Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed: false, modifiers: Default::default() });+ h.run_steps(2);+}++fn scroll_timeline(h: &mut Harness<'_, EffectcraftApp>, dy: f32) {+ let at = rect(h, "timeline.layerMarquee").center();+ h.input_mut().events.push(Event::PointerMoved(at));+ h.input_mut().events.push(Event::MouseWheel {+ unit: egui::MouseWheelUnit::Point,+ delta: vec2(0.0, dy),+ modifiers: Default::default(),+ phase: egui::TouchPhase::Move,+ source: egui::MouseWheelSource::Wheel,+ });+ h.run_steps(20);+}++/// Wheel-scroll far enough to stop drawing the row that owns the held drag, then drop at+/// the other end of the stack. The release must work without that row's Response (#2492).+fn timeline_reorder_after_scroll(down: bool, multiple: bool) {+ let mut h = harness();+ for i in 0..38 {+ h.state_mut().session.execute("layer.newSolid", json!({"name": format!("Layer {i}"), "color": "#406080", "width": 80, "height": 80})).unwrap();+ }+ h.state_mut().session.execute("edit.deselectAll", json!({})).unwrap();+ h.run_steps(3);+ let before = timeline_layer_order(&h);+ let count = if multiple { 2 } else { 1 };+ let moving = if down { before[..count].to_vec() } else { before[before.len() - count..].to_vec() };+ if multiple {+ h.state_mut().session.execute("layer.select", json!({"layers": moving})).unwrap();+ h.run_steps(2);+ }+ if !down {+ scroll_timeline(&mut h, -10_000.0);+ }+ let origin = if down { before[0] } else { *before.last().unwrap() };+ let from = timeline_layer_row(&h, origin).center();+ // Setup fills the bounded Undo history; reserve room to count this gesture's steps.+ h.state_mut().session.history.clear();+ let steps = h.state().session.history.undo.len();+ start_timeline_row_drag(&mut h, from);+ scroll_timeline(&mut h, if down { -10_000.0 } else { 10_000.0 });+ assert!(h.state().auto.find(&format!("timeline.layer.{origin}.row")).is_none(), "the origin row was culled");+ assert_eq!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing, "scrolling keeps the held drag active");+ let target = if down { *before.last().unwrap() } else { before[0] };+ let row = timeline_layer_row(&h, target);+ let to = row.center() + vec2(0.0, row.height() * if down { 0.4 } else { -0.4 });+ release_timeline_row_drag(&mut h, to);++ let mut expected: Vec<u64> = before.iter().copied().filter(|id| !moving.contains(id)).collect();+ let at = if down { expected.len() } else { 0 };+ expected.splice(at..at, moving);+ assert_eq!(timeline_layer_order(&h), expected, "the offscreen origin still completes its drop");+ assert_eq!(h.state().session.history.undo.len(), steps + 1, "one reorder, including a selected group, is one undo step");+ assert_ne!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing, "release clears the drag feedback");+ h.state_mut().session.execute("edit.undo", json!({})).unwrap();+ assert_eq!(timeline_layer_order(&h), before);+ h.state_mut().session.execute("edit.redo", json!({})).unwrap();+ assert_eq!(timeline_layer_order(&h), expected);+ h.state_mut().session.execute("edit.deselectAll", json!({})).unwrap();+ h.run_steps(3);++ // A later, independent drag must not reuse the old gesture or selected group.+ let i = if down { expected.len() - 1 } else { 0 };+ let j = if down { i - 1 } else { 1 };+ let from = timeline_layer_row(&h, expected[i]).center();+ let row = timeline_layer_row(&h, expected[j]);+ drag(&mut h, from, row.center() + vec2(0.0, row.height() * if down { -0.4 } else { 0.4 }));+ expected.swap(i, j);+ assert_eq!(timeline_layer_order(&h), expected, "the next drag is independent");+ assert_eq!(h.state().session.history.undo.len(), steps + 2);+}++#[test]+fn timeline_rows_drag_to_bottom_after_scrolling() {+ for multiple in [false, true] {+ timeline_reorder_after_scroll(true, multiple);+ }+}++#[test]+fn timeline_rows_drag_to_top_after_scrolling() {+ for multiple in [false, true] {+ timeline_reorder_after_scroll(false, multiple);+ }+}++#[test]+fn timeline_rows_drag_cancels_without_a_primary_release() {+ for escape in [false, true] {+ let mut h = harness();+ let before = timeline_layer_order(&h);+ let from = timeline_layer_row(&h, before[0]).center();+ let row = timeline_layer_row(&h, before[1]);+ let to = row.center() + vec2(0.0, row.height() * 0.4);+ let steps = h.state().session.history.undo.len();+ start_timeline_row_drag(&mut h, from);+ h.input_mut().events.push(Event::PointerMoved(to));+ h.step();+ if escape {+ key(&mut h, egui::Key::Escape, Default::default());+ } else {+ // egui stops its drag on another button's release too. It isn't a layer drop.+ for pressed in [true, false] {+ h.input_mut().events.push(Event::PointerButton { pos: to, button: egui::PointerButton::Secondary, pressed, modifiers: Default::default() });+ h.step();+ }+ }+ assert_eq!(timeline_layer_order(&h), before, "cancellation must not arrange layers");+ assert_ne!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing);+ release_timeline_row_drag(&mut h, to);+ assert_eq!(timeline_layer_order(&h), before, "a later release must not replay a cancelled drag");+ assert_eq!(h.state().session.history.undo.len(), steps);+ }+}++#[test]+fn timeline_rows_drag_cancels_when_a_modal_opens() {+ let mut h = harness();+ let before = timeline_layer_order(&h);+ let from = timeline_layer_row(&h, before[0]).center();+ let row = timeline_layer_row(&h, before[1]);+ let to = row.center() + vec2(0.0, row.height() * 0.4);+ let steps = h.state().session.history.undo.len();+ start_timeline_row_drag(&mut h, from);+ let (modifiers, shortcut) = effectcraft_ui_egui::menus::parse_shortcut("Cmd+Alt+;").unwrap();+ key(&mut h, shortcut, modifiers);+ assert_eq!(h.state().dialog, Some(effectcraft_ui_egui::Dialog::Settings));+ assert!(!h.ctx.read_response(egui::Id::unique(("row", before[0]))).unwrap().enabled(), "the modal disables the Timeline row");+ assert!(h.ctx.input(|i| i.pointer.primary_down()), "the modal opened during the held drag");+ release_timeline_row_drag(&mut h, to);+ assert_eq!(timeline_layer_order(&h), before, "a release behind the modal must not arrange layers");+ assert_eq!(h.state().session.history.undo.len(), steps);+ key(&mut h, egui::Key::Escape, Default::default());+ assert!(h.state().dialog.is_none());+ assert_eq!(timeline_layer_order(&h), before, "closing the modal must not replay the drag");+ assert_ne!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing);+ let from = timeline_layer_row(&h, before[0]).center();+ let row = timeline_layer_row(&h, before[1]);+ drag(&mut h, from, row.center() + vec2(0.0, row.height() * 0.4));+ assert_eq!(timeline_layer_order(&h), [before[1], before[0]]);+ assert_eq!(h.state().session.history.undo.len(), steps + 1);+}++#[test]+fn timeline_rows_drag_does_not_replay_a_release_while_hidden() {+ use effectcraft_ui_egui::dock::PanelKind;+ let mut h = harness();+ let before = timeline_layer_order(&h);+ let from = timeline_layer_row(&h, before[0]).center();+ let steps = h.state().session.history.undo.len();+ start_timeline_row_drag(&mut h, from);+ h.state_mut().close_panel(PanelKind::Timeline);+ h.run_steps(2);+ assert!(h.state().auto.find("timeline.layerMarquee").is_none());+ release_timeline_row_drag(&mut h, from);+ h.state_mut().show_panel(PanelKind::Timeline);+ h.run_steps(3);+ assert_eq!(timeline_layer_order(&h), before);+ assert_eq!(h.state().session.history.undo.len(), steps);+ assert_ne!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing);+ let from = timeline_layer_row(&h, before[0]).center();+ let row = timeline_layer_row(&h, before[1]);+ drag(&mut h, from, row.center() + vec2(0.0, row.height() * 0.4));+ assert_eq!(timeline_layer_order(&h), [before[1], before[0]]);+ assert_eq!(h.state().session.history.undo.len(), steps + 1);+}++#[test]+fn timeline_rows_drag_cancels_when_the_composition_changes() {+ let mut h = harness();+ let original = h.state().session.active_comp_id().unwrap();+ let before = timeline_layer_order(&h);+ h.state_mut().session.execute("comp.new", json!({"name": "Other", "width": 640, "height": 360, "duration": 4})).unwrap();+ h.state_mut().session.execute("layer.newSolid", json!({"name": "Other layer", "color": "#406080", "width": 80, "height": 80})).unwrap();+ let other = h.state().session.active_comp_id().unwrap();+ let other_before = timeline_layer_order(&h);+ h.state_mut().session.execute("comp.open", json!({"comp": original.0})).unwrap();+ h.run_steps(3);+ let from = timeline_layer_row(&h, before[0]).center();+ let steps = h.state().session.history.undo.len();+ start_timeline_row_drag(&mut h, from);+ h.state_mut().session.execute("comp.open", json!({"comp": other.0})).unwrap();+ h.run_steps(2);+ assert_ne!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing);+ release_timeline_row_drag(&mut h, from);+ assert_eq!(timeline_layer_order(&h), other_before);+ h.state_mut().session.execute("comp.open", json!({"comp": original.0})).unwrap();+ h.run_steps(3);+ assert_eq!(timeline_layer_order(&h), before);+ assert_eq!(h.state().session.history.undo.len(), steps);+ assert_ne!(h.output().platform_output.cursor_icon, egui::CursorIcon::Grabbing);+}+ 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 }