From 65015e686fa6b9d2c2a566a719b5608757f352c1 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 9 Apr 2018 13:58:12 -0400 Subject: [PATCH 1/5] Tap on quoted replies. --- .../Cells/ConversationViewCell.h | 2 + .../ConversationView/Cells/OWSMessageCell.m | 6 +- .../ConversationViewController.m | 170 ++++++++++++++++-- .../src/Messages/Interactions/TSInteraction.h | 2 +- .../src/Messages/Interactions/TSInteraction.m | 4 +- .../src/Storage/TSDatabaseSecondaryIndexes.h | 4 +- .../src/Storage/TSDatabaseSecondaryIndexes.m | 6 +- 7 files changed, 175 insertions(+), 19 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/ConversationViewCell.h b/Signal/src/ViewControllers/ConversationView/Cells/ConversationViewCell.h index e4f5baec3..3172388f2 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/ConversationViewCell.h +++ b/Signal/src/ViewControllers/ConversationView/Cells/ConversationViewCell.h @@ -12,6 +12,7 @@ NS_ASSUME_NONNULL_BEGIN @class TSInteraction; @class TSMessage; @class TSOutgoingMessage; +@class TSQuotedMessage; @protocol ConversationViewCellDelegate @@ -26,6 +27,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)didTapFailedIncomingAttachment:(ConversationViewItem *)viewItem attachmentPointer:(TSAttachmentPointer *)attachmentPointer; - (void)didTapFailedOutgoingMessage:(TSOutgoingMessage *)message; +- (void)didTapQuotedMessage:(ConversationViewItem *)viewItem quotedMessage:(TSQuotedMessage *)quotedMessage; - (void)didPanWithGestureRecognizer:(UIPanGestureRecognizer *)gestureRecognizer viewItem:(ConversationViewItem *)conversationItem; diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m index 5e807b652..3a950c471 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m @@ -515,7 +515,11 @@ NS_ASSUME_NONNULL_BEGIN [self handleMediaTapGesture]; break; case OWSMessageGestureLocation_QuotedReply: - // TODO: + if (self.message.quotedMessage) { + [self.delegate didTapQuotedMessage:self.viewItem quotedMessage:self.message.quotedMessage]; + } else { + OWSFail(@"%@ Missing quoted message.", self.logTag) + } break; } } diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index ef1de1c83..b83df414d 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -90,6 +90,7 @@ #import #import #import +#import #import #import @@ -1511,32 +1512,37 @@ typedef enum : NSUInteger { } static const CGFloat kThreshold = 50.f; if (self.collectionView.contentOffset.y < kThreshold) { - [self loadMoreMessages]; + [self loadAnotherPageOfMessages]; } } -- (void)loadMoreMessages +- (void)loadAnotherPageOfMessages { BOOL hasEarlierUnseenMessages = self.dynamicInteractions.hasMoreUnseenMessages; + [self loadNMoreMessages:kYapDatabasePageSize]; + + // Don’t auto-scroll after “loading more messages” unless we have “more unseen messages”. + // + // Otherwise, tapping on "load more messages" autoscrolls you downward which is completely wrong. + if (hasEarlierUnseenMessages) { + [self scrollToUnreadIndicatorAnimated]; + } +} + +- (void)loadNMoreMessages:(NSUInteger)numberOfMessagesToLoad +{ // We want to restore the current scroll state after we update the range, update // the dynamic interactions and re-layout. Here we take a "before" snapshot. CGFloat scrollDistanceToBottom = self.safeContentHeight - self.collectionView.contentOffset.y; - self.lastRangeLength = MIN(self.lastRangeLength + kYapDatabasePageSize, (NSUInteger) kYapDatabaseRangeMaxLength); + self.lastRangeLength = MIN(self.lastRangeLength + numberOfMessagesToLoad, (NSUInteger)kYapDatabaseRangeMaxLength); [self resetMappings]; [self.layout prepareLayout]; self.collectionView.contentOffset = CGPointMake(0, self.safeContentHeight - scrollDistanceToBottom); - - // Don’t auto-scroll after “loading more messages” unless we have “more unseen messages”. - // - // Otherwise, tapping on "load more messages" autoscrolls you downward which is completely wrong. - if (hasEarlierUnseenMessages) { - [self scrollToUnreadIndicatorAnimated]; - } } - (void)updateShowLoadMoreHeader @@ -2124,6 +2130,150 @@ typedef enum : NSUInteger { [self handleUnsentMessageTap:message]; } +- (void)didTapQuotedMessage:(ConversationViewItem *)viewItem quotedMessage:(TSQuotedMessage *)quotedMessage +{ + OWSAssertIsOnMainThread(); + OWSAssert(viewItem); + OWSAssert(quotedMessage); + OWSAssert(quotedMessage.timestamp > 0); + OWSAssert(quotedMessage.authorId.length > 0); + + // We try to find the "quoted interaction" AND + // the range within the mapping that includes it. + __block TSInteraction *_Nullable quotedInteraction = nil; + // NOTE: Since the range _IS NOT_ filtered by author, + // and timestamp collisions are possible, it's possible + // for: + // + // * The range to include more than the "quoted interaction". + // * The range to be non-empty but NOT include the "quoted interaction", + // although this would be a bug. + __block NSRange itemRange; + itemRange.location = NSNotFound; + __block NSUInteger threadInteractionCount = 0; + + [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { + quotedInteraction = [self findInteractionInThreadByTimestamp:quotedMessage.timestamp + authorId:quotedMessage.authorId + transaction:transaction]; + if (!quotedInteraction) { + return; + } + + YapDatabaseAutoViewTransaction *_Nullable extension = + [transaction extension:TSMessageDatabaseViewExtensionName]; + if (!extension) { + OWSFail(@"%@ Couldn't load view.", self.logTag); + return; + } + + threadInteractionCount = [extension numberOfItemsInGroup:self.thread.uniqueId]; + + // See comments on YapDatabaseViewFind. + // + // Essentially we define a comparator/sort on timestamp. + YapDatabaseViewFind *viewFind = + [YapDatabaseViewFind withObjectBlock:^NSComparisonResult(NSString *collection, NSString *key, id object) { + if (![object isKindOfClass:[TSInteraction class]]) { + OWSFail(@"%@ Unexpected type in database view.", self.logTag); + return NSOrderedSame; + } + + TSInteraction *interaction = object; + + // For the findBlock, the "left operand" is the row that is passed, + // and the "right operand" is the desired range. + if (interaction.timestamp == quotedMessage.timestamp) { + + return NSOrderedSame; + } else if (interaction.timestamp < quotedMessage.timestamp) { + // NSOrderedAscending : The left operand is smaller than the right operand. + return NSOrderedAscending; + } else { + // NSOrderedDescending : The left operand is greater than the right operand. + return NSOrderedDescending; + } + }]; + itemRange = [extension findRangeInGroup:self.thread.uniqueId using:viewFind]; + }]; + + if (itemRange.location == NSNotFound) { + OWSFail(@"%@ Couldn't find range of quoted reply.", self.logTag); + return; + } + + NSInteger desiredWindowSize = MAX(0, 1 + (NSInteger)threadInteractionCount - (NSInteger)itemRange.location); + NSUInteger oldLoadWindowSize = [self.messageMappings numberOfItemsInGroup:self.thread.uniqueId]; + NSInteger additionalItemsToLoad = MAX(0, desiredWindowSize - (NSInteger)oldLoadWindowSize); + + NSInteger dstIndex = 0; + if (additionalItemsToLoad > 0) { + // Try to load more messages so that the quoted messages + // is in the load window. + // + // This may fail if the quoted message is very old, in which + // case we'll load the max number of messages. + [self loadNMoreMessages:(NSUInteger)additionalItemsToLoad]; + // Scroll to the first item, which should be the quoted message, + // or if we couldn't load it, the oldest loadable message. + dstIndex = 0; + } else { + // Scroll to the quoted message, which is already in the load window. + dstIndex = 1 + (NSInteger)oldLoadWindowSize - desiredWindowSize; + } + + NSIndexPath *indexPath = [NSIndexPath indexPathForRow:dstIndex inSection:0]; + [self.collectionView scrollToItemAtIndexPath:indexPath + atScrollPosition:UICollectionViewScrollPositionTop + animated:YES]; + + // TODO: Highlight the quoted message? +} + +- (nullable TSInteraction *)findInteractionInThreadByTimestamp:(uint64_t)timestamp + authorId:(NSString *)authorId + transaction:(YapDatabaseReadTransaction *)transaction +{ + OWSAssert(timestamp > 0); + OWSAssert(authorId.length > 0); + + NSString *localNumber = [TSAccountManager localNumber]; + if (localNumber.length < 1) { + return nil; + } + + NSArray *interactions = + [TSInteraction interactionsWithTimestamp:timestamp ofClass:[TSMessage class] withTransaction:transaction]; + + TSInteraction *_Nullable result = nil; + for (TSInteraction *interaction in interactions) { + NSString *_Nullable messageAuthorId = nil; + if ([interaction isKindOfClass:[TSIncomingMessage class]]) { + TSIncomingMessage *incomingMessage = (TSIncomingMessage *)interaction; + messageAuthorId = incomingMessage.authorId; + } else if ([interaction isKindOfClass:[TSOutgoingMessage class]]) { + messageAuthorId = localNumber; + } + if (messageAuthorId.length < 1) { + OWSFail(@"%@ Message missing author id.", self.logTag); + continue; + } + if (![authorId isEqualToString:messageAuthorId]) { + continue; + } + if (![interaction.uniqueThreadId isEqualToString:self.thread.uniqueId]) { + continue; + } + if (result) { + // In case of collision, take the first. + DDLogError(@"%@ more than one matching interaction in thread.", self.logTag); + continue; + } + result = interaction; + } + return result; +} + - (void)showMetadataViewForViewItem:(ConversationViewItem *)conversationItem { OWSAssertIsOnMainThread(); diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h index 09e28fa5b..1ea070928 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h @@ -41,7 +41,7 @@ typedef NS_ENUM(NSInteger, OWSInteractionType) { + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp ofClass:(Class)clazz - withTransaction:(YapDatabaseReadWriteTransaction *)transaction; + withTransaction:(YapDatabaseReadTransaction *)transaction; + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp filter:(BOOL (^_Nonnull)(TSInteraction *))filter diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m index 7bfa746eb..b5319d9d0 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m @@ -14,7 +14,7 @@ NS_ASSUME_NONNULL_BEGIN + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp ofClass:(Class)clazz - withTransaction:(YapDatabaseReadWriteTransaction *)transaction + withTransaction:(YapDatabaseReadTransaction *)transaction { OWSAssert(timestamp > 0); @@ -28,7 +28,7 @@ NS_ASSUME_NONNULL_BEGIN + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp filter:(BOOL (^_Nonnull)(TSInteraction *))filter - withTransaction:(YapDatabaseReadWriteTransaction *)transaction + withTransaction:(YapDatabaseReadTransaction *)transaction { OWSAssert(timestamp > 0); diff --git a/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.h b/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.h index 6d3b4b828..5db2ac8cb 100644 --- a/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.h +++ b/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.h @@ -1,5 +1,5 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // #import @@ -11,6 +11,6 @@ + (void)enumerateMessagesWithTimestamp:(uint64_t)timestamp withBlock:(void (^)(NSString *collection, NSString *key, BOOL *stop))block - usingTransaction:(YapDatabaseReadWriteTransaction *)transaction; + usingTransaction:(YapDatabaseReadTransaction *)transaction; @end diff --git a/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.m b/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.m index f63e63366..f61055edd 100644 --- a/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.m +++ b/SignalServiceKit/src/Storage/TSDatabaseSecondaryIndexes.m @@ -1,9 +1,8 @@ // -// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// Copyright (c) 2018 Open Whisper Systems. All rights reserved. // #import "TSDatabaseSecondaryIndexes.h" - #import "TSInteraction.h" #define TSTimeStampSQLiteIndex @"messagesTimeStamp" @@ -34,7 +33,8 @@ + (void)enumerateMessagesWithTimestamp:(uint64_t)timestamp withBlock:(void (^)(NSString *collection, NSString *key, BOOL *stop))block - usingTransaction:(YapDatabaseReadWriteTransaction *)transaction { + usingTransaction:(YapDatabaseReadTransaction *)transaction +{ NSString *formattedString = [NSString stringWithFormat:@"WHERE %@ = %lld", TSTimeStampSQLiteIndex, timestamp]; YapDatabaseQuery *query = [YapDatabaseQuery queryWithFormat:formattedString]; [[transaction ext:@"idx"] enumerateKeysMatchingQuery:query usingBlock:block]; From 8b060a187ce958c9f38c0e6ebc79719ad9309d03 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 9 Apr 2018 14:22:36 -0400 Subject: [PATCH 2/5] Tap on quoted replies. --- .../ConversationViewController.m | 130 +++++++++++++----- 1 file changed, 93 insertions(+), 37 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index b83df414d..572dc31a3 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -2138,9 +2138,96 @@ typedef enum : NSUInteger { OWSAssert(quotedMessage.timestamp > 0); OWSAssert(quotedMessage.authorId.length > 0); + // We try to range of items within the current thread's interactions + // that includs the "quoted interaction". + // NOTE: Since the range _IS NOT_ filtered by author, + // and timestamp collisions are possible, it's possible + // for: + // + // * The range to include more than the "quoted interaction". + // * The range to be non-empty but NOT include the "quoted interaction", + // although this would be a bug. + NSUInteger threadInteractionCount = 0; + NSRange itemRange = [self findRangeOfQuotedMessage:quotedMessage threadInteractionCount:&threadInteractionCount]; + + if (itemRange.location == NSNotFound) { + OWSFail(@"%@ Couldn't find range of quoted reply.", self.logTag); + return; + } + + NSInteger desiredWindowSize = MAX(0, 1 + (NSInteger)threadInteractionCount - (NSInteger)itemRange.location); + NSUInteger oldLoadWindowSize = [self.messageMappings numberOfItemsInGroup:self.thread.uniqueId]; + NSInteger additionalItemsToLoad = MAX(0, desiredWindowSize - (NSInteger)oldLoadWindowSize); + + NSInteger dstIndex = 0; + if (additionalItemsToLoad > 0) { + // Try to load more messages so that the quoted message + // is in the load window. + // + // This may fail if the quoted message is very old, in which + // case we'll load the max number of messages. + [self loadNMoreMessages:(NSUInteger)additionalItemsToLoad]; + + // Scroll to the first item, which should be the quoted message, + // or if we couldn't load it, the oldest loadable message. + // + // We use `indexPathRowOfQuotedMessage` instead of doing + // some index math because: + // + // * Resetting the mapping may integrate other pending changes. + // * Dynamic interactions may change. + dstIndex = (NSInteger)[self indexPathRowOfQuotedMessage:quotedMessage]; + } else { + // Scroll to the quoted message, which is already in the load window. + dstIndex = 1 + (NSInteger)oldLoadWindowSize - desiredWindowSize; + } + + NSIndexPath *indexPath = [NSIndexPath indexPathForRow:dstIndex inSection:0]; + [self.collectionView scrollToItemAtIndexPath:indexPath + atScrollPosition:UICollectionViewScrollPositionTop + animated:YES]; + + // TODO: Highlight the quoted message? +} + +// This method makes a best effort to determine the best scroll position +// after loading more interactions when trying to scroll to a quoted +// message. +// +// It prefers to be simple than to correctly handle edge cases like multiple +// interactions with the same timestamp. +- (NSUInteger)indexPathRowOfQuotedMessage:(TSQuotedMessage *)quotedMessage +{ + OWSAssertIsOnMainThread(); + OWSAssert(quotedMessage); + OWSAssert(quotedMessage.timestamp > 0); + OWSAssert(quotedMessage.authorId.length > 0); + + NSUInteger result = 0; + for (NSUInteger row = 0; row < self.viewItems.count; row++) { + ConversationViewItem *viewItem = self.viewItems[row]; + if (viewItem.interaction.timestamp > quotedMessage.timestamp) { + break; + } + result = row; + } + return result; +} + +// If result range is valid, then threadInteractionCount will have a valid value too. +- (NSRange)findRangeOfQuotedMessage:(TSQuotedMessage *)quotedMessage + threadInteractionCount:(NSUInteger *)threadInteractionCount +{ + OWSAssertIsOnMainThread(); + OWSAssert(quotedMessage); + OWSAssert(quotedMessage.timestamp > 0); + OWSAssert(quotedMessage.authorId.length > 0); + OWSAssert(threadInteractionCount); + + *threadInteractionCount = 0; + // We try to find the "quoted interaction" AND // the range within the mapping that includes it. - __block TSInteraction *_Nullable quotedInteraction = nil; // NOTE: Since the range _IS NOT_ filtered by author, // and timestamp collisions are possible, it's possible // for: @@ -2150,12 +2237,11 @@ typedef enum : NSUInteger { // although this would be a bug. __block NSRange itemRange; itemRange.location = NSNotFound; - __block NSUInteger threadInteractionCount = 0; [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { - quotedInteraction = [self findInteractionInThreadByTimestamp:quotedMessage.timestamp - authorId:quotedMessage.authorId - transaction:transaction]; + TSInteraction *_Nullable quotedInteraction = [self findInteractionInThreadByTimestamp:quotedMessage.timestamp + authorId:quotedMessage.authorId + transaction:transaction]; if (!quotedInteraction) { return; } @@ -2167,7 +2253,7 @@ typedef enum : NSUInteger { return; } - threadInteractionCount = [extension numberOfItemsInGroup:self.thread.uniqueId]; + *threadInteractionCount = [extension numberOfItemsInGroup:self.thread.uniqueId]; // See comments on YapDatabaseViewFind. // @@ -2197,37 +2283,7 @@ typedef enum : NSUInteger { itemRange = [extension findRangeInGroup:self.thread.uniqueId using:viewFind]; }]; - if (itemRange.location == NSNotFound) { - OWSFail(@"%@ Couldn't find range of quoted reply.", self.logTag); - return; - } - - NSInteger desiredWindowSize = MAX(0, 1 + (NSInteger)threadInteractionCount - (NSInteger)itemRange.location); - NSUInteger oldLoadWindowSize = [self.messageMappings numberOfItemsInGroup:self.thread.uniqueId]; - NSInteger additionalItemsToLoad = MAX(0, desiredWindowSize - (NSInteger)oldLoadWindowSize); - - NSInteger dstIndex = 0; - if (additionalItemsToLoad > 0) { - // Try to load more messages so that the quoted messages - // is in the load window. - // - // This may fail if the quoted message is very old, in which - // case we'll load the max number of messages. - [self loadNMoreMessages:(NSUInteger)additionalItemsToLoad]; - // Scroll to the first item, which should be the quoted message, - // or if we couldn't load it, the oldest loadable message. - dstIndex = 0; - } else { - // Scroll to the quoted message, which is already in the load window. - dstIndex = 1 + (NSInteger)oldLoadWindowSize - desiredWindowSize; - } - - NSIndexPath *indexPath = [NSIndexPath indexPathForRow:dstIndex inSection:0]; - [self.collectionView scrollToItemAtIndexPath:indexPath - atScrollPosition:UICollectionViewScrollPositionTop - animated:YES]; - - // TODO: Highlight the quoted message? + return itemRange; } - (nullable TSInteraction *)findInteractionInThreadByTimestamp:(uint64_t)timestamp From f2b416d80042bb2f1f675977160059c4dec51643 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 10 Apr 2018 09:45:58 -0400 Subject: [PATCH 3/5] Respond to CR. --- .../ConversationViewController.m | 240 +++++++----------- SignalMessaging/utils/ThreadUtil.h | 8 + SignalMessaging/utils/ThreadUtil.m | 46 ++++ 3 files changed, 139 insertions(+), 155 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 572dc31a3..10e4e19b0 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -2138,96 +2138,15 @@ typedef enum : NSUInteger { OWSAssert(quotedMessage.timestamp > 0); OWSAssert(quotedMessage.authorId.length > 0); - // We try to range of items within the current thread's interactions - // that includs the "quoted interaction". - // NOTE: Since the range _IS NOT_ filtered by author, - // and timestamp collisions are possible, it's possible - // for: + // We try to find the index of the item within the current thread's + // interactions that includes the "quoted interaction". + // + // NOTE: There are two indices: + // + // * The "group index" of the member of the database views group at + // the db conneciton's current checkpoint. + // * The "index row/section" in the message mapping. // - // * The range to include more than the "quoted interaction". - // * The range to be non-empty but NOT include the "quoted interaction", - // although this would be a bug. - NSUInteger threadInteractionCount = 0; - NSRange itemRange = [self findRangeOfQuotedMessage:quotedMessage threadInteractionCount:&threadInteractionCount]; - - if (itemRange.location == NSNotFound) { - OWSFail(@"%@ Couldn't find range of quoted reply.", self.logTag); - return; - } - - NSInteger desiredWindowSize = MAX(0, 1 + (NSInteger)threadInteractionCount - (NSInteger)itemRange.location); - NSUInteger oldLoadWindowSize = [self.messageMappings numberOfItemsInGroup:self.thread.uniqueId]; - NSInteger additionalItemsToLoad = MAX(0, desiredWindowSize - (NSInteger)oldLoadWindowSize); - - NSInteger dstIndex = 0; - if (additionalItemsToLoad > 0) { - // Try to load more messages so that the quoted message - // is in the load window. - // - // This may fail if the quoted message is very old, in which - // case we'll load the max number of messages. - [self loadNMoreMessages:(NSUInteger)additionalItemsToLoad]; - - // Scroll to the first item, which should be the quoted message, - // or if we couldn't load it, the oldest loadable message. - // - // We use `indexPathRowOfQuotedMessage` instead of doing - // some index math because: - // - // * Resetting the mapping may integrate other pending changes. - // * Dynamic interactions may change. - dstIndex = (NSInteger)[self indexPathRowOfQuotedMessage:quotedMessage]; - } else { - // Scroll to the quoted message, which is already in the load window. - dstIndex = 1 + (NSInteger)oldLoadWindowSize - desiredWindowSize; - } - - NSIndexPath *indexPath = [NSIndexPath indexPathForRow:dstIndex inSection:0]; - [self.collectionView scrollToItemAtIndexPath:indexPath - atScrollPosition:UICollectionViewScrollPositionTop - animated:YES]; - - // TODO: Highlight the quoted message? -} - -// This method makes a best effort to determine the best scroll position -// after loading more interactions when trying to scroll to a quoted -// message. -// -// It prefers to be simple than to correctly handle edge cases like multiple -// interactions with the same timestamp. -- (NSUInteger)indexPathRowOfQuotedMessage:(TSQuotedMessage *)quotedMessage -{ - OWSAssertIsOnMainThread(); - OWSAssert(quotedMessage); - OWSAssert(quotedMessage.timestamp > 0); - OWSAssert(quotedMessage.authorId.length > 0); - - NSUInteger result = 0; - for (NSUInteger row = 0; row < self.viewItems.count; row++) { - ConversationViewItem *viewItem = self.viewItems[row]; - if (viewItem.interaction.timestamp > quotedMessage.timestamp) { - break; - } - result = row; - } - return result; -} - -// If result range is valid, then threadInteractionCount will have a valid value too. -- (NSRange)findRangeOfQuotedMessage:(TSQuotedMessage *)quotedMessage - threadInteractionCount:(NSUInteger *)threadInteractionCount -{ - OWSAssertIsOnMainThread(); - OWSAssert(quotedMessage); - OWSAssert(quotedMessage.timestamp > 0); - OWSAssert(quotedMessage.authorId.length > 0); - OWSAssert(threadInteractionCount); - - *threadInteractionCount = 0; - - // We try to find the "quoted interaction" AND - // the range within the mapping that includes it. // NOTE: Since the range _IS NOT_ filtered by author, // and timestamp collisions are possible, it's possible // for: @@ -2235,13 +2154,15 @@ typedef enum : NSUInteger { // * The range to include more than the "quoted interaction". // * The range to be non-empty but NOT include the "quoted interaction", // although this would be a bug. - __block NSRange itemRange; - itemRange.location = NSNotFound; + __block TSInteraction *_Nullable quotedInteraction; + __block NSUInteger threadInteractionCount = 0; + __block NSNumber *_Nullable groupIndex = nil; [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { - TSInteraction *_Nullable quotedInteraction = [self findInteractionInThreadByTimestamp:quotedMessage.timestamp - authorId:quotedMessage.authorId - transaction:transaction]; + quotedInteraction = [ThreadUtil findInteractionInThreadByTimestamp:quotedMessage.timestamp + authorId:quotedMessage.authorId + threadUniqueId:self.thread.uniqueId + transaction:transaction]; if (!quotedInteraction) { return; } @@ -2253,81 +2174,90 @@ typedef enum : NSUInteger { return; } - *threadInteractionCount = [extension numberOfItemsInGroup:self.thread.uniqueId]; + threadInteractionCount = [extension numberOfItemsInGroup:self.thread.uniqueId]; + + groupIndex = [self findGroupIndexOfThreadInteraction:quotedInteraction transaction:transaction]; + }]; + + if (!quotedInteraction || !groupIndex) { + DDLogError(@"%@ Couldn't find message quoted in quoted reply.", self.logTag); + return; + } + + NSUInteger indexRow = 0; + NSUInteger indexSection = 0; + BOOL isInMappings = [self.messageMappings getRow:&indexRow + section:&indexSection + forIndex:groupIndex.unsignedIntegerValue + inGroup:self.thread.uniqueId]; + + if (!isInMappings) { + NSInteger desiredWindowSize = MAX(0, 1 + (NSInteger)threadInteractionCount - groupIndex.integerValue); + NSUInteger oldLoadWindowSize = [self.messageMappings numberOfItemsInGroup:self.thread.uniqueId]; + NSInteger additionalItemsToLoad = MAX(0, desiredWindowSize - (NSInteger)oldLoadWindowSize); + if (additionalItemsToLoad < 1) { + DDLogError(@"%@ Couldn't determine how to load quoted reply.", self.logTag); + return; + } - // See comments on YapDatabaseViewFind. + // Try to load more messages so that the quoted message + // is in the load window. // - // Essentially we define a comparator/sort on timestamp. - YapDatabaseViewFind *viewFind = - [YapDatabaseViewFind withObjectBlock:^NSComparisonResult(NSString *collection, NSString *key, id object) { - if (![object isKindOfClass:[TSInteraction class]]) { - OWSFail(@"%@ Unexpected type in database view.", self.logTag); - return NSOrderedSame; - } + // This may fail if the quoted message is very old, in which + // case we'll load the max number of messages. + [self loadNMoreMessages:(NSUInteger)additionalItemsToLoad]; - TSInteraction *interaction = object; + // `loadNMoreMessages` will reset the mapping and possibly + // integrate new changes, so we need to reload the "group index" + // of the quoted message. + [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { + groupIndex = [self findGroupIndexOfThreadInteraction:quotedInteraction transaction:transaction]; + }]; - // For the findBlock, the "left operand" is the row that is passed, - // and the "right operand" is the desired range. - if (interaction.timestamp == quotedMessage.timestamp) { + if (!quotedInteraction || !groupIndex) { + DDLogError(@"%@ Failed to find quoted reply in group.", self.logTag); + return; + } - return NSOrderedSame; - } else if (interaction.timestamp < quotedMessage.timestamp) { - // NSOrderedAscending : The left operand is smaller than the right operand. - return NSOrderedAscending; - } else { - // NSOrderedDescending : The left operand is greater than the right operand. - return NSOrderedDescending; - } - }]; - itemRange = [extension findRangeInGroup:self.thread.uniqueId using:viewFind]; - }]; + isInMappings = [self.messageMappings getRow:&indexRow + section:&indexSection + forIndex:groupIndex.unsignedIntegerValue + inGroup:self.thread.uniqueId]; + + if (!isInMappings) { + DDLogError(@"%@ Could not load quoted reply into mapping.", self.logTag); + return; + } + } - return itemRange; + NSIndexPath *indexPath = [NSIndexPath indexPathForRow:(NSInteger)indexRow inSection:(NSInteger)indexSection]; + [self.collectionView scrollToItemAtIndexPath:indexPath + atScrollPosition:UICollectionViewScrollPositionTop + animated:YES]; + + // TODO: Highlight the quoted message? } -- (nullable TSInteraction *)findInteractionInThreadByTimestamp:(uint64_t)timestamp - authorId:(NSString *)authorId - transaction:(YapDatabaseReadTransaction *)transaction +- (nullable NSNumber *)findGroupIndexOfThreadInteraction:(TSInteraction *)interaction + transaction:(YapDatabaseReadTransaction *)transaction { - OWSAssert(timestamp > 0); - OWSAssert(authorId.length > 0); + OWSAssert(interaction); + OWSAssert(transaction); - NSString *localNumber = [TSAccountManager localNumber]; - if (localNumber.length < 1) { + YapDatabaseAutoViewTransaction *_Nullable extension = [transaction extension:TSMessageDatabaseViewExtensionName]; + if (!extension) { + OWSFail(@"%@ Couldn't load view.", self.logTag); return nil; } - NSArray *interactions = - [TSInteraction interactionsWithTimestamp:timestamp ofClass:[TSMessage class] withTransaction:transaction]; - - TSInteraction *_Nullable result = nil; - for (TSInteraction *interaction in interactions) { - NSString *_Nullable messageAuthorId = nil; - if ([interaction isKindOfClass:[TSIncomingMessage class]]) { - TSIncomingMessage *incomingMessage = (TSIncomingMessage *)interaction; - messageAuthorId = incomingMessage.authorId; - } else if ([interaction isKindOfClass:[TSOutgoingMessage class]]) { - messageAuthorId = localNumber; - } - if (messageAuthorId.length < 1) { - OWSFail(@"%@ Message missing author id.", self.logTag); - continue; - } - if (![authorId isEqualToString:messageAuthorId]) { - continue; - } - if (![interaction.uniqueThreadId isEqualToString:self.thread.uniqueId]) { - continue; - } - if (result) { - // In case of collision, take the first. - DDLogError(@"%@ more than one matching interaction in thread.", self.logTag); - continue; - } - result = interaction; + NSUInteger groupIndex = 0; + BOOL foundInGroup = + [extension getGroup:nil index:&groupIndex forKey:interaction.uniqueId inCollection:TSInteraction.collection]; + if (!foundInGroup) { + DDLogError(@"%@ Couldn't find quoted message in group.", self.logTag); + return nil; } - return result; + return @(groupIndex); } - (void)showMetadataViewForViewItem:(ConversationViewItem *)conversationItem diff --git a/SignalMessaging/utils/ThreadUtil.h b/SignalMessaging/utils/ThreadUtil.h index 12d25a510..e6cefa0d6 100644 --- a/SignalMessaging/utils/ThreadUtil.h +++ b/SignalMessaging/utils/ThreadUtil.h @@ -12,6 +12,7 @@ NS_ASSUME_NONNULL_BEGIN @class TSInteraction; @class TSThread; @class YapDatabaseConnection; +@class YapDatabaseReadTransaction; @interface ThreadDynamicInteractions : NSObject @@ -113,6 +114,13 @@ NS_ASSUME_NONNULL_BEGIN + (void)deleteAllContent; +#pragma mark - Find Content + ++ (nullable TSInteraction *)findInteractionInThreadByTimestamp:(uint64_t)timestamp + authorId:(NSString *)authorId + threadUniqueId:(NSString *)threadUniqueId + transaction:(YapDatabaseReadTransaction *)transaction; + @end NS_ASSUME_NONNULL_END diff --git a/SignalMessaging/utils/ThreadUtil.m b/SignalMessaging/utils/ThreadUtil.m index 705dcfcb0..51c692099 100644 --- a/SignalMessaging/utils/ThreadUtil.m +++ b/SignalMessaging/utils/ThreadUtil.m @@ -668,6 +668,52 @@ NS_ASSUME_NONNULL_BEGIN DDLogInfo(@"%@ Deleted %zd/%zd objects from: %@", self.logTag, count, uniqueIds.count, collection); } +#pragma mark - Find Content + + + (nullable TSInteraction *)findInteractionInThreadByTimestamp : (uint64_t)timestamp authorId + : (NSString *)authorId threadUniqueId : (NSString *)threadUniqueId transaction + : (YapDatabaseReadTransaction *)transaction +{ + OWSAssert(timestamp > 0); + OWSAssert(authorId.length > 0); + + NSString *localNumber = [TSAccountManager localNumber]; + if (localNumber.length < 1) { + return nil; + } + + NSArray *interactions = + [TSInteraction interactionsWithTimestamp:timestamp ofClass:[TSMessage class] withTransaction:transaction]; + + TSInteraction *_Nullable result = nil; + for (TSInteraction *interaction in interactions) { + NSString *_Nullable messageAuthorId = nil; + if ([interaction isKindOfClass:[TSIncomingMessage class]]) { + TSIncomingMessage *incomingMessage = (TSIncomingMessage *)interaction; + messageAuthorId = incomingMessage.authorId; + } else if ([interaction isKindOfClass:[TSOutgoingMessage class]]) { + messageAuthorId = localNumber; + } + if (messageAuthorId.length < 1) { + OWSFail(@"%@ Message missing author id.", self.logTag); + continue; + } + if (![authorId isEqualToString:messageAuthorId]) { + continue; + } + if (![interaction.uniqueThreadId isEqualToString:threadUniqueId]) { + continue; + } + if (result) { + // In case of collision, take the first. + DDLogError(@"%@ more than one matching interaction in thread.", self.logTag); + continue; + } + result = interaction; + } + return result; +} + @end NS_ASSUME_NONNULL_END From 6a07599e00ea4471912f04e775d223a8a294a146 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 10 Apr 2018 09:54:28 -0400 Subject: [PATCH 4/5] Respond to CR. --- SignalMessaging/utils/ThreadUtil.m | 61 ++++++++++--------- .../src/Messages/Interactions/TSInteraction.h | 2 +- 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/SignalMessaging/utils/ThreadUtil.m b/SignalMessaging/utils/ThreadUtil.m index 51c692099..bb05900ba 100644 --- a/SignalMessaging/utils/ThreadUtil.m +++ b/SignalMessaging/utils/ThreadUtil.m @@ -672,46 +672,49 @@ NS_ASSUME_NONNULL_BEGIN + (nullable TSInteraction *)findInteractionInThreadByTimestamp : (uint64_t)timestamp authorId : (NSString *)authorId threadUniqueId : (NSString *)threadUniqueId transaction - : (YapDatabaseReadTransaction *)transaction + : (YapDatabaseReadTransaction *)transaction; { OWSAssert(timestamp > 0); OWSAssert(authorId.length > 0); NSString *localNumber = [TSAccountManager localNumber]; if (localNumber.length < 1) { + OWSFail(@"%@ missing long number.", self.logTag); return nil; } NSArray *interactions = - [TSInteraction interactionsWithTimestamp:timestamp ofClass:[TSMessage class] withTransaction:transaction]; - - TSInteraction *_Nullable result = nil; - for (TSInteraction *interaction in interactions) { - NSString *_Nullable messageAuthorId = nil; - if ([interaction isKindOfClass:[TSIncomingMessage class]]) { - TSIncomingMessage *incomingMessage = (TSIncomingMessage *)interaction; - messageAuthorId = incomingMessage.authorId; - } else if ([interaction isKindOfClass:[TSOutgoingMessage class]]) { - messageAuthorId = localNumber; - } - if (messageAuthorId.length < 1) { - OWSFail(@"%@ Message missing author id.", self.logTag); - continue; - } - if (![authorId isEqualToString:messageAuthorId]) { - continue; - } - if (![interaction.uniqueThreadId isEqualToString:threadUniqueId]) { - continue; - } - if (result) { - // In case of collision, take the first. - DDLogError(@"%@ more than one matching interaction in thread.", self.logTag); - continue; - } - result = interaction; + [TSInteraction interactionsWithTimestamp:timestamp + filter:^(TSInteraction *interaction) { + NSString *_Nullable messageAuthorId = nil; + if ([interaction isKindOfClass:[TSIncomingMessage class]]) { + TSIncomingMessage *incomingMessage = (TSIncomingMessage *)interaction; + messageAuthorId = incomingMessage.authorId; + } else if ([interaction isKindOfClass:[TSOutgoingMessage class]]) { + messageAuthorId = localNumber; + } + if (messageAuthorId.length < 1) { + return NO; + } + + if (![authorId isEqualToString:messageAuthorId]) { + return NO; + } + if (![interaction.uniqueThreadId isEqualToString:threadUniqueId]) { + return NO; + } + return YES; + } + withTransaction:transaction]; + if (interactions.count < 1) { + return nil; } - return result; + if (interactions.count > 1) { + // In case of collision, take the first. + DDLogError(@"%@ more than one matching interaction in thread.", self.logTag); + return nil; + } + return interactions[0]; } @end diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h index 1ea070928..67044af1f 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h @@ -45,7 +45,7 @@ typedef NS_ENUM(NSInteger, OWSInteractionType) { + (NSArray *)interactionsWithTimestamp:(uint64_t)timestamp filter:(BOOL (^_Nonnull)(TSInteraction *))filter - withTransaction:(YapDatabaseReadWriteTransaction *)transaction; + withTransaction:(YapDatabaseReadTransaction *)transaction; - (NSDate *)dateForSorting; - (uint64_t)timestampForSorting; From 6ee74eaffc42bcb1e9ec51d2b6395dfd0d9477ef Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 10 Apr 2018 15:56:13 -0400 Subject: [PATCH 5/5] Respond to CR. --- SignalMessaging/utils/ThreadUtil.m | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/SignalMessaging/utils/ThreadUtil.m b/SignalMessaging/utils/ThreadUtil.m index bb05900ba..6437f3dc6 100644 --- a/SignalMessaging/utils/ThreadUtil.m +++ b/SignalMessaging/utils/ThreadUtil.m @@ -670,9 +670,10 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - Find Content - + (nullable TSInteraction *)findInteractionInThreadByTimestamp : (uint64_t)timestamp authorId - : (NSString *)authorId threadUniqueId : (NSString *)threadUniqueId transaction - : (YapDatabaseReadTransaction *)transaction; ++ (nullable TSInteraction *)findInteractionInThreadByTimestamp:(uint64_t)timestamp + authorId:(NSString *)authorId + threadUniqueId:(NSString *)threadUniqueId + transaction:(YapDatabaseReadTransaction *)transaction { OWSAssert(timestamp > 0); OWSAssert(authorId.length > 0); @@ -712,9 +713,8 @@ NS_ASSUME_NONNULL_BEGIN if (interactions.count > 1) { // In case of collision, take the first. DDLogError(@"%@ more than one matching interaction in thread.", self.logTag); - return nil; } - return interactions[0]; + return interactions.firstObject; } @end