From c5c464378245d4adbf7c01e2f402f138e5e58cb6 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 12 Jun 2017 11:51:25 -0400 Subject: [PATCH] Rework how messages are marked read. // FREEBIE --- src/Contacts/TSThread.h | 1 - src/Contacts/TSThread.m | 9 +--- src/Devices/OWSReadReceiptsProcessor.m | 34 ++++++++++++++- src/Messages/Interactions/TSErrorMessage.h | 2 - src/Messages/Interactions/TSErrorMessage.m | 17 ++++---- src/Messages/Interactions/TSIncomingMessage.h | 14 ------- src/Messages/Interactions/TSIncomingMessage.m | 42 ++++++++----------- src/Messages/Interactions/TSInfoMessage.h | 2 - src/Messages/Interactions/TSInfoMessage.m | 17 ++++---- src/Messages/OWSIncomingMessageReadObserver.m | 7 ++-- src/Messages/OWSReadTracking.h | 6 +-- src/Messages/TSCall.m | 20 ++++----- src/Messages/TSMessagesManager.m | 3 +- 13 files changed, 87 insertions(+), 87 deletions(-) diff --git a/src/Contacts/TSThread.h b/src/Contacts/TSThread.h index 1a433684c..7a25f0882 100644 --- a/src/Contacts/TSThread.h +++ b/src/Contacts/TSThread.h @@ -72,7 +72,6 @@ NS_ASSUME_NONNULL_BEGIN - (BOOL)hasSafetyNumbers; -- (void)markAllAsRead; - (void)markAllAsReadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; /** diff --git a/src/Contacts/TSThread.m b/src/Contacts/TSThread.m index fb0e55d2e..7ef6677cb 100644 --- a/src/Contacts/TSThread.m +++ b/src/Contacts/TSThread.m @@ -228,20 +228,13 @@ NS_ASSUME_NONNULL_BEGIN - (void)markAllAsReadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction { for (id message in [self unseenMessagesWithTransaction:transaction]) { - [message markAsReadLocallyWithTransaction:transaction]; + [message markAsReadWithTransaction:transaction sendReadReceipt:YES]; } // Just to be defensive, we'll also check for unread messages. OWSAssert([self unseenMessagesWithTransaction:transaction].count < 1); } -- (void)markAllAsRead -{ - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - [self markAllAsReadWithTransaction:transaction]; - }]; -} - - (TSInteraction *) lastInteraction { __block TSInteraction *last; [TSStorageManager.sharedManager.dbConnection readWithBlock:^(YapDatabaseReadTransaction *transaction){ diff --git a/src/Devices/OWSReadReceiptsProcessor.m b/src/Devices/OWSReadReceiptsProcessor.m index 30d070ef7..8816a4cb4 100644 --- a/src/Devices/OWSReadReceiptsProcessor.m +++ b/src/Devices/OWSReadReceiptsProcessor.m @@ -7,7 +7,10 @@ #import "OWSReadReceipt.h" #import "OWSSignalServiceProtos.pb.h" #import "TSContactThread.h" +#import "TSDatabaseView.h" #import "TSIncomingMessage.h" +#import "TSStorageManager.h" +#import NS_ASSUME_NONNULL_BEGIN @@ -82,7 +85,36 @@ NSString *const OWSReadReceiptsProcessorMarkedMessageAsReadNotification = TSIncomingMessage *message = [TSIncomingMessage findMessageWithAuthorId:readReceipt.senderId timestamp:readReceipt.timestamp]; if (message) { - [message markAsReadFromReadReceipt]; + OWSAssert(message.thread); + + NSMutableArray> *interactionToMarkAsRead = [NSMutableArray new]; + [self.storageManager.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [[transaction ext:TSUnseenDatabaseViewExtensionName] + enumerateRowsInGroup:message.uniqueThreadId + usingBlock:^(NSString *collection, + NSString *key, + id object, + id metadata, + NSUInteger index, + BOOL *stop) { + + TSInteraction *interaction = object; + if (interaction.timestampForSorting < message.timestampForSorting) { + *stop = YES; + return; + } + + id possiblyRead = (id)object; + OWSAssert(!possiblyRead.read); + [interactionToMarkAsRead addObject:possiblyRead]; + }]; + + for (id possiblyRead in interactionToMarkAsRead) { + // Don't send a read receipt in response to a read receipt. + [possiblyRead markAsReadWithTransaction:transaction sendReadReceipt:NO]; + } + }]; + [OWSDisappearingMessagesJob setExpirationForMessage:message expirationStartedAt:readReceipt.timestamp]; // If it was previously saved, no need to keep it around any longer. [readReceipt remove]; diff --git a/src/Messages/Interactions/TSErrorMessage.h b/src/Messages/Interactions/TSErrorMessage.h index b68a9ea82..21dfa59a9 100644 --- a/src/Messages/Interactions/TSErrorMessage.h +++ b/src/Messages/Interactions/TSErrorMessage.h @@ -58,8 +58,6 @@ typedef NS_ENUM(int32_t, TSErrorMessageType) { @property (nonatomic, readonly) TSErrorMessageType errorType; @property (nullable, nonatomic, readonly) NSString *recipientId; -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; - @end NS_ASSUME_NONNULL_END diff --git a/src/Messages/Interactions/TSErrorMessage.m b/src/Messages/Interactions/TSErrorMessage.m index 0623f84f4..aa269701a 100644 --- a/src/Messages/Interactions/TSErrorMessage.m +++ b/src/Messages/Interactions/TSErrorMessage.m @@ -167,19 +167,20 @@ NSUInteger TSErrorMessageSchemaVersion = 1; return NO; } -- (void)markAsReadLocally -{ - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [self markAsReadLocallyWithTransaction:transaction]; - }]; -} - -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction +- (void)markAsReadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction sendReadReceipt:(BOOL)sendReadReceipt { OWSAssert(transaction); + + if (_read) { + return; + } + DDLogInfo(@"%@ marking as read uniqueId: %@ which has timestamp: %llu", self.tag, self.uniqueId, self.timestamp); _read = YES; [self saveWithTransaction:transaction]; + [self touchThreadWithTransaction:transaction]; + + // Ignore sendReadReceipt; it doesn't apply to error messages. } #pragma mark - Logging diff --git a/src/Messages/Interactions/TSIncomingMessage.h b/src/Messages/Interactions/TSIncomingMessage.h index 3b254920a..262852131 100644 --- a/src/Messages/Interactions/TSIncomingMessage.h +++ b/src/Messages/Interactions/TSIncomingMessage.h @@ -108,20 +108,6 @@ extern NSString *const TSIncomingMessageWasReadOnThisDeviceNotification; // This will be 0 for messages created before we were tracking sourceDeviceId @property (nonatomic, readonly) UInt32 sourceDeviceId; -/* - * Marks a message as having been read on this device (as opposed to responding to a remote read receipt). - * - */ -- (void)markAsReadLocally; -// TODO possible to remove? -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; - -/** - * Similar to markAsReadWithTransaction, but doesn't send out read receipts. - * Used for *responding* to a remote read receipt. - */ -- (void)markAsReadFromReadReceipt; - @end NS_ASSUME_NONNULL_END diff --git a/src/Messages/Interactions/TSIncomingMessage.m b/src/Messages/Interactions/TSIncomingMessage.m index 6e9db8409..91cd7f6bb 100644 --- a/src/Messages/Interactions/TSIncomingMessage.m +++ b/src/Messages/Interactions/TSIncomingMessage.m @@ -3,6 +3,8 @@ // #import "TSIncomingMessage.h" +#import "OWSDisappearingMessagesConfiguration.h" +#import "OWSDisappearingMessagesJob.h" #import "TSContactThread.h" #import "TSDatabaseSecondaryIndexes.h" #import "TSGroupThread.h" @@ -115,37 +117,29 @@ NSString *const TSIncomingMessageWasReadOnThisDeviceNotification = @"TSIncomingM return YES; } -- (void)markAsReadFromReadReceipt +- (void)markAsReadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction sendReadReceipt:(BOOL)sendReadReceipt { - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [self markAsReadWithoutNotificationWithTransaction:transaction]; - }]; -} - -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction -{ - [self markAsReadWithoutNotificationWithTransaction:transaction]; - [[NSNotificationCenter defaultCenter] postNotificationName:TSIncomingMessageWasReadOnThisDeviceNotification - object:self]; -} + OWSAssert(transaction); -- (void)markAsReadLocally -{ - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [self markAsReadWithoutNotificationWithTransaction:transaction]; - }]; - // Notification must happen outside of the transaction, else we'll likely crash when the notification receiver - // tries to do anything with the DB. - [[NSNotificationCenter defaultCenter] postNotificationName:TSIncomingMessageWasReadOnThisDeviceNotification - object:self]; -} + if (_read) { + return; + } -- (void)markAsReadWithoutNotificationWithTransaction:(YapDatabaseReadWriteTransaction *)transaction -{ DDLogInfo(@"%@ marking as read uniqueId: %@ which has timestamp: %llu", self.tag, self.uniqueId, self.timestamp); _read = YES; [self saveWithTransaction:transaction]; [self touchThreadWithTransaction:transaction]; + + [OWSDisappearingMessagesJob setExpirationForMessage:self]; + + if (sendReadReceipt) { + // Notification must happen outside of the transaction, else we'll likely crash when the notification receiver + // tries to do anything with the DB. + dispatch_async(dispatch_get_main_queue(), ^{ + [[NSNotificationCenter defaultCenter] postNotificationName:TSIncomingMessageWasReadOnThisDeviceNotification + object:self]; + }); + } } #pragma mark - Logging diff --git a/src/Messages/Interactions/TSInfoMessage.h b/src/Messages/Interactions/TSInfoMessage.h index 4bd00c871..fa9278e2d 100644 --- a/src/Messages/Interactions/TSInfoMessage.h +++ b/src/Messages/Interactions/TSInfoMessage.h @@ -44,8 +44,6 @@ typedef NS_ENUM(NSInteger, TSInfoMessageType) { expiresInSeconds:(uint32_t)expiresInSeconds expireStartedAt:(uint64_t)expireStartedAt NS_UNAVAILABLE; -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; - @end NS_ASSUME_NONNULL_END diff --git a/src/Messages/Interactions/TSInfoMessage.m b/src/Messages/Interactions/TSInfoMessage.m index ad6fa981f..ecf304978 100644 --- a/src/Messages/Interactions/TSInfoMessage.m +++ b/src/Messages/Interactions/TSInfoMessage.m @@ -109,19 +109,20 @@ NSUInteger TSInfoMessageSchemaVersion = 1; return NO; } -- (void)markAsReadLocally -{ - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [self markAsReadLocallyWithTransaction:transaction]; - }]; -} - -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction +- (void)markAsReadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction sendReadReceipt:(BOOL)sendReadReceipt { OWSAssert(transaction); + + if (_read) { + return; + } + DDLogInfo(@"%@ marking as read uniqueId: %@ which has timestamp: %llu", self.tag, self.uniqueId, self.timestamp); _read = YES; [self saveWithTransaction:transaction]; + [self touchThreadWithTransaction:transaction]; + + // Ignore sendReadReceipt; it doesn't apply to info messages. } #pragma mark - Logging diff --git a/src/Messages/OWSIncomingMessageReadObserver.m b/src/Messages/OWSIncomingMessageReadObserver.m index 13c16ff6f..20c581e4d 100644 --- a/src/Messages/OWSIncomingMessageReadObserver.m +++ b/src/Messages/OWSIncomingMessageReadObserver.m @@ -4,8 +4,6 @@ #import "OWSIncomingMessageReadObserver.h" #import "NSDate+millisecondTimeStamp.h" -#import "OWSDisappearingMessagesConfiguration.h" -#import "OWSDisappearingMessagesJob.h" #import "OWSSendReadReceiptsJob.h" #import "TSIncomingMessage.h" @@ -13,7 +11,7 @@ NS_ASSUME_NONNULL_BEGIN @interface OWSIncomingMessageReadObserver () -@property BOOL isObserving; +@property (nonatomic) BOOL isObserving; @property (nonatomic, readonly) OWSSendReadReceiptsJob *sendReadReceiptsJob; @end @@ -41,6 +39,8 @@ NS_ASSUME_NONNULL_BEGIN - (void)startObserving { + OWSAssert([NSThread isMainThread]); + if (self.isObserving) { return; } @@ -60,7 +60,6 @@ NS_ASSUME_NONNULL_BEGIN } TSIncomingMessage *message = (TSIncomingMessage *)notification.object; - [OWSDisappearingMessagesJob setExpirationForMessage:message]; [self.sendReadReceiptsJob runWith:message]; } diff --git a/src/Messages/OWSReadTracking.h b/src/Messages/OWSReadTracking.h index fa981fc9a..6712f4f16 100644 --- a/src/Messages/OWSReadTracking.h +++ b/src/Messages/OWSReadTracking.h @@ -20,10 +20,8 @@ - (BOOL)shouldAffectUnreadCounts; /** - * Call when the user viewed the message/call on this device. "locally" as opposed to being notified via a read receipt - * sync message of a remote read. + * Used for *responding* to a remote read receipt or in response to user activity. */ -- (void)markAsReadLocally; -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; +- (void)markAsReadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction sendReadReceipt:(BOOL)sendReadReceipt; @end diff --git a/src/Messages/TSCall.m b/src/Messages/TSCall.m index 7e135e774..caa6338be 100644 --- a/src/Messages/TSCall.m +++ b/src/Messages/TSCall.m @@ -86,22 +86,22 @@ NSUInteger TSCallCurrentSchemaVersion = 1; return YES; } -- (void)markAsReadLocallyWithTransaction:(YapDatabaseReadWriteTransaction *)transaction +- (void)markAsReadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction sendReadReceipt:(BOOL)sendReadReceipt { - DDLogInfo(@"%@ marking as read uniqueId: %@ which has timestamp: %llu", self.tag, self.uniqueId, self.timestamp); + OWSAssert(transaction); + + if (_read) { + return; + } + DDLogInfo(@"%@ marking as read uniqueId: %@ which has timestamp: %llu", self.tag, self.uniqueId, self.timestamp); _read = YES; [self saveWithTransaction:transaction]; - - // redraw any thread-related unread count UI. [self touchThreadWithTransaction:transaction]; -} -- (void)markAsReadLocally -{ - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - [self markAsReadLocallyWithTransaction:transaction]; - }]; + // Ignore sendReadReceipt; it doesn't apply to calls. + // + // TODO: Should we update expiration of calls? } #pragma mark - Methods diff --git a/src/Messages/TSMessagesManager.m b/src/Messages/TSMessagesManager.m index bf2129a85..7bf459997 100644 --- a/src/Messages/TSMessagesManager.m +++ b/src/Messages/TSMessagesManager.m @@ -924,7 +924,8 @@ NS_ASSUME_NONNULL_BEGIN // automatically marked as read. BOOL shouldMarkMessageAsRead = [envelope.source isEqualToString:localNumber]; if (shouldMarkMessageAsRead) { - [incomingMessage markAsReadLocallyWithTransaction:transaction]; + // Don't send a read receipt for messages sent by ourselves. + [incomingMessage markAsReadWithTransaction:transaction sendReadReceipt:NO]; } // Other clients allow attachments to be sent along with body, we want the text displayed as a separate