Move CNContact logic into our system contact adapter

// FREEBIE

Signal/src/ViewControllers/AddContactShareToExistingContactViewController.swift

@@ -8,8 +8,12 @@ import ContactsUI
 
 class AddContactShareToExistingContactViewController: ContactsPicker, ContactsPickerDelegate, CNContactViewControllerDelegate {
 
-    // TODO actual protocol?
-    weak var addToExistingContactDelegate: UIViewController?
+    // TODO - there are some hard coded assumptions in this VC that assume we are *pushed* onto a
+    // navigation controller. That seems fine for now, but if we need to be presented as a modal,
+    // or need to notify our presenter about our dismisall or other contact actions, a delegate
+    // would be helpful. It seems like this would require some broad changes to the ContactShareViewHelper,
+    // so I've left it as is for now, since it happens to work.
+    // weak var addToExistingContactDelegate: AddContactShareToExistingContactViewControllerDelegate?
 
     let contactShare: ContactShareViewModel
 
@@ -55,8 +59,7 @@ class AddContactShareToExistingContactViewController: ContactsPicker, ContactsPi
         Logger.debug("\(self.logTag) in \(#function)")
 
         guard let mergedContact: CNContact = self.contactShare.cnContact(mergedWithExistingContact: contact) else {
-            // TODO maybe this should not be optional and return a blank contact so we can still save the (not-actually merged) contact
-            owsFail("\(logTag) in \(#function) navigationController was unexpectedly nil")
+            owsFail("\(logTag) in \(#function) mergedContact was unexpectedly nil")
             return
         }
 
@@ -104,7 +107,16 @@ class AddContactShareToExistingContactViewController: ContactsPicker, ContactsPi
             return
         }
 
-        // We want to pop *this* view and the still presented CNContactViewController in a single animation.
+        // TODO this is weird - ideally we'd do something like
+        //     self.delegate?.didFinishAddingContact
+        // and the delegate, which knows about our presentation context could do the right thing.
+        //
+        // As it is, we happen to always be *pushing* this view controller onto a navcontroller, so the
+        // following works in all current cases.
+        //
+        // If we ever wanted to do something different, like present this in a modal, we'd have to rethink.
+
+        // We want to pop *this* view *and* the still presented CNContactViewController in a single animation.
         // Note this happens for *cancel* and for *done*. Unfortunately, I don't know of a way to detect the difference
         // between the two, since both just call this method.
         guard let myIndex = navigationController.viewControllers.index(of: self) else {

SignalMessaging/ViewModels/ContactShareViewModel.swift

@@ -111,61 +111,12 @@ public class ContactShareViewModel: NSObject {
 
     public func cnContact(mergedWithExistingContact existingContact: Contact) -> CNContact? {
 
-        guard let mergedCNContact: CNMutableContact = existingContact.cnContact?.mutableCopy() as? CNMutableContact else {
-            owsFail("\(logTag) in \(#function) mergedCNContact was unexpectedly nil")
-            return nil
-        }
-
         guard let newCNContact = OWSContacts.systemContact(for: self.dbRecord) else {
             owsFail("\(logTag) in \(#function) newCNContact was unexpectedly nil")
             return nil
         }
 
-        let normalize = { (input: String) -> String in
-            return (input as NSString).ows_stripped()
-        }
-
-        // TODO display name - but only if existing is blank
-
-        var existingPhoneNumberSet: Set<String> = Set()
-        mergedCNContact.phoneNumbers.map { $0.value }.forEach { (existingPhoneNumber: CNPhoneNumber) in
-            // compare e164?
-            let normalizedValue = normalize(existingPhoneNumber.stringValue as String)
-            existingPhoneNumberSet.insert(normalizedValue)
-        }
-
-        newCNContact.phoneNumbers.forEach { (newLabeledPhoneNumber: CNLabeledValue<CNPhoneNumber>) in
-            let normalizedValue = normalize(newLabeledPhoneNumber.value.stringValue as String)
-            guard !existingPhoneNumberSet.contains(normalizedValue) else {
-                Logger.debug("\(self.logTag) in \(#function) ignoring matching phone number: \(normalizedValue)")
-                return
-            }
-
-            Logger.debug("\(self.logTag) in \(#function) adding new phone number: \(normalizedValue)")
-            mergedCNContact.phoneNumbers.append(newLabeledPhoneNumber)
-        }
-
-//        // TODO emails
-//        var existingEmailSet: Set<String> = Set()
-//        mergedCNContact.emailAddresses.forEach { (existingEmail: CNLabeledValue<NSString>) in
-//            existingEmailSet.insert(normalize(existingEmail.value as String))
-//        }
-
-        // TODO address
-
-//        newCNContact.emailAddresses.forEach { newEmail in
-//            let match = mergedCNContact.emailAddresses.first { existingEmail in
-//                return normalize(existingEmail) == normalize(newEmail)
-//            }
-//            if match == nil {
-//                mergedCNContact.emailAddresses.add
-//            }
-//        }
-//
-//
-//
-
-        return mergedCNContact.copy() as? CNContact
+        return existingContact.buildCNContact(mergedWithNewContact: newCNContact)
     }
 
     public func copy(withNamePrefix namePrefix: String?,

SignalServiceKit/src/Contacts/Contact.h

@@ -7,10 +7,7 @@
 NS_ASSUME_NONNULL_BEGIN
 
 /**
- *
- * Contact represents relevant information related to a contact from the user's
- * contact list.
- *
+ * An adapter for the system contacts
  */
 
 @class CNContact;
@@ -49,6 +46,9 @@ NS_ASSUME_NONNULL_BEGIN
 #endif // TARGET_OS_IOS
 
 + (NSComparator)comparatorSortingNamesByFirstThenLast:(BOOL)firstNameOrdering;
++ (NSString *)formattedFullNameWithCNContact:(CNContact *)cnContact NS_SWIFT_NAME(formattedFullName(cnContact:));
+
+- (CNContact *)buildCNContactMergedWithNewContact:(CNContact *)newCNContact NS_SWIFT_NAME(buildCNContact(mergedWithNewContact:));
 
 @end
 

SignalServiceKit/src/Contacts/Contact.m

@@ -4,6 +4,7 @@
 
 #import "Contact.h"
 #import "Cryptography.h"
+#import "NSString+SSK.h"
 #import "OWSPrimaryStorage.h"
 #import "PhoneNumber.h"
 #import "SignalRecipient.h"
@@ -38,9 +39,9 @@ NS_ASSUME_NONNULL_BEGIN
     }
 
     _cnContact = contact;
-    _firstName = [self trimName:contact.givenName];
-    _lastName = [self trimName:contact.familyName];
-    _fullName = [CNContactFormatter stringFromContact:contact style:CNContactFormatterStyleFullName];
+    _firstName = contact.givenName.ows_stripped;
+    _lastName = contact.familyName.ows_stripped;
+    _fullName = [Contact formattedFullNameWithCNContact:contact];
     _uniqueId = contact.identifier;
 
     NSMutableArray<NSString *> *phoneNumbers = [NSMutableArray new];
@@ -125,11 +126,6 @@ NS_ASSUME_NONNULL_BEGIN
     return _image;
 }
 
-- (NSString *)trimName:(NSString *)name
-{
-    return [name stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
-}
-
 + (MTLPropertyStorage)storageBehaviorForPropertyWithKey:(NSString *)propertyKey
 {
     if ([propertyKey isEqualToString:@"cnContact"] || [propertyKey isEqualToString:@"image"]) {
@@ -250,6 +246,11 @@ NS_ASSUME_NONNULL_BEGIN
     };
 }
 
++ (NSString *)formattedFullNameWithCNContact:(CNContact *)cnContact
+{
+    return [CNContactFormatter stringFromContact:cnContact style:CNContactFormatterStyleFullName].ows_stripped;
+}
+
 - (NSString *)nameForPhoneNumber:(NSString *)recipientId
 {
     OWSAssert(recipientId.length > 0);
@@ -290,6 +291,62 @@ NS_ASSUME_NONNULL_BEGIN
     return hash;
 }
 
+- (CNContact *)buildCNContactMergedWithNewContact:(CNContact *)newCNContact
+{
+    CNMutableContact *_Nullable mergedCNContact = [self.cnContact mutableCopy];
+    if (!mergedCNContact) {
+        OWSFail(@"%@ in %s mergedCNContact was unexpectedly nil", self.logTag, __PRETTY_FUNCTION__);
+        return [CNContact new];
+    }
+    
+    // Name
+    NSString *formattedFullName =  [self.class formattedFullNameWithCNContact:mergedCNContact];
+
+    // merged all or nothing - do not try to piece-meal merge.
+    if (formattedFullName.length == 0) {
+        mergedCNContact.namePrefix = newCNContact.namePrefix.ows_stripped;
+        mergedCNContact.givenName = newCNContact.givenName.ows_stripped;
+        mergedCNContact.middleName = newCNContact.middleName.ows_stripped;
+        mergedCNContact.familyName = newCNContact.familyName.ows_stripped;
+        mergedCNContact.nameSuffix = newCNContact.nameSuffix.ows_stripped;
+    }
+    
+    // Phone Numbers
+    NSSet<PhoneNumber *> *existingPhoneNumberSet = [NSSet setWithArray:self.parsedPhoneNumbers];
+    
+    NSMutableArray<CNLabeledValue<CNPhoneNumber *> *> *mergedPhoneNumbers = [mergedCNContact.phoneNumbers mutableCopy];
+    for (CNLabeledValue<CNPhoneNumber *> *labeledPhoneNumber in newCNContact.phoneNumbers) {
+        PhoneNumber *_Nullable parsedPhoneNumber = [PhoneNumber tryParsePhoneNumberFromUserSpecifiedText:labeledPhoneNumber.value.stringValue];
+        if (parsedPhoneNumber && ![existingPhoneNumberSet containsObject:parsedPhoneNumber]) {
+            [mergedPhoneNumbers addObject:labeledPhoneNumber];
+        }
+    }
+    mergedCNContact.phoneNumbers = mergedPhoneNumbers;
+    
+    // Emails
+    NSSet<NSString *> *existingEmailSet = [NSSet setWithArray:self.emails];
+    NSMutableArray<CNLabeledValue<NSString *> *> *mergedEmailAddresses = [mergedCNContact.emailAddresses mutableCopy];
+    for (CNLabeledValue<NSString *> *labeledEmail in newCNContact.emailAddresses) {
+        NSString *normalizedValue = labeledEmail.value.ows_stripped;
+        if (![existingEmailSet containsObject:normalizedValue]) {
+            [mergedEmailAddresses addObject:labeledEmail];
+        }
+    }
+    mergedCNContact.emailAddresses = mergedEmailAddresses;
+    
+    // Address
+    // merged all or nothing - do not try to piece-meal merge.
+    BOOL hasExistingAddress = NO;
+    for (CNLabeledValue<CNPostalAddress *> *labeledPostalAddress in mergedCNContact.postalAddresses) {
+        hasExistingAddress = YES;
+    }
+    if (!hasExistingAddress) {
+        mergedCNContact.postalAddresses = newCNContact.postalAddresses;
+    }
+
+    return [mergedCNContact copy];
+}
+
 @end
 
 NS_ASSUME_NONNULL_END

SignalServiceKit/src/Messages/Interactions/OWSContact.m

@@ -3,6 +3,7 @@
 //
 
 #import "OWSContact.h"
+#import "Contact.h"
 #import "MimeTypeUtil.h"
 #import "NSString+SSK.h"
 #import "OWSContact+Private.h"
@@ -371,8 +372,9 @@ NSString *NSStringForContactAddressType(OWSContactAddressType value)
 - (void)ensureDisplayName
 {
     if (_displayName.length < 1) {
-        CNContact *_Nullable systemContact = [OWSContacts systemContactForContact:self];
-        _displayName = [CNContactFormatter stringFromContact:systemContact style:CNContactFormatterStyleFullName];
+        
+        CNContact *_Nullable cnContact = [OWSContacts systemContactForContact:self];
+        _displayName = [Contact formattedFullNameWithCNContact:cnContact];
     }
     if (_displayName.length < 1) {
         // Fall back to using the organization name.