Merge pull request #72 from jonassneu/fix/project-delete-shortcuts

Project: delete selected items with Delete and Backspace

crates/ui-egui/src/menus.rs

@@ -1307,9 +1307,20 @@ pub fn handle_shortcuts(app: &mut EffectcraftApp, ctx: &egui::Context) {
         if key == egui::Key::Enter && !mods.any() && app.ui.focused == PanelKind::Project {
             continue;
         }
-        // Delete/Backspace clears selection.
+        // The Project panel has its own selection; a previously selected Timeline layer
+        // must not be deleted while the Project panel has focus.
         if matches!(key, egui::Key::Delete | egui::Key::Backspace) && !mods.any() {
-            let _ = invoke(app, ctx, "edit.clear", json!({}));
+            let command = if app.ui.focused == PanelKind::Project {
+                if app.session.state.project_selection.is_empty() {
+                    continue;
+                }
+                "project.delete"
+            } else {
+                "edit.clear"
+            };
+            if let Err(e) = invoke(app, ctx, command, json!({})) {
+                app.ui.status = e;
+            }
             continue;
         }
         if let Some((_, _, id, params)) = binds.iter().find(|(m, k, ..)| *k == key && mods_match(*m, mods)) {

crates/ui-egui/tests/ui_project_delete.rs

@@ -0,0 +1,141 @@
+//! Project-panel deletion uses its selection, even when a Timeline layer is still selected.
+
+use effectcraft_engine::Session;
+use effectcraft_engine::project::{Footage, FootageKind, ItemId, ItemKind, LayerId};
+use effectcraft_ui_egui::EffectcraftApp;
+use effectcraft_ui_egui::dock::PanelKind;
+use egui::{Event, Key, Modifiers, pos2};
+use egui_kittest::Harness;
+use serde_json::json;
+
+fn harness() -> (Harness<'static, EffectcraftApp>, Vec<ItemId>, LayerId) {
+    let mut s = Session::default();
+    s.execute("comp.new", json!({"name": "Delete me", "width": 320, "height": 180, "duration": 4})).unwrap();
+    let comp = s.active_comp_id().unwrap();
+    s.execute("comp.new", json!({"name": "Working composition", "width": 320, "height": 180, "duration": 4})).unwrap();
+    s.execute("layer.newSolid", json!({"name": "Solid", "color": "#406080"})).unwrap();
+    let solid = s.project.items.values().find(|i| matches!(i.kind, ItemKind::Solid(_))).unwrap().id;
+    let footage = std::sync::Arc::make_mut(&mut s.project).add_item(
+        "Data asset",
+        Default::default(),
+        None,
+        ItemKind::Footage(Footage { kind: FootageKind::Data, path: "original.json".into(), data: Some("{}".into()), ..Default::default() }),
+    );
+    let folder = ItemId(s.execute("project.newFolder", json!({"name": "Folder"})).unwrap()["item"].as_u64().unwrap());
+    let keep = LayerId(s.execute("layer.newNull", json!({"name": "Keep this layer"})).unwrap()["layer"].as_u64().unwrap());
+    let mut h = Harness::builder().with_size(egui::vec2(1600.0, 1000.0)).build_eframe(|_| EffectcraftApp::new(s));
+    let folders: Vec<u64> = h.state().session.project.items.values().filter(|i| i.is_folder()).map(|i| i.id.0).collect();
+    h.state_mut().ui.project_open_folders.extend(folders);
+    h.run_steps(3);
+    (h, vec![comp, solid, footage, folder], keep)
+}
+
+fn click(h: &mut Harness<'_, EffectcraftApp>, id: &str, modifiers: Modifiers) {
+    let e = h.state().auto.find(id).unwrap_or_else(|| panic!("no {id}")).clone();
+    let p = pos2(e.rect[0] + e.rect[2] / 2.0, e.rect[1] + e.rect[3] / 2.0);
+    h.input_mut().events.push(Event::ModifiersChanged(modifiers));
+    h.input_mut().events.push(Event::PointerMoved(p));
+    h.input_mut().events.push(Event::PointerButton { pos: p, button: egui::PointerButton::Primary, pressed: true, modifiers });
+    h.step();
+    h.input_mut().events.push(Event::PointerButton { pos: p, button: egui::PointerButton::Primary, pressed: false, modifiers });
+    h.run_steps(2);
+    h.input_mut().events.push(Event::ModifiersChanged(Modifiers::NONE));
+}
+
+fn key(h: &mut Harness<'_, EffectcraftApp>, key: Key, modifiers: Modifiers) {
+    h.input_mut().events.push(Event::Key { key, physical_key: None, pressed: true, repeat: false, modifiers });
+    h.input_mut().events.push(Event::Key { key, physical_key: None, pressed: false, repeat: false, modifiers });
+    h.run_steps(2);
+}
+
+#[test]
+fn delete_and_backspace_remove_project_items_and_undo() {
+    for delete_key in [Key::Delete, Key::Backspace] {
+        for target in 0..4 {
+            let (mut h, items, keep) = harness();
+            let item = items[target];
+            click(&mut h, &format!("project.item.{}.name", item.0), Modifiers::NONE);
+            assert_eq!(h.state().ui.focused, PanelKind::Project);
+            assert_eq!(h.state().session.state.project_selection, vec![item]);
+            assert_eq!(h.state().session.state.selected_layers, vec![keep]);
+            let project = h.state().session.project.clone();
+            let undo = h.state().session.history.undo.len();
+            key(&mut h, delete_key, Modifiers::NONE);
+            assert!(h.state().session.project.item(item).is_none(), "{delete_key:?} should delete project item {target}");
+            assert!(h.state().session.active_comp().unwrap().layer(keep).is_some(), "the unrelated Timeline layer stays");
+            assert!(h.state().session.state.project_selection.is_empty());
+            assert_eq!(h.state().session.project.items.len(), project.items.len() - 1);
+            assert_eq!(h.state().session.history.undo.len(), undo + 1);
+            h.state_mut().session.execute("edit.undo", json!({})).unwrap();
+            assert_eq!(h.state().session.project, project, "one undo restores the item and its references");
+        }
+    }
+}
+
+#[test]
+fn project_multi_selection_deletes_together_and_undoes_once() {
+    let (mut h, items, keep) = harness();
+    for (i, item) in items.iter().enumerate() {
+        click(&mut h, &format!("project.item.{}.name", item.0), if i == 0 { Modifiers::NONE } else { Modifiers::SHIFT });
+    }
+    assert_eq!(h.state().session.state.project_selection, items);
+    let project = h.state().session.project.clone();
+    let undo = h.state().session.history.undo.len();
+    key(&mut h, Key::Backspace, Modifiers::NONE);
+    assert!(items.iter().all(|i| h.state().session.project.item(*i).is_none()));
+    assert!(h.state().session.active_comp().unwrap().layer(keep).is_some());
+    assert_eq!(h.state().session.history.undo.len(), undo + 1);
+    h.state_mut().session.execute("edit.undo", json!({})).unwrap();
+    assert_eq!(h.state().session.project, project);
+}
+
+#[test]
+fn empty_project_selection_does_not_delete_a_timeline_layer() {
+    let (mut h, items, keep) = harness();
+    click(&mut h, &format!("project.item.{}.name", items[0].0), Modifiers::NONE);
+    h.state_mut().session.execute("project.select", json!({"items": []})).unwrap();
+    let undo = h.state().session.history.undo.len();
+    for delete_key in [Key::Delete, Key::Backspace] {
+        key(&mut h, delete_key, Modifiers::NONE);
+        assert!(h.state().session.active_comp().unwrap().layer(keep).is_some());
+        assert_eq!(h.state().session.history.undo.len(), undo);
+    }
+}
+
+#[test]
+fn viewer_focus_keeps_layer_deletion_with_a_project_selection() {
+    for delete_key in [Key::Delete, Key::Backspace] {
+        let (mut h, items, keep) = harness();
+        click(&mut h, &format!("project.item.{}.name", items[0].0), Modifiers::NONE);
+        click(&mut h, "viewer.comp", Modifiers::NONE);
+        h.state_mut().session.execute("layer.select", json!({"layers": [keep.0]})).unwrap();
+        assert_eq!(h.state().ui.focused, PanelKind::Composition);
+        key(&mut h, delete_key, Modifiers::NONE);
+        assert!(h.state().session.active_comp().unwrap().layer(keep).is_none());
+        assert!(h.state().session.project.item(items[0]).is_some());
+    }
+}
+
+#[test]
+fn typing_dialogs_and_modified_keys_do_not_delete_project_items() {
+    let (mut h, items, _) = harness();
+    let item = items[0];
+    click(&mut h, &format!("project.item.{}.name", item.0), Modifiers::NONE);
+    let project = h.state().session.project.clone();
+    for delete_key in [Key::Delete, Key::Backspace] {
+        key(&mut h, delete_key, Modifiers::SHIFT);
+        assert_eq!(h.state().session.project, project);
+    }
+    h.state_mut().dialog = Some(effectcraft_ui_egui::Dialog::About);
+    key(&mut h, Key::Delete, Modifiers::NONE);
+    assert_eq!(h.state().session.project, project);
+    h.state_mut().dialog = None;
+    h.run_steps(2);
+    key(&mut h, Key::Enter, Modifiers::NONE);
+    h.run_steps(2);
+    assert!(h.ctx.egui_wants_keyboard_input(), "the inline rename field has focus");
+    for delete_key in [Key::Backspace, Key::Delete] {
+        key(&mut h, delete_key, Modifiers::NONE);
+        assert_eq!(h.state().session.project, project, "typing must not delete the item");
+    }
+}

docs/gaps.md

@@ -72,6 +72,11 @@ The most important missing piece: it turns every other estimate here into a meas
   pointer tests in `ui_viewer` and `ui_text_edit`, checked headlessly on Linux, cover viewer clicks,
   marquee selection and text picking when layers are hidden or excluded by solo. They also check
   hidden/locked solo layers and solo layers outside their active time range.
+- Regression evidence for [#67](https://github.com/storytold/effectcraft/issues/67): scripted
+  input tests in `ui_project_delete`, checked headlessly on Linux, cover Delete/Backspace on
+  Project items, multiple selection and undo. They check that an unrelated Timeline layer stays,
+  an empty Project selection does nothing, viewer focus still deletes layers, and typing or
+  dialogs do not delete items.
 - Done when: no open bug blocks a basic workflow (import, arrange, animate, preview, render) on
   any of the three desktop platforms, and every bug users report gets triaged within a day.