From 83f11ad79b1473be12d90d68b956349e2205116a Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 18 Jun 2018 17:20:27 -0400 Subject: [PATCH] Don't cache CNContact. --- ...ShareToExistingContactViewController.swift | 25 ++++----- .../src/ViewControllers/ContactsPicker.swift | 4 +- .../ConversationViewController.m | 11 ++-- .../OWSAddToContactViewController.m | 8 +-- Signal/src/views/ContactCell.swift | 3 +- .../ViewModels/ContactShareViewModel.swift | 16 ------ SignalMessaging/Views/ContactsViewHelper.m | 2 +- SignalMessaging/contacts/OWSContactsManager.h | 7 +-- SignalMessaging/contacts/OWSContactsManager.m | 27 +++++++--- .../contacts/SystemContactsFetcher.swift | 54 ++----------------- SignalServiceKit/src/Contacts/Contact.h | 3 +- SignalServiceKit/src/Contacts/Contact.m | 19 ++++--- 12 files changed, 65 insertions(+), 114 deletions(-) diff --git a/Signal/src/ViewControllers/AddContactShareToExistingContactViewController.swift b/Signal/src/ViewControllers/AddContactShareToExistingContactViewController.swift index 94eee89c9..3ba404a14 100644 --- a/Signal/src/ViewControllers/AddContactShareToExistingContactViewController.swift +++ b/Signal/src/ViewControllers/AddContactShareToExistingContactViewController.swift @@ -55,24 +55,21 @@ class AddContactShareToExistingContactViewController: ContactsPicker, ContactsPi navigationController.popViewController(animated: true) } - func contactsPicker(_: ContactsPicker, didSelectContact newContact: Contact) { + func contactsPicker(_: ContactsPicker, didSelectContact oldContact: Contact) { Logger.debug("\(self.logTag) in \(#function)") let contactsManager = Environment.current().contactsManager - contactsManager?.cnContact(withId: self.contactShare.cnContactId, - success: { [weak self] oldCNContact in - contactsManager?.cnContact(withId: newContact.cnContactId, - success: { newCNContact in - guard let strongSelf = weakSelf else { - return - } - strongSelf.merge(oldCNContact: oldCNContact, newCNContact: newCNContact) - }, failure: { - // TODO: Alert? - }) - }, failure: { + guard let oldCNContact = contactsManager?.cnContact(withId: oldContact.cnContactId) else { // TODO: Alert? - }) + Logger.warn("\(logTag) could not load old CNContact.") + return + } + guard let newCNContact = OWSContacts.systemContact(for: self.contactShare.dbRecord, imageData: self.contactShare.avatarImageData) else { + // TODO: Alert? + Logger.warn("\(logTag) could not load new CNContact.") + return + } + merge(oldCNContact: oldCNContact, newCNContact: newCNContact) } func merge(oldCNContact: CNContact, newCNContact: CNContact) { diff --git a/Signal/src/ViewControllers/ContactsPicker.swift b/Signal/src/ViewControllers/ContactsPicker.swift index fbd920cbb..c42f071a2 100644 --- a/Signal/src/ViewControllers/ContactsPicker.swift +++ b/Signal/src/ViewControllers/ContactsPicker.swift @@ -237,7 +237,7 @@ public class ContactsPicker: OWSViewController, UITableViewDelegate, UITableView let contact = Contact(systemContact: cnContact) cell.configure(contact: contact, subtitleType: subtitleCellType, showsWhenSelected: self.allowsMultipleSelection, contactsManager: self.contactsManager) - let isSelected = selectedContacts.contains(where: { $0.uniqueId == contact.uniqueId }) + let isSelected = selectedContacts.contains(where: { $0.cnContactId == contact.cnContactId }) cell.isSelected = isSelected // Make sure we preserve selection across tableView.reloadData which happens when toggling between @@ -256,7 +256,7 @@ public class ContactsPicker: OWSViewController, UITableViewDelegate, UITableView let deselectedContact = cell.contact! selectedContacts = selectedContacts.filter { - return $0.uniqueId != deselectedContact.uniqueId + return $0.cnContactId != deselectedContact.cnContactId } } diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index bab602eb6..7ff1f1805 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -5118,13 +5118,18 @@ interactionControllerForAnimationController:(id CNContact? { -// public func cnContact(mergedWithExistingContact existingContact: Contact) -> CNContact? { -//// success successParam: @escaping (CNContact) -> Void, -//// failure failureParam: @escaping () -> Void) { -// -// guard let newCNContact = OWSContacts.systemContact(for: self.dbRecord, imageData: self.avatarImageData) else { -// owsFail("\(logTag) in \(#function) newCNContact was unexpectedly nil") -// return nil -// } -// -// let mergedCNContact = Contact.merge(cnContact: <#T##CNContact#>, newCNContact: <#T##CNContact#>) -// return existingContact.buildCNContact(mergedWithNewContact: newCNContact) -// } - @objc public func copy(withName name: OWSContactName) -> ContactShareViewModel { @@ -181,5 +166,4 @@ public class ContactShareViewModel: NSObject { // If we want to keep the avatar image, the caller will need to re-apply it. return ContactShareViewModel(contactShareRecord: newDbRecord, avatarImageData: nil) } - } diff --git a/SignalMessaging/Views/ContactsViewHelper.m b/SignalMessaging/Views/ContactsViewHelper.m index 60820ae6a..b6872af6b 100644 --- a/SignalMessaging/Views/ContactsViewHelper.m +++ b/SignalMessaging/Views/ContactsViewHelper.m @@ -355,7 +355,7 @@ NS_ASSUME_NONNULL_BEGIN cnContact = updatedContact; } if (signalAccount && !cnContact) { - cnContact = signalAccount.contact.cnContact; + cnContact = [self.contactsManager cnContactWithId:signalAccount.contact.cnContactId]; } if (cnContact) { if (shouldEditImmediately) { diff --git a/SignalMessaging/contacts/OWSContactsManager.h b/SignalMessaging/contacts/OWSContactsManager.h index 70fea7791..8ab416be8 100644 --- a/SignalMessaging/contacts/OWSContactsManager.h +++ b/SignalMessaging/contacts/OWSContactsManager.h @@ -9,9 +9,6 @@ NS_ASSUME_NONNULL_BEGIN extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification; -typedef void (^CNContactFetchSuccess)(CNContact *cnContact); -typedef void (^CNContactFetchFailure)(void); - @class ImageCache; @class SignalAccount; @class UIFont; @@ -62,9 +59,7 @@ typedef void (^CNContactFetchFailure)(void); // contacts haven't changed, and will clear out any stale cached SignalAccounts - (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler; -- (void)cnContactWithId:(NSString *)contactId - success:(CNContactFetchSuccess)success - failure:(CNContactFetchFailure)failure; +- (nullable CNContact *)cnContactWithId:(nullable NSString *)contactId; #pragma mark - Util diff --git a/SignalMessaging/contacts/OWSContactsManager.m b/SignalMessaging/contacts/OWSContactsManager.m index 86d5a81bf..8ab68c022 100644 --- a/SignalMessaging/contacts/OWSContactsManager.m +++ b/SignalMessaging/contacts/OWSContactsManager.m @@ -36,6 +36,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification @property (nonatomic, readonly) SystemContactsFetcher *systemContactsFetcher; @property (nonatomic, readonly) YapDatabaseConnection *dbReadConnection; @property (nonatomic, readonly) YapDatabaseConnection *dbWriteConnection; +@property (nonatomic, readonly) NSCache *cnContactCache; @end @@ -60,6 +61,8 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification _signalAccounts = @[]; _systemContactsFetcher = [SystemContactsFetcher new]; _systemContactsFetcher.delegate = self; + _cnContactCache = [NSCache new]; + _cnContactCache.countLimit = 50; OWSSingletonAssert(); @@ -133,15 +136,24 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification return self.systemContactsFetcher.supportsContactEditing; } -- (void)cnContactWithId:(NSString *)contactId - success:(CNContactFetchSuccess)success - failure:(CNContactFetchFailure)failure +- (nullable CNContact *)cnContactWithId:(nullable NSString *)contactId { OWSAssert(contactId.length > 0); - OWSAssert(success); - OWSAssert(failure); + OWSAssert(self.cnContactCache); - return [self.systemContactsFetcher fetchCNContactWithContactId:contactId success:success failure:failure]; + if (!contactId) { + return nil; + } + + CNContact *_Nullable cnContact = [self.cnContactCache objectForKey:contactId]; + if (!cnContact) { + cnContact = [self.systemContactsFetcher fetchCNContactWithContactId:contactId]; + if (cnContact) { + [self.cnContactCache setObject:cnContact forKey:contactId]; + } + } + + return cnContact; } #pragma mark - SystemContactsFetcherDelegate @@ -595,7 +607,8 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification NSAttributedString *lastName = [[NSAttributedString alloc] initWithString:cachedLastName attributes:lastNameAttributes]; - CNContact *_Nullable cnContact = self.allContactsMap[recipientId].cnContactForFormatting; + NSString *_Nullable cnContactId = self.allContactsMap[recipientId].cnContactId; + CNContact *_Nullable cnContact = [self cnContactWithId:cnContactId]; if (!cnContact) { // If we don't have a CNContact for this recipient id, make one. // Presumably [CNContactFormatter nameOrderForContact:] tries diff --git a/SignalMessaging/contacts/SystemContactsFetcher.swift b/SignalMessaging/contacts/SystemContactsFetcher.swift index e82a5b55a..d963772fe 100644 --- a/SignalMessaging/contacts/SystemContactsFetcher.swift +++ b/SignalMessaging/contacts/SystemContactsFetcher.swift @@ -395,63 +395,15 @@ public class SystemContactsFetcher: NSObject { } @objc - public func fetchCNContact(contactId: String, - success successParam: @escaping (CNContact) -> Void, - failure failureParam: @escaping () -> Void) { + public func fetchCNContact(contactId: String) -> CNContact? { SwiftAssertIsOnMainThread(#function) guard authorizationStatus == .authorized else { Logger.error("\(logTag) contact fetch failed; no access.") - failureParam() - return - } - - var backgroundTask: OWSBackgroundTask? = OWSBackgroundTask(label: "\(#function)", completionBlock: { [weak self] status in - SwiftAssertIsOnMainThread(#function) - - guard status == .expired else { - return - } - - guard let _ = self else { - return - } - Logger.error("background task time ran out before contact fetch completed.") - }) - - // Ensure success is invoked on main thread. - let success: (CNContact) -> Void = { contact in - DispatchMainThreadSafe({ - successParam(contact) - - assert(backgroundTask != nil) - backgroundTask = nil - }) - } - - // Ensure success is invoked on main thread. - let failure: () -> Void = { - DispatchMainThreadSafe({ - failureParam() - - assert(backgroundTask != nil) - backgroundTask = nil - }) + return nil } - // Don't use the serial queue. - DispatchQueue.global().async { - - Logger.info("\(self.logTag) fetching contact") - - if let cnContact = self.contactStoreAdapter.fetchCNContact(contactId: contactId) { - Logger.info("\(self.logTag) contact found") - success(cnContact) - } else { - Logger.info("\(self.logTag) contact not found") - failure() - } - } + return contactStoreAdapter.fetchCNContact(contactId: contactId) } } diff --git a/SignalServiceKit/src/Contacts/Contact.h b/SignalServiceKit/src/Contacts/Contact.h index 250611f12..d46624548 100644 --- a/SignalServiceKit/src/Contacts/Contact.h +++ b/SignalServiceKit/src/Contacts/Contact.h @@ -40,6 +40,7 @@ NS_ASSUME_NONNULL_BEGIN #if TARGET_OS_IOS - (instancetype)initWithSystemContact:(CNContact *)contact NS_AVAILABLE_IOS(9_0); ++ (nullable Contact *)contactWithVCardData:(NSData *)data; + (nullable CNContact *)cnContactWithVCardData:(NSData *)data; - (NSString *)nameForPhoneNumber:(NSString *)recipientId; @@ -53,8 +54,6 @@ NS_ASSUME_NONNULL_BEGIN + (CNContact *)mergeCNContact:(CNContact *)oldCNContact newCNContact:(CNContact *)newCNContact NS_SWIFT_NAME(merge(cnContact:newCNContact:)); -- (CNContact *)cnContactForFormatting; - @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Contacts/Contact.m b/SignalServiceKit/src/Contacts/Contact.m index 89d1cf8de..be582d401 100644 --- a/SignalServiceKit/src/Contacts/Contact.m +++ b/SignalServiceKit/src/Contacts/Contact.m @@ -113,6 +113,17 @@ NS_ASSUME_NONNULL_BEGIN return self; } ++ (nullable Contact *)contactWithVCardData:(NSData *)data +{ + CNContact *_Nullable cnContact = [self cnContactWithVCardData:data]; + + if (!cnContact) { + return nil; + } + + return [[self alloc] initWithSystemContact:cnContact]; +} + - (nullable UIImage *)image { if (_image) { @@ -409,14 +420,6 @@ NS_ASSUME_NONNULL_BEGIN return localizedLabel; } -- (CNContact *)cnContactForFormatting -{ - CNMutableContact *cnContact = [CNMutableContact new]; - cnContact.givenName = self.firstName; - cnContact.familyName = self.lastName; - return cnContact; -} - @end NS_ASSUME_NONNULL_END