From 54f9cb54fbee28b08d74506790b9f958e75ec31a Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Mon, 24 Aug 2026 19:50:21 +0200 Subject: [PATCH] Take the whole run a shift-click names, not the part that happens to be loaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shift-clicking two photographs selected only the cells between them that were in the loaded window. The grid is a window of a hundred or so over a library of twenty thousand, and `apply_press` resolved the range against that window — `ids[lo_row..=hi_row]`, clamped to what was there. Everything else in the run had no id anywhere in the UI, so it was silently dropped. The user cannot see that: the selection count is off screen along with the photographs, and the gesture only announces itself when the drop files a dozen images instead of two hundred. The two ends are *ordinals*, and only the catalog knows what lies between them. `read_ids_span` asks it, through the same predicates, the same rating filter and the same ordering the window itself is read with — an ordinal names a photograph only relative to an ordering, so a run taken through any other one is a run through a different library. That ordering is now a constant, `GRID_ORDER`, shared by the window, the trash's own order beside it, and the run: capture time first, with the file name breaking ties and nothing more. A card written by two cameras interleaves names that have nothing to do with each other, and what "everything between these two" means to a photographer is a stretch of an afternoon. The query is reached through a closure handed to `CollectionsController` at wiring time rather than a catalog handle, because the scope and the filter that bound the run belong to the grid's controller. `apply_press` stays a pure function of what it is given, which is what keeps the selection rules testable with no library open — and the tests pass a run that reads a plain slice. Where there is nothing to ask, the loaded window is still used: a poorer answer than the catalog's and a far better one than a gesture that appears to do nothing. Co-Authored-By: Claude Opus 5 (1M context) --- ui/dr-ui/src/collections_ui.rs | 269 +++++++++++++++++++++++++-------- ui/dr-ui/src/lib.rs | 5 + ui/dr-ui/src/library.rs | 169 ++++++++++++++++++++- ui/dr-ui/src/library_ui.rs | 32 ++++ 4 files changed, 405 insertions(+), 70 deletions(-) diff --git a/ui/dr-ui/src/collections_ui.rs b/ui/dr-ui/src/collections_ui.rs index 81e8438..d59960c 100644 --- a/ui/dr-ui/src/collections_ui.rs +++ b/ui/dr-ui/src/collections_ui.rs @@ -86,6 +86,13 @@ impl PressUndo { } } +/// TRACES: FR-CAT-5 +/// How a run of grid ordinals is turned into the ids it names. +/// +/// See [`CollectionsController::span_source`] for why this is supplied rather +/// than reached for. +type SpanIds = Rc Vec>; + /// Selection, drag, and tree state for the running window. /// /// Everything is `RefCell` because Slint callbacks are `Fn`, not `FnMut`, and @@ -233,6 +240,18 @@ pub struct CollectionsController { /// the tree, the grid cells — and doing that from inside the `dropped` /// handler destroys the elements Slint is still using to deliver the event. dropped_on: RefCell>, + /// TRACES: FR-CAT-5 + /// How a run of grid ordinals is turned into ids. + /// + /// Supplied by [`crate::library_ui`] at wiring time, because the catalog, + /// the scope and the rating filter — everything the run depends on — belong + /// to the grid's controller, not to this one. Held as a closure rather than + /// reached for through a handle so the selection rules stay testable with + /// no library open: the tests pass a run that reads a plain slice. + /// + /// `None` before wiring, which the caller reads as "nothing to ask" and + /// falls back to the loaded window. + span_source: RefCell>, /// Where the trash workers report what they are doing. /// /// Shared with [`crate::library_ui`]: a delete and a scan are two jobs in @@ -318,6 +337,15 @@ impl CollectionsController { self.selection.borrow().iter().copied().collect() } + /// TRACES: FR-CAT-5 + /// The ids of grid rows `first..=last`, in the order the grid shows them. + /// + /// Empty when nothing has been wired in — see [`Self::span_source`]. + fn span(&self, first: usize, last: usize) -> Vec { + let source = self.span_source.borrow().clone(); + source.map(|f| f(first, last)).unwrap_or_default() + } + /// Drop the selection — after a scrub, or when the scope changes. /// /// Selection is by id and survives a window change, but a selection the @@ -359,6 +387,11 @@ impl CollectionsController { /// `ids` is the **loaded window**, `offset` where it starts in the library and /// `row` a position within it; the anchor is kept as `offset + row`, an /// ordinal that still names the same photograph after the window has moved. +/// +/// `span` turns a run of ordinals into the ids it names — the catalog's job, +/// since the run is mostly not loaded. Passed in rather than reached for so +/// this stays a pure function of what it is given. +#[allow(clippy::too_many_arguments)] pub fn apply_press( selection: &mut BTreeSet, anchor: &mut Option, @@ -367,6 +400,7 @@ pub fn apply_press( row: usize, ctrl: bool, shift: bool, + span: &dyn Fn(usize, usize) -> Vec, ) { let Some(&id) = ids.get(row) else { return }; let here = offset + row; @@ -401,22 +435,29 @@ pub fn apply_press( (here, from) }; - // The range is described in ordinals and applied to the window: only - // ids that are loaded can be selected, because selection is by id and - // an image the catalog has not been asked for has no id here. A range - // longer than the loaded window is therefore truncated to it rather - // than silently selecting the wrong photographs — which is what - // holding the anchor as a window row used to do. - // - // The slice is always in range and needs no guard: `ids.get(row)` - // succeeded, so the window is non-empty and `here` is inside it, and - // `here` is one of the two bounds. - let last = ids.len() - 1; - let lo_row = lo.saturating_sub(offset); - let hi_row = hi.saturating_sub(offset).min(last); - for id in &ids[lo_row..=hi_row] { - selection.insert(*id); + // The run is described in ordinals and resolved by the catalog, which + // is the only thing that knows what lies between them. Selection is by + // id, and an image outside the loaded window has no id here — so an + // earlier version truncated the run to what was on screen, and + // shift-clicking two ends of a morning selected the dozen cells that + // happened to be loaded. The user cannot see that they did not get + // what they asked for until the drop files a dozen photographs + // instead of two hundred. + let mut run = span(lo, hi); + if run.is_empty() { + // Nothing to ask — no library open, or a query that failed. The + // loaded window is a poorer answer than the catalog's and a far + // better one than selecting nothing. + // + // The slice is always in range and needs no guard: `ids.get(row)` + // succeeded, so the window is non-empty and `here` is inside it, + // and `here` is one of the two bounds. + let last = ids.len() - 1; + let lo_row = lo.saturating_sub(offset); + let hi_row = hi.saturating_sub(offset).min(last); + run = ids[lo_row..=hi_row].to_vec(); } + selection.extend(run); return; } @@ -458,9 +499,10 @@ pub fn press_remembering_anchor( row: usize, ctrl: bool, shift: bool, + span: &dyn Fn(usize, usize) -> Vec, ) { let before = *anchor; - apply_press(selection, anchor, ids, offset, row, ctrl, shift); + apply_press(selection, anchor, ids, offset, row, ctrl, shift, span); // Only a press that *moved* the anchor updates this. The second tap of a // double tap lands on the cell the first tap made the anchor, so it changes // nothing and the origin survives to be extended from. @@ -476,6 +518,7 @@ pub fn press_remembering_anchor( /// selection mode, which the user entered deliberately, and a gesture that /// silently discarded the run they gathered a moment ago would make gathering /// two runs impossible. +#[allow(clippy::too_many_arguments)] pub fn extend_to_row( selection: &mut BTreeSet, anchor: &mut Option, @@ -483,11 +526,12 @@ pub fn extend_to_row( ids: &[ImageId], offset: usize, row: usize, + span: &dyn Fn(usize, usize) -> Vec, ) { // Falling back to the current anchor makes a double tap with no history // select just that cell, which is what a double tap already did. *anchor = previous.or(*anchor); - apply_press(selection, anchor, ids, offset, row, true, true); + apply_press(selection, anchor, ids, offset, row, true, true, span); } /// Apply a press and push the result into the grid — the whole of what a @@ -525,6 +569,7 @@ pub fn select_row( row, ctrl, shift, + &|first, last| ctl.span(first, last), ); ctl.previous_anchor.set(previous); // The cursor follows the press, so an arrow key after a click continues @@ -1401,16 +1446,19 @@ fn format_bytes(bytes: u64) -> String { /// `on_scope_changed` reloads the grid — that lives in [`crate::library_ui`], /// which owns the window and the thumbnail workers, so it is passed in rather /// than reached for. -pub fn wire( +#[allow(clippy::too_many_arguments)] +pub fn wire( window: &AppWindow, ctl: Rc, catalog: Rc>>, on_scope_changed: S, visible_ids: R, + span_ids: P, session: C, ) where S: Fn() + 'static, R: Fn() -> Vec + 'static, + P: Fn(usize, usize) -> Vec + 'static, C: Fn() -> Option<( dr_sync_nextcloud::AppCredentials, dr_sync_nextcloud::Session, @@ -1421,6 +1469,9 @@ pub fn wire( // make each of them a separate instantiation for no gain. let on_scope_changed: Rc = Rc::new(on_scope_changed); let visible_ids = Rc::new(visible_ids); + // A shift-click asks the catalog what lies between its two ends, and the + // catalog belongs to the grid's controller — see `span_source`. + *ctl.span_source.borrow_mut() = Some(Rc::new(span_ids)); let session: Rc< dyn Fn() -> Option<( dr_sync_nextcloud::AppCredentials, @@ -1500,6 +1551,7 @@ pub fn wire( &ids, offset, row as usize, + &|first, last| ctl.span(first, last), ); ctl.set_cursor(Some(offset + row as usize)); sync_selection(&w, &ctl, &ids); @@ -2494,6 +2546,28 @@ mod tests { (1..=n).map(ImageId).collect() } + /// The catalog's answer to "what is between these two ordinals". + /// + /// Stands in for [`crate::library_ui::LibraryController::ids_in_span`], + /// which does the same thing with a `LIMIT`/`OFFSET`: the whole library is + /// available to the query whatever the grid happens to have loaded, and an + /// ordinal indexes it directly. + fn library(all: &[ImageId]) -> impl Fn(usize, usize) -> Vec + '_ { + move |first, last| { + all.iter() + .copied() + .skip(first) + .take((last + 1).saturating_sub(first)) + .collect() + } + } + + /// No catalog to ask — what a press sees before a library is open, and if + /// the query fails. + fn unloaded(_first: usize, _last: usize) -> Vec { + Vec::new() + } + #[test] fn dragging_a_collection_onto_another_moves_it() { // The gesture the tree rearrangement exists for: no images carried, a @@ -2535,11 +2609,12 @@ mod tests { #[test] fn a_plain_press_replaces_the_selection() { let all = ids(5); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 2, false, false); + apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 2, false, false, &span); assert_eq!(sel.iter().copied().collect::>(), vec![ImageId(3)]); } @@ -2547,26 +2622,28 @@ mod tests { #[test] fn ctrl_press_adds_and_then_removes() { let all = ids(5); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false); + apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false, &span); assert_eq!(sel.len(), 2); // Toggling: a second ctrl-press on the same cell takes it out again. - apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false); + apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false, &span); assert_eq!(sel.iter().copied().collect::>(), vec![ImageId(1)]); } #[test] fn shift_press_extends_a_contiguous_range() { let all = ids(10); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 2, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 6, false, true); + apply_press(&mut sel, &mut anchor, &all, 0, 2, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 6, false, true, &span); assert_eq!(sel.len(), 5, "rows 2..=6 inclusive"); assert!(sel.contains(&ImageId(3)) && sel.contains(&ImageId(7))); @@ -2575,11 +2652,12 @@ mod tests { #[test] fn shift_extends_backwards_too() { let all = ids(10); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 6, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 2, false, true); + apply_press(&mut sel, &mut anchor, &all, 0, 6, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 2, false, true, &span); assert_eq!(sel.len(), 5); } @@ -2589,15 +2667,16 @@ mod tests { // clearing would turn the correction into a union, and the user would // drag cells they believed they had just deselected. let all = ids(20); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 5, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 15, false, true); + apply_press(&mut sel, &mut anchor, &all, 0, 5, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 15, false, true, &span); assert_eq!(sel.len(), 11, "rows 5..=15"); // Corrected to a shorter range from the same anchor. - apply_press(&mut sel, &mut anchor, &all, 0, 8, false, true); + apply_press(&mut sel, &mut anchor, &all, 0, 8, false, true, &span); assert_eq!(sel.len(), 4, "rows 5..=8, and nothing from the first range"); assert!(!sel.contains(&ImageId(16)), "row 15 is no longer selected"); } @@ -2607,12 +2686,13 @@ mod tests { // If the anchor moved to each shift-click, a range could only ever be // grown, never corrected inward. let all = ids(20); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 10, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 14, false, true); - apply_press(&mut sel, &mut anchor, &all, 0, 12, false, true); + apply_press(&mut sel, &mut anchor, &all, 0, 10, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 14, false, true, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 12, false, true, &span); assert_eq!(anchor, Some(10)); assert_eq!(sel.len(), 3, "rows 10..=12"); @@ -2627,7 +2707,17 @@ mod tests { all: &[ImageId], row: usize, ) { - press_remembering_anchor(sel, anchor, previous, all, 0, row, true, false); + press_remembering_anchor( + sel, + anchor, + previous, + all, + 0, + row, + true, + false, + &library(all), + ); } #[test] @@ -2638,6 +2728,7 @@ mod tests { // it on, the second turns it off — and the double tap that follows has // to select the range anyway. let all = ids(20); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; let mut previous = None; @@ -2653,6 +2744,7 @@ mod tests { 4, false, false, + &span, ); tap(&mut sel, &mut anchor, &mut previous, &all, 11); @@ -2663,7 +2755,7 @@ mod tests { job to select the range rather than to add one cell" ); - extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 11); + extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 11, &span); assert_eq!(sel.len(), 8, "rows 4..=11"); assert!(sel.contains(&ImageId(5)) && sel.contains(&ImageId(12))); @@ -2675,6 +2767,7 @@ mod tests { // user in selection mode is gathering, and the second gesture must not // throw away the first. let all = ids(30); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; let mut previous = None; @@ -2688,17 +2781,18 @@ mod tests { 0, false, false, + &span, ); tap(&mut sel, &mut anchor, &mut previous, &all, 3); tap(&mut sel, &mut anchor, &mut previous, &all, 3); - extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 3); + extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 3, &span); assert_eq!(sel.len(), 4, "rows 0..=3"); // A second run, begun with a plain tap somewhere else. tap(&mut sel, &mut anchor, &mut previous, &all, 20); tap(&mut sel, &mut anchor, &mut previous, &all, 25); tap(&mut sel, &mut anchor, &mut previous, &all, 25); - extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 25); + extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 25, &span); assert_eq!(sel.len(), 10, "rows 0..=3 and 20..=25"); assert!(sel.contains(&ImageId(1)) && sel.contains(&ImageId(26))); @@ -2709,11 +2803,12 @@ mod tests { // The degenerate case: selection mode entered from the header's button // rather than by holding a cell, so nothing has anchored yet. let all = ids(10); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; let previous = None; - extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 6); + extend_to_row(&mut sel, &mut anchor, previous, &all, 0, 6, &span); assert_eq!(sel.iter().copied().collect::>(), vec![ImageId(7)]); } @@ -2722,16 +2817,17 @@ mod tests { // Picking up a second run without losing the first: the one case where // a shift-click must not clear. let all = ids(20); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 2, false, true); + apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 2, false, true, &span); assert_eq!(sel.len(), 3); // A new anchor by ctrl-click, then a ctrl+shift range from it. - apply_press(&mut sel, &mut anchor, &all, 0, 10, true, false); - apply_press(&mut sel, &mut anchor, &all, 0, 12, true, true); + apply_press(&mut sel, &mut anchor, &all, 0, 10, true, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 12, true, true, &span); assert_eq!(sel.len(), 6, "rows 0..=2 and 10..=12"); assert!(sel.contains(&ImageId(1)) && sel.contains(&ImageId(13))); @@ -2742,16 +2838,17 @@ mod tests { // This is what makes a multi-image drag possible: the press that starts // the drag must not collapse what it is about to carry. let all = ids(5); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 1, true, false); - apply_press(&mut sel, &mut anchor, &all, 0, 2, true, false); + apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 1, true, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 2, true, false, &span); assert_eq!(sel.len(), 3); // Pressing one of the three to begin a drag. - apply_press(&mut sel, &mut anchor, &all, 0, 1, false, false); + apply_press(&mut sel, &mut anchor, &all, 0, 1, false, false, &span); assert_eq!(sel.len(), 3, "the selection survived the press"); } @@ -2763,17 +2860,18 @@ mod tests { // the selection but left the anchor moved would make the next // shift-click select a run from a cell nobody pointed at. let all = ids(6); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; // A selection built the ordinary way, and the state it leaves behind. - apply_press(&mut sel, &mut anchor, &all, 0, 1, false, false); - apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false); + apply_press(&mut sel, &mut anchor, &all, 0, 1, false, false, &span); + apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false, &span); let before = (sel.clone(), anchor); // The finger that opens a pinch. let undo = PressUndo::capture(&sel, anchor, Some(1), Some(3)); - apply_press(&mut sel, &mut anchor, &all, 0, 5, false, false); + apply_press(&mut sel, &mut anchor, &all, 0, 5, false, false, &span); assert_ne!( (sel.clone(), anchor), before, @@ -2791,10 +2889,11 @@ mod tests { fn a_press_past_the_end_of_the_window_is_ignored() { // The grid is windowed and a stale row index can arrive after a scrub. let all = ids(3); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 99, false, false); + apply_press(&mut sel, &mut anchor, &all, 0, 99, false, false, &span); assert!(sel.is_empty()); } @@ -2806,6 +2905,7 @@ mod tests { // that row — so shift-clicking after scrolling selected a run the user // never pointed at, silently and with no way to tell. let all = ids(20); + let span = library(&all); let first: Vec<_> = all[0..10].to_vec(); let later: Vec<_> = all[4..14].to_vec(); @@ -2813,12 +2913,12 @@ mod tests { let mut anchor = None; // Anchor on the sixth image, in a window starting at the beginning. - apply_press(&mut sel, &mut anchor, &first, 0, 5, false, false); + apply_press(&mut sel, &mut anchor, &first, 0, 5, false, false, &span); assert_eq!(anchor, Some(5), "the anchor is an ordinal, not a row"); // The user scrolls — the window now starts four images in — and // shift-clicks the image at ordinal 10. - apply_press(&mut sel, &mut anchor, &later, 4, 6, false, true); + apply_press(&mut sel, &mut anchor, &later, 4, 6, false, true, &span); assert_eq!(sel.len(), 6, "ordinals 5..=10"); assert!(sel.contains(&ImageId(6)), "the anchored image is still in"); @@ -2827,51 +2927,86 @@ mod tests { } #[test] - fn a_range_reaching_outside_the_window_selects_what_is_loaded() { - // Selection is by id, so a range can only cover images the window - // holds. Truncating is the honest answer; the alternative — indexing - // from the window's start as though it were the library's — selects - // the wrong photographs and looks like it worked. + fn a_range_reaching_outside_the_window_selects_the_whole_run() { + // The bug this exists for: the anchor is far above the loaded window, + // so all but four of the photographs in the range are off screen. They + // are still what the user asked for, and the catalog is what knows + // their ids — reading the range from the window selected the four that + // happened to be loaded and looked as though it had worked. let all = ids(30); + let span = library(&all); let window: Vec<_> = all[10..20].to_vec(); let mut sel = BTreeSet::new(); let mut anchor = Some(0); - apply_press(&mut sel, &mut anchor, &window, 10, 3, false, true); + apply_press(&mut sel, &mut anchor, &window, 10, 3, false, true, &span); - assert_eq!(sel.len(), 4, "ordinals 10..=13, the loaded part of 0..=13"); - assert!(sel.contains(&ImageId(11)) && sel.contains(&ImageId(14))); + assert_eq!(sel.len(), 14, "ordinals 0..=13, loaded or not"); + assert!( + sel.contains(&ImageId(1)), + "the anchored image, never loaded" + ); + assert!(sel.contains(&ImageId(14)), "up to the cell pressed"); + assert!(!sel.contains(&ImageId(15)), "and no further"); } #[test] - fn a_range_running_off_the_far_end_stops_at_the_window() { + fn a_range_running_off_the_far_end_is_whole_too() { // The mirror of the case above, with the anchor *ahead* of the press - // instead of behind it. Both directions truncate to what is loaded, - // and neither may wrap round to the other end of the window. + // instead of behind it. Neither direction may stop at the window, and + // neither may wrap round to the other end of it. let all = ids(30); + let span = library(&all); let window: Vec<_> = all[0..5].to_vec(); let mut sel = BTreeSet::new(); let mut anchor = Some(25); - apply_press(&mut sel, &mut anchor, &window, 0, 2, false, true); + apply_press(&mut sel, &mut anchor, &window, 0, 2, false, true, &span); - assert_eq!(sel.len(), 3, "ordinals 2..=4, the loaded part of 2..=25"); - assert!(sel.contains(&ImageId(3)) && sel.contains(&ImageId(5))); + assert_eq!(sel.len(), 24, "ordinals 2..=25"); + assert!(sel.contains(&ImageId(3)) && sel.contains(&ImageId(26))); assert!( !sel.contains(&ImageId(1)), "nothing before the pressed cell" ); } + #[test] + fn a_range_with_no_catalog_to_ask_falls_back_to_the_window() { + // Before a library is open, and if the query fails. What is loaded is + // a poorer answer than the catalog's and a far better one than a + // gesture that appears to do nothing. + let all = ids(30); + let window: Vec<_> = all[10..20].to_vec(); + + let mut sel = BTreeSet::new(); + let mut anchor = Some(0); + + apply_press( + &mut sel, + &mut anchor, + &window, + 10, + 3, + false, + true, + &unloaded, + ); + + assert_eq!(sel.len(), 4, "ordinals 10..=13, the loaded part of 0..=13"); + assert!(sel.contains(&ImageId(11)) && sel.contains(&ImageId(14))); + } + #[test] fn shift_without_an_anchor_selects_just_the_one() { let all = ids(5); + let span = library(&all); let mut sel = BTreeSet::new(); let mut anchor = None; - apply_press(&mut sel, &mut anchor, &all, 0, 3, false, true); + apply_press(&mut sel, &mut anchor, &all, 0, 3, false, true, &span); assert_eq!(sel.iter().copied().collect::>(), vec![ImageId(4)]); } diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index 3d81eac..ec7e249 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -869,6 +869,7 @@ pub fn run(paths: Vec) -> Result<()> { let lib = library.clone(); let coll = collections.clone(); let lib_ids = library.clone(); + let lib_span = library.clone(); let lib_session = library.clone(); collections_ui::wire( &window, @@ -885,6 +886,10 @@ pub fn run(paths: Vec) -> Result<()> { library_ui::reload(&w, &lib); }, move || lib_ids.visible_ids(), + // What a shift-click selects: the whole run between its two + // ends, read from the catalog rather than from the hundred or + // so rows that happen to be loaded. + move |first, last| lib_span.ids_in_span(first, last), // The trash's MOVE and DELETE go to the same account the scan // and thumbnail workers use. move || lib_session.session(), diff --git a/ui/dr-ui/src/library.rs b/ui/dr-ui/src/library.rs index 981d4d3..85b1fca 100644 --- a/ui/dr-ui/src/library.rs +++ b/ui/dr-ui/src/library.rs @@ -176,6 +176,30 @@ const VISIBLE_UNALIASED: &str = "shadowed_by IS NULL AND trashed_at IS NULL"; /// restore the same frame twice. const TRASHED: &str = "i.shadowed_by IS NULL AND i.trashed_at IS NOT NULL"; +/// TRACES: FR-CAT-4 +/// The order the grid lists photographs in: when they were taken. +/// +/// The file name breaks ties and nothing more — two frames of one burst, or a +/// RAW beside the JPEG the camera wrote with it. What a photographer looks for +/// is the afternoon, not what the camera called the file, and a grid ordered by +/// name interleaves every camera and every card that ever wrote into the same +/// folder. +/// +/// Shared rather than spelled out per query, for the same reason [`VISIBLE`] is: +/// the window, the count and the run a shift-click resolves are three answers +/// about one list, and an ordering that drifted between them would select +/// photographs the user never saw without any of it looking wrong. +/// +/// Undated images sort last in either direction. EXIF is read as thumbnails +/// load, so a freshly scanned library would otherwise open on the images it +/// knows least about. +const GRID_ORDER: &str = "ORDER BY i.captured_at IS NULL, i.captured_at ASC, i.source_ref ASC"; + +/// TRACES: FR-CAT-15 +/// [`GRID_ORDER`] for the trash, which is ordered by when a thing was deleted — +/// see [`read_trashed_cells`] for why that view answers a different question. +const TRASH_ORDER: &str = "ORDER BY i.trashed_at DESC, i.source_ref ASC"; + /// TRACES: FR-CAT-6 | FR-CULL-4 /// What the grid is narrowed to by the rating filter bar. /// @@ -2919,7 +2943,7 @@ pub fn read_cells_scoped( WHERE {VISIBLE}{rated} AND i.id IN (SELECT image_id FROM collection_members WHERE collection_id IN ({placeholders})) - ORDER BY i.captured_at IS NULL, i.captured_at ASC, i.source_ref ASC + {GRID_ORDER} LIMIT ? OFFSET ?" ); @@ -2950,7 +2974,7 @@ fn read_cells_all( FROM images i LEFT JOIN remote r ON r.image_id = i.id WHERE {VISIBLE}{rated} - ORDER BY i.captured_at IS NULL, i.captured_at ASC, i.source_ref ASC + {GRID_ORDER} LIMIT ?1 OFFSET ?2" ))?; let rows = stmt @@ -2982,7 +3006,7 @@ pub fn read_trashed_cells( FROM images i LEFT JOIN remote r ON r.image_id = i.id WHERE {TRASHED} - ORDER BY i.trashed_at DESC, i.source_ref ASC + {TRASH_ORDER} LIMIT ?1 OFFSET ?2" ))?; let rows = stmt @@ -3005,6 +3029,68 @@ pub fn total_trashed(catalog: &Catalog) -> Result, + filter: &RatingFilter, + trash: bool, + first: usize, + last: usize, +) -> Result, dr_catalog::CatalogError> { + let Some(count) = (last + 1).checked_sub(first) else { + return Ok(Vec::new()); + }; + + let (sql, mut params) = if trash { + ( + format!("SELECT i.id FROM images i WHERE {TRASHED} {TRASH_ORDER} LIMIT ? OFFSET ?"), + Vec::new(), + ) + } else { + let (clause, params) = scope_clause(catalog, scope)?; + let rated = filter.sql(); + ( + format!( + "SELECT i.id FROM images i + WHERE {VISIBLE}{rated}{clause} + {GRID_ORDER} + LIMIT ? OFFSET ?" + ), + params, + ) + }; + params.push(rusqlite::types::Value::Integer(count as i64)); + params.push(rusqlite::types::Value::Integer(first as i64)); + + let mut stmt = catalog.connection().prepare(&sql)?; + let ids = stmt + .query_map(rusqlite::params_from_iter(params.iter()), |r| { + Ok(dr_types::ImageId(r.get::<_, i64>(0)? as u64)) + })? + .collect::, _>>()?; + Ok(ids) +} + /// Shared row mapping, so the scoped and unscoped queries cannot drift. fn row_to_cell(r: &rusqlite::Row) -> rusqlite::Result { let path: String = r.get(1)?; @@ -4214,6 +4300,83 @@ mod tests { assert!(read_cells_all(&catalog, &ranged, 0, 50).unwrap().is_empty()); } + #[test] + fn a_span_reads_the_whole_run_whether_or_not_it_is_loaded() { + // The shift-click this exists for. The grid holds a window of five and + // the user names a run of twelve, so seven of them have no cell and no + // id anywhere in the UI — but they are still what was asked for, and + // the catalog is what knows them. + let catalog = scanned(12); + let filter = RatingFilter::default(); + + let loaded = read_cells_all(&catalog, &filter, 0, 5).unwrap(); + assert_eq!(loaded.len(), 5, "the window is smaller than the run"); + + let whole: Vec<_> = read_cells_all(&catalog, &filter, 0, 50) + .unwrap() + .iter() + .map(|c| dr_types::ImageId(c.image_id as u64)) + .collect(); + let span = read_ids_span(&catalog, None, &filter, false, 0, 11).unwrap(); + + assert_eq!(span.len(), 12); + assert_eq!(span, whole, "the run is the grid's own list, in its order"); + } + + #[test] + fn a_span_starts_and_ends_where_it_was_asked_to() { + // Ordinals index the grid's list, so a run has to be exactly the slice + // of it the two ends name — one off at either end selects a + // photograph the user did not point at. + let catalog = scanned(12); + let filter = RatingFilter::default(); + let whole: Vec<_> = read_cells_all(&catalog, &filter, 0, 50) + .unwrap() + .iter() + .map(|c| dr_types::ImageId(c.image_id as u64)) + .collect(); + + let span = read_ids_span(&catalog, None, &filter, false, 4, 6).unwrap(); + assert_eq!(span, whole[4..=6], "ordinals 4..=6, inclusive at both ends"); + } + + #[test] + fn a_span_is_ordered_by_capture_time_rather_than_by_name() { + // A card written by two cameras interleaves names that have nothing to + // do with each other. What a photographer means by "everything between + // these two" is a stretch of an afternoon, so the run has to be taken + // through capture time — the ordering the grid draws them in. + let catalog = scanned(3); + let conn = catalog.connection(); + for (n, at) in [(1, 9_000), (2, 5_000), (3, 1_000)] { + conn.execute( + "UPDATE images SET captured_at = ?2 WHERE source_ref LIKE ?1", + rusqlite::params![format!("%IMG_000{n}%"), at], + ) + .unwrap(); + } + + let span = read_ids_span(&catalog, None, &RatingFilter::default(), false, 0, 2).unwrap(); + let names: Vec = span + .iter() + .map(|id| { + conn.query_row( + "SELECT source_ref FROM images WHERE id = ?1", + [id.0 as i64], + |r| r.get::<_, String>(0), + ) + .unwrap() + }) + .collect(); + + assert!( + names[0].ends_with("IMG_0003.CR2") + && names[1].ends_with("IMG_0002.CR2") + && names[2].ends_with("IMG_0001.CR2"), + "earliest first, which here is the reverse of the file names: {names:?}" + ); + } + #[test] fn the_histogram_ignores_the_range_it_is_used_to_choose() { // Drawing the axis through the chosen range would collapse it onto the diff --git a/ui/dr-ui/src/library_ui.rs b/ui/dr-ui/src/library_ui.rs index 121aa43..d1958fe 100644 --- a/ui/dr-ui/src/library_ui.rs +++ b/ui/dr-ui/src/library_ui.rs @@ -590,6 +590,38 @@ impl LibraryController { .collect() } + /// TRACES: FR-CAT-5 + /// Catalog ids of grid rows `first..=last`, loaded or not. + /// + /// The counterpart to [`Self::visible_ids`], and the reason both exist: a + /// shift-click names a run by its two ends, and everything between them is + /// usually off screen. Answered from the catalog, through the same scope, + /// filter and ordering this controller reads its window with — so the run + /// is the run the user can see themselves selecting, continued past the + /// edge of what has been loaded. + /// + /// Empty before a library is open, and empty if the query fails: a + /// selection gesture is not worth an error dialog, and the caller keeps + /// what was already selected. + pub fn ids_in_span(&self, first: usize, last: usize) -> Vec { + let borrow = self.catalog.borrow(); + let Some(catalog) = borrow.as_ref() else { + return Vec::new(); + }; + library::read_ids_span( + catalog, + *self.scope.borrow(), + &self.filter.borrow(), + self.viewing_trash.get(), + first, + last, + ) + .unwrap_or_else(|e| { + log::warn!("reading the selected range: {e}"); + Vec::new() + }) + } + /// TRACES: FR-EXP-7 /// Remote paths for a set of selected images, in the order they were given. ///