Offer the merge when two people turn out to share a name
Over-clustering is the normal state of a freshly indexed library — FR-CULL-10 says so — which means one person arrives as several groups and the user names each of them the same thing. Until now that produced several people called Anna and no way to join them: `identity::merge` existed, `faces::merge_people` existed with its redirect tombstone, and `merge-into(int)` sat in identity.slint declared, never emitted and never wired. The screen had a split button and no merge. So a rename that collides now offers one. Type a name another person already carries and a strip appears under the field: "Someone else is already called Anna (14 faces). Merge them?" Offered, not performed. `people.uuid` is the identity and the name is not — the schema comment on that column is explicit that two devices naming the same cluster independently is the case it was built for — so two people sharing a name is legal, and folding them together on a keystroke would be the screen making an identity decision on the user's behalf. That is the thing this screen spends a whole button avoiding. The rename always lands first, and declining leaves it exactly as typed. There is nothing to undo because nothing was done. Details that are not arbitrary: The comparison is trimmed and case-insensitive. "anna" on a phone keyboard and "Anna" on a desktop are one intention, and an offer that appeared only when the capitalisation matched would read as a bug. An empty name collides with nothing. Every unnamed cluster renders as "Unnamed (n faces)"; if that counted as a collision the offer would appear on every cluster in a fresh library, and accepting it would fold the library into one person. The newly-named person folds into the one that already held the name, not the reverse. The older person is the one other devices have seen and the one whose confirmations are more likely to be real. Selection follows the merge, because landing on an empty screen after a successful action reads as a failure. The offer is retired when the person changes, and when a refresh finds its target gone — merged from the other side of a sync, or deleted. An offer left standing would fold whoever happens to be selected now. A merged-away person is not a namesake: `faces::people` already excludes redirects, so the offer does not reappear the instant it is accepted. Four tests over the collision rules, and the existing 467 still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -449,6 +449,45 @@ pub fn reject(
|
||||
faces::reject(catalog.connection(), face, person)
|
||||
}
|
||||
|
||||
/// Another person already carrying this name, if there is one.
|
||||
///
|
||||
/// A name is not an identity here — `people.uuid` is, which is what lets two
|
||||
/// devices name the same cluster independently and still merge cleanly
|
||||
/// (catalog.md, `people.uuid`). So this reports a *collision* for the screen to
|
||||
/// offer a merge on and does nothing else: two people are allowed to share a
|
||||
/// name, and a library with two Annas in it is a real library, not a mistake to
|
||||
/// be corrected without being asked.
|
||||
///
|
||||
/// Compared case-insensitively and trimmed. "anna" typed on a phone keyboard
|
||||
/// and "Anna" typed on a desktop are one intention, and an offer that appeared
|
||||
/// only when the capitalisation happened to match would read as a bug.
|
||||
///
|
||||
/// Merged-away people cannot come back: `faces::people` already excludes them,
|
||||
/// so a name freed by an earlier merge collides with nothing.
|
||||
pub fn namesake(
|
||||
catalog: &Catalog,
|
||||
person: PersonId,
|
||||
name: &str,
|
||||
) -> Result<Option<PersonRow>, dr_catalog::CatalogError> {
|
||||
let key = name.trim().to_lowercase();
|
||||
// An unnamed cluster is not a namesake of every other unnamed cluster.
|
||||
// They all render as "Unnamed (n faces)" and folding them together on that
|
||||
// basis would merge the whole library into one person.
|
||||
if key.is_empty() {
|
||||
return Ok(None);
|
||||
}
|
||||
Ok(dr_catalog::faces::people(catalog.connection())?
|
||||
.into_iter()
|
||||
.find(|p| p.id != person && p.name.trim().to_lowercase() == key)
|
||||
.map(|p| PersonRow {
|
||||
id: p.id,
|
||||
name: p.name,
|
||||
confirmed_faces: p.confirmed_faces,
|
||||
suggested_faces: p.suggested_faces,
|
||||
cover: None,
|
||||
}))
|
||||
}
|
||||
|
||||
/// Fold one person into another.
|
||||
pub fn merge(
|
||||
catalog: &Catalog,
|
||||
@@ -753,6 +792,66 @@ mod tests {
|
||||
assert_eq!(people[0].id, anna);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_namesake_is_found_whatever_the_capitalisation() {
|
||||
let c = catalog();
|
||||
let anna = faces::create_person(c.connection(), "Anna Smith").unwrap();
|
||||
let other = faces::create_person(c.connection(), "").unwrap();
|
||||
|
||||
rename(&c, other, " anna smith ").unwrap();
|
||||
let found = namesake(&c, other, " anna smith ")
|
||||
.unwrap()
|
||||
.expect("a namesake");
|
||||
assert_eq!(found.id, anna);
|
||||
assert_eq!(
|
||||
found.name, "Anna Smith",
|
||||
"the existing spelling is reported"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_person_is_not_their_own_namesake() {
|
||||
let c = catalog();
|
||||
let anna = faces::create_person(c.connection(), "Anna").unwrap();
|
||||
assert!(
|
||||
namesake(&c, anna, "Anna").unwrap().is_none(),
|
||||
"renaming someone to the name they already have offered a merge with themselves"
|
||||
);
|
||||
}
|
||||
|
||||
/// Every unnamed cluster renders as "Unnamed (n faces)". If an empty name
|
||||
/// counted as a collision, the offer would appear on every cluster in a
|
||||
/// freshly indexed library and accepting it would fold the library into one
|
||||
/// person.
|
||||
#[test]
|
||||
fn an_empty_name_collides_with_nothing() {
|
||||
let c = catalog();
|
||||
faces::create_person(c.connection(), "").unwrap();
|
||||
let second = faces::create_person(c.connection(), "").unwrap();
|
||||
assert!(namesake(&c, second, "").unwrap().is_none());
|
||||
assert!(namesake(&c, second, " ").unwrap().is_none());
|
||||
}
|
||||
|
||||
/// A merge leaves a redirect behind rather than deleting the row, so the
|
||||
/// name it carried must not keep colliding — otherwise the offer would
|
||||
/// reappear immediately after being accepted, pointing at a person the
|
||||
/// rail no longer shows.
|
||||
#[test]
|
||||
fn a_merged_away_person_is_not_a_namesake() {
|
||||
let c = catalog();
|
||||
let anna = faces::create_person(c.connection(), "Anna").unwrap();
|
||||
let dup = faces::create_person(c.connection(), "Anna").unwrap();
|
||||
merge(&c, anna, dup).unwrap();
|
||||
|
||||
let third = faces::create_person(c.connection(), "").unwrap();
|
||||
rename(&c, third, "Anna").unwrap();
|
||||
let found = namesake(&c, third, "Anna").unwrap().expect("a namesake");
|
||||
assert_eq!(
|
||||
found.id, anna,
|
||||
"the offer pointed at the merged-away person"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn merging_folds_one_person_into_the_other() {
|
||||
let c = catalog();
|
||||
|
||||
@@ -57,6 +57,13 @@ pub struct IdentityController {
|
||||
/// a row it would not, and the button would be the only sign it was
|
||||
/// running — invisible from every other screen.
|
||||
activity: RefCell<Option<crate::activity::Activity>>,
|
||||
/// The person a namesake merge is currently being offered against.
|
||||
///
|
||||
/// Held here rather than read back off the window because the window
|
||||
/// carries the *name* for the strip to print, and a name is not a key —
|
||||
/// two people called Anna are exactly the case this feature exists for, so
|
||||
/// re-deriving the target from the displayed text could pick the wrong one.
|
||||
merge_offer: std::cell::Cell<Option<PersonId>>,
|
||||
/// Rail portraits, kept between refreshes.
|
||||
///
|
||||
/// Every mutating action reloads the whole screen, and cutting a portrait
|
||||
@@ -149,6 +156,15 @@ pub fn refresh(
|
||||
}
|
||||
}
|
||||
|
||||
// So may the person an offer points at — a sync, or a merge performed from
|
||||
// the other side. An offer whose target no longer exists would put a button
|
||||
// on screen that cannot do anything.
|
||||
if let Some(target) = ctl.merge_offer.get() {
|
||||
if !ctl.people.borrow().iter().any(|p| p.id == target) {
|
||||
clear_merge_offer(window, ctl);
|
||||
}
|
||||
}
|
||||
|
||||
match (ctl.selected.get(), store) {
|
||||
(Some(person), Some(store)) => {
|
||||
let cells =
|
||||
@@ -210,6 +226,17 @@ pub fn refresh_coverage(
|
||||
}
|
||||
}
|
||||
|
||||
/// Take the namesake offer off the screen.
|
||||
///
|
||||
/// The name is what the strip keys on being visible, so emptying it is what
|
||||
/// hides the strip; the target is cleared alongside so a later accept cannot
|
||||
/// act on an offer the user can no longer see.
|
||||
fn clear_merge_offer(window: &AppWindow, ctl: &IdentityController) {
|
||||
ctl.merge_offer.set(None);
|
||||
window.set_identity_merge_offer_name(Default::default());
|
||||
window.set_identity_merge_offer_faces(0);
|
||||
}
|
||||
|
||||
fn push_faces(window: &AppWindow, ctl: &IdentityController, cells: &[FaceCell]) {
|
||||
let picked = ctl.picked.borrow();
|
||||
let rows: Vec<IdentityFace> = cells
|
||||
@@ -323,6 +350,9 @@ pub fn wire<S, M, P>(
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
ctl.selected.set(Some(PersonId(id as u64)));
|
||||
ctl.clear_picks();
|
||||
// The offer was about the person being navigated away from. Left
|
||||
// up, its "Merge" would fold whoever is selected *now*.
|
||||
clear_merge_offer(&w, &ctl);
|
||||
reload!(w, ctl, catalog, store);
|
||||
});
|
||||
}
|
||||
@@ -337,15 +367,81 @@ pub fn wire<S, M, P>(
|
||||
let Some(person) = ctl.selected.get() else {
|
||||
return;
|
||||
};
|
||||
// The rename always happens. The merge is a second question, asked
|
||||
// afterwards, and answering "keep separate" must leave the name the
|
||||
// user typed exactly where they typed it.
|
||||
clear_merge_offer(&w, &ctl);
|
||||
if let Some(cat) = catalog.borrow().as_ref() {
|
||||
if let Err(e) = identity::rename(cat, person, &name) {
|
||||
log::warn!("identity: rename: {e}");
|
||||
}
|
||||
match identity::namesake(cat, person, &name) {
|
||||
Ok(Some(other)) => {
|
||||
log::info!(
|
||||
"identity: {:?} is now called {:?}, which {:?} already is",
|
||||
person,
|
||||
other.name,
|
||||
other.id
|
||||
);
|
||||
ctl.merge_offer.set(Some(other.id));
|
||||
w.set_identity_merge_offer_name(other.name.as_str().into());
|
||||
w.set_identity_merge_offer_faces(
|
||||
(other.confirmed_faces + other.suggested_faces) as i32,
|
||||
);
|
||||
}
|
||||
Ok(None) => {}
|
||||
Err(e) => log::warn!("identity: looking for a namesake: {e}"),
|
||||
}
|
||||
}
|
||||
reload!(w, ctl, catalog, store);
|
||||
});
|
||||
}
|
||||
|
||||
// Accepting the namesake offer. The person the user just named is folded
|
||||
// **into** the one that already had the name, not the other way round: the
|
||||
// older person is the one other devices have already seen and the one whose
|
||||
// confirmations are more likely to be real, and `merge_people` leaves a
|
||||
// redirect behind so neither side of a sync resurrects what was merged.
|
||||
{
|
||||
let weak = window.as_weak();
|
||||
let ctl = ctl.clone();
|
||||
let catalog = catalog.clone();
|
||||
let store = store.clone();
|
||||
window.on_identity_merge_accept(move || {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
let (Some(target), Some(source)) = (ctl.merge_offer.get(), ctl.selected.get()) else {
|
||||
return;
|
||||
};
|
||||
if let Some(cat) = catalog.borrow().as_ref() {
|
||||
match identity::merge(cat, target, source) {
|
||||
Ok(moved) => {
|
||||
log::info!("identity: merged {source:?} into {target:?} ({moved} faces)");
|
||||
// Follow the merge. The group the user was looking at
|
||||
// no longer exists, and landing on an empty screen
|
||||
// after a successful action reads as a failure.
|
||||
ctl.selected.set(Some(target));
|
||||
ctl.clear_picks();
|
||||
}
|
||||
Err(e) => log::warn!("identity: merge: {e}"),
|
||||
}
|
||||
}
|
||||
clear_merge_offer(&w, &ctl);
|
||||
reload!(w, ctl, catalog, store);
|
||||
});
|
||||
}
|
||||
|
||||
// Declining it. Nothing to undo — the rename already happened — so this
|
||||
// only takes the strip away, and the library keeps two people with one
|
||||
// name, which is allowed.
|
||||
{
|
||||
let weak = window.as_weak();
|
||||
let ctl = ctl.clone();
|
||||
window.on_identity_merge_decline(move || {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
clear_merge_offer(&w, &ctl);
|
||||
});
|
||||
}
|
||||
|
||||
{
|
||||
let weak = window.as_weak();
|
||||
let ctl = ctl.clone();
|
||||
|
||||
@@ -564,11 +564,16 @@ export component AppWindow inherits Window {
|
||||
in property <string> identity-coverage;
|
||||
in property <bool> identity-coverage-complete: false;
|
||||
in property <int> identity-picked: 0;
|
||||
/// The namesake merge the Identity screen is offering. See IdentityScreen.
|
||||
in property <string> identity-merge-offer-name;
|
||||
in property <int> identity-merge-offer-faces: 0;
|
||||
|
||||
callback identity-open();
|
||||
callback identity-close();
|
||||
callback identity-person-picked(int);
|
||||
callback identity-rename(string);
|
||||
callback identity-merge-accept();
|
||||
callback identity-merge-decline();
|
||||
callback identity-confirm-face(int);
|
||||
callback identity-reject-face(int);
|
||||
callback identity-toggle-pick(int);
|
||||
@@ -1370,9 +1375,13 @@ in property <bool> panel-visible: true;
|
||||
back-label: root.identity-back-label;
|
||||
coverage: root.identity-coverage;
|
||||
coverage-complete: root.identity-coverage-complete;
|
||||
merge-offer-name: root.identity-merge-offer-name;
|
||||
merge-offer-faces: root.identity-merge-offer-faces;
|
||||
|
||||
person-picked(id) => { root.identity-person-picked(id); }
|
||||
rename(name) => { root.identity-rename(name); }
|
||||
merge-accept() => { root.identity-merge-accept(); }
|
||||
merge-decline() => { root.identity-merge-decline(); }
|
||||
confirm-face(id) => { root.identity-confirm-face(id); }
|
||||
reject-face(id) => { root.identity-reject-face(id); }
|
||||
toggle-pick(id) => { root.identity-toggle-pick(id); }
|
||||
|
||||
@@ -181,7 +181,18 @@ export component IdentityScreen inherits Rectangle {
|
||||
callback toggle-pick(int);
|
||||
callback confirm-all();
|
||||
callback split-picked();
|
||||
callback merge-into(int);
|
||||
/// A merge the screen is offering, because the name just typed is already
|
||||
/// someone else's. Empty when there is nothing to offer.
|
||||
///
|
||||
/// Offered rather than performed: `people.uuid` is the identity and the
|
||||
/// name is not, so two people sharing one is legal and folding them
|
||||
/// together silently would be the screen making an identity decision on
|
||||
/// the user's behalf — the thing FR-CULL-10 spends the split button to
|
||||
/// avoid.
|
||||
in property <string> merge-offer-name;
|
||||
in property <int> merge-offer-faces: 0;
|
||||
callback merge-accept();
|
||||
callback merge-decline();
|
||||
callback recluster();
|
||||
callback index-faces();
|
||||
callback stop-indexing();
|
||||
@@ -352,6 +363,43 @@ export component IdentityScreen inherits Rectangle {
|
||||
}
|
||||
}
|
||||
|
||||
// The namesake offer. Sits directly under the name field that
|
||||
// caused it, because it is a question about what was just typed
|
||||
// and anywhere else it would read as a status line.
|
||||
if root.merge-offer-name != "": Rectangle {
|
||||
height: 44px;
|
||||
border-radius: Theme.radius;
|
||||
background: Theme.surface-raised;
|
||||
|
||||
HorizontalLayout {
|
||||
padding-left: Theme.gap;
|
||||
padding-right: Theme.gap-sm;
|
||||
spacing: Theme.gap-sm;
|
||||
|
||||
Text {
|
||||
text: "Someone else is already called " + root.merge-offer-name
|
||||
+ " (" + root.merge-offer-faces + " faces). Merge them?";
|
||||
color: Theme.ink-dim;
|
||||
font-size: Theme.text-sm;
|
||||
vertical-alignment: center;
|
||||
wrap: word-wrap;
|
||||
}
|
||||
Rectangle { }
|
||||
// Decline first and merge second, so the destructive-
|
||||
// feeling half is not the one under a thumb reaching for
|
||||
// the edge of the strip.
|
||||
Button {
|
||||
text: "Keep separate";
|
||||
clicked => { root.merge-decline(); }
|
||||
}
|
||||
Button {
|
||||
text: "Merge";
|
||||
primary: true;
|
||||
clicked => { root.merge-accept(); }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if root.model-missing: Rectangle {
|
||||
height: 40px;
|
||||
border-radius: Theme.radius;
|
||||
|
||||
Reference in New Issue
Block a user