dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Let a manual collection be put in the order it is meant to be seen in

Browse Source 
`collection_members.position` and `Sort::CollectionPosition` have been in the
catalog since collections were, and nothing above dr-catalog has ever written
or read either: `collections::set_order` had no callers, and the grid ordered
everything by capture time whatever it was scoped to — dr-ui does not construct
a `Query` at all, it has its own `GRID_ORDER` constant. So a manual collection
was a set with an order nobody could see or change.

Three pieces, because it could not be fewer:

`grid_order_for` decides the ordering from the scope, and both readers take it
from there. That is the load-bearing part. An ordinal only names a photograph
relative to an ordering, so the window read and the span read have to agree —
a shift-click resolved through a different ORDER BY than the cells were drawn
with selects a different run than the one on screen, and the user finds out
when the export runs. `read_ids_span` already stated that invariant about
`GRID_ORDER`; this widens it to an ordering that depends on the scope.

Only a single manual collection has one. A set draws its descendants' images
too, and two children's positions are unrelated integers that interleave
arbitrarily; a smart collection has no member rows to carry a position at all.
Both fall back to capture time and refuse the drop rather than pretending.

The drop is on the cell, on whichever half of it the finger landed — the
trailing edge is the only way to name the last place in a collection, since
there is no cell beyond the last one to drop in front of.

`reordered` is pure and the membership is rewritten whole. `set_order` sets the
positions it is given and leaves the rest, so a partial write would interleave
the moved run with rows nobody touched; and it is read unfiltered, so what the
filter is hiding keeps its place relative to what the user can see.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-29 20:04:17 +02:00
co-authored by Claude Opus 5
parent 6d25d85f18
commit 574107bc39
6 changed files with 574 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/library.rs
+330 -6
View File
@@ -3794,9 +3794,10 @@ pub fn read_cells(
/// look broken. The id list comes from
/// [`dr_catalog::collections::descendants`], which is depth-guarded.
///
/// Ordering matches the unscoped grid (capture time, then name) rather than
/// manual position: position is only meaningful inside one collection and this
/// query also serves sets, where two children's positions are unrelated.
/// Ordering comes from [`grid_order_for`]: manual position for a single manual
/// collection, capture time for a set — because position is only meaningful
/// inside one collection, and this query also serves sets, where two children's
/// positions are unrelated integers.
pub fn read_cells_scoped(
catalog: &Catalog,
scope: Option<dr_types::CollectionId>,
@@ -3814,20 +3815,24 @@ pub fn read_cells_scoped(
.collect::<Vec<_>>()
.join(",");
let rated = filter.sql();
let (order, order_params) = grid_order_for(catalog, Some(scope));
let sql = format!(
"SELECT {CELL_COLUMNS}
FROM images i
WHERE {VISIBLE}{rated}
AND i.id IN (SELECT image_id FROM collection_members
WHERE collection_id IN ({placeholders}))
{GRID_ORDER}
{order}
LIMIT ? OFFSET ?"
);
// Bound in the order the `?`s appear: the scope's ids in the WHERE, then
// whatever the ORDER BY needs, then the window.
let mut params: Vec<rusqlite::types::Value> = ids
.iter()
.map(|c| rusqlite::types::Value::Integer(c.0 as i64))
.collect();
params.extend(order_params);
params.push(rusqlite::types::Value::Integer(limit as i64));
params.push(rusqlite::types::Value::Integer(offset as i64));
@@ -3952,13 +3957,18 @@ pub fn read_ids_span(
Vec::new(),
)
} else {
let (clause, params) = scope_clause(catalog, scope)?;
let (clause, mut params) = scope_clause(catalog, scope)?;
let rated = filter.sql();
// The same ordering the cells were drawn with, from the same place.
// A range is a pair of ordinals, and an ordinal read through a
// different ORDER BY names a different photograph.
let (order, order_params) = grid_order_for(catalog, scope);
params.extend(order_params);
(
format!(
"SELECT i.id FROM images i
WHERE {VISIBLE}{rated}{clause}
{GRID_ORDER}
{order}
LIMIT ? OFFSET ?"
),
params,
@@ -4105,6 +4115,146 @@ pub fn total_images_scoped(
Ok(n as usize)
}
/// TRACES: FR-CAT-7
/// Every member of `scope`, in the order its positions put them.
///
/// The *whole* membership, not the window and not the filtered view. A reorder
/// rewrites positions, and [`dr_catalog::collections::set_order`] only touches
/// the rows it is given — so writing back a filtered subset would leave the
/// images the filter is hiding at their old positions, interleaved with the new
/// ones arbitrarily. The user reorders what they can see; the rows they cannot
/// keep their place relative to it.
pub fn read_member_order(
catalog: &Catalog,
scope: dr_types::CollectionId,
) -> Result<Vec<dr_types::ImageId>, dr_catalog::CatalogError> {
let mut stmt = catalog.connection().prepare(
"SELECT image_id FROM collection_members
WHERE collection_id = ?1
ORDER BY position ASC, image_id ASC",
)?;
let ids = stmt
.query_map([scope.0 as i64], |r| {
Ok(dr_types::ImageId(r.get::<_, i64>(0)? as u64))
})?
.collect::<Result<Vec<_>, _>>()?;
Ok(ids)
}
/// TRACES: FR-CAT-7
/// `current` with `moving` lifted out and set down beside `target`.
///
/// `target` names a *photograph*, not an index, and that is the point: the grid
/// may be filtered, so the cell the user dropped on sits at one position in
/// what they can see and another in the membership being rewritten. An id
/// survives both. `after` puts the run on the far side of it, which is the only
/// way to name the last place in a collection — there is no cell beyond the
/// last one to drop in front of.
///
/// The run keeps the order `current` has it in rather than the order the
/// selection was built in: the user is looking at the grid, and a selection
/// gathered by tapping the last frame first should not reverse itself on being
/// moved.
///
/// A `target` that is itself being moved leaves the run at the end. There is no
/// gap between a run and itself to land in, so the caller refuses that drop
/// before it gets here; this is what the function does rather than panicking if
/// one ever arrives.
///
/// Pure, so the awkward half of a drag can be tested without a window.
pub fn reordered(
current: &[dr_types::ImageId],
moving: &[dr_types::ImageId],
target: dr_types::ImageId,
after: bool,
) -> Vec<dr_types::ImageId> {
let lifting: std::collections::BTreeSet<_> = moving.iter().copied().collect();
let rest: Vec<_> = current
.iter()
.copied()
.filter(|id| !lifting.contains(id))
.collect();
let run: Vec<_> = current
.iter()
.copied()
.filter(|id| lifting.contains(id))
.collect();
// Resolved against `rest`, not against `current`: the run has already been
// lifted, so an index into the original list would be off by however many
// of it sat ahead of the target.
let at = match rest.iter().position(|id| *id == target) {
Some(at) if after => at + 1,
Some(at) => at,
None => rest.len(),
};
let mut out = Vec::with_capacity(current.len());
out.extend_from_slice(&rest[..at]);
out.extend(run);
out.extend_from_slice(&rest[at..]);
out
}
/// TRACES: FR-CAT-7
/// The ORDER BY the grid reads `scope` with, and the parameters it binds.
///
/// Manual position where the grid is scoped to a single manual collection with
/// no children; [`GRID_ORDER`] — capture time, then filename — everywhere else.
///
/// **Why the narrowing.** `position` is a column of `collection_members`, so it
/// only exists relative to one collection. A collection *set* shows its
/// descendants' images too, and two children's positions are unrelated integers
/// that would interleave arbitrarily; a smart collection has no member rows to
/// carry a position at all. Outside those cases there is no manual order to
/// read, and falling back is the only honest answer.
///
/// **Why every reader must agree.** An ordinal only names a photograph relative
/// to an ordering. The window read and the span read are two halves of one
/// grid: a shift-click resolved through a different ORDER BY than the cells
/// were drawn with selects a different run than the one on screen, and the user
/// finds out when the export runs. That is the same invariant
/// [`read_ids_span`] already states about `GRID_ORDER`, widened to cover the
/// case where the ordering depends on the scope.
///
/// A correlated subquery rather than a join, so the FROM and WHERE the two
/// readers already share are untouched: position is looked up per row through
/// `collection_members`' primary key, which is `(collection_id, image_id)`.
fn grid_order_for(
catalog: &Catalog,
scope: Option<dr_types::CollectionId>,
) -> (String, Vec<rusqlite::types::Value>) {
let Some(id) = scope else {
return (GRID_ORDER.to_string(), Vec::new());
};
// A set orders by capture time. `descendants` includes the collection
// itself, so one entry means it has no children.
let alone = dr_catalog::collections::descendants(catalog.connection(), id)
.map(|d| d.len() == 1)
.unwrap_or(false);
let manual = matches!(
dr_catalog::collections::kind(catalog.connection(), id),
Ok(Some(dr_catalog::collections::CollectionKind::Manual))
);
if !alone || !manual {
return (GRID_ORDER.to_string(), Vec::new());
}
// `i.id` breaks the tie. Positions are dense after a `set_order`, but a
// collection that has never been reordered by hand has whatever
// `add_images` assigned, and two rows can share a position if a merge from
// another device brought one in — an ordering that is not total is an
// ordering the window read and the span read can disagree about.
(
"ORDER BY (SELECT cm.position FROM collection_members cm
WHERE cm.collection_id = ? AND cm.image_id = i.id) ASC,
i.id ASC"
.to_string(),
vec![rusqlite::types::Value::Integer(id.0 as i64)],
)
}
/// The SQL restricting a query to `scope` and its descendants, with the bound
/// parameters to go with it.
///
@@ -5333,6 +5483,180 @@ mod tests {
assert_eq!(read_trashed_cells(&catalog, 0, 50).unwrap().len(), 3);
}
// --- manual order within a collection (FR-CAT-7) ------------------------
fn ids(n: &[u64]) -> Vec<dr_types::ImageId> {
n.iter().copied().map(dr_types::ImageId).collect()
}
#[test]
fn a_run_moved_forward_lands_before_the_photograph_it_was_dropped_on() {
let current = ids(&[1, 2, 3, 4, 5]);
assert_eq!(
reordered(&current, &ids(&[4]), dr_types::ImageId(2), false),
ids(&[1, 4, 2, 3, 5])
);
}
#[test]
fn a_run_moved_backward_lands_before_it_too() {
// The direction of travel must not change what "before this one" means,
// or the same drop would land in two different places depending on
// where the photograph came from.
let current = ids(&[1, 2, 3, 4, 5]);
assert_eq!(
reordered(&current, &ids(&[2]), dr_types::ImageId(5), false),
ids(&[1, 3, 4, 2, 5])
);
}
#[test]
fn the_trailing_half_of_the_last_cell_is_how_the_end_is_reached() {
// There is no cell beyond the last one to drop in front of, so without
// `after` the final position is unreachable — which is exactly the
// place a "put this at the end" drag is aiming for.
let current = ids(&[1, 2, 3]);
assert_eq!(
reordered(&current, &ids(&[1]), dr_types::ImageId(3), true),
ids(&[2, 3, 1])
);
}
#[test]
fn a_moved_run_keeps_the_order_the_grid_shows_it_in() {
// Not the order the selection was built in. A user who tapped the last
// frame first has said nothing about how the run should be arranged —
// only about where it should go.
let current = ids(&[1, 2, 3, 4, 5]);
assert_eq!(
reordered(&current, &ids(&[5, 1]), dr_types::ImageId(3), false),
ids(&[2, 1, 5, 3, 4])
);
}
#[test]
fn a_run_dropped_on_one_of_its_own_members_stays_together() {
// The caller refuses this drop, so it is only reachable if that guard
// is ever lost. It must not lose photographs when it is.
let current = ids(&[1, 2, 3, 4]);
let moved = reordered(&current, &ids(&[2, 3]), dr_types::ImageId(3), false);
assert_eq!(moved.len(), current.len(), "nothing was dropped");
let mut sorted = moved.clone();
sorted.sort();
assert_eq!(sorted, ids(&[1, 2, 3, 4]), "and nothing was invented");
}
#[test]
fn a_reorder_never_loses_or_duplicates_a_member() {
// The property that matters most: this writes the whole membership
// back, so a run that dropped one image would delete it from the
// collection.
let current = ids(&[1, 2, 3, 4, 5, 6]);
for target in [1u64, 2, 3, 4, 5, 6] {
for after in [false, true] {
let moved = reordered(&current, &ids(&[2, 5]), dr_types::ImageId(target), after);
let mut sorted = moved.clone();
sorted.sort();
assert_eq!(
sorted,
ids(&[1, 2, 3, 4, 5, 6]),
"target {target}, after {after}"
);
}
}
}
/// The scoped grid and the range a shift-click resolves are two halves of
/// one ordering. This is the assertion that keeps them one: an ordinal read
/// through a different ORDER BY names a different photograph, and the user
/// finds out when the export runs.
#[test]
fn a_manual_collection_is_read_and_spanned_in_the_order_it_was_given() {
let catalog = with_images(5);
let all = image_ids(&catalog);
let id = dr_catalog::collections::create(
catalog.connection(),
"Trip",
None,
dr_catalog::collections::CollectionKind::Manual,
)
.unwrap();
dr_catalog::collections::add_images(catalog.connection(), id, &all).unwrap();
// Reversed, so position and capture time disagree about everything.
let wanted: Vec<_> = all.iter().rev().copied().collect();
dr_catalog::collections::set_order(catalog.connection(), id, &wanted).unwrap();
let cells = read_cells_scoped(&catalog, Some(id), &RatingFilter::default(), 0, 50).unwrap();
let drawn: Vec<_> = cells
.iter()
.map(|c| dr_types::ImageId(c.image_id as u64))
.collect();
assert_eq!(drawn, wanted, "the grid draws the order that was written");
let spanned =
read_ids_span(&catalog, Some(id), &RatingFilter::default(), false, 0, 4).unwrap();
assert_eq!(spanned, wanted, "and a range resolves through the same one");
assert_eq!(
read_member_order(&catalog, id).unwrap(),
wanted,
"and so does the membership a reorder rewrites"
);
}
#[test]
fn a_collection_with_children_falls_back_to_capture_time() {
// A set draws its descendants' images too, and two children's positions
// are unrelated integers. Ordering by them interleaves the two
// arbitrarily, which is worse than an order that at least means
// something.
let catalog = with_images(4);
let all = image_ids(&catalog);
let parent = dr_catalog::collections::create(
catalog.connection(),
"Iceland",
None,
dr_catalog::collections::CollectionKind::Manual,
)
.unwrap();
dr_catalog::collections::create(
catalog.connection(),
"Day one",
Some(parent),
dr_catalog::collections::CollectionKind::Manual,
)
.unwrap();
dr_catalog::collections::add_images(catalog.connection(), parent, &all).unwrap();
let reversed: Vec<_> = all.iter().rev().copied().collect();
dr_catalog::collections::set_order(catalog.connection(), parent, &reversed).unwrap();
let cells =
read_cells_scoped(&catalog, Some(parent), &RatingFilter::default(), 0, 50).unwrap();
let drawn: Vec<_> = cells
.iter()
.map(|c| dr_types::ImageId(c.image_id as u64))
.collect();
assert_eq!(drawn, all, "capture time, not the positions that were set");
}
#[test]
fn a_smart_collection_has_no_manual_order_to_read() {
// No member rows at all, so `position` is not a column any of its
// images have. Falling back is the only thing there is to do.
let catalog = with_images(3);
let id = dr_catalog::collections::create(
catalog.connection(),
"Picks",
None,
dr_catalog::collections::CollectionKind::Smart,
)
.unwrap();
let (order, params) = grid_order_for(&catalog, Some(id));
assert_eq!(order, GRID_ORDER);
assert!(params.is_empty());
}
#[test]
fn the_most_recently_trashed_image_is_listed_first() {
// A mistaken delete is corrected within seconds, so the row the user