From 81bc357bbbb2af194d27174b43a5b8378d67abf6 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 14 Dec 2018 10:15:26 -0700 Subject: [PATCH] more robust handling of unsaved outgoing messages --- .../ConversationViewController.m | 2 +- .../ConversationView/ConversationViewModel.h | 2 +- .../ConversationView/ConversationViewModel.m | 179 +++++++++--------- SignalMessaging/utils/ThreadUtil.m | 5 +- .../src/Network/MessageSenderJobQueue.swift | 1 - 5 files changed, 90 insertions(+), 99 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 6b70c1241..32e8d56ca 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -702,7 +702,7 @@ typedef enum : NSUInteger { - (NSArray> *)viewItems { - return self.conversationViewModel.allViewItems; + return self.conversationViewModel.viewItems; } - (ThreadDynamicInteractions *)dynamicInteractions diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h index 8143eddd6..bdbf8f530 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h @@ -80,7 +80,7 @@ typedef NS_ENUM(NSUInteger, ConversationUpdateItemType) { @interface ConversationViewModel : NSObject -@property (nonatomic, readonly) NSArray> *allViewItems; +@property (nonatomic, readonly) NSArray> *viewItems; @property (nonatomic, nullable) NSString *focusMessageIdOnOpen; @property (nonatomic, readonly, nullable) ThreadDynamicInteractions *dynamicInteractions; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m index 777937932..4dac9732c 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m @@ -24,6 +24,7 @@ #import #import #import +#import NS_ASSUME_NONNULL_BEGIN @@ -154,7 +155,7 @@ static const int kYapDatabaseRangeMinLength = 0; // * If we do the third step, we must call resetContentAndLayout afterward. @property (nonatomic) YapDatabaseViewMappings *messageMappings; -@property (nonatomic) NSArray> *allViewItems; +@property (nonatomic) NSArray> *viewItems; @property (nonatomic) NSMutableDictionary> *viewItemCache; @property (nonatomic) NSUInteger lastRangeLength; @@ -165,7 +166,9 @@ static const int kYapDatabaseRangeMinLength = 0; @property (nonatomic, nullable) ConversationProfileState *conversationProfileState; @property (nonatomic) BOOL hasTooManyOutgoingMessagesToBlockCached; + @property (nonatomic) NSArray> *persistedViewItems; +@property (nonatomic) NSArray *unsavedOutgoingMessages; @end @@ -187,6 +190,8 @@ static const int kYapDatabaseRangeMinLength = 0; _thread = thread; _delegate = delegate; + _persistedViewItems = @[]; + _unsavedOutgoingMessages = @[]; self.focusMessageIdOnOpen = focusMessageIdOnOpen; [self configure]; @@ -490,7 +495,7 @@ static const int kYapDatabaseRangeMinLength = 0; - (nullable id)viewItemForUnreadMessagesIndicator { - for (id viewItem in self.allViewItems) { + for (id viewItem in self.viewItems) { if (viewItem.unreadIndicator) { return viewItem; } @@ -596,8 +601,38 @@ static const int kYapDatabaseRangeMinLength = 0; return [self.delegate conversationViewModelDidUpdate:ConversationUpdate.minorUpdate]; } + for (TSOutgoingMessage *unsavedOutgoingMessage in self.unsavedOutgoingMessages) { + NSUInteger index = [rowChanges indexOfObjectPassingTest:^BOOL( + YapDatabaseViewRowChange *_Nonnull rowChange, NSUInteger idx, BOOL *_Nonnull stop) { + return [rowChange.collectionKey.key isEqualToString:unsavedOutgoingMessage.uniqueId]; + }]; + + if (index != NSNotFound) { + // Replace the "Insert" RowChange to be an "Update" RowChange. + YapDatabaseViewRowChange *rowChange = rowChanges[index]; + OWSAssertDebug(rowChange); + + OWSLogVerbose(@"unsaved item has since been saved. collection key: %@", rowChange.collectionKey.key); + + YapDatabaseViewRowChange *update = + [YapDatabaseViewRowChange updateCollectionKey:rowChange.collectionKey + inGroup:rowChange.originalGroup + atIndex:rowChange.finalIndex + withChanges:YapDatabaseViewChangedObject]; + + NSMutableArray *mutableRowChanges = [rowChanges mutableCopy]; + mutableRowChanges[index] = update; + rowChanges = [mutableRowChanges copy]; + + // Remove the unsavedOutgoingViewItem since it now exists as a persistedViewItem + NSMutableArray *unsavedOutgoingMessages = [self.unsavedOutgoingMessages mutableCopy]; + [unsavedOutgoingMessages removeObject:unsavedOutgoingMessage]; + self.unsavedOutgoingMessages = [unsavedOutgoingMessages copy]; + } + } + NSMutableArray *oldItemIdList = [NSMutableArray new]; - for (id viewItem in self.allViewItems) { + for (id viewItem in self.viewItems) { [oldItemIdList addObject:viewItem.itemId]; } @@ -615,7 +650,7 @@ static const int kYapDatabaseRangeMinLength = 0; [self reloadInteractionForViewItem:viewItem]; [updatedItemSet addObject:viewItem.itemId]; } else { - OWSLogError(@"Update is missing view item"); + OWSFailDebug(@"Update is missing view item"); hasMalformedRowChange = YES; } } else { @@ -660,7 +695,7 @@ static const int kYapDatabaseRangeMinLength = 0; return; } - OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.allViewItems.count); + OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewItems.count); [self updateViewWithOldItemIdList:oldItemIdList updatedItemSet:updatedItemSet]; } @@ -674,7 +709,7 @@ static const int kYapDatabaseRangeMinLength = 0; OWSLogVerbose(@""); NSMutableArray *oldItemIdList = [NSMutableArray new]; - for (id viewItem in self.allViewItems) { + for (id viewItem in self.viewItems) { [oldItemIdList addObject:viewItem.itemId]; } @@ -686,7 +721,7 @@ static const int kYapDatabaseRangeMinLength = 0; return; } - OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.allViewItems.count); + OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewItems.count); [self updateViewWithOldItemIdList:oldItemIdList updatedItemSet:[NSSet set]]; } @@ -711,7 +746,7 @@ static const int kYapDatabaseRangeMinLength = 0; NSMutableArray *newItemIdList = [NSMutableArray new]; NSMutableDictionary> *newViewItemMap = [NSMutableDictionary new]; - for (id viewItem in self.allViewItems) { + for (id viewItem in self.viewItems) { [newItemIdList addObject:viewItem.itemId]; newViewItemMap[viewItem.itemId] = viewItem; } @@ -1151,32 +1186,6 @@ static const int kYapDatabaseRangeMinLength = 0; return offersMessage; } - -- (id)addViewItemWithInteraction:(TSInteraction *)interaction - conversationStyle:(ConversationStyle *)conversationStyle - isGroupThread:(BOOL)isGroupThread - transaction:(YapDatabaseReadTransaction *)transaction - viewItemCache: - (NSMutableDictionary> *)viewItemCache - toViewItems:(NSMutableArray> *)viewItems -{ - OWSAssertDebug(interaction.uniqueId.length > 0); - - id _Nullable viewItem = self.viewItemCache[interaction.uniqueId]; - if (!viewItem) { - viewItem = [[ConversationInteractionViewItem alloc] initWithInteraction:interaction - isGroupThread:isGroupThread - transaction:transaction - conversationStyle:conversationStyle]; - } - [viewItems addObject:viewItem]; - OWSAssertDebug( - !viewItemCache[interaction.uniqueId] || [interaction isKindOfClass:[OWSTypingIndicatorInteraction class]]); - viewItemCache[interaction.uniqueId] = viewItem; - - return viewItem; -} - // This is a key method. It builds or rebuilds the list of // cell view models. // @@ -1195,12 +1204,20 @@ static const int kYapDatabaseRangeMinLength = 0; __block BOOL hasError = NO; id (^tryToAddViewItem)(TSInteraction *, YapDatabaseReadTransaction *) = ^(TSInteraction *interaction, YapDatabaseReadTransaction *transaction) { - return [self addViewItemWithInteraction:interaction - conversationStyle:conversationStyle - isGroupThread:isGroupThread - transaction:transaction - viewItemCache:viewItemCache - toViewItems:viewItems]; + OWSAssertDebug(interaction.uniqueId.length > 0); + + id _Nullable viewItem = self.viewItemCache[interaction.uniqueId]; + if (!viewItem) { + viewItem = [[ConversationInteractionViewItem alloc] initWithInteraction:interaction + isGroupThread:isGroupThread + transaction:transaction + conversationStyle:conversationStyle]; + } + [viewItems addObject:viewItem]; + OWSAssertDebug(!viewItemCache[interaction.uniqueId]); + viewItemCache[interaction.uniqueId] = viewItem; + + return viewItem; }; [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { @@ -1263,41 +1280,29 @@ static const int kYapDatabaseRangeMinLength = 0; return [left.interaction compareForSorting:right.interaction]; }]; - // Flag to ensure that we only increment once per launch. - if (hasError) { - OWSLogWarn(@"incrementing version of: %@", TSMessageDatabaseViewExtensionName); - [OWSPrimaryStorage incrementVersionOfDatabaseExtension:TSMessageDatabaseViewExtensionName]; + if (self.unsavedOutgoingMessages.count > 0) { + [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + for (TSOutgoingMessage *outgoingMessage in self.unsavedOutgoingMessages) { + tryToAddViewItem(outgoingMessage, transaction); + } + }]; } - self.viewItemCache = viewItemCache; - self.persistedViewItems = [viewItems copy]; - self.allViewItems = [self prepareViewItemsForDisplay:viewItems]; - - return !hasError; -} - -- (NSArray> *)prepareViewItemsForDisplay:(NSArray> *)viewItemsArg -{ - NSArray> *viewItems = viewItemsArg; if (self.typingIndicatorsSender) { - NSMutableArray> *mutableViewItems = [viewItems mutableCopy]; - [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { - id _Nullable lastViewItem = viewItems.lastObject; - uint64_t typingIndicatorTimestamp = (lastViewItem ? lastViewItem.interaction.timestamp + 1 : 1); - TSInteraction *interaction = - [[OWSTypingIndicatorInteraction alloc] initWithThread:self.thread - timestamp:typingIndicatorTimestamp - recipientId:self.typingIndicatorsSender]; - - [self addViewItemWithInteraction:interaction - conversationStyle:self.delegate.conversationStyle - isGroupThread:self.thread.isGroupThread - transaction:transaction - viewItemCache:self.viewItemCache - toViewItems:mutableViewItems]; + OWSTypingIndicatorInteraction *typingIndicatorInteraction = + [[OWSTypingIndicatorInteraction alloc] initWithThread:self.thread + timestamp:[NSDate ows_millisecondTimeStamp] + recipientId:self.typingIndicatorsSender]; + [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + tryToAddViewItem(typingIndicatorInteraction, transaction); }]; - viewItems = [mutableViewItems copy]; + } + + // Flag to ensure that we only increment once per launch. + if (hasError) { + OWSLogWarn(@"incrementing version of: %@", TSMessageDatabaseViewExtensionName); + [OWSPrimaryStorage incrementVersionOfDatabaseExtension:TSMessageDatabaseViewExtensionName]; } // Update the "break" properties (shouldShowDate and unreadIndicator) of the view items. @@ -1528,7 +1533,10 @@ static const int kYapDatabaseRangeMinLength = 0; viewItem.senderName = senderName; } - return viewItems; + self.viewItems = viewItems; + self.viewItemCache = viewItemCache; + + return !hasError; } - (void)appendUnsavedOutgoingTextMessage:(TSOutgoingMessage *)outgoingMessage @@ -1538,28 +1546,11 @@ static const int kYapDatabaseRangeMinLength = 0; OWSAssertDebug(outgoingMessage.attachmentIds.count == 0); OWSAssertDebug(outgoingMessage.contactShare == nil); - __block ConversationInteractionViewItem *viewItem; - [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction * _Nonnull transaction) { - viewItem = - [[ConversationInteractionViewItem alloc] initWithInteraction:outgoingMessage - isGroupThread:self.thread.isGroupThread - transaction:transaction - conversationStyle:self.delegate.conversationStyle]; - }]; + NSMutableArray *unsavedOutgoingMessages = [self.unsavedOutgoingMessages mutableCopy]; + [unsavedOutgoingMessages addObject:outgoingMessage]; + self.unsavedOutgoingMessages = unsavedOutgoingMessages; - ConversationUpdateItem *updateItem = - [[ConversationUpdateItem alloc] initWithUpdateItemType:ConversationUpdateItemType_Insert - oldIndex:0 - newIndex:self.persistedViewItems.count - viewItem:viewItem]; - - NSMutableArray> *viewItems = [self.persistedViewItems mutableCopy]; - [viewItems addObject:viewItem]; - self.allViewItems = [self prepareViewItemsForDisplay:viewItems]; - ConversationUpdate *conversationUpdate = [[ConversationUpdate alloc] initWithConversationUpdateType:ConversationUpdateType_Diff - updateItems:@[updateItem] - shouldAnimateUpdates:NO]; - [self.delegate conversationViewModelDidUpdate:conversationUpdate]; + [self updateForTransientItems]; } // Whenever an interaction is modified, we need to reload it from the DB @@ -1705,7 +1696,7 @@ static const int kYapDatabaseRangeMinLength = 0; OWSFailDebug(@"Could not locate view item for quoted reply."); return nil; } - NSUInteger viewItemIndex = [self.allViewItems indexOfObject:viewItem]; + NSUInteger viewItemIndex = [self.viewItems indexOfObject:viewItem]; if (viewItemIndex == NSNotFound) { OWSFailDebug(@"Could not locate view item index for quoted reply."); return nil; @@ -1757,7 +1748,7 @@ static const int kYapDatabaseRangeMinLength = 0; // Update the view items if necessary. // We don't have to do this if they haven't been configured yet. - if (didChange && self.allViewItems != nil) { + if (didChange && self.viewItems != nil) { [self updateForTransientItems]; } } diff --git a/SignalMessaging/utils/ThreadUtil.m b/SignalMessaging/utils/ThreadUtil.m index 87bff957e..f56bf438a 100644 --- a/SignalMessaging/utils/ThreadUtil.m +++ b/SignalMessaging/utils/ThreadUtil.m @@ -82,12 +82,13 @@ NS_ASSUME_NONNULL_BEGIN expiresInSeconds:expiresInSeconds quotedMessage:[quotedReplyModel buildQuotedMessageForSending]]; - [BenchManager benchWithTitle:@"Saving outgoing message" block:^{ + [BenchManager benchAsyncWithTitle:@"Saving outgoing message" block:^(void (^benchmarkCompletion)(void)) { // To avoid blocking the send flow, we disapatch an async write from within this read transaction [self.dbConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction * _Nonnull writeTransaction) { [message saveWithTransaction:writeTransaction]; [self.messageSenderJobQueue addMessage:message transaction:writeTransaction]; - }]; + } + completionBlock:benchmarkCompletion]; }]; return message; diff --git a/SignalServiceKit/src/Network/MessageSenderJobQueue.swift b/SignalServiceKit/src/Network/MessageSenderJobQueue.swift index 6f93f05eb..ea121ca0c 100644 --- a/SignalServiceKit/src/Network/MessageSenderJobQueue.swift +++ b/SignalServiceKit/src/Network/MessageSenderJobQueue.swift @@ -38,7 +38,6 @@ public class MessageSenderJobQueue: NSObject, JobQueue { @objc(addMessage:transaction:) public func add(message: TSOutgoingMessage, transaction: YapDatabaseReadWriteTransaction) { - message.save(with: transaction) self.add(message: message, removeMessageAfterSending: false, transaction: transaction) }