From f4e471e0db955191f122a36a5c1d15669db1732f Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 15 Dec 2017 16:15:25 -0500 Subject: [PATCH 1/6] SignalAccount cache perf improvments - only persist models that have changed - remove duplicate contact SignalAccounts - ensure serial execution of buildAccounts - only buildSignalAccounts when intersection succeeds // FREEBIE --- Signal/src/contact/OWSContactsManager.m | 106 ++++++++++++++------ SignalServiceKit/src/Contacts/Contact.m | 27 +++++ SignalServiceKit/src/Contacts/PhoneNumber.m | 10 ++ 3 files changed, 114 insertions(+), 29 deletions(-) diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 49cdc9230..2a5dffbd0 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -63,14 +63,17 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)loadSignalAccountsFromCache { __block NSMutableArray *signalAccounts; - [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction * _Nonnull transaction) { - signalAccounts = [[NSMutableArray alloc] initWithCapacity:[SignalAccount numberOfKeysInCollectionWithTransaction:transaction]]; - + [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + NSUInteger signalAccountCount = [SignalAccount numberOfKeysInCollectionWithTransaction:transaction]; + DDLogInfo(@"%@ loading %lu signal accounts from cache.", self.logTag, (unsigned long)signalAccountCount); + + signalAccounts = [[NSMutableArray alloc] initWithCapacity:signalAccountCount]; + [SignalAccount enumerateCollectionObjectsWithTransaction:transaction usingBlock:^(SignalAccount *signalAccount, BOOL * _Nonnull stop) { [signalAccounts addObject:signalAccount]; }]; }]; - + [self updateSignalAccounts:signalAccounts]; } @@ -122,21 +125,23 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification #pragma mark - Intersection -- (void)intersectContacts +- (void)intersectContactsWithCompletion:(void (^)(NSError *_Nullable error))completionBlock { - [self intersectContactsWithRetryDelay:1]; + [self intersectContactsWithRetryDelay:1 completion:completionBlock]; } - (void)intersectContactsWithRetryDelay:(double)retryDelaySeconds + completion:(void (^)(NSError *_Nullable error))completionBlock { void (^success)(void) = ^{ DDLogInfo(@"%@ Successfully intersected contacts.", self.logTag); - [self buildSignalAccounts]; + completionBlock(nil); }; void (^failure)(NSError *error) = ^(NSError *error) { if ([error.domain isEqualToString:OWSSignalServiceKitErrorDomain] && error.code == OWSErrorCodeContactsUpdaterRateLimit) { DDLogError(@"Contact intersection hit rate limit with error: %@", error); + completionBlock(error); return; } @@ -147,7 +152,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification // TODO: Abort if another contact intersection succeeds in the meantime. dispatch_after( dispatch_time(DISPATCH_TIME_NOW, (int64_t)(retryDelaySeconds * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ - [self intersectContactsWithRetryDelay:retryDelaySeconds * 2]; + [self intersectContactsWithRetryDelay:retryDelaySeconds * 2 completion:completionBlock]; }); }; [[ContactsUpdater sharedUpdater] updateSignalContactIntersectionWithABContacts:self.allContacts @@ -193,17 +198,23 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImages]; - [self intersectContacts]; - - [self buildSignalAccounts]; + [self intersectContactsWithCompletion:^(NSError *_Nullable error) { + [self buildSignalAccounts]; + }]; }); }); } - (void)buildSignalAccounts { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - NSMutableDictionary *signalAccountMap = [NSMutableDictionary new]; + // Ensure we're not running concurrently since one invocation could affect the other. + static dispatch_queue_t _serialQueue; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + _serialQueue = dispatch_queue_create("org.whispersystems.contacts.buildSignalAccount", DISPATCH_QUEUE_SERIAL); + }); + + dispatch_async(_serialQueue, ^{ NSMutableArray *signalAccounts = [NSMutableArray new]; NSArray *contacts = self.allContacts; @@ -218,9 +229,15 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification } }]; + NSMutableSet *seenRecipientIds = [NSMutableSet new]; for (Contact *contact in contacts) { NSArray *signalRecipients = contactIdToSignalRecipientsMap[contact.uniqueId]; for (SignalRecipient *signalRecipient in [signalRecipients sortedArrayUsingSelector:@selector(compare:)]) { + if ([seenRecipientIds containsObject:signalRecipient.recipientId]) { + DDLogDebug(@"Ignoring duplicate contact: %@, %@", signalRecipient.recipientId, contact.fullName); + continue; + } + [seenRecipientIds addObject:signalRecipient.recipientId]; SignalAccount *signalAccount = [[SignalAccount alloc] initWithSignalRecipient:signalRecipient]; signalAccount.contact = contact; if (signalRecipients.count > 1) { @@ -228,33 +245,64 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification signalAccount.multipleAccountLabelText = [[self class] accountLabelForContact:contact recipientId:signalRecipient.recipientId]; } - if (signalAccountMap[signalAccount.recipientId]) { - DDLogDebug(@"Ignoring duplicate contact: %@, %@", signalAccount.recipientId, contact.fullName); - continue; - } [signalAccounts addObject:signalAccount]; } } + NSMutableDictionary *oldSignalAccounts = [NSMutableDictionary new]; + [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + [SignalAccount + enumerateCollectionObjectsWithTransaction:transaction + usingBlock:^(id _Nonnull object, BOOL *_Nonnull stop) { + if (![object isKindOfClass:[SignalAccount class]]) { + OWSFail(@"%@ Unexpected object in signal account collection: %@", + self.logTag, + object); + return; + } + SignalAccount *oldSignalAccount = (SignalAccount *)object; + + oldSignalAccounts[oldSignalAccount.uniqueId] = oldSignalAccount; + }]; + }]; + + NSMutableArray *accountsToSave = [NSMutableArray new]; + for (SignalAccount *signalAccount in signalAccounts) { + SignalAccount *_Nullable oldSignalAccount = oldSignalAccounts[signalAccount.uniqueId]; + + // keep track of which accounts are still relevant, so we can clean up orphans + [oldSignalAccounts removeObjectForKey:signalAccount.uniqueId]; + + if (oldSignalAccount == nil) { + // new Signal Account + [accountsToSave addObject:signalAccount]; + continue; + } + + if ([oldSignalAccount isEqual:signalAccount]) { + // Same value, no need to save. + continue; + } + + // value changed, save account + [accountsToSave addObject:signalAccount]; + } + // Update cached SignalAccounts on disk [self.dbWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - NSArray *allKeys = [transaction allKeysInCollection:[SignalAccount collection]]; - NSMutableSet *orphanedKeys = [NSMutableSet setWithArray:allKeys]; - - DDLogInfo(@"%@ Saving %lu SignalAccounts", self.logTag, signalAccounts.count); - for (SignalAccount *signalAccount in signalAccounts) { - // TODO only save the ones that changed - [orphanedKeys removeObject:signalAccount.uniqueId]; + DDLogInfo(@"%@ Saving %lu new SignalAccounts", self.logTag, (unsigned long)accountsToSave.count); + for (SignalAccount *signalAccount in accountsToSave) { + DDLogVerbose(@"%@ Adding new SignalAccount: %@", self.logTag, signalAccount); [signalAccount saveWithTransaction:transaction]; } - - if (orphanedKeys.count > 0) { - DDLogInfo(@"%@ Removing %lu orphaned SignalAccounts", self.logTag, (unsigned long)orphanedKeys.count); - [transaction removeObjectsForKeys:orphanedKeys.allObjects inCollection:[SignalAccount collection]]; + + DDLogInfo(@"%@ Removing %lu old SignalAccounts.", self.logTag, (unsigned long)oldSignalAccounts.count); + for (SignalAccount *signalAccount in oldSignalAccounts.allValues) { + DDLogVerbose(@"%@ Removing old SignalAccount: %@", self.logTag, signalAccount); + [signalAccount removeWithTransaction:transaction]; } }]; - dispatch_async(dispatch_get_main_queue(), ^{ [self updateSignalAccounts:signalAccounts]; }); diff --git a/SignalServiceKit/src/Contacts/Contact.m b/SignalServiceKit/src/Contacts/Contact.m index e7f5e9837..af98a4cdc 100644 --- a/SignalServiceKit/src/Contacts/Contact.m +++ b/SignalServiceKit/src/Contacts/Contact.m @@ -16,6 +16,7 @@ NS_ASSUME_NONNULL_BEGIN @interface Contact () @property (readonly, nonatomic) NSMutableDictionary *phoneNumberNameMap; +@property (readonly, nonatomic) NSData *imageData; @end @@ -133,6 +134,23 @@ NS_ASSUME_NONNULL_BEGIN return self; } +- (instancetype)initWithCoder:(NSCoder *)coder +{ + self = [super initWithCoder:coder]; + if (_imageData) { + _image = [UIImage imageWithData:_imageData]; + } + return self; +} + +- (void)encodeWithCoder:(NSCoder *)coder +{ + if (_image) { + _imageData = UIImagePNGRepresentation(_image); + } + [super encodeWithCoder:coder]; +} + - (NSString *)trimName:(NSString *)name { return [name stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]]; @@ -143,6 +161,15 @@ NS_ASSUME_NONNULL_BEGIN return [NSString stringWithFormat:@"ABRecordId:%d", recordId]; } ++ (MTLPropertyStorage)storageBehaviorForPropertyWithKey:(NSString *)propertyKey +{ + if ([propertyKey isEqualToString:@"cnContact"] || [propertyKey isEqualToString:@"image"]) { + return MTLPropertyStorageTransitory; + } else { + return [super storageBehaviorForPropertyWithKey:propertyKey]; + } +} + #endif // TARGET_OS_IOS - (NSArray *)parsedPhoneNumbersFromUserTextPhoneNumbers:(NSArray *)userTextPhoneNumbers diff --git a/SignalServiceKit/src/Contacts/PhoneNumber.m b/SignalServiceKit/src/Contacts/PhoneNumber.m index b26b6848a..dab6785a6 100644 --- a/SignalServiceKit/src/Contacts/PhoneNumber.m +++ b/SignalServiceKit/src/Contacts/PhoneNumber.m @@ -377,4 +377,14 @@ static NSString *const RPDefaultsKeyPhoneNumberCanonical = @"RPDefaultsKeyPhoneN return [self.toE164 compare:other.toE164]; } +- (BOOL)isEqual:(id)other +{ + if (![other isMemberOfClass:[self class]]) { + return NO; + } + PhoneNumber *otherPhoneNumber = (PhoneNumber *)other; + + return [self.phoneNumber isEqual:otherPhoneNumber.phoneNumber]; +} + @end From 49196f8013c3a01f779caad3c5936ca97135de60 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 15 Dec 2017 16:31:26 -0500 Subject: [PATCH 2/6] Spin activity indicator until contacts are fetched // FREEBIE --- .../ViewControllers/NewContactThreadViewController.m | 10 +++++++--- Signal/src/contact/OWSContactsManager.h | 4 +++- Signal/src/contact/OWSContactsManager.m | 5 +++-- Signal/src/contact/SystemContactsFetcher.swift | 4 ++-- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/Signal/src/ViewControllers/NewContactThreadViewController.m b/Signal/src/ViewControllers/NewContactThreadViewController.m index 8f0f759c0..137228bcf 100644 --- a/Signal/src/ViewControllers/NewContactThreadViewController.m +++ b/Signal/src/ViewControllers/NewContactThreadViewController.m @@ -147,9 +147,13 @@ NS_ASSUME_NONNULL_BEGIN { OWSAssert([NSThread isMainThread]); - [self.contactsViewHelper.contactsManager fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify]; - - [refreshControl endRefreshing]; + [self.contactsViewHelper.contactsManager + fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:^(NSError *_Nullable error) { + if (error) { + DDLogError(@"%@ refreshing contacts failed with error: %@", self.logTag, error); + } + [refreshControl endRefreshing]; + }]; } - (void)showContactsPermissionReminder:(BOOL)isVisible diff --git a/Signal/src/contact/OWSContactsManager.h b/Signal/src/contact/OWSContactsManager.h index e26cf05a4..01d56dc35 100644 --- a/Signal/src/contact/OWSContactsManager.h +++ b/Signal/src/contact/OWSContactsManager.h @@ -52,9 +52,11 @@ extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification; // Ensure's the app has the latest contacts, but won't prompt the user for contact // access if they haven't granted it. - (void)fetchSystemContactsOnceIfAlreadyAuthorized; + // This variant will fetch system contacts if contact access has already been granted, // but not prompt for contact access. Also, it will always fire a notification. -- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify; +- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion: + (void (^)(NSError *_Nullable error))completionHandler; #pragma mark - Util diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 2a5dffbd0..ac3c27a7a 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -95,9 +95,10 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.systemContactsFetcher fetchOnceIfAlreadyAuthorized]; } -- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify +- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion: + (void (^)(NSError *_Nullable error))completionHandler { - [self.systemContactsFetcher fetchIfAlreadyAuthorizedAndAlwaysNotify]; + [self.systemContactsFetcher fetchIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:completionHandler]; } - (BOOL)isSystemContactsAuthorized diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index 75ab49e14..b1621c57f 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -434,13 +434,13 @@ class SystemContactsFetcher: NSObject { updateContacts(completion: nil, alwaysNotify: false) } - public func fetchIfAlreadyAuthorizedAndAlwaysNotify() { + public func fetchIfAlreadyAuthorizedAndAlwaysNotify(completion: ((Error?) -> Void)?) { AssertIsOnMainThread() guard authorizationStatus == .authorized else { return } - updateContacts(completion: nil, alwaysNotify: true) + updateContacts(completion: completion, alwaysNotify: true) } private func updateContacts(completion completionParam: ((Error?) -> Void)?, alwaysNotify: Bool = false) { From e78edcde873ce19b6a8f504e9d4ad37ee3674fc3 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 15 Dec 2017 17:36:06 -0500 Subject: [PATCH 3/6] Only clear cache when user pulls-to-refresh // FREEBIE --- .../NewContactThreadViewController.m | 2 +- Signal/src/contact/OWSContactsManager.h | 6 +- Signal/src/contact/OWSContactsManager.m | 76 +++++++++++++------ .../src/contact/SystemContactsFetcher.swift | 33 ++++---- SignalServiceKit/src/Contacts/Contact.h | 10 +-- SignalServiceKit/src/Contacts/Contact.m | 34 ++++----- 6 files changed, 95 insertions(+), 66 deletions(-) diff --git a/Signal/src/ViewControllers/NewContactThreadViewController.m b/Signal/src/ViewControllers/NewContactThreadViewController.m index 137228bcf..acd419397 100644 --- a/Signal/src/ViewControllers/NewContactThreadViewController.m +++ b/Signal/src/ViewControllers/NewContactThreadViewController.m @@ -148,7 +148,7 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert([NSThread isMainThread]); [self.contactsViewHelper.contactsManager - fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:^(NSError *_Nullable error) { + userRequestedSystemContactsRefreshWithCompletion:^(NSError *_Nullable error) { if (error) { DDLogError(@"%@ refreshing contacts failed with error: %@", self.logTag, error); } diff --git a/Signal/src/contact/OWSContactsManager.h b/Signal/src/contact/OWSContactsManager.h index 01d56dc35..2e829309d 100644 --- a/Signal/src/contact/OWSContactsManager.h +++ b/Signal/src/contact/OWSContactsManager.h @@ -54,9 +54,9 @@ extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification; - (void)fetchSystemContactsOnceIfAlreadyAuthorized; // This variant will fetch system contacts if contact access has already been granted, -// but not prompt for contact access. Also, it will always fire a notification. -- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion: - (void (^)(NSError *_Nullable error))completionHandler; +// but not prompt for contact access. Also, it will always notify delegates, even if +// contacts haven't changed, and will clear out any stale cached SignalAccount's +- (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler; #pragma mark - Util diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index ac3c27a7a..05a037731 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -77,6 +77,17 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self updateSignalAccounts:signalAccounts]; } +- (dispatch_queue_t)serialQueue +{ + static dispatch_queue_t _serialQueue; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + _serialQueue = dispatch_queue_create("org.whispersystems.contacts.buildSignalAccount", DISPATCH_QUEUE_SERIAL); + }); + + return _serialQueue; +} + #pragma mark - System Contact Fetching // Request contacts access if you haven't asked recently. @@ -95,10 +106,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.systemContactsFetcher fetchOnceIfAlreadyAuthorized]; } -- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion: - (void (^)(NSError *_Nullable error))completionHandler +- (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler { - [self.systemContactsFetcher fetchIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:completionHandler]; + [self.systemContactsFetcher userRequestedRefreshWithCompletion:completionHandler]; } - (BOOL)isSystemContactsAuthorized @@ -120,8 +130,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)systemContactsFetcher:(SystemContactsFetcher *)systemsContactsFetcher updatedContacts:(NSArray *)contacts + userRequested:(BOOL)userRequested { - [self updateWithContacts:contacts]; + [self updateWithContacts:contacts clearStaleCache:userRequested]; } #pragma mark - Intersection @@ -179,10 +190,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImagesForKey:recipientId]; } -- (void)updateWithContacts:(NSArray *)contacts +- (void)updateWithContacts:(NSArray *)contacts clearStaleCache:(BOOL)clearStaleCache { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - + dispatch_async(self.serialQueue, ^{ NSMutableDictionary *allContactsMap = [NSMutableDictionary new]; for (Contact *contact in contacts) { for (PhoneNumber *phoneNumber in contact.parsedPhoneNumbers) { @@ -200,22 +210,15 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImages]; [self intersectContactsWithCompletion:^(NSError *_Nullable error) { - [self buildSignalAccounts]; + [self buildSignalAccountsAndClearStaleCache:clearStaleCache]; }]; }); }); } -- (void)buildSignalAccounts +- (void)buildSignalAccountsAndClearStaleCache:(BOOL)clearStaleCache; { - // Ensure we're not running concurrently since one invocation could affect the other. - static dispatch_queue_t _serialQueue; - static dispatch_once_t onceToken; - dispatch_once(&onceToken, ^{ - _serialQueue = dispatch_queue_create("org.whispersystems.contacts.buildSignalAccount", DISPATCH_QUEUE_SERIAL); - }); - - dispatch_async(_serialQueue, ^{ + dispatch_async(self.serialQueue, ^{ NSMutableArray *signalAccounts = [NSMutableArray new]; NSArray *contacts = self.allContacts; @@ -291,16 +294,43 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification // Update cached SignalAccounts on disk [self.dbWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - DDLogInfo(@"%@ Saving %lu new SignalAccounts", self.logTag, (unsigned long)accountsToSave.count); + DDLogInfo(@"%@ Saving %lu SignalAccounts", self.logTag, (unsigned long)accountsToSave.count); for (SignalAccount *signalAccount in accountsToSave) { - DDLogVerbose(@"%@ Adding new SignalAccount: %@", self.logTag, signalAccount); + DDLogVerbose(@"%@ Saving SignalAccount: %@", self.logTag, signalAccount); [signalAccount saveWithTransaction:transaction]; } - DDLogInfo(@"%@ Removing %lu old SignalAccounts.", self.logTag, (unsigned long)oldSignalAccounts.count); - for (SignalAccount *signalAccount in oldSignalAccounts.allValues) { - DDLogVerbose(@"%@ Removing old SignalAccount: %@", self.logTag, signalAccount); - [signalAccount removeWithTransaction:transaction]; + if (clearStaleCache) { + DDLogInfo(@"%@ Removing %lu old SignalAccounts.", self.logTag, (unsigned long)oldSignalAccounts.count); + for (SignalAccount *signalAccount in oldSignalAccounts.allValues) { + DDLogVerbose(@"%@ Removing old SignalAccount: %@", self.logTag, signalAccount); + [signalAccount removeWithTransaction:transaction]; + } + } else { + // In theory we want to remove SignalAccounts if the user deletes the corresponding system contact. + // However, as of iOS11.2 CNContactStore occasionally gives us only a subset of the system contacts. + // Because of that, it's not safe to clear orphaned accounts. + // Because we still want to give users a way to clear their stale accounts, if they pull-to-refresh + // their contacts we'll clear the cached ones. + // RADAR: https://bugreport.apple.com/web/?problemID=36082946 + if (oldSignalAccounts.allValues.count > 0) { + DDLogWarn(@"%@ NOT Removing %lu old SignalAccounts.", + self.logTag, + (unsigned long)oldSignalAccounts.count); + for (SignalAccount *signalAccount in oldSignalAccounts.allValues) { + DDLogVerbose( + @"%@ Ensuring old SignalAccount is not inadvertently lost: %@", self.logTag, signalAccount); + [signalAccounts addObject:signalAccount]; + } + + // re-sort signal accounts since we've appended some orphans + [signalAccounts sortUsingComparator:^NSComparisonResult(SignalAccount *left, SignalAccount *right) { + NSString *leftName = [self comparableNameForSignalAccount:left]; + NSString *rightName = [self comparableNameForSignalAccount:right]; + + return [leftName compare:rightName]; + }]; + } } }]; diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index b1621c57f..41baba6b8 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -185,11 +185,11 @@ class AddressBookContactStoreAdaptee: ContactStoreAdaptee { } } - return Contact(contactWithFirstName: firstName, - andLastName: lastName, - andUserTextPhoneNumbers: phoneNumbers, - andImage: addressBookRecord.image, - andContactID: addressBookRecord.recordId) + return Contact(firstName: firstName, + lastName: lastName, + userTextPhoneNumbers: phoneNumbers, + imageData: addressBookRecord.imageData, + contactID: addressBookRecord.recordId) } } @@ -243,14 +243,14 @@ struct OWSABRecord { } } - var image: UIImage? { + var imageData: Data? { guard ABPersonHasImageData(abRecord) else { return nil } guard let data = ABPersonCopyImageData(abRecord)?.takeRetainedValue() else { return nil } - return UIImage(data: data as Data) + return data as Data } private func extractProperty(_ propertyName: ABPropertyID) -> T? { @@ -316,7 +316,7 @@ class ContactStoreAdapter: ContactStoreAdaptee { } @objc protocol SystemContactsFetcherDelegate: class { - func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact]) + func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact], userRequested: Bool) } @objc @@ -361,7 +361,7 @@ class SystemContactsFetcher: NSObject { hasSetupObservation = true self.contactStoreAdapter.startObservingChanges { [weak self] in DispatchQueue.main.async { - self?.updateContacts(completion: nil, alwaysNotify: false) + self?.updateContacts(completion: nil, userRequested: false) } } } @@ -431,19 +431,20 @@ class SystemContactsFetcher: NSObject { return } - updateContacts(completion: nil, alwaysNotify: false) + updateContacts(completion: nil, userRequested: false) } - public func fetchIfAlreadyAuthorizedAndAlwaysNotify(completion: ((Error?) -> Void)?) { + public func userRequestedRefresh(completion: @escaping (Error?) -> Void) { AssertIsOnMainThread() guard authorizationStatus == .authorized else { + owsFail("should have already requested contact access") return } - updateContacts(completion: completion, alwaysNotify: true) + updateContacts(completion: completion, userRequested: true) } - private func updateContacts(completion completionParam: ((Error?) -> Void)?, alwaysNotify: Bool = false) { + private func updateContacts(completion completionParam: ((Error?) -> Void)?, userRequested: Bool = false) { AssertIsOnMainThread() // Ensure completion is invoked on main thread. @@ -483,8 +484,8 @@ class SystemContactsFetcher: NSObject { if self.lastContactUpdateHash != contactsHash { Logger.info("\(self.TAG) contact hash changed. new contactsHash: \(contactsHash)") shouldNotifyDelegate = true - } else if alwaysNotify { - Logger.info("\(self.TAG) ignoring debounce.") + } else if userRequested { + Logger.info("\(self.TAG) ignoring debounce due to user request") shouldNotifyDelegate = true } else { @@ -516,7 +517,7 @@ class SystemContactsFetcher: NSObject { self.lastDelegateNotificationDate = Date() self.lastContactUpdateHash = contactsHash - self.delegate?.systemContactsFetcher(self, updatedContacts: contacts) + self.delegate?.systemContactsFetcher(self, updatedContacts: contacts, userRequested: userRequested) completion(nil) } } diff --git a/SignalServiceKit/src/Contacts/Contact.h b/SignalServiceKit/src/Contacts/Contact.h index 94fbf42b4..8c7729aec 100644 --- a/SignalServiceKit/src/Contacts/Contact.h +++ b/SignalServiceKit/src/Contacts/Contact.h @@ -44,11 +44,11 @@ NS_ASSUME_NONNULL_BEGIN #if TARGET_OS_IOS -- (instancetype)initWithContactWithFirstName:(nullable NSString *)firstName - andLastName:(nullable NSString *)lastName - andUserTextPhoneNumbers:(NSArray *)phoneNumbers - andImage:(nullable UIImage *)image - andContactID:(ABRecordID)record; +- (instancetype)initWithFirstName:(nullable NSString *)firstName + lastName:(nullable NSString *)lastName + userTextPhoneNumbers:(NSArray *)phoneNumbers + imageData:(nullable NSData *)imageData + contactID:(ABRecordID)record; - (instancetype)initWithSystemContact:(CNContact *)contact; diff --git a/SignalServiceKit/src/Contacts/Contact.m b/SignalServiceKit/src/Contacts/Contact.m index af98a4cdc..c0c291c16 100644 --- a/SignalServiceKit/src/Contacts/Contact.m +++ b/SignalServiceKit/src/Contacts/Contact.m @@ -27,13 +27,14 @@ NS_ASSUME_NONNULL_BEGIN @synthesize fullName = _fullName; @synthesize comparableNameFirstLast = _comparableNameFirstLast; @synthesize comparableNameLastFirst = _comparableNameLastFirst; +@synthesize image = _image; #if TARGET_OS_IOS -- (instancetype)initWithContactWithFirstName:(nullable NSString *)firstName - andLastName:(nullable NSString *)lastName - andUserTextPhoneNumbers:(NSArray *)phoneNumbers - andImage:(nullable UIImage *)image - andContactID:(ABRecordID)record +- (instancetype)initWithFirstName:(nullable NSString *)firstName + lastName:(nullable NSString *)lastName + userTextPhoneNumbers:(NSArray *)phoneNumbers + imageData:(nullable NSData *)imageData + contactID:(ABRecordID)record { self = [super init]; if (!self) { @@ -47,7 +48,7 @@ NS_ASSUME_NONNULL_BEGIN _userTextPhoneNumbers = phoneNumbers; _phoneNumberNameMap = [NSMutableDictionary new]; _parsedPhoneNumbers = [self parsedPhoneNumbersFromUserTextPhoneNumbers:phoneNumbers phoneNumberNameMap:@{}]; - _image = image; + _imageData = imageData; // Not using emails for old AB style contacts. _emails = [NSMutableArray new]; @@ -128,27 +129,24 @@ NS_ASSUME_NONNULL_BEGIN _emails = [emailAddresses copy]; if (contact.thumbnailImageData) { - _image = [UIImage imageWithData:contact.thumbnailImageData]; + _imageData = contact.thumbnailImageData; } return self; } -- (instancetype)initWithCoder:(NSCoder *)coder +- (nullable UIImage *)image { - self = [super initWithCoder:coder]; - if (_imageData) { - _image = [UIImage imageWithData:_imageData]; + if (_image) { + return _image; } - return self; -} -- (void)encodeWithCoder:(NSCoder *)coder -{ - if (_image) { - _imageData = UIImagePNGRepresentation(_image); + if (!self.imageData) { + return nil; } - [super encodeWithCoder:coder]; + + _image = [UIImage imageWithData:self.imageData]; + return _image; } - (NSString *)trimName:(NSString *)name From 27c99cf4d26e49ac97e6bb5d1916b2e385596e9b Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Sat, 16 Dec 2017 12:26:47 -0500 Subject: [PATCH 4/6] sort SignalAccounts loaded from cache --- Signal/src/contact/OWSContactsManager.m | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 05a037731..435ef2bc4 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -74,6 +74,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification }]; }]; + [signalAccounts sortUsingComparator:self.signalAccountComparator]; [self updateSignalAccounts:signalAccounts]; } @@ -324,12 +325,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification } // re-sort signal accounts since we've appended some orphans - [signalAccounts sortUsingComparator:^NSComparisonResult(SignalAccount *left, SignalAccount *right) { - NSString *leftName = [self comparableNameForSignalAccount:left]; - NSString *rightName = [self comparableNameForSignalAccount:right]; - - return [leftName compare:rightName]; - }]; + [signalAccounts sortUsingComparator:self.signalAccountComparator]; } } }]; @@ -699,6 +695,16 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification return image; } +- (NSComparisonResult (^)(SignalAccount *left, SignalAccount *right))signalAccountComparator +{ + return ^NSComparisonResult(SignalAccount *left, SignalAccount *right) { + NSString *leftName = [self comparableNameForSignalAccount:left]; + NSString *rightName = [self comparableNameForSignalAccount:right]; + + return [leftName compare:rightName]; + }; +} + - (NSString *)comparableNameForSignalAccount:(SignalAccount *)signalAccount { NSString *_Nullable name; From 60eac4e0bfa9060792c6d242d2ef74f4b5169a36 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Sat, 16 Dec 2017 12:31:56 -0500 Subject: [PATCH 5/6] notify only when SignalAccounts actually change // FREEBIE --- Signal/src/contact/OWSContactsManager.m | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 435ef2bc4..20f811c02 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -339,7 +339,11 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)updateSignalAccounts:(NSArray *)signalAccounts { AssertIsOnMainThread(); - + if ([signalAccounts isEqual:self.signalAccounts]) { + DDLogDebug(@"%@ SignalAccounts unchanged.", self.logTag); + return; + } + NSMutableDictionary *signalAccountMap = [NSMutableDictionary new]; for (SignalAccount *signalAccount in signalAccounts) { signalAccountMap[signalAccount.recipientId] = signalAccount; From 1955f3664b6067d09aca39fdd75907af936d1ff2 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Sat, 16 Dec 2017 13:21:34 -0500 Subject: [PATCH 6/6] CR: clarify names, comments, asserts // FREEBIE --- Signal/src/contact/OWSContactsManager.h | 2 +- Signal/src/contact/OWSContactsManager.m | 19 +++++++------------ .../src/contact/SystemContactsFetcher.swift | 14 +++++++------- 3 files changed, 15 insertions(+), 20 deletions(-) diff --git a/Signal/src/contact/OWSContactsManager.h b/Signal/src/contact/OWSContactsManager.h index 2e829309d..bdd8487d7 100644 --- a/Signal/src/contact/OWSContactsManager.h +++ b/Signal/src/contact/OWSContactsManager.h @@ -55,7 +55,7 @@ extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification; // This variant will fetch system contacts if contact access has already been granted, // but not prompt for contact access. Also, it will always notify delegates, even if -// contacts haven't changed, and will clear out any stale cached SignalAccount's +// contacts haven't changed, and will clear out any stale cached SignalAccounts - (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler; #pragma mark - Util diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 20f811c02..90c351a68 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -131,9 +131,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)systemContactsFetcher:(SystemContactsFetcher *)systemsContactsFetcher updatedContacts:(NSArray *)contacts - userRequested:(BOOL)userRequested + isUserRequested:(BOOL)isUserRequested { - [self updateWithContacts:contacts clearStaleCache:userRequested]; + [self updateWithContacts:contacts shouldClearStaleCache:isUserRequested]; } #pragma mark - Intersection @@ -191,7 +191,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImagesForKey:recipientId]; } -- (void)updateWithContacts:(NSArray *)contacts clearStaleCache:(BOOL)clearStaleCache +- (void)updateWithContacts:(NSArray *)contacts shouldClearStaleCache:(BOOL)shouldClearStaleCache { dispatch_async(self.serialQueue, ^{ NSMutableDictionary *allContactsMap = [NSMutableDictionary new]; @@ -211,13 +211,13 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImages]; [self intersectContactsWithCompletion:^(NSError *_Nullable error) { - [self buildSignalAccountsAndClearStaleCache:clearStaleCache]; + [self buildSignalAccountsAndClearStaleCache:shouldClearStaleCache]; }]; }); }); } -- (void)buildSignalAccountsAndClearStaleCache:(BOOL)clearStaleCache; +- (void)buildSignalAccountsAndClearStaleCache:(BOOL)shouldClearStaleCache; { dispatch_async(self.serialQueue, ^{ NSMutableArray *signalAccounts = [NSMutableArray new]; @@ -259,12 +259,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [SignalAccount enumerateCollectionObjectsWithTransaction:transaction usingBlock:^(id _Nonnull object, BOOL *_Nonnull stop) { - if (![object isKindOfClass:[SignalAccount class]]) { - OWSFail(@"%@ Unexpected object in signal account collection: %@", - self.logTag, - object); - return; - } + OWSAssert([object isKindOfClass:[SignalAccount class]]); SignalAccount *oldSignalAccount = (SignalAccount *)object; oldSignalAccounts[oldSignalAccount.uniqueId] = oldSignalAccount; @@ -301,7 +296,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [signalAccount saveWithTransaction:transaction]; } - if (clearStaleCache) { + if (shouldClearStaleCache) { DDLogInfo(@"%@ Removing %lu old SignalAccounts.", self.logTag, (unsigned long)oldSignalAccounts.count); for (SignalAccount *signalAccount in oldSignalAccounts.allValues) { DDLogVerbose(@"%@ Removing old SignalAccount: %@", self.logTag, signalAccount); diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index 41baba6b8..a70d8b838 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -316,7 +316,7 @@ class ContactStoreAdapter: ContactStoreAdaptee { } @objc protocol SystemContactsFetcherDelegate: class { - func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact], userRequested: Bool) + func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact], isUserRequested: Bool) } @objc @@ -361,7 +361,7 @@ class SystemContactsFetcher: NSObject { hasSetupObservation = true self.contactStoreAdapter.startObservingChanges { [weak self] in DispatchQueue.main.async { - self?.updateContacts(completion: nil, userRequested: false) + self?.updateContacts(completion: nil, isUserRequested: false) } } } @@ -431,7 +431,7 @@ class SystemContactsFetcher: NSObject { return } - updateContacts(completion: nil, userRequested: false) + updateContacts(completion: nil, isUserRequested: false) } public func userRequestedRefresh(completion: @escaping (Error?) -> Void) { @@ -441,10 +441,10 @@ class SystemContactsFetcher: NSObject { return } - updateContacts(completion: completion, userRequested: true) + updateContacts(completion: completion, isUserRequested: true) } - private func updateContacts(completion completionParam: ((Error?) -> Void)?, userRequested: Bool = false) { + private func updateContacts(completion completionParam: ((Error?) -> Void)?, isUserRequested: Bool = false) { AssertIsOnMainThread() // Ensure completion is invoked on main thread. @@ -484,7 +484,7 @@ class SystemContactsFetcher: NSObject { if self.lastContactUpdateHash != contactsHash { Logger.info("\(self.TAG) contact hash changed. new contactsHash: \(contactsHash)") shouldNotifyDelegate = true - } else if userRequested { + } else if isUserRequested { Logger.info("\(self.TAG) ignoring debounce due to user request") shouldNotifyDelegate = true } else { @@ -517,7 +517,7 @@ class SystemContactsFetcher: NSObject { self.lastDelegateNotificationDate = Date() self.lastContactUpdateHash = contactsHash - self.delegate?.systemContactsFetcher(self, updatedContacts: contacts, userRequested: userRequested) + self.delegate?.systemContactsFetcher(self, updatedContacts: contacts, isUserRequested: isUserRequested) completion(nil) } }