From 4de4a4b2292c59c9070b007d8c76673d134e2a47 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 24 Apr 2018 16:11:10 -0400 Subject: [PATCH] Respond to CR. --- .../Utils/MessageRecipientStatusUtils.swift | 29 ------------------- .../Messages/Interactions/TSOutgoingMessage.h | 2 +- .../Messages/Interactions/TSOutgoingMessage.m | 23 +++++---------- .../src/Messages/OWSMessageSender.m | 7 +---- 4 files changed, 10 insertions(+), 51 deletions(-) diff --git a/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift b/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift index 6ff08c473..b57380a21 100644 --- a/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift +++ b/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift @@ -16,35 +16,6 @@ import SignalMessaging case skipped } -// Our per-recipient status messages are "biased towards success" -// and reflect the most successful known state for that recipient. -// -// Our per-message status messages are "biased towards failure" -// and reflect the least successful known state for that message. -// -// Why? -// -// When showing the per-recipient status, we want to show the message -// as "read" even if delivery failed to another recipient of the same -// message. -// -// When showing the per-message status, we want to show the message -// as "failed" if delivery failed to any recipient, even if another -// receipient has read the message. -// -// Note also that for legacy reasons we have redundant and possibly -// conflicting state. Examples: -// -// * We could have an entry in the recipientReadMap for a message -// that has no entries in its recipientDeliveryMap. -// * We could have an entry in the recipientReadMap or recipientDeliveryMap -// for a message whose status is "attempting out" or "unsent". -// * We could have a message whose wasDelivered property is false but -// which has entries in its recipientDeliveryMap or recipientReadMap. -// * Etc. -// -// To resolve this ambiguity, we apply a "bias" towards success or -// failure. class MessageRecipientStatusUtils: NSObject { // MARK: Initializers diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h index 701514c00..665681e4d 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h @@ -156,7 +156,7 @@ typedef NS_ENUM(NSInteger, TSGroupMetaMessage) { // All recipients of this message. - (NSArray *)recipientIds; -// All recipients of this message who we are currently trying to send to (queued, uploading or doing send). +// All recipients of this message who we are currently trying to send to (queued, uploading or during send). - (NSArray *)sendingRecipientIds; // All recipients of this message to whom it has been sent and delivered. diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index e8fb5a37b..092115721 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -124,6 +124,7 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec } NSString *_Nullable singleGroupRecipient = [coder decodeObjectForKey:@"singleGroupRecipient"]; if (singleGroupRecipient) { + OWSFail(@"%@ unexpected single group recipient message.", self.logTag); // If this is a "single group recipient message", treat it as such. recipientIds = @[ singleGroupRecipient, @@ -334,22 +335,14 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec [super saveWithTransaction:transaction]; } -- (OWSOutgoingMessageRecipientState)maxMessageState +- (BOOL)hasSentToAnyRecipient { - OWSOutgoingMessageRecipientState result = OWSOutgoingMessageRecipientStateMin; for (TSOutgoingMessageRecipientState *recipientState in self.recipientStateMap.allValues) { - result = MAX(recipientState.state, result); - } - return result; -} - -- (OWSOutgoingMessageRecipientState)minMessageState -{ - OWSOutgoingMessageRecipientState result = OWSOutgoingMessageRecipientStateMax; - for (TSOutgoingMessageRecipientState *recipientState in self.recipientStateMap.allValues) { - result = MIN(recipientState.state, result); + if (recipientState.state == OWSOutgoingMessageRecipientStateSent) { + return YES; + } } - return result; + return NO; } - (BOOL)shouldStartExpireTimer:(YapDatabaseReadTransaction *)transaction @@ -366,7 +359,7 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec } else if (self.recipientStateMap.count < 1) { return YES; } else { - return self.maxMessageState >= OWSOutgoingMessageRecipientStateSent; + return self.hasSentToAnyRecipient; } } @@ -382,7 +375,7 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec - (NSArray *)recipientIds { - return [self.recipientStateMap.allKeys copy]; + return self.recipientStateMap.allKeys; } - (NSArray *)sendingRecipientIds diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 413d2953c..342105321 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -607,6 +607,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; failure:(RetryableFailureHandler)failureHandler { [self saveGroupMessage:message inThread:thread]; + NSMutableArray *futures = [NSMutableArray array]; for (SignalRecipient *recipient in recipients) { @@ -616,12 +617,6 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; if ([recipientId isEqualToString:[TSAccountManager localNumber]]) { continue; } - if (![message.sendingRecipientIds containsObject:recipientId]) { - // Skip recipients we have already sent this message to (on an - // earlier retry, perhaps). - DDLogInfo(@"%@ Skipping group message recipient; already sent: %@", self.logTag, recipient.uniqueId); - continue; - } // ...otherwise we send. [futures addObject:[self sendMessageFuture:message recipient:recipient thread:thread]];