diff --git a/ui/dr-ui/src/identity.rs b/ui/dr-ui/src/identity.rs index dc7782c..acb16e9 100644 --- a/ui/dr-ui/src/identity.rs +++ b/ui/dr-ui/src/identity.rs @@ -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, 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(); diff --git a/ui/dr-ui/src/identity_ui.rs b/ui/dr-ui/src/identity_ui.rs index 89a1a4a..9052fd9 100644 --- a/ui/dr-ui/src/identity_ui.rs +++ b/ui/dr-ui/src/identity_ui.rs @@ -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>, + /// 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>, /// 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 = cells @@ -323,6 +350,9 @@ pub fn wire( 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( 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(); diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 776f8fe..23a813a 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -564,11 +564,16 @@ export component AppWindow inherits Window { in property identity-coverage; in property identity-coverage-complete: false; in property identity-picked: 0; + /// The namesake merge the Identity screen is offering. See IdentityScreen. + in property identity-merge-offer-name; + in property 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 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); } diff --git a/ui/dr-ui/ui/identity.slint b/ui/dr-ui/ui/identity.slint index 3c40c7b..a261fdf 100644 --- a/ui/dr-ui/ui/identity.slint +++ b/ui/dr-ui/ui/identity.slint @@ -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 merge-offer-name; + in property 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;