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

Stop the press that starts a drag from deselecting what it grabbed

Browse Source 
Dragging a selection of forty photographs onto a collection filed one.

Selection mode reports every press as a ctrl-press — deliberately, so
touch and pointer go through one set of rules rather than two — and ctrl
toggled. So the press that took hold of one of the forty took it *out*
of the selection on the way down. `drag-started` then looked at the cell
under the finger, found it unselected, and did exactly what it is meant
to do with an unselected cell: made it the whole selection and carried
it alone. The only sign was the grabbed cell's ring blinking out at the
moment the user began to move.

A plain press has never had this problem, because pressing an
already-selected cell has always been documented to leave the selection
alone — for precisely this reason. Ctrl now does the same: adding still
happens on the press, since the drag reads the selection immediately,
but *removing* is handed back as `Press::Deferred` and applied by the
click. Slint reports a click only for a press that stayed within
`tap-slop`, so a tap still toggles and a drag never does.

The unit tests now go through a `click` helper — a press and the release
that follows it — because that is the only thing a user can perform, and
calling `apply_press` alone would assert against half the policy.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-30 10:49:51 +02:00
co-authored by Claude Opus 5
parent 1ff52102b6
commit 1a59fb33c1
3 changed files with 268 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/collections_ui.rs
+243 -50
View File
@@ -114,6 +114,15 @@ pub struct CollectionsController {
/// same argument applied to the one index that has to survive a move.
anchor: RefCell<Option<usize>>,
/// TRACES: FR-UI-2 | FR-UI-4
/// TRACES: FR-CAT-7 | FR-UI-4
/// A photograph the most recent press asked to take *out* of the
/// selection, held until the release says the press was a tap.
///
/// See [`Press::Deferred`] for why removal cannot happen on the press:
/// this is the whole of what stops a drag of forty photographs carrying
/// one. Overwritten by the next press and dropped by the drag that
/// consumes it, so at most one is ever pending.
pending_toggle: std::cell::Cell<Option<ImageId>>,
/// TRACES: FR-UI-4
/// What the selection was immediately before the most recent press, so a
/// gesture that turns out not to have been a press can put it back.
@@ -353,22 +362,57 @@ impl CollectionsController {
}
}
/// TRACES: FR-CAT-7 | FR-UI-4
/// What a press did, and what is left for the release to do.
///
/// Only one press has anything left over, and it is the one every multi-image
/// drag depends on — see [`Press::Deferred`].
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
#[must_use]
pub enum Press {
/// The selection is already what this press means. Nothing to finish.
Applied,
/// **Taking a photograph out of the selection waits for the release.**
///
/// Putting one *in* has to happen on the press: the drag that may follow
/// reads the selection to decide what it carries, and by the time the
/// finger lifts it is over the sidebar. Taking one out is the opposite —
/// nothing between the press and the release needs the image gone, and one
/// thing very much needs it to stay.
///
/// A plain press has said so since selection was written: pressing an
/// already-selected cell leaves the selection alone. Ctrl did not, and on a
/// tablet *every* press is a ctrl-press — that is what selection mode is.
/// So grabbing one of forty selected photographs deselected it on the way
/// down; the drag that followed found the cell under the finger no longer
/// in the selection, took that to mean an unselected image was being
/// dragged, and carried it alone. Forty photographs became one, and the
/// only clue was the grabbed cell's ring blinking out.
///
/// So the removal is handed back for the *click* to apply, and a click
/// fires only for a press that stayed put — never for one that became a
/// drag. See `tap-slop` in `library.slint`.
Deferred(ImageId),
}
/// Apply a press to the selection.
///
/// Split from the callback so the policy is testable without a window: this is
/// the part a user notices being wrong.
///
/// - **plain** — replace the selection with this one image
/// - **ctrl** — toggle this image, keeping the rest, and move the anchor here
/// - **ctrl** — add this image to the selection, keeping the rest, and move the
/// anchor here; taking one *out* is [deferred](Press::Deferred) to the release
/// - **shift** — select the range from the anchor to here, *replacing* what was
/// selected; the anchor stays put, so an overshoot is corrected by
/// shift-clicking the right cell rather than starting again
/// - **ctrl+shift** — the same range, *added* to the selection, for picking up a
/// second run without losing the first
///
/// A plain press on an image that is *already* selected leaves the selection
/// alone. That is what makes dragging a multi-selection possible at all — the
/// press that begins the drag would otherwise collapse the selection to one.
/// A press on an image that is *already* selected never removes it here —
/// plainly or with ctrl. That is what makes dragging a multi-selection possible
/// at all: the press that begins the drag would otherwise take the grabbed
/// photograph out from under it.
///
/// `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
@@ -387,8 +431,10 @@ pub fn apply_press(
ctrl: bool,
shift: bool,
span: &dyn Fn(usize, usize) -> Vec<ImageId>,
) {
let Some(&id) = ids.get(row) else { return };
) -> Press {
let Some(&id) = ids.get(row) else {
return Press::Applied;
};
let here = offset + row;
if shift {
@@ -398,7 +444,7 @@ pub fn apply_press(
selection.clear();
selection.insert(id);
*anchor = Some(here);
return;
return Press::Applied;
};
// The anchor deliberately does **not** move. Shift-clicking again
@@ -444,15 +490,19 @@ pub fn apply_press(
run = ids[lo_row..=hi_row].to_vec();
}
selection.extend(run);
return;
return Press::Applied;
}
if ctrl {
if !selection.remove(&id) {
selection.insert(id);
}
*anchor = Some(here);
return;
// Taking one out waits for the release — see [`Press::Deferred`].
if selection.contains(&id) {
return Press::Deferred(id);
}
selection.insert(id);
return Press::Applied;
}
// Plain press on something already selected: leave it. The drag that may
@@ -460,12 +510,13 @@ pub fn apply_press(
// multi-image drag impossible to start.
if selection.contains(&id) {
*anchor = Some(here);
return;
return Press::Applied;
}
selection.clear();
selection.insert(id);
*anchor = Some(here);
Press::Applied
}
/// Apply a press and push the result into the grid — the whole of what a
@@ -492,7 +543,11 @@ pub fn select_row(
ctl.cursor(),
));
apply_press(
// Whatever the last press left pending is answered by this one: two
// presses without a click in between means the first never was a tap.
ctl.pending_toggle.set(None);
if let Press::Deferred(id) = apply_press(
&mut ctl.selection.borrow_mut(),
&mut ctl.anchor.borrow_mut(),
ids,
@@ -501,7 +556,9 @@ pub fn select_row(
ctrl,
shift,
&|first, last| ctl.span(first, last),
);
) {
ctl.pending_toggle.set(Some(id));
}
// The cursor follows the press, so an arrow key after a click continues
// from the cell that was clicked rather than from wherever the keyboard
// was last.
@@ -509,6 +566,26 @@ pub fn select_row(
sync_selection(window, ctl, ids);
}
/// TRACES: FR-CAT-7 | FR-UI-4
/// Finish a press that turned out to be a tap.
///
/// The other half of [`Press::Deferred`]: a ctrl-press on an already-selected
/// photograph leaves it in, because the drag that may follow has to be able to
/// carry it, and this takes it out again once the release has proved there was
/// no drag.
///
/// Called from the click, which Slint reports only for a press that stayed
/// within `tap-slop` of where it landed — so a press that became a drag never
/// reaches here, and a press taken over by the Flickable never reaches here
/// either. Both leave the pending removal to be dropped by the next press.
pub fn commit_press(window: &AppWindow, ctl: &Rc<CollectionsController>, ids: &[ImageId]) {
let Some(id) = ctl.pending_toggle.take() else {
return;
};
ctl.selection.borrow_mut().remove(&id);
sync_selection(window, ctl, ids);
}
/// TRACES: FR-UI-4
/// Put the selection back as it was before the most recent press.
///
@@ -520,6 +597,11 @@ pub fn select_row(
/// Idempotent, and a no-op when there is nothing to undo, so it is safe to
/// call on every gesture start rather than only on the ones that need it.
pub fn cancel_press(window: &AppWindow, ctl: &Rc<CollectionsController>, ids: &[ImageId]) {
// The press is being unmade, so what it left for the release to finish is
// unmade with it. Cleared before the early return: a press that changed
// nothing to undo can still have deferred a removal.
ctl.pending_toggle.set(None);
let Some(undo) = ctl.press_undo.borrow_mut().take() else {
return;
};
@@ -1850,6 +1932,15 @@ pub fn wire<S, R, P, C>(
// meant. The pinch already cancels for exactly this reason.
*ctl.hold_timer.borrow_mut() = None;
// TRACES: FR-CAT-7
// And this press was not a tap, so it never gets to take anything
// out of the selection — see [`Press::Deferred`]. Dropped here as
// well as on the next press because the drag reads the selection
// one line below, and a removal still pending would be a
// photograph the user can see is selected and the drop would not
// carry.
ctl.pending_toggle.set(None);
// Dragging an *unselected* cell carries only that one, and makes it
// the selection — otherwise the images that travel are not the ones
// the user grabbed. Dragging a selected cell carries the whole
@@ -2608,6 +2699,30 @@ fn unique_name(conn: &rusqlite::Connection, parent: Option<CollectionId>) -> Str
#[cfg(test)]
mod tests {
/// A press and the release that follows it — which is what a click is.
///
/// These tests are about what a *user* sees, and a user only ever presses
/// and lets go. Calling [`apply_press`] alone would drop the half of the
/// policy that waits for the release ([`Press::Deferred`]) and quietly
/// assert the wrong thing about ctrl.
#[allow(clippy::too_many_arguments)]
fn click(
selection: &mut BTreeSet<ImageId>,
anchor: &mut Option<usize>,
ids: &[ImageId],
offset: usize,
row: usize,
ctrl: bool,
shift: bool,
span: &dyn Fn(usize, usize) -> Vec<ImageId>,
) {
if let Press::Deferred(id) =
apply_press(selection, anchor, ids, offset, row, ctrl, shift, span)
{
selection.remove(&id);
}
}
use super::*;
fn ids(n: u64) -> Vec<ImageId> {
@@ -2681,8 +2796,8 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 2, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 0, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 2, false, false, &span);
assert_eq!(sel.iter().copied().collect::<Vec<_>>(), vec![ImageId(3)]);
}
@@ -2694,12 +2809,12 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false, &span);
click(&mut sel, &mut anchor, &all, 0, 0, false, false, &span);
click(&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, &span);
click(&mut sel, &mut anchor, &all, 0, 3, true, false, &span);
assert_eq!(sel.iter().copied().collect::<Vec<_>>(), vec![ImageId(1)]);
}
@@ -2710,8 +2825,8 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 2, false, false, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 6, false, true, &span);
click(&mut sel, &mut anchor, &all, 0, 2, false, false, &span);
click(&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)));
@@ -2724,8 +2839,8 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 6, false, false, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 2, false, true, &span);
click(&mut sel, &mut anchor, &all, 0, 6, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 2, false, true, &span);
assert_eq!(sel.len(), 5);
}
@@ -2739,12 +2854,12 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 5, false, false, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 15, false, true, &span);
click(&mut sel, &mut anchor, &all, 0, 5, false, false, &span);
click(&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, &span);
click(&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");
}
@@ -2758,9 +2873,9 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
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);
click(&mut sel, &mut anchor, &all, 0, 10, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 14, false, true, &span);
click(&mut sel, &mut anchor, &all, 0, 12, false, true, &span);
assert_eq!(anchor, Some(10));
assert_eq!(sel.len(), 3, "rows 10..=12");
@@ -2784,7 +2899,7 @@ mod tests {
// Selecting began somewhere else, so a range gesture would have had an
// anchor to sweep from.
apply_press(&mut sel, &mut anchor, &all, 0, 4, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 4, false, false, &span);
assert_eq!(sel.len(), 1);
tap(&mut sel, &mut anchor, &all, 11);
@@ -2800,7 +2915,85 @@ mod tests {
/// One tap in selection mode: a press reported as ctrl-held, which is what
/// `library.slint` sends while the mode is on.
fn tap(sel: &mut BTreeSet<ImageId>, anchor: &mut Option<usize>, all: &[ImageId], row: usize) {
apply_press(sel, anchor, all, 0, row, true, false, &library(all));
click(sel, anchor, all, 0, row, true, false, &library(all));
}
/// TRACES: FR-CAT-7 | FR-UI-4
/// The bug that made a forty-image drag file one photograph.
///
/// In selection mode every press arrives as a ctrl-press, so grabbing one
/// of the selected cells to drag them all used to *deselect* the cell being
/// grabbed. `drag-started` then saw a press on an image that was not in the
/// selection, concluded that was the gesture, and carried it alone.
#[test]
fn grabbing_a_selected_cell_leaves_the_whole_selection_to_drag() {
let all = ids(20);
let span = library(&all);
let mut sel = BTreeSet::new();
let mut anchor = None;
// Three photographs picked in selection mode, which reports ctrl.
for row in [2, 5, 9] {
click(&mut sel, &mut anchor, &all, 0, row, true, false, &span);
}
assert_eq!(sel.len(), 3);
// The press that begins the drag, on one of the three.
let outcome = apply_press(&mut sel, &mut anchor, &all, 0, 5, true, false, &span);
assert_eq!(
outcome,
Press::Deferred(ImageId(6)),
"the removal has to be handed back, not performed"
);
assert_eq!(
sel.len(),
3,
"the drag reads the selection next, and all three have to still be in it"
);
assert!(sel.contains(&ImageId(6)), "least of all the one being held");
}
/// And the other half: with no drag, the release still takes it out.
///
/// A tap in selection mode has to toggle, or there is no way to correct a
/// mis-tap short of leaving the mode.
#[test]
fn a_tap_on_a_selected_cell_still_takes_it_out() {
let all = ids(20);
let span = library(&all);
let mut sel = BTreeSet::new();
let mut anchor = None;
for row in [2, 5, 9] {
click(&mut sel, &mut anchor, &all, 0, row, true, false, &span);
}
click(&mut sel, &mut anchor, &all, 0, 5, true, false, &span);
assert_eq!(
sel.iter().copied().collect::<Vec<_>>(),
vec![ImageId(3), ImageId(10)],
"the tapped photograph is out and the other two stayed"
);
}
/// The anchor moves on the press whether or not the removal does.
///
/// "Select to…" measures from the anchor, and a run taken after tapping a
/// selected cell has to start where the user last touched — otherwise the
/// gesture sweeps from wherever the anchor happened to be left.
#[test]
fn a_deferred_press_still_moves_the_anchor() {
let all = ids(20);
let span = library(&all);
let mut sel = BTreeSet::new();
let mut anchor = None;
click(&mut sel, &mut anchor, &all, 0, 3, true, false, &span);
let _ = apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false, &span);
assert_eq!(anchor, Some(3));
}
#[test]
@@ -2812,13 +3005,13 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 0, false, false, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 2, false, true, &span);
click(&mut sel, &mut anchor, &all, 0, 0, false, false, &span);
click(&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, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 12, true, true, &span);
click(&mut sel, &mut anchor, &all, 0, 10, true, false, &span);
click(&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)));
@@ -2833,13 +3026,13 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
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);
click(&mut sel, &mut anchor, &all, 0, 0, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 1, true, false, &span);
click(&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, &span);
click(&mut sel, &mut anchor, &all, 0, 1, false, false, &span);
assert_eq!(sel.len(), 3, "the selection survived the press");
}
@@ -2856,13 +3049,13 @@ mod tests {
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, &span);
apply_press(&mut sel, &mut anchor, &all, 0, 3, true, false, &span);
click(&mut sel, &mut anchor, &all, 0, 1, false, false, &span);
click(&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(3));
apply_press(&mut sel, &mut anchor, &all, 0, 5, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 5, false, false, &span);
assert_ne!(
(sel.clone(), anchor),
before,
@@ -2883,7 +3076,7 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 99, false, false, &span);
click(&mut sel, &mut anchor, &all, 0, 99, false, false, &span);
assert!(sel.is_empty());
}
@@ -2903,12 +3096,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, &span);
click(&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, &span);
click(&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");
@@ -2930,7 +3123,7 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = Some(0);
apply_press(&mut sel, &mut anchor, &window, 10, 3, false, true, &span);
click(&mut sel, &mut anchor, &window, 10, 3, false, true, &span);
assert_eq!(sel.len(), 14, "ordinals 0..=13, loaded or not");
assert!(
@@ -2953,7 +3146,7 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = Some(25);
apply_press(&mut sel, &mut anchor, &window, 0, 2, false, true, &span);
click(&mut sel, &mut anchor, &window, 0, 2, false, true, &span);
assert_eq!(sel.len(), 24, "ordinals 2..=25");
assert!(sel.contains(&ImageId(3)) && sel.contains(&ImageId(26)));
@@ -2974,7 +3167,7 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = Some(0);
apply_press(
click(
&mut sel,
&mut anchor,
&window,
@@ -2996,7 +3189,7 @@ mod tests {
let mut sel = BTreeSet::new();
let mut anchor = None;
apply_press(&mut sel, &mut anchor, &all, 0, 3, false, true, &span);
click(&mut sel, &mut anchor, &all, 0, 3, false, true, &span);
assert_eq!(sel.iter().copied().collect::<Vec<_>>(), vec![ImageId(4)]);
}