From f684482c54244a9a8b70408a3470d6725726a0f5 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 8 Dec 2017 15:21:08 -0500 Subject: [PATCH] Don't emit "user profile changed" notifications if nothing changed. --- Signal/src/ViewControllers/AvatarViewHelper.m | 4 +- .../ViewControllers/NewGroupViewController.m | 1 + .../ViewControllers/ProfileViewController.m | 1 + .../UpdateGroupViewController.m | 1 + .../ExperienceUpgrade.swift | 2 +- SignalMessaging/profiles/OWSUserProfile.m | 257 +++++++++++++----- .../src/Storage/TSYapDatabaseObject.h | 2 + .../src/Storage/TSYapDatabaseObject.m | 19 +- 8 files changed, 215 insertions(+), 72 deletions(-) diff --git a/Signal/src/ViewControllers/AvatarViewHelper.m b/Signal/src/ViewControllers/AvatarViewHelper.m index ce3d23f7f..9b8848729 100644 --- a/Signal/src/ViewControllers/AvatarViewHelper.m +++ b/Signal/src/ViewControllers/AvatarViewHelper.m @@ -130,7 +130,9 @@ NS_ASSUME_NONNULL_BEGIN CropScaleImageViewController *vc = [[CropScaleImageViewController alloc] initWithSrcImage:rawAvatar successCompletion:^(UIImage *_Nonnull dstImage) { - [self.delegate avatarDidChange:dstImage]; + dispatch_async(dispatch_get_main_queue(), ^{ + [self.delegate avatarDidChange:dstImage]; + }); }]; [self.delegate.fromViewController presentViewController:vc diff --git a/Signal/src/ViewControllers/NewGroupViewController.m b/Signal/src/ViewControllers/NewGroupViewController.m index 85281dd26..184154740 100644 --- a/Signal/src/ViewControllers/NewGroupViewController.m +++ b/Signal/src/ViewControllers/NewGroupViewController.m @@ -619,6 +619,7 @@ const NSUInteger kNewGroupViewControllerAvatarWidth = 68; - (void)avatarDidChange:(UIImage *)image { + OWSAssert([NSThread isMainThread]); OWSAssert(image); self.groupAvatar = image; diff --git a/Signal/src/ViewControllers/ProfileViewController.m b/Signal/src/ViewControllers/ProfileViewController.m index 9d25acc0f..210579c3e 100644 --- a/Signal/src/ViewControllers/ProfileViewController.m +++ b/Signal/src/ViewControllers/ProfileViewController.m @@ -577,6 +577,7 @@ NSString *const kProfileView_LastPresentedDate = @"kProfileView_LastPresentedDat - (void)avatarDidChange:(UIImage *)image { + OWSAssert([NSThread isMainThread]); OWSAssert(image); self.avatar = [image resizedImageToFillPixelSize:CGSizeMake(kOWSProfileManager_MaxAvatarDiameter, diff --git a/Signal/src/ViewControllers/UpdateGroupViewController.m b/Signal/src/ViewControllers/UpdateGroupViewController.m index 5d2f3f9c0..dd8bdbaa5 100644 --- a/Signal/src/ViewControllers/UpdateGroupViewController.m +++ b/Signal/src/ViewControllers/UpdateGroupViewController.m @@ -496,6 +496,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)avatarDidChange:(UIImage *)image { + OWSAssert([NSThread isMainThread]); OWSAssert(image); self.groupAvatar = image; diff --git a/Signal/src/environment/ExperienceUpgrades/ExperienceUpgrade.swift b/Signal/src/environment/ExperienceUpgrades/ExperienceUpgrade.swift index d3e5735bc..9a325efd2 100644 --- a/Signal/src/environment/ExperienceUpgrades/ExperienceUpgrade.swift +++ b/Signal/src/environment/ExperienceUpgrades/ExperienceUpgrade.swift @@ -54,7 +54,7 @@ class ExperienceUpgrade: TSYapDatabaseObject { // these models in a "change log" archive. if propertyKey == "title" || propertyKey == "body" || propertyKey == "image" { return MTLPropertyStorageNone - } else if propertyKey == "uniqueId" || propertyKey == "seenAt" { + } else if propertyKey == "uniqueId" || propertyKey == "seenAt" || propertyKey == "hasEverBeenSaved" { return super.storageBehaviorForProperty(withKey: propertyKey) } else { // Being conservative here in case we rename a property. diff --git a/SignalMessaging/profiles/OWSUserProfile.m b/SignalMessaging/profiles/OWSUserProfile.m index 1b9e4edf1..00593e071 100644 --- a/SignalMessaging/profiles/OWSUserProfile.m +++ b/SignalMessaging/profiles/OWSUserProfile.m @@ -87,18 +87,21 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; #pragma mark - Update With... Methods -- (void)finalizeWithCompletion:(nullable OWSUserProfileCompletion)externalCompletion didChange:(BOOL)didChange +- (void)finalizeWithCompletion:(nullable OWSUserProfileCompletion)externalCompletion { + DDLogVerbose(@"%@, after: %@", self.logTag, self.debugDescription); + if (externalCompletion) { dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), externalCompletion); } - if (!didChange) { - return; - } - BOOL isLocalUserProfile = [self.recipientId isEqualToString:kLocalProfileUniqueId]; + // if (isLocalUserProfile) { + // [DDLog flushLog]; + // DDLogError(@"%@", self.logTag); + // } + dispatch_async(dispatch_get_main_queue(), ^{ if (isLocalUserProfile) { // We populate an initial (empty) profile on launch of a new install, but until @@ -134,24 +137,34 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - __block BOOL didChange = NO; + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didStringChange:self.profileName newValue:[profileName ows_stripped]]; + didChange |= [self didStringChange:self.avatarUrlPath newValue:avatarUrlPath]; + didChange |= [self didStringChange:self.avatarFileName newValue:avatarFileName]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= [self didStringChange:userProfile.profileName - newValue:[profileName ows_stripped]]; - didChange |= - [self didStringChange:userProfile.avatarUrlPath newValue:avatarUrlPath]; - didChange |= - [self didStringChange:userProfile.avatarFileName newValue:avatarFileName]; - [userProfile setProfileName:[profileName ows_stripped]]; [userProfile setAvatarUrlPath:avatarUrlPath]; [userProfile setAvatarFileName:avatarFileName]; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)updateWithProfileName:(nullable NSString *)profileName @@ -161,18 +174,29 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - __block BOOL didChange = NO; + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didStringChange:self.profileName newValue:[profileName ows_stripped]]; + didChange |= [self didStringChange:self.avatarUrlPath newValue:avatarUrlPath]; + didChange |= [self didStringChange:self.avatarFileName newValue:avatarFileName]; + didChange |= [self didDateChange:self.lastUpdateDate newValue:lastUpdateDate]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= [self didStringChange:userProfile.profileName - newValue:[profileName ows_stripped]]; - didChange |= [self didStringChange:userProfile.avatarUrlPath newValue:avatarUrlPath]; - didChange |= - [self didStringChange:userProfile.avatarFileName newValue:avatarFileName]; - didChange |= [self didDateChange:userProfile.lastUpdateDate newValue:lastUpdateDate]; - [userProfile setProfileName:[profileName ows_stripped]]; [userProfile setAvatarUrlPath:avatarUrlPath]; [userProfile setAvatarFileName:avatarFileName]; @@ -180,7 +204,7 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)updateWithProfileName:(nullable NSString *)profileName @@ -189,23 +213,35 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - __block BOOL didChange = NO; + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didStringChange:self.profileName newValue:[profileName ows_stripped]]; + didChange |= [self didStringChange:self.avatarUrlPath newValue:avatarUrlPath]; + didChange |= [self didDateChange:self.lastUpdateDate newValue:lastUpdateDate]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= [self didStringChange:userProfile.profileName - newValue:[profileName ows_stripped]]; - didChange |= [self didStringChange:userProfile.avatarUrlPath newValue:avatarUrlPath]; - didChange |= [self didDateChange:userProfile.lastUpdateDate newValue:lastUpdateDate]; - [userProfile setProfileName:[profileName ows_stripped]]; [userProfile setAvatarUrlPath:avatarUrlPath]; [userProfile setLastUpdateDate:lastUpdateDate]; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)updateWithProfileName:(nullable NSString *)profileName @@ -215,18 +251,28 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - __block BOOL didChange = NO; + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didStringChange:self.profileName newValue:[profileName ows_stripped]]; + didChange |= [self didKeyChange:self.profileKey newValue:profileKey]; + didChange |= [self didStringChange:self.avatarUrlPath newValue:avatarUrlPath]; + didChange |= [self didStringChange:self.avatarFileName newValue:avatarFileName]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= [self didStringChange:userProfile.profileName - newValue:[profileName ows_stripped]]; - didChange |= [self didKeyChange:userProfile.profileKey newValue:profileKey]; - didChange |= - [self didStringChange:userProfile.avatarUrlPath newValue:avatarUrlPath]; - didChange |= - [self didStringChange:userProfile.avatarFileName newValue:avatarFileName]; - [userProfile setProfileName:[profileName ows_stripped]]; [userProfile setProfileKey:profileKey]; [userProfile setAvatarUrlPath:avatarUrlPath]; @@ -234,7 +280,7 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)updateWithAvatarUrlPath:(nullable NSString *)avatarUrlPath @@ -242,75 +288,124 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - __block BOOL didChange = NO; + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didStringChange:self.avatarUrlPath newValue:avatarUrlPath]; + didChange |= [self didStringChange:self.avatarFileName newValue:avatarFileName]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= - [self didStringChange:userProfile.avatarUrlPath newValue:avatarUrlPath]; - didChange |= - [self didStringChange:userProfile.avatarFileName newValue:avatarFileName]; [userProfile setAvatarUrlPath:avatarUrlPath]; [userProfile setAvatarFileName:avatarFileName]; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)updateWithAvatarFileName:(nullable NSString *)avatarFileName dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - __block BOOL didChange = NO; + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didStringChange:self.avatarFileName newValue:avatarFileName]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= - [self didStringChange:userProfile.avatarFileName newValue:avatarFileName]; - [userProfile setAvatarFileName:avatarFileName]; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)updateWithLastUpdateDate:(nullable NSDate *)lastUpdateDate dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - __block BOOL didChange = NO; + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didDateChange:self.lastUpdateDate newValue:lastUpdateDate]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= - [self didDateChange:userProfile.lastUpdateDate newValue:lastUpdateDate]; - [userProfile setLastUpdateDate:lastUpdateDate]; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)clearWithProfileKey:(OWSAES256Key *)profileKey dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion; { + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + OWSAssert(profileKey); - __block BOOL didChange = NO; + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didKeyChange:self.profileKey newValue:profileKey]; + didChange |= [self didStringChange:self.profileName newValue:nil]; + didChange |= [self didStringChange:self.avatarUrlPath newValue:nil]; + didChange |= [self didStringChange:self.avatarFileName newValue:nil]; + didChange |= [self didDateChange:self.lastUpdateDate newValue:nil]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } + [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= [self didKeyChange:userProfile.profileKey newValue:profileKey]; - didChange |= [self didStringChange:userProfile.profileName newValue:nil]; - didChange |= [self didStringChange:userProfile.avatarUrlPath newValue:nil]; - didChange |= [self didStringChange:userProfile.avatarFileName newValue:nil]; - didChange |= [self didDateChange:userProfile.lastUpdateDate newValue:nil]; - [userProfile setProfileKey:profileKey]; [userProfile setProfileName:nil]; [userProfile setAvatarUrlPath:nil]; @@ -319,28 +414,39 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (void)updateWithProfileKey:(OWSAES256Key *)profileKey dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion; { + DDLogVerbose(@"%@ %s, before: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + OWSAssert(profileKey); - self.profileKey = profileKey; + if (self.hasEverBeenSaved) { + BOOL didChange = NO; + + didChange |= [self didKeyChange:self.profileKey newValue:profileKey]; + + if (!didChange) { + DDLogVerbose(@"%@ Ignoring update in %s: %@", self.logTag, __PRETTY_FUNCTION__, self.debugDescription); + if (completion) { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), completion); + } + return; + } + } - __block BOOL didChange = NO; [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { [self applyChangeToSelfAndLatestCopy:transaction changeBlock:^(OWSUserProfile *userProfile) { - didChange |= [self didKeyChange:userProfile.profileKey newValue:profileKey]; - [userProfile setProfileKey:profileKey]; } saveIfMissing:YES]; }]; - [self finalizeWithCompletion:completion didChange:didChange]; + [self finalizeWithCompletion:completion]; } - (BOOL)didStringChange:(NSString *_Nullable)oldValue newValue:(NSString *_Nullable)newValue @@ -406,6 +512,19 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; return TSYapDatabaseObject.dbReadWriteConnection; } +- (NSString *)debugDescription +{ + return [NSString stringWithFormat:@"%@ %p %@ %@ %@ %@ %@ %f", + self.logTag, + self, + self.recipientId, + self.profileKey.keyData.hexadecimalString, + self.profileName, + self.avatarUrlPath, + self.avatarFileName, + self.lastUpdateDate.timeIntervalSinceNow]; +} + @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h index 728b7b38d..c516edd0b 100644 --- a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h +++ b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h @@ -13,6 +13,8 @@ NS_ASSUME_NONNULL_BEGIN @interface TSYapDatabaseObject : MTLModel +@property (atomic, readonly) BOOL hasEverBeenSaved; + /** * Initializes a new database object with a unique identifier * diff --git a/SignalServiceKit/src/Storage/TSYapDatabaseObject.m b/SignalServiceKit/src/Storage/TSYapDatabaseObject.m index 0b6d4e541..b189d8f3a 100644 --- a/SignalServiceKit/src/Storage/TSYapDatabaseObject.m +++ b/SignalServiceKit/src/Storage/TSYapDatabaseObject.m @@ -8,6 +8,14 @@ NS_ASSUME_NONNULL_BEGIN +@interface TSYapDatabaseObject () + +@property (atomic) BOOL hasEverBeenSaved; + +@end + +#pragma mark - + @implementation TSYapDatabaseObject - (instancetype)init @@ -29,12 +37,21 @@ NS_ASSUME_NONNULL_BEGIN - (nullable instancetype)initWithCoder:(NSCoder *)coder { - return [super initWithCoder:coder]; + self = [super initWithCoder:coder]; + if (!self) { + return self; + } + + self.hasEverBeenSaved = YES; + + return self; } - (void)saveWithTransaction:(YapDatabaseReadWriteTransaction *)transaction { [transaction setObject:self forKey:self.uniqueId inCollection:[[self class] collection]]; + + self.hasEverBeenSaved = YES; } - (void)save