From f1cd6ed5b392f87643d7167599a17f843829cad9 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Mon, 17 Aug 2026 23:12:11 +0200 Subject: [PATCH] Make the drop decision a rule that can be tested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gesture is Slint's and cannot be driven from a test — synthetic drags do not reach a DragArea at all — but the decision it leads to is where this can actually go wrong, and it was buried in a callback. `decide_drop` names the three outcomes and the order that separates them: an image drag always fills the payload, so photographs pressed after a row was clicked are still filed rather than read as a rearrangement. A row dropped on itself is a no-op here rather than a cycle error from the catalog, and an empty drag with nothing remembered — a file from another application — leaves the tree alone. Co-Authored-By: Claude Opus 5 --- ui/dr-ui/src/collections_ui.rs | 94 +++++++++++++++++++++++++++++----- 1 file changed, 82 insertions(+), 12 deletions(-) diff --git a/ui/dr-ui/src/collections_ui.rs b/ui/dr-ui/src/collections_ui.rs index 75632de..50d8226 100644 --- a/ui/dr-ui/src/collections_ui.rs +++ b/ui/dr-ui/src/collections_ui.rs @@ -187,6 +187,40 @@ pub struct CollectionsController { activity: Rc, } +/// What a release over a collection row means. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Drop { + /// File these photographs in the target. + FileImages(Vec), + /// Move this collection under the target. + Reparent(CollectionId), + /// Nothing to do — an empty drag, or a row dropped on itself. + Nothing, +} + +/// Decide what a drop on `target` should do. +/// +/// Separated from the callback because the gesture cannot be driven from a +/// test — Slint owns it — while this decision is where it can actually go +/// wrong. The order matters: an image drag always fills `carried`, so +/// photographs can never be mistaken for a rearrangement, and only an empty +/// payload consults the row remembered from the press. +pub fn decide_drop( + carried: &[ImageId], + pressed_row: Option, + target: CollectionId, +) -> Drop { + if !carried.is_empty() { + return Drop::FileImages(carried.to_vec()); + } + match pressed_row { + // A collection cannot go inside itself. Caught here as well as in the + // catalog so the common case is a no-op rather than an error message. + Some(source) if source != target => Drop::Reparent(source), + _ => Drop::Nothing, + } +} + impl CollectionsController { pub fn new(activity: Rc) -> Rc { Rc::new(Self { @@ -1601,17 +1635,14 @@ pub fn wire( let Some(w) = weak.upgrade() else { return }; let id = CollectionId(id as u64); let carried = ctl.dragging.borrow().clone(); + // Taken, not read: a remembered press must be spent by the drop it + // belongs to, or a later empty drop — a file dragged in from + // outside, say — would move a collection nobody touched. + let pressed = ctl.dragging_collection.borrow_mut().take(); - // An empty image payload with a remembered source row means the - // tree is being rearranged, not filed into. Checked in this order - // because an image drag always fills `dragging`, so photographs can - // never be mistaken for a reparent. - if carried.is_empty() { - // Taken, not read: a remembered press must be spent by the drop - // it belongs to, or a later empty drop — a file dragged in from - // outside, say — would move a collection nobody touched. - let source = ctl.dragging_collection.borrow_mut().take(); - if let Some(source) = source.filter(|s| *s != id) { + let carried = match decide_drop(&carried, pressed, id) { + Drop::Nothing => return, + Drop::Reparent(source) => { let result = { let borrow = catalog.borrow(); let Some(cat) = borrow.as_ref() else { return }; @@ -1631,9 +1662,10 @@ pub fn wire( // silently does nothing. Err(e) => w.set_collection_error(format!("moving collection: {e}").into()), } + return; } - return; - } + Drop::FileImages(images) => images, + }; // Scoped so the borrow is released before the spring cleanup below, // which needs the catalog itself. @@ -2288,6 +2320,44 @@ mod tests { (1..=n).map(ImageId).collect() } + #[test] + fn dragging_a_collection_onto_another_moves_it() { + // The gesture the tree rearrangement exists for: no images carried, a + // row remembered from the press, dropped somewhere else. + assert_eq!( + decide_drop(&[], Some(CollectionId(7)), CollectionId(9)), + Drop::Reparent(CollectionId(7)) + ); + } + + #[test] + fn photographs_are_filed_even_when_a_row_was_pressed_first() { + // Clicking a collection and then dragging photographs into another must + // file them, not move the collection that happens to be remembered. + // This is the ordering the whole discrimination rests on. + assert_eq!( + decide_drop(&ids(3), Some(CollectionId(7)), CollectionId(9)), + Drop::FileImages(ids(3)) + ); + } + + #[test] + fn a_collection_dropped_on_itself_does_nothing() { + // The catalog would refuse it as a cycle; catching it here makes the + // commonest slip a no-op rather than an error the user has to read. + assert_eq!( + decide_drop(&[], Some(CollectionId(7)), CollectionId(7)), + Drop::Nothing + ); + } + + #[test] + fn an_empty_drag_with_nothing_remembered_moves_nothing() { + // A file dragged in from another application lands here with no images + // and no pressed row. It must not disturb the tree. + assert_eq!(decide_drop(&[], None, CollectionId(9)), Drop::Nothing); + } + #[test] fn a_plain_press_replaces_the_selection() { let all = ids(5);