Let a manual collection be put in the order the photographer wants
`collections::set_order` and `Sort::CollectionPosition` have been in the catalog since collections were, and nothing above dr-catalog has ever called either. A manual order existed, could not be seen, and could not be set. This is the half that was missing. Three pieces, because it needed all three to be visible at all. The catalog gains `orders_manually` and `members_in_order`. The first is the rule about *when* a manual order means anything, kept in one place with one name: a collection must be manual, and it must have no children. A set shows its descendants' images, and positions are only ever assigned within one collection — so two children's positions are unrelated integers, and ordering by them would sort the grid by a coincidence. The existing comment on `read_cells_scoped` already argued this; now something enforces it. `members_in_order` returns the *whole* membership rather than the filtered view, because `set_order` renumbers exactly what it is handed. Reordering a filtered list would renumber those and leave every hidden image on a stale position — two images sharing one, and a grid that rearranges itself the moment the filter comes off. The grid reads position where the scope qualifies and capture time everywhere else. Manual order joins the member row rather than testing membership with `IN`, which is safe from fanning out rows *because* that branch is a single collection. The gesture is a DropArea over the viewport, drawn only where a reorder means something, with a caret in the gap the photographs would go into — a line between two images rather than a highlight on one, because lighting up a cell would say the drop replaces it. The trap worth naming: `DropEvent.position` is in **window** coordinates. Slint maps it through `map_to_window` when the drag begins and hands every target the same event untranslated, so a target inside a Flickable has to subtract its own `absolute-position`. Getting that wrong is invisible until the grid is scrolled, because at the top the two frames coincide. `reordered` is pure and names its destination by the image it goes before rather than by an index, because the grid can only name a gap in what it is showing and the ids are what survive a window swap. A drop that changes nothing returns the order untouched: that counter is what a cross-device merge resolves by, and spending a revision on a no-op makes this device win an argument it did not have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -482,6 +482,54 @@ pub fn apply_press(
|
||||
*anchor = Some(here);
|
||||
}
|
||||
|
||||
/// TRACES: FR-CAT-7
|
||||
/// Move `moved` so that it lands immediately before `before`.
|
||||
///
|
||||
/// `order` is the collection's **whole** membership, not the filtered view the
|
||||
/// grid is showing. [`dr_catalog::collections::set_order`] renumbers exactly
|
||||
/// what it is handed, so reordering a filtered list would renumber those and
|
||||
/// leave every hidden image on a stale position — two images sharing one, and
|
||||
/// a grid that rearranges itself the moment the filter comes off.
|
||||
///
|
||||
/// The destination is named by the image it goes *before* rather than by an
|
||||
/// index, because the two live in different spaces: the grid can only name a
|
||||
/// gap in what it is showing, and the caller resolves that to an id. `None`
|
||||
/// means the end.
|
||||
///
|
||||
/// The run keeps its own relative order. Dragging five frames somewhere else
|
||||
/// should not also shuffle them among themselves.
|
||||
///
|
||||
/// Dropping onto the run itself is no change. `before` is then one of the
|
||||
/// images being moved, and "put these before one of themselves" has no
|
||||
/// destination that is not where they already are — which is also the right
|
||||
/// answer for the commonest case of it, an image dropped back on its own gap.
|
||||
pub fn reordered(
|
||||
order: &[ImageId],
|
||||
moved: &BTreeSet<ImageId>,
|
||||
before: Option<ImageId>,
|
||||
) -> Vec<ImageId> {
|
||||
if let Some(b) = before {
|
||||
if moved.contains(&b) {
|
||||
return order.to_vec();
|
||||
}
|
||||
}
|
||||
|
||||
let (run, rest): (Vec<ImageId>, Vec<ImageId>) = order.iter().partition(|id| moved.contains(id));
|
||||
|
||||
// `before` not being in `rest` means it is not a member at all — a stale
|
||||
// id from a window read that raced a delete. Appending is the recoverable
|
||||
// answer; refusing would lose the drag for a reason the user cannot see.
|
||||
let at = before
|
||||
.and_then(|b| rest.iter().position(|x| *x == b))
|
||||
.unwrap_or(rest.len());
|
||||
|
||||
let mut out = Vec::with_capacity(order.len());
|
||||
out.extend_from_slice(&rest[..at]);
|
||||
out.extend_from_slice(&run);
|
||||
out.extend_from_slice(&rest[at..]);
|
||||
out
|
||||
}
|
||||
|
||||
/// TRACES: FR-UI-2 | FR-UI-4
|
||||
/// Apply a press, and remember where the anchor was before it moved.
|
||||
///
|
||||
@@ -1625,6 +1673,78 @@ pub fn wire<S, R, P, C>(
|
||||
});
|
||||
}
|
||||
|
||||
// TRACES: FR-CAT-7
|
||||
// A drag that lands inside the grid, rather than on a collection.
|
||||
//
|
||||
// `set_order` and `Sort::CollectionPosition` have been in the catalog since
|
||||
// collections were, and nothing above it ever called them: a manual order
|
||||
// existed and could not be seen or set. This is the half that was missing.
|
||||
//
|
||||
// Only for a collection that has an order of its own — the grid does not
|
||||
// draw the caret otherwise, and this refuses again rather than trusting it
|
||||
// to. A set is a union of its descendants and a saved filter is a query;
|
||||
// neither has a position to write.
|
||||
{
|
||||
let weak = window.as_weak();
|
||||
let ctl = ctl.clone();
|
||||
let catalog = catalog.clone();
|
||||
let reload = on_scope_changed.clone();
|
||||
window.on_library_reorder_to(move |at| {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
let Some(scope) = *ctl.scope.borrow() else {
|
||||
return;
|
||||
};
|
||||
let moved = ctl.selection.borrow().clone();
|
||||
if moved.is_empty() {
|
||||
return;
|
||||
}
|
||||
|
||||
// **Before the catalog is borrowed.** `span` reaches back into the
|
||||
// grid's controller, which borrows the same `RefCell` — asking it
|
||||
// anything while holding a borrow here is a panic, not a deadlock,
|
||||
// and it would only fire on the drop path.
|
||||
let at = at.max(0) as usize;
|
||||
let total = w.get_library_total().max(0) as usize;
|
||||
let before = if at >= total {
|
||||
None
|
||||
} else {
|
||||
ctl.span(at, at).first().copied()
|
||||
};
|
||||
|
||||
let borrow = catalog.borrow();
|
||||
let Some(cat) = borrow.as_ref() else { return };
|
||||
|
||||
if !coll::orders_manually(cat.connection(), scope).unwrap_or(false) {
|
||||
return;
|
||||
}
|
||||
|
||||
let Ok(order) = coll::members_in_order(cat.connection(), scope) else {
|
||||
return;
|
||||
};
|
||||
let next = reordered(&order, &moved, before);
|
||||
|
||||
// A drop that changes nothing — onto the run itself, most often —
|
||||
// must not bump the collection's revision. That counter is what a
|
||||
// cross-device merge resolves by, and spending one on a no-op
|
||||
// makes this device win an argument it did not have.
|
||||
if next == order {
|
||||
return;
|
||||
}
|
||||
|
||||
match coll::set_order(cat.connection(), scope, &next) {
|
||||
Ok(()) => {
|
||||
w.set_collection_error(slint::SharedString::new());
|
||||
drop(borrow);
|
||||
reload();
|
||||
}
|
||||
Err(e) => {
|
||||
drop(borrow);
|
||||
w.set_collection_error(format!("reordering: {e}").into());
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
// TRACES: FR-CAT-5
|
||||
// A collection holding exactly what is selected.
|
||||
//
|
||||
@@ -3418,6 +3538,101 @@ mod tests {
|
||||
assert_eq!(name_of(&cat, id), "Iceland", "the old name stands");
|
||||
}
|
||||
|
||||
fn list(v: &[u64]) -> Vec<ImageId> {
|
||||
v.iter().map(|i| ImageId(*i)).collect()
|
||||
}
|
||||
|
||||
fn moving(v: &[u64]) -> BTreeSet<ImageId> {
|
||||
v.iter().map(|i| ImageId(*i)).collect()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_frame_moves_to_the_gap_it_was_dropped_in() {
|
||||
let order = list(&[1, 2, 3, 4, 5]);
|
||||
assert_eq!(
|
||||
reordered(&order, &moving(&[5]), Some(ImageId(2))),
|
||||
list(&[1, 5, 2, 3, 4]),
|
||||
"dropped before 2, it lands before 2"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_frame_dropped_past_the_end_goes_last() {
|
||||
let order = list(&[1, 2, 3]);
|
||||
assert_eq!(
|
||||
reordered(&order, &moving(&[1]), None),
|
||||
list(&[2, 3, 1]),
|
||||
"no image to land before means the end"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_run_keeps_its_own_order_when_it_moves() {
|
||||
// Dragging five frames somewhere else must not also shuffle them
|
||||
// among themselves.
|
||||
let order = list(&[1, 2, 3, 4, 5, 6]);
|
||||
assert_eq!(
|
||||
reordered(&order, &moving(&[2, 4, 6]), Some(ImageId(1))),
|
||||
list(&[2, 4, 6, 1, 3, 5])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_run_gathers_even_where_it_was_scattered() {
|
||||
// The selection need not be contiguous. What lands is one run, in the
|
||||
// order the collection already had them.
|
||||
let order = list(&[1, 2, 3, 4, 5, 6]);
|
||||
assert_eq!(
|
||||
reordered(&order, &moving(&[1, 5]), Some(ImageId(3))),
|
||||
list(&[2, 1, 5, 3, 4, 6])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn dropping_a_frame_on_its_own_gap_changes_nothing() {
|
||||
// The commonest miss: picked up and put back. It must not rewrite the
|
||||
// order, because the caller spends a merge revision on any change.
|
||||
let order = list(&[1, 2, 3, 4]);
|
||||
assert_eq!(reordered(&order, &moving(&[3]), Some(ImageId(3))), order);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn dropping_a_run_inside_itself_changes_nothing() {
|
||||
let order = list(&[1, 2, 3, 4, 5]);
|
||||
assert_eq!(
|
||||
reordered(&order, &moving(&[2, 3, 4]), Some(ImageId(3))),
|
||||
order,
|
||||
"there is no destination that is not where they already are"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_reorder_never_gains_or_loses_a_frame() {
|
||||
// The result is written straight to `set_order`, which renumbers what
|
||||
// it is handed — so a permutation that dropped one would silently
|
||||
// strand it on a stale position.
|
||||
let order = list(&[1, 2, 3, 4, 5, 6, 7]);
|
||||
for before in [None, Some(ImageId(1)), Some(ImageId(4)), Some(ImageId(7))] {
|
||||
let out = reordered(&order, &moving(&[2, 6]), before);
|
||||
let mut sorted = out.clone();
|
||||
sorted.sort();
|
||||
assert_eq!(sorted, list(&[1, 2, 3, 4, 5, 6, 7]), "before = {before:?}");
|
||||
assert_eq!(out.len(), order.len(), "before = {before:?}");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unknown_destination_appends_rather_than_losing_the_drag() {
|
||||
// A stale id from a window read that raced a delete. Appending is
|
||||
// recoverable; refusing would lose the drag for a reason the user
|
||||
// cannot see.
|
||||
let order = list(&[1, 2, 3]);
|
||||
assert_eq!(
|
||||
reordered(&order, &moving(&[1]), Some(ImageId(99))),
|
||||
list(&[2, 3, 1])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn renaming_to_the_current_name_writes_nothing() {
|
||||
// Every write bumps the revision, and a no-op rename would let an idle
|
||||
|
||||
+150
-16
@@ -3794,9 +3794,20 @@ 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.
|
||||
/// TRACES: FR-CAT-7
|
||||
/// # Ordering
|
||||
///
|
||||
/// A manual collection with no children is shown in **manual order** — the
|
||||
/// order [`dr_catalog::collections::set_order`] wrote, which is what makes a
|
||||
/// drag-to-reorder visible at all. Everything else falls back to the unscoped
|
||||
/// grid's capture time.
|
||||
///
|
||||
/// The fallback is not conservatism. A set shows its descendants' images, and
|
||||
/// positions are only ever assigned within one collection, so two children's
|
||||
/// positions are unrelated integers — ordering by them would sort the grid by
|
||||
/// a coincidence. A saved filter has no member rows to carry a position at
|
||||
/// all. [`dr_catalog::collections::orders_manually`] holds both halves of that
|
||||
/// rule; this function only asks.
|
||||
pub fn read_cells_scoped(
|
||||
catalog: &Catalog,
|
||||
scope: Option<dr_types::CollectionId>,
|
||||
@@ -3814,20 +3825,46 @@ pub fn read_cells_scoped(
|
||||
.collect::<Vec<_>>()
|
||||
.join(",");
|
||||
let rated = filter.sql();
|
||||
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}
|
||||
LIMIT ? OFFSET ?"
|
||||
);
|
||||
|
||||
let mut params: Vec<rusqlite::types::Value> = ids
|
||||
.iter()
|
||||
.map(|c| rusqlite::types::Value::Integer(c.0 as i64))
|
||||
.collect();
|
||||
// Manual order needs the member row in hand to sort by, so it joins where
|
||||
// the capture-time path only tests membership with `IN`. The join is safe
|
||||
// from duplicating rows *because* this branch is a single collection: one
|
||||
// image has at most one member row per collection, so there is nothing to
|
||||
// fan out. That is not true of the set branch, which is the second reason
|
||||
// it keeps the subquery.
|
||||
let manual = dr_catalog::collections::orders_manually(catalog.connection(), scope)?;
|
||||
let sql = if manual {
|
||||
format!(
|
||||
"SELECT {CELL_COLUMNS}
|
||||
FROM images i
|
||||
JOIN collection_members cm ON cm.image_id = i.id
|
||||
WHERE {VISIBLE}{rated}
|
||||
AND cm.collection_id = ?
|
||||
ORDER BY cm.position, cm.image_id
|
||||
LIMIT ? OFFSET ?"
|
||||
)
|
||||
} else {
|
||||
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}
|
||||
LIMIT ? OFFSET ?"
|
||||
)
|
||||
};
|
||||
|
||||
// One id for the join, the whole descendant list for the subquery — the
|
||||
// two branches bind different things, so the parameters are built to match
|
||||
// the SQL that was chosen rather than assumed.
|
||||
let mut params: Vec<rusqlite::types::Value> = if manual {
|
||||
vec![rusqlite::types::Value::Integer(scope.0 as i64)]
|
||||
} else {
|
||||
ids.iter()
|
||||
.map(|c| rusqlite::types::Value::Integer(c.0 as i64))
|
||||
.collect()
|
||||
};
|
||||
params.push(rusqlite::types::Value::Integer(limit as i64));
|
||||
params.push(rusqlite::types::Value::Integer(offset as i64));
|
||||
|
||||
@@ -5043,6 +5080,103 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_manual_collection_is_shown_in_manual_order() {
|
||||
// The whole point of `set_order`: a drag that reorders has to be
|
||||
// visible the next time the grid reads, or it did nothing.
|
||||
use dr_catalog::collections::{self as coll, CollectionKind};
|
||||
|
||||
let catalog = with_images(10);
|
||||
let ids = image_ids(&catalog);
|
||||
let c = coll::create(catalog.connection(), "Trip", None, CollectionKind::Manual).unwrap();
|
||||
coll::add_images(catalog.connection(), c, &ids[0..4]).unwrap();
|
||||
|
||||
let wanted = vec![ids[2], ids[0], ids[3], ids[1]];
|
||||
coll::set_order(catalog.connection(), c, &wanted).unwrap();
|
||||
|
||||
let cells = read_cells_scoped(&catalog, Some(c), &RatingFilter::default(), 0, 120).unwrap();
|
||||
assert_eq!(
|
||||
cells
|
||||
.iter()
|
||||
.map(|c| dr_types::ImageId(c.image_id as u64))
|
||||
.collect::<Vec<_>>(),
|
||||
wanted,
|
||||
"the grid reads the order that was written, not capture time"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn manual_order_survives_paging() {
|
||||
// The window is applied by SQL after the ORDER BY, so a second page
|
||||
// must continue the order rather than restart it.
|
||||
use dr_catalog::collections::{self as coll, CollectionKind};
|
||||
|
||||
let catalog = with_images(10);
|
||||
let ids = image_ids(&catalog);
|
||||
let c = coll::create(catalog.connection(), "Trip", None, CollectionKind::Manual).unwrap();
|
||||
coll::add_images(catalog.connection(), c, &ids[0..6]).unwrap();
|
||||
|
||||
let wanted: Vec<_> = ids[0..6].iter().rev().copied().collect();
|
||||
coll::set_order(catalog.connection(), c, &wanted).unwrap();
|
||||
|
||||
let first = read_cells_scoped(&catalog, Some(c), &RatingFilter::default(), 0, 3).unwrap();
|
||||
let second = read_cells_scoped(&catalog, Some(c), &RatingFilter::default(), 3, 3).unwrap();
|
||||
let read: Vec<_> = first
|
||||
.iter()
|
||||
.chain(second.iter())
|
||||
.map(|c| dr_types::ImageId(c.image_id as u64))
|
||||
.collect();
|
||||
|
||||
assert_eq!(read, wanted, "the two pages join up in manual order");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_collection_set_keeps_capture_order() {
|
||||
// A set shows its descendants' images, whose positions were assigned
|
||||
// in different collections and mean nothing against each other. It
|
||||
// must not sort by them — see `read_cells_scoped`.
|
||||
use dr_catalog::collections::{self as coll, CollectionKind};
|
||||
|
||||
let catalog = with_images(10);
|
||||
let ids = image_ids(&catalog);
|
||||
let trips =
|
||||
coll::create(catalog.connection(), "Trips", None, CollectionKind::Manual).unwrap();
|
||||
let iceland = coll::create(
|
||||
catalog.connection(),
|
||||
"Iceland",
|
||||
Some(trips),
|
||||
CollectionKind::Manual,
|
||||
)
|
||||
.unwrap();
|
||||
coll::add_images(catalog.connection(), iceland, &ids[0..4]).unwrap();
|
||||
// Reverse the child's own order. Scoped to the child this shows;
|
||||
// scoped to the parent it must not.
|
||||
let reversed: Vec<_> = ids[0..4].iter().rev().copied().collect();
|
||||
coll::set_order(catalog.connection(), iceland, &reversed).unwrap();
|
||||
|
||||
let child =
|
||||
read_cells_scoped(&catalog, Some(iceland), &RatingFilter::default(), 0, 120).unwrap();
|
||||
assert_eq!(
|
||||
child
|
||||
.iter()
|
||||
.map(|c| dr_types::ImageId(c.image_id as u64))
|
||||
.collect::<Vec<_>>(),
|
||||
reversed,
|
||||
"the leaf orders manually"
|
||||
);
|
||||
|
||||
let parent =
|
||||
read_cells_scoped(&catalog, Some(trips), &RatingFilter::default(), 0, 120).unwrap();
|
||||
assert_eq!(
|
||||
parent
|
||||
.iter()
|
||||
.map(|c| dr_types::ImageId(c.image_id as u64))
|
||||
.collect::<Vec<_>>(),
|
||||
ids[0..4].to_vec(),
|
||||
"the set falls back to capture time"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_collection_set_shows_its_childrens_images() {
|
||||
// A parent whose children hold everything must not read as empty —
|
||||
|
||||
@@ -2114,6 +2114,23 @@ fn load_window(window: &AppWindow, ctl: &Rc<LibraryController>) {
|
||||
refresh_timeline(window, catalog, ctl);
|
||||
}
|
||||
|
||||
// TRACES: FR-CAT-7
|
||||
// Whether this scope has an order a drag can change, which is what lets the
|
||||
// grid draw an insertion caret and accept a drop.
|
||||
//
|
||||
// Answered here because this is the one place that runs on every re-read —
|
||||
// a scope change, a reorder, a collection gaining a child that turns it
|
||||
// into a set — so the caret cannot outlive the scope that justified it.
|
||||
//
|
||||
// The trash is never reorderable. It is a view of what was deleted, ordered
|
||||
// by when, and it is not a collection at all.
|
||||
window.set_library_manual_order(
|
||||
!trash
|
||||
&& scope.is_some_and(|c| {
|
||||
dr_catalog::collections::orders_manually(catalog.connection(), c).unwrap_or(false)
|
||||
}),
|
||||
);
|
||||
|
||||
let cells = if trash {
|
||||
library::read_trashed_cells(catalog, offset, window_size)
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user