From f69945ea2010600b64cfeaf39b02e4a262da30f5 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 13 Jul 2018 10:57:18 -0400 Subject: [PATCH 1/3] Fix 'breaks vs. collapse' issue. --- .../ConversationView/Cells/OWSMessageCell.m | 4 +- .../ConversationViewController.m | 65 ++++++++++--------- .../ConversationView/ConversationViewItem.h | 2 +- .../ConversationView/ConversationViewItem.m | 4 +- 4 files changed, 38 insertions(+), 37 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m index 50d0b0830c..76bbf744a3 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m @@ -143,7 +143,7 @@ NS_ASSUME_NONNULL_BEGIN [self.messageBubbleView configureViews]; [self.messageBubbleView loadContent]; - if (self.viewItem.hasCellHeader) { + if (self.viewItem.hasDateOrUnreadBreak) { CGFloat headerHeight = [self.headerView measureWithConversationViewItem:self.viewItem conversationStyle:self.conversationStyle] .height; @@ -342,7 +342,7 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(cellSize.width > 0 && cellSize.height > 0); - if (self.viewItem.hasCellHeader) { + if (self.viewItem.hasDateOrUnreadBreak) { cellSize.height += [self.headerView measureWithConversationViewItem:self.viewItem conversationStyle:self.conversationStyle] .height; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index a568443c17..9b7858258d 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -4847,9 +4847,10 @@ typedef enum : NSUInteger { } }]; - // Update the "shouldShowDate" property of the view items. + // Update the "break" properties (shouldShowDate and unreadIndicator) of the view items. BOOL shouldShowDateOnNextViewItem = YES; uint64_t previousViewItemTimestamp = 0; + OWSUnreadIndicator *_Nullable unreadIndicator = self.dynamicInteractions.unreadIndicator; for (ConversationViewItem *viewItem in viewItems) { BOOL canShowDate = NO; switch (viewItem.interaction.interactionType) { @@ -4884,21 +4885,6 @@ typedef enum : NSUInteger { viewItem.shouldShowDate = shouldShowDate; previousViewItemTimestamp = viewItemTimestamp; - } - - // Update the properties of the view items. - // - // NOTE: This logic uses shouldShowDate which is set in the previous pass. - OWSUnreadIndicator *_Nullable unreadIndicator = self.dynamicInteractions.unreadIndicator; - for (NSUInteger i = 0; i < viewItems.count; i++) { - ConversationViewItem *viewItem = viewItems[i]; - ConversationViewItem *_Nullable previousViewItem = (i > 0 ? viewItems[i - 1] : nil); - ConversationViewItem *_Nullable nextViewItem = (i + 1 < viewItems.count ? viewItems[i + 1] : nil); - BOOL shouldShowSenderAvatar = NO; - BOOL shouldHideFooter = NO; - BOOL isFirstInCluster = YES; - BOOL isLastInCluster = YES; - NSAttributedString *_Nullable senderName = nil; // Place the unread indicator onto the first appropriate view item, // if any. @@ -4908,6 +4894,25 @@ typedef enum : NSUInteger { } else { viewItem.unreadIndicator = nil; } + } + if (unreadIndicator) { + // This isn't necessarily a bug - all of the interactions after the + // unread indicator may have disappeared or been deleted. + DDLogWarn(@"%@ Couldn't find an interaction to hang the unread indicator on.", self.logTag); + } + + // Update the properties of the view items. + // + // NOTE: This logic uses the break properties which are set in the previous pass. + for (NSUInteger i = 0; i < viewItems.count; i++) { + ConversationViewItem *viewItem = viewItems[i]; + ConversationViewItem *_Nullable previousViewItem = (i > 0 ? viewItems[i - 1] : nil); + ConversationViewItem *_Nullable nextViewItem = (i + 1 < viewItems.count ? viewItems[i + 1] : nil); + BOOL shouldShowSenderAvatar = NO; + BOOL shouldHideFooter = NO; + BOOL isFirstInCluster = YES; + BOOL isLastInCluster = YES; + NSAttributedString *_Nullable senderName = nil; OWSInteractionType interactionType = viewItem.interaction.interactionType; NSString *timestampText = [DateUtil formatTimestampShort:viewItem.interaction.timestamp]; @@ -4930,14 +4935,14 @@ typedef enum : NSUInteger { // ...and always show the "disappearing messages" animation. shouldHideFooter = ([timestampText isEqualToString:nextTimestampText] && receiptStatus == nextReceiptStatus - && outgoingMessage.messageState != TSOutgoingMessageStateFailed && !nextViewItem.shouldShowDate - && !isDisappearingMessage); + && outgoingMessage.messageState != TSOutgoingMessageStateFailed + && !nextViewItem.hasDateOrUnreadBreak && !isDisappearingMessage); } // clustering if (previousViewItem == nil) { isFirstInCluster = YES; - } else if (viewItem.shouldShowDate) { + } else if (viewItem.hasDateOrUnreadBreak) { isFirstInCluster = YES; } else { isFirstInCluster = previousViewItem.interaction.interactionType != OWSInteractionType_OutgoingMessage; @@ -4945,7 +4950,7 @@ typedef enum : NSUInteger { if (nextViewItem == nil) { isLastInCluster = YES; - } else if (nextViewItem.shouldShowDate) { + } else if (nextViewItem.hasDateOrUnreadBreak) { isLastInCluster = YES; } else { isLastInCluster = nextViewItem.interaction.interactionType != OWSInteractionType_OutgoingMessage; @@ -4969,15 +4974,16 @@ typedef enum : NSUInteger { // We can skip the "incoming message status" footer in a cluster if the next message // has the same footer and no "date break" separates us. // ...but always show the "disappearing messages" animation. - shouldHideFooter = ([timestampText isEqualToString:nextTimestampText] && !nextViewItem.shouldShowDate && - [NSObject isNullableObject:nextIncomingSenderId equalTo:incomingSenderId] - && !isDisappearingMessage); + shouldHideFooter + = ([timestampText isEqualToString:nextTimestampText] && !nextViewItem.hasDateOrUnreadBreak && + [NSObject isNullableObject:nextIncomingSenderId equalTo:incomingSenderId] + && !isDisappearingMessage); } // clustering if (previousViewItem == nil) { isFirstInCluster = YES; - } else if (viewItem.shouldShowDate) { + } else if (viewItem.hasDateOrUnreadBreak) { isFirstInCluster = YES; } else if (previousViewItem.interaction.interactionType != OWSInteractionType_IncomingMessage) { isFirstInCluster = YES; @@ -4990,7 +4996,7 @@ typedef enum : NSUInteger { isLastInCluster = YES; } else if (nextViewItem.interaction.interactionType != OWSInteractionType_IncomingMessage) { isLastInCluster = YES; - } else if (nextViewItem.shouldShowDate) { + } else if (nextViewItem.hasDateOrUnreadBreak) { isLastInCluster = YES; } else { TSIncomingMessage *nextIncomingMessage = (TSIncomingMessage *)nextViewItem.interaction; @@ -5010,7 +5016,7 @@ typedef enum : NSUInteger { shouldShowSenderName = (![NSObject isNullableObject:previousIncomingSenderId equalTo:incomingSenderId] - || viewItem.shouldShowDate); + || viewItem.hasDateOrUnreadBreak); } if (shouldShowSenderName) { senderName = [self.contactsManager @@ -5027,7 +5033,7 @@ typedef enum : NSUInteger { shouldShowSenderAvatar = YES; if (nextViewItem && nextViewItem.interaction.interactionType == interactionType) { shouldShowSenderAvatar = (![NSObject isNullableObject:nextIncomingSenderId equalTo:incomingSenderId] - || nextViewItem.shouldShowDate); + || nextViewItem.hasDateOrUnreadBreak); } } } @@ -5038,11 +5044,6 @@ typedef enum : NSUInteger { viewItem.shouldHideFooter = shouldHideFooter; viewItem.senderName = senderName; } - if (unreadIndicator) { - // This isn't necessarily a bug - all of the interactions after the - // unread indicator may have disappeared or been deleted. - DDLogWarn(@"%@ Couldn't find an interaction to hang the unread indicator on.", self.logTag); - } self.viewItems = viewItems; self.viewItemCache = viewItemCache; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h index 94c2b0fc83..80c5dee70e 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h @@ -54,7 +54,7 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType); @property (nonatomic, readonly) BOOL isQuotedReply; @property (nonatomic, readonly) BOOL hasQuotedAttachment; @property (nonatomic, readonly) BOOL hasQuotedText; -@property (nonatomic, readonly) BOOL hasCellHeader; +@property (nonatomic, readonly) BOOL hasDateOrUnreadBreak; @property (nonatomic, readonly) BOOL isExpiringMessage; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m index edb6b0fc93..ec2ac7515b 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m @@ -150,7 +150,7 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType) return message.isExpiringMessage; } -- (BOOL)hasCellHeader +- (BOOL)hasDateOrUnreadBreak { return self.shouldShowDate || self.unreadIndicator; } @@ -277,7 +277,7 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType) { OWSAssert(previousLayoutItem); - if (self.hasCellHeader) { + if (self.hasDateOrUnreadBreak) { return OWSMessageHeaderViewDateHeaderVMargin; } From 96a8df5f8803df09660d839c90a051e0d2335fc8 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 13 Jul 2018 11:15:29 -0400 Subject: [PATCH 2/3] Fix "new unread indicator animation" issue. --- .../ConversationViewController.m | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 9b7858258d..41bd8fb7d6 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -4851,6 +4851,7 @@ typedef enum : NSUInteger { BOOL shouldShowDateOnNextViewItem = YES; uint64_t previousViewItemTimestamp = 0; OWSUnreadIndicator *_Nullable unreadIndicator = self.dynamicInteractions.unreadIndicator; + BOOL hasPlacedUnreadIndicator = NO; for (ConversationViewItem *viewItem in viewItems) { BOOL canShowDate = NO; switch (viewItem.interaction.interactionType) { @@ -4886,11 +4887,33 @@ typedef enum : NSUInteger { previousViewItemTimestamp = viewItemTimestamp; + // When a conversation without unread messages receives an incoming message, + // we call ensureDynamicInteractions to ensure that the unread indicator (etc.) + // state is updated accordingly. However this is done in a separate transaction. + // We don't want to show the incoming message _without_ an unread indicator and + // then immediately re-render it _with_ an unread indicator. + // + // To avoid this, we use a temporary instance of OWSUnreadIndicator whenever + // we find an unread message that _should_ have an unread indicator, but no + // unread indicator exists yet on dynamicInteractions. + BOOL isItemUnread = ([viewItem.interaction conformsToProtocol:@protocol(OWSReadTracking)] + && !((id)viewItem.interaction).wasRead); + if (isItemUnread && !unreadIndicator && !hasPlacedUnreadIndicator) { + + unreadIndicator = + [[OWSUnreadIndicator alloc] initUnreadIndicatorWithTimestamp:viewItem.interaction.timestamp + hasMoreUnseenMessages:NO + missingUnseenSafetyNumberChangeCount:0 + unreadIndicatorPosition:0 + firstUnseenInteractionTimestamp:viewItem.interaction.timestamp]; + } + // Place the unread indicator onto the first appropriate view item, // if any. if (unreadIndicator && viewItem.interaction.timestampForSorting >= unreadIndicator.timestamp) { viewItem.unreadIndicator = unreadIndicator; unreadIndicator = nil; + hasPlacedUnreadIndicator = YES; } else { viewItem.unreadIndicator = nil; } From 687efabede8584ec466316967f07f9fb7915fad6 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 13 Jul 2018 16:24:36 -0400 Subject: [PATCH 3/3] Respond to CR. --- .../ConversationView/Cells/OWSMessageCell.m | 4 ++-- .../ConversationViewController.m | 23 +++++++++---------- .../ConversationView/ConversationViewItem.h | 2 +- .../ConversationView/ConversationViewItem.m | 4 ++-- 4 files changed, 16 insertions(+), 17 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m index 76bbf744a3..50d0b0830c 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageCell.m @@ -143,7 +143,7 @@ NS_ASSUME_NONNULL_BEGIN [self.messageBubbleView configureViews]; [self.messageBubbleView loadContent]; - if (self.viewItem.hasDateOrUnreadBreak) { + if (self.viewItem.hasCellHeader) { CGFloat headerHeight = [self.headerView measureWithConversationViewItem:self.viewItem conversationStyle:self.conversationStyle] .height; @@ -342,7 +342,7 @@ NS_ASSUME_NONNULL_BEGIN OWSAssert(cellSize.width > 0 && cellSize.height > 0); - if (self.viewItem.hasDateOrUnreadBreak) { + if (self.viewItem.hasCellHeader) { cellSize.height += [self.headerView measureWithConversationViewItem:self.viewItem conversationStyle:self.conversationStyle] .height; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index 41bd8fb7d6..fb8c891426 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -4958,14 +4958,14 @@ typedef enum : NSUInteger { // ...and always show the "disappearing messages" animation. shouldHideFooter = ([timestampText isEqualToString:nextTimestampText] && receiptStatus == nextReceiptStatus - && outgoingMessage.messageState != TSOutgoingMessageStateFailed - && !nextViewItem.hasDateOrUnreadBreak && !isDisappearingMessage); + && outgoingMessage.messageState != TSOutgoingMessageStateFailed && !nextViewItem.hasCellHeader + && !isDisappearingMessage); } // clustering if (previousViewItem == nil) { isFirstInCluster = YES; - } else if (viewItem.hasDateOrUnreadBreak) { + } else if (viewItem.hasCellHeader) { isFirstInCluster = YES; } else { isFirstInCluster = previousViewItem.interaction.interactionType != OWSInteractionType_OutgoingMessage; @@ -4973,7 +4973,7 @@ typedef enum : NSUInteger { if (nextViewItem == nil) { isLastInCluster = YES; - } else if (nextViewItem.hasDateOrUnreadBreak) { + } else if (nextViewItem.hasCellHeader) { isLastInCluster = YES; } else { isLastInCluster = nextViewItem.interaction.interactionType != OWSInteractionType_OutgoingMessage; @@ -4997,16 +4997,15 @@ typedef enum : NSUInteger { // We can skip the "incoming message status" footer in a cluster if the next message // has the same footer and no "date break" separates us. // ...but always show the "disappearing messages" animation. - shouldHideFooter - = ([timestampText isEqualToString:nextTimestampText] && !nextViewItem.hasDateOrUnreadBreak && - [NSObject isNullableObject:nextIncomingSenderId equalTo:incomingSenderId] - && !isDisappearingMessage); + shouldHideFooter = ([timestampText isEqualToString:nextTimestampText] && !nextViewItem.hasCellHeader && + [NSObject isNullableObject:nextIncomingSenderId equalTo:incomingSenderId] + && !isDisappearingMessage); } // clustering if (previousViewItem == nil) { isFirstInCluster = YES; - } else if (viewItem.hasDateOrUnreadBreak) { + } else if (viewItem.hasCellHeader) { isFirstInCluster = YES; } else if (previousViewItem.interaction.interactionType != OWSInteractionType_IncomingMessage) { isFirstInCluster = YES; @@ -5019,7 +5018,7 @@ typedef enum : NSUInteger { isLastInCluster = YES; } else if (nextViewItem.interaction.interactionType != OWSInteractionType_IncomingMessage) { isLastInCluster = YES; - } else if (nextViewItem.hasDateOrUnreadBreak) { + } else if (nextViewItem.hasCellHeader) { isLastInCluster = YES; } else { TSIncomingMessage *nextIncomingMessage = (TSIncomingMessage *)nextViewItem.interaction; @@ -5039,7 +5038,7 @@ typedef enum : NSUInteger { shouldShowSenderName = (![NSObject isNullableObject:previousIncomingSenderId equalTo:incomingSenderId] - || viewItem.hasDateOrUnreadBreak); + || viewItem.hasCellHeader); } if (shouldShowSenderName) { senderName = [self.contactsManager @@ -5056,7 +5055,7 @@ typedef enum : NSUInteger { shouldShowSenderAvatar = YES; if (nextViewItem && nextViewItem.interaction.interactionType == interactionType) { shouldShowSenderAvatar = (![NSObject isNullableObject:nextIncomingSenderId equalTo:incomingSenderId] - || nextViewItem.hasDateOrUnreadBreak); + || nextViewItem.hasCellHeader); } } } diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h index 80c5dee70e..94c2b0fc83 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h @@ -54,7 +54,7 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType); @property (nonatomic, readonly) BOOL isQuotedReply; @property (nonatomic, readonly) BOOL hasQuotedAttachment; @property (nonatomic, readonly) BOOL hasQuotedText; -@property (nonatomic, readonly) BOOL hasDateOrUnreadBreak; +@property (nonatomic, readonly) BOOL hasCellHeader; @property (nonatomic, readonly) BOOL isExpiringMessage; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m index ec2ac7515b..edb6b0fc93 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m @@ -150,7 +150,7 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType) return message.isExpiringMessage; } -- (BOOL)hasDateOrUnreadBreak +- (BOOL)hasCellHeader { return self.shouldShowDate || self.unreadIndicator; } @@ -277,7 +277,7 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType) { OWSAssert(previousLayoutItem); - if (self.hasDateOrUnreadBreak) { + if (self.hasCellHeader) { return OWSMessageHeaderViewDateHeaderVMargin; }