From dfa082238e291f849a5e0f60dcc78450d8ac2492 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 2 Mar 2018 16:01:08 -0500 Subject: [PATCH 1/6] Fix profile avatar downloads. --- SignalMessaging/profiles/OWSProfileManager.m | 30 +++++++++++++------- SignalMessaging/profiles/OWSUserProfile.h | 6 +++- SignalMessaging/profiles/OWSUserProfile.m | 24 +++++++++++++--- 3 files changed, 44 insertions(+), 16 deletions(-) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index 90c280a52..47e91fcb4 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -785,12 +785,9 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; return nil; } -- (void)downloadAvatarForUserProfile:(OWSUserProfile *)userProfileParameter +- (void)downloadAvatarForUserProfile:(OWSUserProfile *)userProfile { - OWSAssert(userProfileParameter); - - // Make a local copy. - OWSUserProfile *userProfile = [userProfileParameter copy]; + OWSAssert(userProfile); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ if (userProfile.avatarUrlPath.length < 1) { @@ -915,14 +912,25 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; return; } + // If we're transitioning from (no avatar -> no avatar) or from (same avatar -> same avatar), + // don't bother updating the avatar. + BOOL canSkipAvatarUpdate = ((avatarUrlPath.length == 0 && userProfile.avatarUrlPath.length == 0 + && userProfile.avatarFileName.length == 0) + || (avatarUrlPath.length > 0 && userProfile.avatarUrlPath.length > 0 && + [avatarUrlPath isEqualToString:userProfile.avatarUrlPath] && userProfile.avatarFileName)); + NSString *_Nullable profileName = [self decryptProfileNameData:profileNameEncrypted profileKey:userProfile.profileKey]; - [userProfile updateWithProfileName:profileName - avatarUrlPath:avatarUrlPath - avatarFileName:userProfile.avatarFileName // use existing file name if already downloaded - dbConnection:self.dbConnection - completion:nil]; + if (canSkipAvatarUpdate) { + [userProfile updateWithProfileName:profileName dbConnection:self.dbConnection completion:nil]; + } else { + [userProfile updateWithProfileName:profileName + avatarUrlPath:avatarUrlPath + avatarFileName:nil + dbConnection:self.dbConnection + completion:nil]; + } // If we're updating the profile that corresponds to our local number, // update the local profile as well. @@ -942,7 +950,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; completion:nil]; } - if (userProfile.avatarUrlPath.length > 0 && userProfile.avatarFileName.length == 0) { + if (avatarUrlPath.length > 0) { [self downloadAvatarForUserProfile:userProfile]; } }); diff --git a/SignalMessaging/profiles/OWSUserProfile.h b/SignalMessaging/profiles/OWSUserProfile.h index c58e59e79..6499db695 100644 --- a/SignalMessaging/profiles/OWSUserProfile.h +++ b/SignalMessaging/profiles/OWSUserProfile.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // #import @@ -41,6 +41,10 @@ extern NSString *const kLocalProfileUniqueId; #pragma mark - Update With... Methods +- (void)updateWithProfileName:(nullable NSString *)profileName + dbConnection:(YapDatabaseConnection *)dbConnection + completion:(nullable OWSUserProfileCompletion)completion; + - (void)updateWithProfileName:(nullable NSString *)profileName avatarUrlPath:(nullable NSString *)avatarUrlPath avatarFileName:(nullable NSString *)avatarFileName diff --git a/SignalMessaging/profiles/OWSUserProfile.m b/SignalMessaging/profiles/OWSUserProfile.m index 5bcf35de0..0b229a93b 100644 --- a/SignalMessaging/profiles/OWSUserProfile.m +++ b/SignalMessaging/profiles/OWSUserProfile.m @@ -115,6 +115,8 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; didChange = [_avatarUrlPath isEqualToString:avatarUrlPath]; } + _avatarUrlPath = avatarUrlPath; + if (didChange) { // If the avatarURL changed, the avatarFileName can't be valid. // Clear it. @@ -137,8 +139,6 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { - NSDictionary *beforeSnapshot = self.dictionaryValue; - changeBlock(self); __block BOOL didChange = YES; @@ -146,12 +146,16 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; NSString *collection = [[self class] collection]; OWSUserProfile *_Nullable latestInstance = [transaction objectForKey:self.uniqueId inCollection:collection]; if (latestInstance) { + NSDictionary *beforeSnapshot = latestInstance.dictionaryValue; + changeBlock(latestInstance); NSDictionary *afterSnapshot = latestInstance.dictionaryValue; if ([beforeSnapshot isEqual:afterSnapshot]) { - DDLogVerbose( - @"%@ Ignoring redundant update in %s: %@", self.logTag, functionName, self.debugDescription); + DDLogVerbose(@"%@ Ignoring redundant update in %s: %@", + self.logTag, + functionName, + self.debugDescription); didChange = NO; } else { [latestInstance saveWithTransaction:transaction]; @@ -200,6 +204,18 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; }); } +- (void)updateWithProfileName:(nullable NSString *)profileName + dbConnection:(YapDatabaseConnection *)dbConnection + completion:(nullable OWSUserProfileCompletion)completion +{ + [self applyChanges:^(OWSUserProfile *userProfile) { + [userProfile setProfileName:[profileName ows_stripped]]; + } + functionName:__PRETTY_FUNCTION__ + dbConnection:dbConnection + completion:completion]; +} + - (void)updateWithProfileName:(nullable NSString *)profileName avatarUrlPath:(nullable NSString *)avatarUrlPath avatarFileName:(nullable NSString *)avatarFileName From 15921fa0b574646d5881dd9f9f13b0aa5a7cd928 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 2 Mar 2018 16:24:33 -0500 Subject: [PATCH 2/6] Fix profile avatar downloads. --- SignalMessaging/profiles/OWSProfileManager.m | 20 ++++---------------- SignalMessaging/profiles/OWSUserProfile.h | 4 ---- SignalMessaging/profiles/OWSUserProfile.m | 14 +------------- 3 files changed, 5 insertions(+), 33 deletions(-) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index 47e91fcb4..d2383ea5a 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -912,25 +912,13 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; return; } - // If we're transitioning from (no avatar -> no avatar) or from (same avatar -> same avatar), - // don't bother updating the avatar. - BOOL canSkipAvatarUpdate = ((avatarUrlPath.length == 0 && userProfile.avatarUrlPath.length == 0 - && userProfile.avatarFileName.length == 0) - || (avatarUrlPath.length > 0 && userProfile.avatarUrlPath.length > 0 && - [avatarUrlPath isEqualToString:userProfile.avatarUrlPath] && userProfile.avatarFileName)); - NSString *_Nullable profileName = [self decryptProfileNameData:profileNameEncrypted profileKey:userProfile.profileKey]; - if (canSkipAvatarUpdate) { - [userProfile updateWithProfileName:profileName dbConnection:self.dbConnection completion:nil]; - } else { - [userProfile updateWithProfileName:profileName - avatarUrlPath:avatarUrlPath - avatarFileName:nil - dbConnection:self.dbConnection - completion:nil]; - } + [userProfile updateWithProfileName:profileName + avatarUrlPath:avatarUrlPath + dbConnection:self.dbConnection + completion:nil]; // If we're updating the profile that corresponds to our local number, // update the local profile as well. diff --git a/SignalMessaging/profiles/OWSUserProfile.h b/SignalMessaging/profiles/OWSUserProfile.h index 6499db695..35402fff1 100644 --- a/SignalMessaging/profiles/OWSUserProfile.h +++ b/SignalMessaging/profiles/OWSUserProfile.h @@ -41,10 +41,6 @@ extern NSString *const kLocalProfileUniqueId; #pragma mark - Update With... Methods -- (void)updateWithProfileName:(nullable NSString *)profileName - dbConnection:(YapDatabaseConnection *)dbConnection - completion:(nullable OWSUserProfileCompletion)completion; - - (void)updateWithProfileName:(nullable NSString *)profileName avatarUrlPath:(nullable NSString *)avatarUrlPath avatarFileName:(nullable NSString *)avatarFileName diff --git a/SignalMessaging/profiles/OWSUserProfile.m b/SignalMessaging/profiles/OWSUserProfile.m index 0b229a93b..4017f3a4f 100644 --- a/SignalMessaging/profiles/OWSUserProfile.m +++ b/SignalMessaging/profiles/OWSUserProfile.m @@ -109,7 +109,7 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; BOOL didChange; if (_avatarUrlPath == nil && avatarUrlPath == nil) { didChange = NO; - } else if (_avatarUrlPath != nil && avatarUrlPath != nil) { + } else if (_avatarUrlPath != nil || avatarUrlPath != nil) { didChange = YES; } else { didChange = [_avatarUrlPath isEqualToString:avatarUrlPath]; @@ -204,18 +204,6 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; }); } -- (void)updateWithProfileName:(nullable NSString *)profileName - dbConnection:(YapDatabaseConnection *)dbConnection - completion:(nullable OWSUserProfileCompletion)completion -{ - [self applyChanges:^(OWSUserProfile *userProfile) { - [userProfile setProfileName:[profileName ows_stripped]]; - } - functionName:__PRETTY_FUNCTION__ - dbConnection:dbConnection - completion:completion]; -} - - (void)updateWithProfileName:(nullable NSString *)profileName avatarUrlPath:(nullable NSString *)avatarUrlPath avatarFileName:(nullable NSString *)avatarFileName From b62a43217f3eadfee8d61f5571d2ee5cbd8f9947 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 2 Mar 2018 16:31:44 -0500 Subject: [PATCH 3/6] Fix profile avatar downloads. --- SignalMessaging/profiles/OWSUserProfile.m | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/SignalMessaging/profiles/OWSUserProfile.m b/SignalMessaging/profiles/OWSUserProfile.m index 4017f3a4f..c0a55b95b 100644 --- a/SignalMessaging/profiles/OWSUserProfile.m +++ b/SignalMessaging/profiles/OWSUserProfile.m @@ -139,6 +139,10 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; dbConnection:(YapDatabaseConnection *)dbConnection completion:(nullable OWSUserProfileCompletion)completion { + // self might be the latest instance, so take a "before" snapshot + // before any changes have been made. + __block NSDictionary *beforeSnapshot = [self.dictionaryValue copy]; + changeBlock(self); __block BOOL didChange = YES; @@ -146,11 +150,16 @@ NSString *const kLocalProfileUniqueId = @"kLocalProfileUniqueId"; NSString *collection = [[self class] collection]; OWSUserProfile *_Nullable latestInstance = [transaction objectForKey:self.uniqueId inCollection:collection]; if (latestInstance) { - NSDictionary *beforeSnapshot = latestInstance.dictionaryValue; + // If self is NOT the latest instance, take a new "before" snapshot + // before updating. + if (self != latestInstance) { + beforeSnapshot = [latestInstance.dictionaryValue copy]; + } changeBlock(latestInstance); - NSDictionary *afterSnapshot = latestInstance.dictionaryValue; + NSDictionary *afterSnapshot = [latestInstance.dictionaryValue copy]; + if ([beforeSnapshot isEqual:afterSnapshot]) { DDLogVerbose(@"%@ Ignoring redundant update in %s: %@", self.logTag, From 69c49d4a7bf08a0e3aba2c3eac4a79f3fa816590 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 2 Mar 2018 16:33:38 -0500 Subject: [PATCH 4/6] Fix profile avatar downloads. --- SignalMessaging/profiles/OWSProfileManager.m | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index d2383ea5a..024c8d14b 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -912,6 +912,13 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; return; } + // If we're transitioning from (no avatar -> no avatar) or from (same avatar -> same avatar), + // don't bother updating the avatar. + BOOL canSkipAvatarUpdate = ((avatarUrlPath.length == 0 && userProfile.avatarUrlPath.length == 0 + && userProfile.avatarFileName.length == 0) + || (avatarUrlPath.length > 0 && userProfile.avatarUrlPath.length > 0 && + [avatarUrlPath isEqualToString:userProfile.avatarUrlPath] && userProfile.avatarFileName)); + NSString *_Nullable profileName = [self decryptProfileNameData:profileNameEncrypted profileKey:userProfile.profileKey]; @@ -938,7 +945,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; completion:nil]; } - if (avatarUrlPath.length > 0) { + if (avatarUrlPath.length > 0 && !canSkipAvatarUpdate) { [self downloadAvatarForUserProfile:userProfile]; } }); From 5e02032fc36696e3d59a17e20619c1f004729714 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 2 Mar 2018 16:38:46 -0500 Subject: [PATCH 5/6] Fix profile avatar downloads. --- SignalMessaging/profiles/OWSProfileManager.m | 2 ++ 1 file changed, 2 insertions(+) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index 024c8d14b..4a53d34d1 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -814,6 +814,8 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; [self.currentAvatarDownloads addObject:userProfile.recipientId]; } + DDLogVerbose(@"%@ downloading profile avatar: %@", self.logTag, userProfile.uniqueId); + NSString *tempDirectory = NSTemporaryDirectory(); NSString *tempFilePath = [tempDirectory stringByAppendingPathComponent:fileName]; From e89a0f815b0d4ba46458bb1aea15a126fc36e268 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 2 Mar 2018 17:33:36 -0500 Subject: [PATCH 6/6] Respond to CR. --- SignalMessaging/profiles/OWSProfileManager.m | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/SignalMessaging/profiles/OWSProfileManager.m b/SignalMessaging/profiles/OWSProfileManager.m index 4a53d34d1..9bcedf4da 100644 --- a/SignalMessaging/profiles/OWSProfileManager.m +++ b/SignalMessaging/profiles/OWSProfileManager.m @@ -914,13 +914,6 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; return; } - // If we're transitioning from (no avatar -> no avatar) or from (same avatar -> same avatar), - // don't bother updating the avatar. - BOOL canSkipAvatarUpdate = ((avatarUrlPath.length == 0 && userProfile.avatarUrlPath.length == 0 - && userProfile.avatarFileName.length == 0) - || (avatarUrlPath.length > 0 && userProfile.avatarUrlPath.length > 0 && - [avatarUrlPath isEqualToString:userProfile.avatarUrlPath] && userProfile.avatarFileName)); - NSString *_Nullable profileName = [self decryptProfileNameData:profileNameEncrypted profileKey:userProfile.profileKey]; @@ -936,18 +929,17 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; OWSUserProfile *localUserProfile = self.localUserProfile; OWSAssert(localUserProfile); - // Don't clear avatarFileName optimistically. - // * The profile avatar probably isn't out of sync. - // * If the profile avatar is out of sync, it can be synced on next app launch. - // * We don't want to touch local avatar state until we've - // downloaded the latest avatar by downloadAvatarForUserProfile. [localUserProfile updateWithProfileName:profileName avatarUrlPath:avatarUrlPath dbConnection:self.dbConnection completion:nil]; } - if (avatarUrlPath.length > 0 && !canSkipAvatarUpdate) { + // Whenever we change avatarUrlPath, OWSUserProfile clears avatarFileName. + // So if avatarUrlPath is set and avatarFileName is not set, we should to + // download this avatar. downloadAvatarForUserProfile will de-bounce + // downloads. + if (userProfile.avatarUrlPath.length > 0 && userProfile.avatarFileName.length < 1) { [self downloadAvatarForUserProfile:userProfile]; } });