Take the whole run a shift-click names, not the part that happens to be loaded
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) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
+202
-67
@@ -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<dyn Fn(usize, usize) -> Vec<ImageId>>;
|
||||
|
||||
/// 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<Option<CollectionId>>,
|
||||
/// 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<Option<SpanIds>>,
|
||||
/// 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<ImageId> {
|
||||
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<ImageId>,
|
||||
anchor: &mut Option<usize>,
|
||||
@@ -367,6 +400,7 @@ pub fn apply_press(
|
||||
row: usize,
|
||||
ctrl: bool,
|
||||
shift: bool,
|
||||
span: &dyn Fn(usize, usize) -> Vec<ImageId>,
|
||||
) {
|
||||
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<ImageId>,
|
||||
) {
|
||||
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<ImageId>,
|
||||
anchor: &mut Option<usize>,
|
||||
@@ -483,11 +526,12 @@ pub fn extend_to_row(
|
||||
ids: &[ImageId],
|
||||
offset: usize,
|
||||
row: usize,
|
||||
span: &dyn Fn(usize, usize) -> Vec<ImageId>,
|
||||
) {
|
||||
// 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<S, R, C>(
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub fn wire<S, R, P, C>(
|
||||
window: &AppWindow,
|
||||
ctl: Rc<CollectionsController>,
|
||||
catalog: Rc<RefCell<Option<Catalog>>>,
|
||||
on_scope_changed: S,
|
||||
visible_ids: R,
|
||||
span_ids: P,
|
||||
session: C,
|
||||
) where
|
||||
S: Fn() + 'static,
|
||||
R: Fn() -> Vec<ImageId> + 'static,
|
||||
P: Fn(usize, usize) -> Vec<ImageId> + 'static,
|
||||
C: Fn() -> Option<(
|
||||
dr_sync_nextcloud::AppCredentials,
|
||||
dr_sync_nextcloud::Session,
|
||||
@@ -1421,6 +1469,9 @@ pub fn wire<S, R, C>(
|
||||
// make each of them a separate instantiation for no gain.
|
||||
let on_scope_changed: Rc<dyn Fn()> = 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<S, R, C>(
|
||||
&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<ImageId> + '_ {
|
||||
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<ImageId> {
|
||||
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<_>>(), 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<_>>(), 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<_>>(), 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<_>>(), vec![ImageId(4)]);
|
||||
}
|
||||
|
||||
|
||||
@@ -869,6 +869,7 @@ pub fn run(paths: Vec<PathBuf>) -> 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<PathBuf>) -> 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(),
|
||||
|
||||
+166
-3
@@ -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<usize, dr_catalog::CatalogErro
|
||||
Ok(n as usize)
|
||||
}
|
||||
|
||||
/// TRACES: FR-CAT-5
|
||||
/// The ids of grid rows `first..=last`, in the order the grid lists them.
|
||||
///
|
||||
/// What a shift-click actually means. The gesture names two *ordinals* and the
|
||||
/// photographs between them are mostly not loaded — the grid is a window of a
|
||||
/// hundred or so over a library of twenty thousand — so a range answered from
|
||||
/// the window selected the handful on screen and silently dropped the rest.
|
||||
/// The catalog knows the whole run, and with the ordering indexed it is one
|
||||
/// seek rather than a scan.
|
||||
///
|
||||
/// Bounded by the same predicates, the same filter and the same [`GRID_ORDER`]
|
||||
/// the window itself is read with. An ordinal only names a photograph relative
|
||||
/// to an ordering, so a range taken through any other one is a range through a
|
||||
/// different library.
|
||||
///
|
||||
/// `trash` picks the trash view's list, which is the other thing the grid can
|
||||
/// be showing and is ordered by deletion time rather than capture time.
|
||||
///
|
||||
/// An empty result means the run is empty or the query failed; callers treat
|
||||
/// the two the same, because both leave the selection where it was.
|
||||
pub fn read_ids_span(
|
||||
catalog: &Catalog,
|
||||
scope: Option<dr_types::CollectionId>,
|
||||
filter: &RatingFilter,
|
||||
trash: bool,
|
||||
first: usize,
|
||||
last: usize,
|
||||
) -> Result<Vec<dr_types::ImageId>, 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::<Result<Vec<_>, _>>()?;
|
||||
Ok(ids)
|
||||
}
|
||||
|
||||
/// Shared row mapping, so the scoped and unscoped queries cannot drift.
|
||||
fn row_to_cell(r: &rusqlite::Row) -> rusqlite::Result<LibraryCell> {
|
||||
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<String> = 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
|
||||
|
||||
@@ -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<dr_types::ImageId> {
|
||||
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.
|
||||
///
|
||||
|
||||
Reference in New Issue
Block a user