Narrow the grid to several people at once, either way

"Show photos" could only ever mean one person. The two questions a photographer
actually asks are "every picture of Anna or Bob" and "the pictures they are both
in", and the second is not reachable by any sequence of single-person filters —
no amount of switching between one person and another finds the frame they
share.

So the filter holds a *set* of people and a mode. `RatingFilter` was already the
right home, as its own doc says: every query path threads it, so the count in
the header and the cells in the grid are narrowed by the same thing, and this
composes with stars, flags and the date range for free.

The union is `EXISTS ... person_id IN (...)`. The intersection counts
**distinct** people per image and compares against the size of the selection —
one subquery rather than one per person, and it does not grow the statement with
the selection. `DISTINCT` is what makes it correct: three faces of Anna in one
frame must not satisfy a filter asking for Anna and Bob, and there is a test
that says so.

Any rather than All is the default. With one person the modes are the same
filter, and adding a second to a union can only ever show more — so a user who
has not noticed the toggle never ends up staring at an empty grid wondering what
they broke. The toggle only appears at two people, because a control that
demonstrably does nothing is a control that teaches the user to ignore it.

Building the set needs no picker of its own: the Identity screen gains "And
also…" beside "Show photos", offered only once the grid is already narrowed to
somebody. Each person is a chip on the filter bar and each chip removes just
that person, so a selection of three can be taken apart one at a time rather
than only cleared wholesale.

`RatingFilter` stops being `Copy`, since it now holds a `Vec`. Every query path
already took it by reference; the casualties were two struct updates and one
`Cell` that becomes a `RefCell`.

484 dr-ui tests pass, including the union, the intersection, that one person
reads the same in both modes, and the repeated-faces trap.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-27 22:17:59 +02:00
co-authored by Claude Opus 5
parent a1790c3e67
commit af5a13b3f7
5 changed files with 374 additions and 67 deletions
+214 -14
View File
@@ -219,7 +219,10 @@ const TRASH_ORDER: &str = "ORDER BY i.trashed_at DESC, i.source_ref ASC";
/// fetch, which is the transfer FR-NC-3 exists to avoid — and the count in the
/// header has to agree with the cells, which it cannot if the two are computed
/// at different stages.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
/// Not `Copy`: [`RatingFilter::people`] is a `Vec`. Every query path already
/// takes this by reference, so the only casualties were two `..*self` struct
/// updates, which clone instead.
#[derive(Debug, Clone, PartialEq, Eq, Default)]
pub struct RatingFilter {
/// Minimum stars. 0 means no star constraint.
pub min_rating: u8,
@@ -250,7 +253,7 @@ pub struct RatingFilter {
pub captured_from: Option<i64>,
pub captured_to: Option<i64>,
/// TRACES: FR-CULL-11
/// Only photographs this person appears in.
/// Only photographs these people appear in.
///
/// The way back from a face to the pictures it came from, which is the
/// question the People screen leaves the user holding: they have just
@@ -267,7 +270,35 @@ pub struct RatingFilter {
/// grouped someone and not yet confirmed a single face would otherwise get
/// an empty grid, which reads as "no photographs of this person" rather
/// than "you have not ticked anything yet".
pub person: Option<u64>,
///
/// A set rather than one id, because the two questions a photographer
/// actually asks are "every picture of Anna *or* Bob" and "the pictures
/// they are *both* in", and the second is not reachable by any sequence of
/// single-person filters. [`RatingFilter::people_mode`] picks between them.
pub people: Vec<u64>,
/// Whether [`RatingFilter::people`] is a union or an intersection.
pub people_mode: PeopleMode,
}
/// How several people combine when the grid is narrowed by identity.
///
/// TRACES: FR-CULL-11
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
pub enum PeopleMode {
/// Photographs holding **any** of them — the union.
///
/// The default, and the right one for one person, where the two modes are
/// identical. It is also the forgiving direction: adding a second person to
/// a union can only ever show more, so a user who has not noticed the
/// toggle never ends up staring at an empty grid wondering what they broke.
#[default]
Any,
/// Photographs holding **all** of them — the intersection.
///
/// "Pictures of the two of them together", which is the one worth having a
/// mode for: it is how you find the photograph you remember rather than
/// scrolling everything either of them appears in.
All,
}
impl RatingFilter {
@@ -279,7 +310,7 @@ impl RatingFilter {
&& !self.local_only
&& self.captured_from.is_none()
&& self.captured_to.is_none()
&& self.person.is_none()
&& self.people.is_empty()
}
/// Whether a date range is narrowing the grid.
@@ -296,7 +327,7 @@ impl RatingFilter {
Self {
captured_from: None,
captured_to: None,
..*self
..self.clone()
}
}
@@ -353,15 +384,38 @@ impl RatingFilter {
);
}
if let Some(person) = self.person {
// An integer this code owns, like every other term here. `EXISTS`
// rather than a join, so a photograph holding three faces of the
// same person appears once — the grid shows pictures, not faces.
terms.push(format!(
"EXISTS (SELECT 1 FROM faces f
JOIN face_person fp ON fp.face_id = f.id
WHERE f.image_id = i.id AND fp.person_id = {person})"
));
if !self.people.is_empty() {
// Integers this code owns, like every other term here — the ids
// come from the catalog, never from typed text, so there is
// nothing to escape.
let ids = self
.people
.iter()
.map(|p| p.to_string())
.collect::<Vec<_>>()
.join(",");
terms.push(match self.people_mode {
// `EXISTS` rather than a join, so a photograph holding three
// faces of the same person appears once — the grid shows
// pictures, not faces.
PeopleMode::Any => format!(
"EXISTS (SELECT 1 FROM faces f
JOIN face_person fp ON fp.face_id = f.id
WHERE f.image_id = i.id AND fp.person_id IN ({ids}))"
),
// Counting *distinct* people rather than ANDing one EXISTS per
// person: same result, one subquery instead of n, and it does
// not grow the statement with the selection. `DISTINCT` is
// what makes it correct — three faces of Anna in one frame
// must not satisfy a filter asking for Anna and Bob.
PeopleMode::All => format!(
"(SELECT COUNT(DISTINCT fp.person_id) FROM faces f
JOIN face_person fp ON fp.face_id = f.id
WHERE f.image_id = i.id AND fp.person_id IN ({ids})) = {}",
self.people.len()
),
});
}
if let Some(flag) = self.flag {
@@ -5497,4 +5551,150 @@ mod tests {
}
.is_unfiltered());
}
// ── narrowing the grid by identity ────────────────────────────────────
/// Put `person` on the given images, as a suggestion.
fn assign(
catalog: &Catalog,
person: dr_catalog::faces::PersonId,
images: &[dr_types::ImageId],
) {
for img in images {
let face = dr_catalog::faces::DetectedFace {
x: 0.1,
y: 0.1,
w: 0.2,
h: 0.2,
landmarks: [(0.0, 0.0); 5],
confidence: 0.9,
embedding: vec![0u8; 1024],
crop_px: 120.0,
crop: Vec::new(),
model_id: "w600k_mbf".into(),
};
// Appends rather than replaces across calls for *different*
// people, because `record_detections` clears the image first —
// so the second person's face is added by hand.
let existing: Vec<i64> = {
let mut q = catalog
.connection()
.prepare("SELECT id FROM faces WHERE image_id = ?1")
.unwrap();
q.query_map([img.0 as i64], |r| r.get(0))
.unwrap()
.map(Result::unwrap)
.collect()
};
let id = if existing.is_empty() {
dr_catalog::faces::record_detections(
catalog.connection(),
*img,
"w600k_mbf",
1024,
std::slice::from_ref(&face),
)
.unwrap()[0]
} else {
catalog
.connection()
.execute(
"INSERT INTO faces
(image_id, x, y, w, h, landmarks, detector_confidence,
embedding, crop_px, model_id, detected_at)
VALUES (?1, 0.5, 0.5, 0.2, 0.2, X'00', 0.9, X'00', 120.0, 'w600k_mbf', 0)",
[img.0 as i64],
)
.unwrap();
dr_catalog::faces::FaceId(catalog.connection().last_insert_rowid() as u64)
};
dr_catalog::faces::suggest(catalog.connection(), id, person, 0.9).unwrap();
}
}
/// The two questions a photographer actually asks, and the reason the
/// filter holds a set rather than one id: the intersection is not reachable
/// by any sequence of single-person filters.
#[test]
fn people_narrow_the_grid_as_a_union_or_an_intersection() {
let catalog = with_images(4);
let ids = image_ids(&catalog);
let anna = dr_catalog::faces::create_person(catalog.connection(), "Anna").unwrap();
let bob = dr_catalog::faces::create_person(catalog.connection(), "Bob").unwrap();
// 0: Anna. 1: both. 2: Bob. 3: neither.
assign(&catalog, anna, &[ids[0], ids[1]]);
assign(&catalog, bob, &[ids[1], ids[2]]);
let count = |people: Vec<u64>, mode: PeopleMode| {
let f = RatingFilter {
people,
people_mode: mode,
..Default::default()
};
total_images_scoped(&catalog, None, &f).unwrap()
};
assert_eq!(count(vec![anna.0], PeopleMode::Any), 2, "Anna alone");
assert_eq!(count(vec![bob.0], PeopleMode::Any), 2, "Bob alone");
assert_eq!(
count(vec![anna.0, bob.0], PeopleMode::Any),
3,
"the union should hold every picture either is in"
);
assert_eq!(
count(vec![anna.0, bob.0], PeopleMode::All),
1,
"the intersection should hold only the picture they share"
);
}
/// One person is the same filter either way, and the UI leans on that to
/// hide the toggle until there are two.
#[test]
fn one_person_reads_the_same_in_both_modes() {
let catalog = with_images(3);
let ids = image_ids(&catalog);
let anna = dr_catalog::faces::create_person(catalog.connection(), "Anna").unwrap();
assign(&catalog, anna, &[ids[0], ids[1]]);
for mode in [PeopleMode::Any, PeopleMode::All] {
let f = RatingFilter {
people: vec![anna.0],
people_mode: mode,
..Default::default()
};
assert_eq!(total_images_scoped(&catalog, None, &f).unwrap(), 2);
}
}
/// Three faces of one person in a frame must not satisfy "Anna and Bob".
/// This is what `COUNT(DISTINCT ...)` is for, and it is the intersection's
/// one real trap.
#[test]
fn repeated_faces_of_one_person_do_not_satisfy_an_intersection() {
let catalog = with_images(2);
let ids = image_ids(&catalog);
let anna = dr_catalog::faces::create_person(catalog.connection(), "Anna").unwrap();
let bob = dr_catalog::faces::create_person(catalog.connection(), "Bob").unwrap();
// Two separate faces, both Anna, in the same photograph.
assign(&catalog, anna, &[ids[0]]);
assign(&catalog, anna, &[ids[0]]);
let f = RatingFilter {
people: vec![anna.0, bob.0],
people_mode: PeopleMode::All,
..Default::default()
};
assert_eq!(total_images_scoped(&catalog, None, &f).unwrap(), 0);
}
#[test]
fn no_people_narrows_nothing() {
let catalog = with_images(3);
let f = RatingFilter::default();
assert!(f.is_unfiltered());
assert_eq!(total_images_scoped(&catalog, None, &f).unwrap(), 3);
}
}
+102 -34
View File
@@ -22,7 +22,7 @@ use dr_types::FormatFilter;
use slint::{ComponentHandle, Model as _};
use crate::library::{self, ScanMessage, ThumbnailMessage};
use crate::{AppWindow, KeywordRow, LibraryCell, TimelineBar};
use crate::{AppWindow, KeywordRow, LibraryCell, PersonChip, TimelineBar};
/// A screenful before the grid has reported its geometry.
///
@@ -77,7 +77,8 @@ const MAX_CELL_SIZE: f32 = 420.0;
/// every load for the scrollbar, and a scan landing, a delete or a restore all
/// move it. What it cannot see — a rating edited under an unchanged count — is
/// covered because the paths that do that refresh the chips themselves.
#[derive(Clone, Copy, PartialEq, Eq)]
/// Not `Copy` since `RatingFilter` stopped being — it holds a set of people.
#[derive(Clone, PartialEq, Eq)]
struct LibraryFacts {
scope: Option<dr_types::CollectionId>,
filter: library::RatingFilter,
@@ -141,7 +142,7 @@ pub struct LibraryController {
window: RefCell<usize>,
/// What the whole-library readouts on screen were last computed for, so a
/// window that merely moved does not recompute them. See [`LibraryFacts`].
library_facts: std::cell::Cell<Option<LibraryFacts>>,
library_facts: RefCell<Option<LibraryFacts>>,
/// How many cells the viewport shows at once, as the grid last reported.
///
/// Kept beside `window` rather than divided back out of it, because
@@ -347,7 +348,7 @@ impl LibraryController {
resume_at: std::cell::Cell::new(0),
window: RefCell::new((INITIAL_VIEWPORT_CELLS * SCREENFULS).max(MIN_WINDOW)),
viewport_cells: std::cell::Cell::new(INITIAL_VIEWPORT_CELLS),
library_facts: std::cell::Cell::new(None),
library_facts: RefCell::new(None),
requested: RefCell::new(Default::default()),
scan_timer: RefCell::new(None),
thumb_timer: RefCell::new(None),
@@ -2034,7 +2035,7 @@ fn load_window(window: &AppWindow, ctl: &Rc<LibraryController>) {
// looking at, which on a remote library is the cost FR-NC-3 exists to
// avoid.
let scope = *ctl.scope.borrow();
let filter = *ctl.filter.borrow();
let filter = ctl.filter.borrow().clone();
// The trash lists what every other view excludes, so it takes its own
// query rather than another predicate threaded through the scoped one.
let trash = ctl.viewing_trash.get();
@@ -2048,12 +2049,12 @@ fn load_window(window: &AppWindow, ctl: &Rc<LibraryController>) {
// What the whole-library readouts below describe. See [`LibraryFacts`].
let facts = LibraryFacts {
scope,
filter,
filter: filter.clone(),
trash,
total,
};
let describes_something_new = ctl.library_facts.get() != Some(facts);
ctl.library_facts.set(Some(facts));
let describes_something_new = ctl.library_facts.borrow().as_ref() != Some(&facts);
*ctl.library_facts.borrow_mut() = Some(facts);
// TRACES: FR-NC-6a
// Whether the newly scoped collection is already pinned. Read here rather
@@ -3790,7 +3791,7 @@ fn refresh_timeline(window: &AppWindow, catalog: &Catalog, ctl: &Rc<LibraryContr
// over the whole library's span said almost nothing: every bar for a
// fortnight in Arosa landed in one column of a fifteen-year axis.
let scope = *ctl.scope.borrow();
let filter = *ctl.filter.borrow();
let filter = ctl.filter.borrow().clone();
// Passed whole, date range included, and both queries below lift that
// range themselves — see `span_scoped` and `timeline_uniform`. Every other
// term still applies, because a histogram of the five-star frames is a
@@ -5049,31 +5050,23 @@ pub fn wire<F>(
{
let weak = window.as_weak();
let ctl = ctl.clone();
window.on_identity_show_photos(move |id| {
window.on_identity_show_photos(move |id, add| {
let Some(w) = weak.upgrade() else { return };
let person = dr_catalog::faces::PersonId(id.max(0) as u64);
// The chip needs a name, and an unnamed group has none — so it
// borrows the rail's own wording rather than inventing a second
// way to describe the same thing.
let label = {
let borrow = ctl.catalog.borrow();
borrow
.as_ref()
.and_then(|cat| dr_catalog::faces::people(cat.connection()).ok())
.and_then(|people| people.into_iter().find(|p| p.id == person))
.map(|p| {
if p.name.trim().is_empty() {
format!("Unnamed ({} faces)", p.confirmed_faces + p.suggested_faces)
} else {
p.name
}
})
.unwrap_or_else(|| "Person".to_string())
};
ctl.filter.borrow_mut().person = Some(person.0);
w.set_library_filter_person_name(label.into());
{
let mut f = ctl.filter.borrow_mut();
if !add {
f.people.clear();
}
// Adding the same person twice is a no-op rather than a
// duplicate chip — and, under `All`, rather than a filter that
// silently asks for them twice and matches the same pictures.
if !f.people.contains(&person.0) {
f.people.push(person.0);
}
}
push_people_chips(&w, &ctl);
// Leaving the Identity screen for the grid is the whole point of
// the button: the answer to "who is this" is a set of photographs,
@@ -5087,10 +5080,30 @@ pub fn wire<F>(
{
let weak = window.as_weak();
let ctl = ctl.clone();
window.on_library_filter_person_cleared(move || {
window.on_library_filter_person_cleared(move |id| {
let Some(w) = weak.upgrade() else { return };
ctl.filter.borrow_mut().person = None;
w.set_library_filter_person_name(Default::default());
ctl.filter
.borrow_mut()
.people
.retain(|p| *p != id.max(0) as u64);
push_people_chips(&w, &ctl);
refilter(&w, &ctl);
});
}
{
let weak = window.as_weak();
let ctl = ctl.clone();
window.on_library_filter_people_mode_toggled(move || {
let Some(w) = weak.upgrade() else { return };
{
let mut f = ctl.filter.borrow_mut();
f.people_mode = match f.people_mode {
library::PeopleMode::Any => library::PeopleMode::All,
library::PeopleMode::All => library::PeopleMode::Any,
};
}
push_people_chips(&w, &ctl);
refilter(&w, &ctl);
});
}
@@ -5424,6 +5437,61 @@ pub fn wire<F>(
/// The offset is reset because it is an ordinal into the filtered set: keeping
/// it would land the user in the middle of a narrowed library with no sense of
/// how they got there, or past its end entirely.
/// Put the identity filter's people on the filter bar.
///
/// One chip each, so a selection of three is three things to look at and three
/// things to remove — a single chip saying "3 people" would be a filter the
/// user can only clear wholesale.
///
/// The label is the People screen's own wording, unnamed groups included, so
/// the same group does not read one way on one screen and another way on the
/// next.
fn push_people_chips(window: &AppWindow, ctl: &Rc<LibraryController>) {
let ids = ctl.filter.borrow().people.clone();
if ids.is_empty() {
window
.set_library_filter_people(slint::ModelRc::new(slint::VecModel::from(
Vec::<PersonChip>::new(),
)));
return;
}
let borrow = ctl.catalog.borrow();
let people = borrow
.as_ref()
.and_then(|cat| dr_catalog::faces::people(cat.connection()).ok())
.unwrap_or_default();
let chips: Vec<PersonChip> = ids
.iter()
.map(|id| {
let name = people
.iter()
.find(|p| p.id.0 == *id)
.map(|p| {
if p.name.trim().is_empty() {
format!("Unnamed ({} faces)", p.confirmed_faces + p.suggested_faces)
} else {
p.name.clone()
}
})
// A person deleted from under the filter still needs a chip,
// or there would be no way to take them back out of it.
.unwrap_or_else(|| "Person".to_string());
PersonChip {
id: *id as i32,
name: name.into(),
}
})
.collect();
window.set_library_filter_people(slint::ModelRc::new(slint::VecModel::from(chips)));
window.set_library_filter_people_all(matches!(
ctl.filter.borrow().people_mode,
library::PeopleMode::All
));
}
fn refilter(window: &AppWindow, ctl: &Rc<LibraryController>) {
*ctl.offset.borrow_mut() = 0;
ctl.requested.borrow_mut().clear();