From 204d3796038948c98b4415c362de8961980079be Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 24 Apr 2018 16:38:35 -0400 Subject: [PATCH] Respond to CR. --- .../ConversationView/Cells/OWSMessageCell.m | 2 +- .../ConversationViewController.m | 11 +-- .../MessageDetailViewController.swift | 9 +- .../Utils/MessageRecipientStatusUtils.swift | 82 ++++++++----------- .../Messages/Interactions/TSOutgoingMessage.h | 4 +- .../Messages/Interactions/TSOutgoingMessage.m | 12 ++- .../src/Messages/OWSMessageManager.m | 2 +- 7 files changed, 60 insertions(+), 62 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m index 9bc84276c..7e08837e7 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m @@ -316,7 +316,7 @@ NS_ASSUME_NONNULL_BEGIN if (!self.viewItem.shouldHideRecipientStatus || hasExpirationTimer) { TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)message; NSString *statusMessage = - [MessageRecipientStatusUtils statusMessageWithOutgoingMessage:outgoingMessage referenceView:self]; + [MessageRecipientStatusUtils receiptMessageWithOutgoingMessage:outgoingMessage referenceView:self]; attributedText = [[NSAttributedString alloc] initWithString:statusMessage attributes:@{}]; } } else if (self.viewItem.isGroupThread) { diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index d3f0287ee..59a4910ee 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -4651,7 +4651,7 @@ typedef enum : NSUInteger { // Update the "shouldShowDate" property of the view items. OWSInteractionType lastInteractionType = OWSInteractionType_Unknown; - MessageRecipientStatus lastRecipientStatus = MessageRecipientStatusUploading; + MessageReceiptStatus lastReceiptStatus = MessageReceiptStatusUploading; NSString *_Nullable lastIncomingSenderId = nil; for (ConversationViewItem *viewItem in viewItems.reverseObjectEnumerator) { BOOL shouldHideRecipientStatus = NO; @@ -4660,20 +4660,21 @@ typedef enum : NSUInteger { if (interactionType == OWSInteractionType_OutgoingMessage) { TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)viewItem.interaction; - MessageRecipientStatus recipientStatus = - [MessageRecipientStatusUtils recipientStatusWithOutgoingMessage:outgoingMessage]; + MessageReceiptStatus receiptStatus = + [MessageRecipientStatusUtils recipientStatusWithOutgoingMessage:outgoingMessage + referenceView:self.view]; if (outgoingMessage.messageState == TSOutgoingMessageStateFailed) { // always show "failed to send" status shouldHideRecipientStatus = NO; } else { shouldHideRecipientStatus - = (interactionType == lastInteractionType && recipientStatus == lastRecipientStatus); + = (interactionType == lastInteractionType && receiptStatus == lastReceiptStatus); } shouldHideBubbleTail = interactionType == lastInteractionType; - lastRecipientStatus = recipientStatus; + lastReceiptStatus = receiptStatus; } else if (interactionType == OWSInteractionType_IncomingMessage) { TSIncomingMessage *incomingMessage = (TSIncomingMessage *)viewItem.interaction; NSString *incomingSenderId = incomingMessage.authorId; diff --git a/Signal/src/ViewControllers/MessageDetailViewController.swift b/Signal/src/ViewControllers/MessageDetailViewController.swift index ce219e439..3c77d3433 100644 --- a/Signal/src/ViewControllers/MessageDetailViewController.swift +++ b/Signal/src/ViewControllers/MessageDetailViewController.swift @@ -193,13 +193,14 @@ class MessageDetailViewController: OWSViewController, MediaGalleryDataSourceDele let isGroupThread = thread.isGroupThread() - let recipientStatusGroups: [MessageRecipientStatus] = [ + let recipientStatusGroups: [MessageReceiptStatus] = [ .read, .uploading, .delivered, .sent, .sending, - .failed + .failed, + .skipped ] for recipientStatusGroup in recipientStatusGroups { var groupRows = [UIView]() @@ -548,8 +549,8 @@ class MessageDetailViewController: OWSViewController, MediaGalleryDataSourceDele updateContent() } - private func string(for messageRecipientStatus: MessageRecipientStatus) -> String { - switch messageRecipientStatus { + private func string(for messageReceiptStatus: MessageReceiptStatus) -> String { + switch messageReceiptStatus { case .uploading: return NSLocalizedString("MESSAGE_METADATA_VIEW_MESSAGE_STATUS_UPLOADING", comment: "Status label for messages which are uploading.") diff --git a/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift b/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift index 49f8643ea..9c25b0b25 100644 --- a/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift +++ b/Signal/src/ViewControllers/Utils/MessageRecipientStatusUtils.swift @@ -6,7 +6,7 @@ import Foundation import SignalServiceKit import SignalMessaging -@objc enum MessageRecipientStatus: Int { +@objc enum MessageReceiptStatus: Int { case uploading case sending case sent @@ -24,15 +24,17 @@ public class MessageRecipientStatusUtils: NSObject { private override init() { } + // This method is per-recipient. class func recipientStatus(outgoingMessage: TSOutgoingMessage, recipientState: TSOutgoingMessageRecipientState, - referenceView: UIView) -> MessageRecipientStatus { - let (messageRecipientStatus, _, _) = recipientStatusAndStatusMessage(outgoingMessage: outgoingMessage, + referenceView: UIView) -> MessageReceiptStatus { + let (messageReceiptStatus, _, _) = recipientStatusAndStatusMessage(outgoingMessage: outgoingMessage, recipientState: recipientState, referenceView: referenceView) - return messageRecipientStatus + return messageReceiptStatus } + // This method is per-recipient. @objc public class func shortStatusMessage(outgoingMessage: TSOutgoingMessage, recipientState: TSOutgoingMessageRecipientState, @@ -43,6 +45,7 @@ public class MessageRecipientStatusUtils: NSObject { return shortStatusMessage } + // This method is per-recipient. @objc public class func longStatusMessage(outgoingMessage: TSOutgoingMessage, recipientState: TSOutgoingMessageRecipientState, @@ -53,9 +56,10 @@ public class MessageRecipientStatusUtils: NSObject { return longStatusMessage } + // This method is per-recipient. class func recipientStatusAndStatusMessage(outgoingMessage: TSOutgoingMessage, recipientState: TSOutgoingMessageRecipientState, - referenceView: UIView) -> (status: MessageRecipientStatus, shortStatusMessage: String, longStatusMessage: String) { + referenceView: UIView) -> (status: MessageReceiptStatus, shortStatusMessage: String, longStatusMessage: String) { switch recipientState.state { case .failed: @@ -105,65 +109,51 @@ public class MessageRecipientStatusUtils: NSObject { } } - // This method is per-message and "biased towards failure". - // See comments above. - public class func statusMessage(outgoingMessage: TSOutgoingMessage, - referenceView: UIView) -> String { + // This method is per-message. + internal class func receiptStatusAndMessage(outgoingMessage: TSOutgoingMessage, + referenceView: UIView) -> (status: MessageReceiptStatus, message: String) { switch outgoingMessage.messageState { case .failed: // Use the "long" version of this message here. - return NSLocalizedString("MESSAGE_STATUS_FAILED", comment: "message footer for failed messages") + return (.failed, NSLocalizedString("MESSAGE_STATUS_FAILED", comment: "message footer for failed messages")) case .sending: if outgoingMessage.hasAttachments() { - return NSLocalizedString("MESSAGE_STATUS_UPLOADING", - comment: "message footer while attachment is uploading") + return (.uploading, NSLocalizedString("MESSAGE_STATUS_UPLOADING", + comment: "message footer while attachment is uploading")) } else { - return NSLocalizedString("MESSAGE_STATUS_SENDING", - comment: "message status while message is sending.") + return (.sending, NSLocalizedString("MESSAGE_STATUS_SENDING", + comment: "message status while message is sending.")) } case .sent: if outgoingMessage.readRecipientIds().count > 0 { - return NSLocalizedString("MESSAGE_STATUS_READ", comment: "message footer for read messages") + return (.read, NSLocalizedString("MESSAGE_STATUS_READ", comment: "message footer for read messages")) } if outgoingMessage.deliveredRecipientIds().count > 0 { - return NSLocalizedString("MESSAGE_STATUS_DELIVERED", - comment: "message status for message delivered to their recipient.") + return (.delivered, NSLocalizedString("MESSAGE_STATUS_DELIVERED", + comment: "message status for message delivered to their recipient.")) } - return NSLocalizedString("MESSAGE_STATUS_SENT", - comment: "message footer for sent messages") + return (.sent, NSLocalizedString("MESSAGE_STATUS_SENT", + comment: "message footer for sent messages")) default: owsFail("\(self.logTag) Message has unexpected status: \(outgoingMessage.messageState).") - return NSLocalizedString("MESSAGE_STATUS_SENT", - comment: "message footer for sent messages") + return (.sent, NSLocalizedString("MESSAGE_STATUS_SENT", + comment: "message footer for sent messages")) } } - // This method is per-message and "biased towards failure". - // See comments above. - class func recipientStatus(outgoingMessage: TSOutgoingMessage) -> MessageRecipientStatus { - switch outgoingMessage.messageState { - case .failed: - return .failed - case .sending: - if outgoingMessage.hasAttachments() { - return .uploading - } else { - return .sending - } - case .sent: - if outgoingMessage.readRecipientIds().count > 0 { - return .read - } - if outgoingMessage.deliveredRecipientIds().count > 0 { - return .delivered - } - - return .sent - default: - owsFail("\(self.logTag) Message has unexpected status: \(outgoingMessage.messageState).") + // This method is per-message. + public class func receiptMessage(outgoingMessage: TSOutgoingMessage, + referenceView: UIView) -> String { + let (_, message ) = receiptStatusAndMessage(outgoingMessage: outgoingMessage, + referenceView: referenceView) + return message + } - return .sent - } + // This method is per-message. + class func recipientStatus(outgoingMessage: TSOutgoingMessage, referenceView: UIView) -> MessageReceiptStatus { + let (status, _ ) = receiptStatusAndMessage(outgoingMessage: outgoingMessage, + referenceView: referenceView) + return status } } diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h index 665681e4d..87aa0fcca 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.h @@ -213,8 +213,8 @@ typedef NS_ENUM(NSInteger, TSGroupMetaMessage) { // This method is used to rewrite the recipient list with a single recipient. // It is used to reply to a "group info request", which should only be // delivered to the requestor. -- (void)updateWithSingleGroupRecipient:(NSString *)singleGroupRecipient - transaction:(YapDatabaseReadWriteTransaction *)transaction; +- (void)updateWithSendingToSingleGroupRecipient:(NSString *)singleGroupRecipient + transaction:(YapDatabaseReadWriteTransaction *)transaction; // This method is used to record a successful "read" by one recipient. - (void)updateWithReadRecipientId:(NSString *)recipientId diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index 092115721..bc977b470 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -271,7 +271,7 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec // New outgoing messages should immediately determine their // recipient list from current thread state. NSMutableDictionary *recipientStateMap = [NSMutableDictionary new]; - NSArray *recipientIds = [self.thread recipientIdentifiers]; + NSArray *recipientIds = [thread recipientIdentifiers]; for (NSString *recipientId in recipientIds) { TSOutgoingMessageRecipientState *recipientState = [TSOutgoingMessageRecipientState new]; recipientState.state = OWSOutgoingMessageRecipientStateSending; @@ -565,6 +565,9 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec recipientId); return; } + if (recipientState.state != OWSOutgoingMessageRecipientStateSent) { + DDLogWarn(@"%@ marking unsent message as delivered.", self.logTag); + } recipientState.state = OWSOutgoingMessageRecipientStateSent; recipientState.deliveryTimestamp = deliveryTimestamp; }]; @@ -587,6 +590,9 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec recipientId); return; } + if (recipientState.state != OWSOutgoingMessageRecipientStateSent) { + DDLogWarn(@"%@ marking unsent message as delivered.", self.logTag); + } recipientState.state = OWSOutgoingMessageRecipientStateSent; recipientState.readTimestamp = @(readTimestamp); }]; @@ -609,8 +615,8 @@ NSString *const kTSOutgoingMessageSentRecipientAll = @"kTSOutgoingMessageSentRec }]; } -- (void)updateWithSingleGroupRecipient:(NSString *)singleGroupRecipient - transaction:(YapDatabaseReadWriteTransaction *)transaction +- (void)updateWithSendingToSingleGroupRecipient:(NSString *)singleGroupRecipient + transaction:(YapDatabaseReadWriteTransaction *)transaction { OWSAssert(transaction); OWSAssert(singleGroupRecipient.length > 0); diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index a95908a2f..10934acae 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -891,7 +891,7 @@ NS_ASSUME_NONNULL_BEGIN [TSOutgoingMessage outgoingMessageInThread:gThread groupMetaMessage:TSGroupMessageUpdate]; [message updateWithCustomMessage:updateGroupInfo transaction:transaction]; // Only send this group update to the requester. - [message updateWithSingleGroupRecipient:envelope.source transaction:transaction]; + [message updateWithSendingToSingleGroupRecipient:envelope.source transaction:transaction]; [self sendGroupUpdateForThread:gThread message:message]; }