From 6ab8ea9b6ee46b3b292775e56a2f9dd02806e934 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 19 Mar 2019 11:13:06 -0400 Subject: [PATCH] Respond to CR. --- Signal/Signal-Info.plist | 2 +- .../ConversationViewController.m | 93 +++++++++---------- .../ConversationView/ConversationViewModel.h | 15 ++- .../ConversationView/ConversationViewModel.m | 82 ++++++++++------ .../MenuActionsViewController.swift | 6 +- 5 files changed, 118 insertions(+), 80 deletions(-) diff --git a/Signal/Signal-Info.plist b/Signal/Signal-Info.plist index 0e7535531..d5218f796 100644 --- a/Signal/Signal-Info.plist +++ b/Signal/Signal-Info.plist @@ -9,7 +9,7 @@ OSXVersion 10.14.3 WebRTCCommit - 55de5593cc261fa9368c5ccde98884ed1e278ba0 M72 + 1445d719bf05280270e9f77576f80f973fd847f8 M73 CFBundleDevelopmentRegion en diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m index c0b18ad11..9aa7c893d 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewController.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewController.m @@ -212,7 +212,7 @@ typedef enum : NSUInteger { @property (nonatomic, nullable) NSString *lastSearchedText; @property (nonatomic) BOOL isShowingSearchUI; @property (nonatomic, nullable) MenuActionsViewController *menuActionsViewController; -@property (nonatomic) CGFloat contentInsetPadding; +@property (nonatomic) CGFloat extraContentInsetPadding; @end @@ -749,7 +749,7 @@ typedef enum : NSUInteger { if (!self.viewHasEverAppeared) { [self scrollToDefaultPosition]; } else if (self.menuActionsViewController != nil) { - [self scrollToFocusInteraction:NO]; + [self scrollToMenuActionInteraction:NO]; } [self updateLastVisibleSortId]; @@ -763,7 +763,7 @@ typedef enum : NSUInteger { - (NSArray> *)viewItems { - return self.conversationViewModel.viewItems; + return self.conversationViewModel.viewState.viewItems; } - (ThreadDynamicInteractions *)dynamicInteractions @@ -773,14 +773,11 @@ typedef enum : NSUInteger { - (NSIndexPath *_Nullable)indexPathOfUnreadMessagesIndicator { - NSInteger row = 0; - for (id viewItem in self.viewItems) { - if (viewItem.unreadIndicator) { - return [NSIndexPath indexPathForRow:row inSection:0]; - } - row++; + NSNumber *_Nullable unreadIndicatorIndex = self.conversationViewModel.viewState.unreadIndicatorIndex; + if (unreadIndicatorIndex == nil) { + return nil; } - return nil; + return [NSIndexPath indexPathForRow:unreadIndicatorIndex.integerValue inSection:0]; } - (NSIndexPath *_Nullable)indexPathOfMessageOnOpen @@ -1981,11 +1978,11 @@ typedef enum : NSUInteger { // which we might want to scroll to the bottom of the screen to // pin above the menu actions popup. CGSize mainScreenSize = UIScreen.mainScreen.bounds.size; - self.contentInsetPadding = MAX(mainScreenSize.width, mainScreenSize.height); + self.extraContentInsetPadding = MAX(mainScreenSize.width, mainScreenSize.height); UIEdgeInsets contentInset = self.collectionView.contentInset; - contentInset.top += self.contentInsetPadding; - contentInset.bottom += self.contentInsetPadding; + contentInset.top += self.extraContentInsetPadding; + contentInset.bottom += self.extraContentInsetPadding; self.collectionView.contentInset = contentInset; self.menuActionsViewController = menuActionsViewController; @@ -1996,14 +1993,14 @@ typedef enum : NSUInteger { OWSLogVerbose(@""); // Changes made in this "is presenting" callback are animated by the caller. - [self scrollToFocusInteraction:NO]; + [self scrollToMenuActionInteraction:NO]; } - (void)menuActionsDidPresent:(MenuActionsViewController *)menuActionsViewController { OWSLogVerbose(@""); - [self scrollToFocusInteraction:NO]; + [self scrollToMenuActionInteraction:NO]; } - (void)menuActionsIsDismissing:(MenuActionsViewController *)menuActionsViewController @@ -2038,24 +2035,26 @@ typedef enum : NSUInteger { } UIEdgeInsets contentInset = self.collectionView.contentInset; - contentInset.top -= self.contentInsetPadding; - contentInset.bottom -= self.contentInsetPadding; + contentInset.top -= self.extraContentInsetPadding; + contentInset.bottom -= self.extraContentInsetPadding; self.collectionView.contentInset = contentInset; self.menuActionsViewController = nil; - self.contentInsetPadding = 0; + self.extraContentInsetPadding = 0; } -- (void)scrollToFocusInteractionIfNecessary +- (void)scrollToMenuActionInteractionIfNecessary { if (self.menuActionsViewController != nil) { - [self scrollToFocusInteraction:NO]; + [self scrollToMenuActionInteraction:NO]; } } -- (void)scrollToFocusInteraction:(BOOL)animated +- (void)scrollToMenuActionInteraction:(BOOL)animated { - NSValue *_Nullable contentOffset = [self contentOffsetForFocusInteraction]; + OWSAssertDebug(self.menuActionsViewController); + + NSValue *_Nullable contentOffset = [self contentOffsetForMenuActionInteraction]; if (contentOffset == nil) { OWSFailDebug(@"Missing contentOffset."); return; @@ -2063,11 +2062,13 @@ typedef enum : NSUInteger { [self.collectionView setContentOffset:contentOffset.CGPointValue animated:animated]; } -- (nullable NSValue *)contentOffsetForFocusInteraction +- (nullable NSValue *)contentOffsetForMenuActionInteraction { - NSString *_Nullable focusedInteractionId = self.menuActionsViewController.focusedInteraction.uniqueId; - if (focusedInteractionId == nil) { - // This is expected if there is no focus interaction. + OWSAssertDebug(self.menuActionsViewController); + + NSString *_Nullable menuActionInteractionId = self.menuActionsViewController.focusedInteraction.uniqueId; + if (menuActionInteractionId == nil) { + OWSFailDebug(@"Missing menu action interaction."); return nil; } CGPoint modalTopWindow = [self.menuActionsViewController.focusUI convertPoint:CGPointZero toView:nil]; @@ -2075,18 +2076,13 @@ typedef enum : NSUInteger { CGPoint offset = modalTopLocal; CGFloat focusTop = offset.y - self.menuActionsViewController.vSpacing; - NSIndexPath *_Nullable indexPath = nil; - for (NSUInteger i = 0; i < self.viewItems.count; i++) { - id viewItem = self.viewItems[i]; - if ([viewItem.interaction.uniqueId isEqualToString:focusedInteractionId]) { - indexPath = [NSIndexPath indexPathForRow:(NSInteger)i inSection:0]; - break; - } - } - if (indexPath == nil) { - // This is expected if the focus interaction is being deleted. + NSNumber *_Nullable interactionIndex + = self.conversationViewModel.viewState.interactionIndexMap[menuActionInteractionId]; + if (interactionIndex == nil) { + // This is expected if the menu action interaction is being deleted. return nil; } + NSIndexPath *indexPath = [NSIndexPath indexPathForRow:interactionIndex.integerValue inSection:0]; UICollectionViewLayoutAttributes *_Nullable layoutAttributes = [self.layout layoutAttributesForItemAtIndexPath:indexPath]; if (layoutAttributes == nil) { @@ -2109,16 +2105,12 @@ typedef enum : NSUInteger { if (!OWSWindowManager.sharedManager.isPresentingMenuActions) { return NO; } - NSString *_Nullable focusedInteractionId = self.menuActionsViewController.focusedInteraction.uniqueId; - if (focusedInteractionId == nil) { + NSString *_Nullable menuActionInteractionId = self.menuActionsViewController.focusedInteraction.uniqueId; + if (menuActionInteractionId == nil) { return NO; } - for (id viewItem in self.viewItems) { - if ([viewItem.interaction.uniqueId isEqualToString:focusedInteractionId]) { - return NO; - } - } - return YES; + // Check whether there is still a view item for this interaction. + return (self.conversationViewModel.viewState.interactionIndexMap[menuActionInteractionId] == nil); } #pragma mark - ConversationViewCellDelegate @@ -3857,8 +3849,8 @@ typedef enum : NSUInteger { newInsets.top = 0; newInsets.bottom = MAX(0, self.view.height - self.bottomLayoutGuide.length - keyboardEndFrameConverted.origin.y); - newInsets.top += self.contentInsetPadding; - newInsets.bottom += self.contentInsetPadding; + newInsets.top += self.extraContentInsetPadding; + newInsets.bottom += self.extraContentInsetPadding; BOOL wasScrolledToBottom = [self isScrolledToBottom]; @@ -4515,7 +4507,7 @@ typedef enum : NSUInteger { targetContentOffsetForProposedContentOffset:(CGPoint)proposedContentOffset { if (self.menuActionsViewController != nil) { - NSValue *_Nullable contentOffset = [self contentOffsetForFocusInteraction]; + NSValue *_Nullable contentOffset = [self contentOffsetForMenuActionInteraction]; if (contentOffset != nil) { return contentOffset.CGPointValue; } @@ -4768,6 +4760,11 @@ typedef enum : NSUInteger { OWSAssertDebug(conversationUpdate); OWSAssertDebug(self.conversationViewModel); + if (!self.viewLoaded) { + OWSLogVerbose(@"Ignoring update; view has not yet loaded."); + return; + } + [self updateBackButtonUnreadCount]; [self updateNavigationBarSubtitleLabel]; [self dismissMenuActionsIfNecessary]; @@ -5018,7 +5015,7 @@ typedef enum : NSUInteger { [strongSelf updateInputToolbarLayout]; if (self.menuActionsViewController != nil) { - [self scrollToFocusInteraction:NO]; + [self scrollToMenuActionInteraction:NO]; } else if (lastVisibleIndexPath) { [strongSelf.collectionView scrollToItemAtIndexPath:lastVisibleIndexPath atScrollPosition:UICollectionViewScrollPositionBottom diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h index b9fafcd2f..a4025f1f7 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.h @@ -34,6 +34,19 @@ typedef NS_ENUM(NSUInteger, ConversationUpdateItemType) { #pragma mark - +@interface ConversationViewState : NSObject + +@property (nonatomic, readonly) NSArray> *viewItems; +@property (nonatomic, readonly) NSDictionary *interactionIndexMap; +// We have to track interactionIds separately. We can't just use interactionIndexMap.allKeys, +// as that won't preserve ordering. +@property (nonatomic, readonly) NSArray *interactionIds; +@property (nonatomic, readonly, nullable) NSNumber *unreadIndicatorIndex; + +@end + +#pragma mark - + @interface ConversationUpdateItem : NSObject @property (nonatomic, readonly) ConversationUpdateItemType updateItemType; @@ -82,7 +95,7 @@ typedef NS_ENUM(NSUInteger, ConversationUpdateItemType) { @interface ConversationViewModel : NSObject -@property (nonatomic, readonly) NSArray> *viewItems; +@property (nonatomic, readonly) ConversationViewState *viewState; @property (nonatomic, nullable) NSString *focusMessageIdOnOpen; @property (nonatomic, readonly, nullable) ThreadDynamicInteractions *dynamicInteractions; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m index 2bb7925a9..3c34ab266 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m @@ -44,6 +44,50 @@ NS_ASSUME_NONNULL_BEGIN #pragma mark - +@implementation ConversationViewState + +- (instancetype)initWithViewItems:(NSArray> *)viewItems +{ + self = [super init]; + if (!self) { + return self; + } + + _viewItems = viewItems; + NSMutableDictionary *interactionIndexMap = [NSMutableDictionary new]; + NSMutableArray *interactionIds = [NSMutableArray new]; + for (NSUInteger i = 0; i < self.viewItems.count; i++) { + id viewItem = self.viewItems[i]; + interactionIndexMap[viewItem.interaction.uniqueId] = @(i); + [interactionIds addObject:viewItem.interaction.uniqueId]; + + if (viewItem.unreadIndicator != nil) { + _unreadIndicatorIndex = @(i); + } + } + _interactionIndexMap = [interactionIndexMap copy]; + _interactionIds = [interactionIds copy]; + + return self; +} + +- (nullable id)unreadIndicatorViewItem +{ + if (self.unreadIndicatorIndex == nil) { + return nil; + } + NSUInteger index = self.unreadIndicatorIndex.unsignedIntegerValue; + if (index >= self.viewItems.count) { + OWSFailDebug(@"Invalid index."); + return nil; + } + return self.viewItems[index]; +} + +@end + +#pragma mark - + @implementation ConversationUpdateItem - (instancetype)initWithUpdateItemType:(ConversationUpdateItemType)updateItemType @@ -150,7 +194,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; // * Afterward, we must prod the view controller to update layout & view state. @property (nonatomic) ConversationMessageMapping *messageMapping; -@property (nonatomic) NSArray> *viewItems; +@property (nonatomic) ConversationViewState *viewState; @property (nonatomic) NSMutableDictionary> *viewItemCache; @property (nonatomic, nullable) ThreadDynamicInteractions *dynamicInteractions; @@ -187,6 +231,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; _persistedViewItems = @[]; _unsavedOutgoingMessages = @[]; self.focusMessageIdOnOpen = focusMessageIdOnOpen; + _viewState = [[ConversationViewState alloc] initWithViewItems:@[]]; [self configure]; @@ -492,22 +537,12 @@ static const int kYapDatabaseRangeMaxLength = 25000; } } -- (nullable id)viewItemForUnreadMessagesIndicator -{ - for (id viewItem in self.viewItems) { - if (viewItem.unreadIndicator) { - return viewItem; - } - } - return nil; -} - - (void)clearUnreadMessagesIndicator { OWSAssertIsOnMainThread(); // TODO: Remove by making unread indicator a view model concern. - id _Nullable oldIndicatorItem = [self viewItemForUnreadMessagesIndicator]; + id _Nullable oldIndicatorItem = [self.viewState unreadIndicatorViewItem]; if (oldIndicatorItem) { // TODO ideally this would be happening within the *same* transaction that caused the unreadMessageIndicator // to be cleared. @@ -613,10 +648,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; } } - NSMutableArray *oldItemIdList = [NSMutableArray new]; - for (id viewItem in self.viewItems) { - [oldItemIdList addObject:viewItem.itemId]; - } + NSArray *oldItemIdList = self.viewState.interactionIds; // We need to reload any modified interactions _before_ we call // reloadViewItems. @@ -655,7 +687,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; return; } - OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewItems.count); + OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewState.viewItems.count); [self updateViewWithOldItemIdList:oldItemIdList updatedItemSet:updatedItemSet]; } @@ -668,10 +700,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; OWSLogVerbose(@""); - NSMutableArray *oldItemIdList = [NSMutableArray new]; - for (id viewItem in self.viewItems) { - [oldItemIdList addObject:viewItem.itemId]; - } + NSArray *oldItemIdList = self.viewState.interactionIds; if (![self reloadViewItems]) { // These errors are rare. @@ -682,7 +711,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; return; } - OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewItems.count); + OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewState.viewItems.count); [self updateViewWithOldItemIdList:oldItemIdList updatedItemSet:[NSSet set]]; } @@ -698,10 +727,9 @@ static const int kYapDatabaseRangeMaxLength = 25000; return; } - NSMutableArray *newItemIdList = [NSMutableArray new]; + NSArray *newItemIdList = self.viewState.interactionIds; NSMutableDictionary> *newViewItemMap = [NSMutableDictionary new]; - for (id viewItem in self.viewItems) { - [newItemIdList addObject:viewItem.itemId]; + for (id viewItem in self.viewState.viewItems) { newViewItemMap[viewItem.itemId] = viewItem; } @@ -1510,7 +1538,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; viewItem.senderName = senderName; } - self.viewItems = viewItems; + self.viewState = [[ConversationViewState alloc] initWithViewItems:viewItems]; self.viewItemCache = viewItemCache; return !hasError; @@ -1661,7 +1689,7 @@ static const int kYapDatabaseRangeMaxLength = 25000; // Update the view items if necessary. // We don't have to do this if they haven't been configured yet. - if (didChange && self.viewItems != nil) { + if (didChange && self.viewState.viewItems != nil) { // When we receive an incoming message, we clear any typing indicators // from that sender. Ideally, we'd like both changes (disappearance of // the typing indicators, appearance of the incoming message) to show up diff --git a/Signal/src/ViewControllers/MenuActionsViewController.swift b/Signal/src/ViewControllers/MenuActionsViewController.swift index c54dc0546..4e04f6c76 100644 --- a/Signal/src/ViewControllers/MenuActionsViewController.swift +++ b/Signal/src/ViewControllers/MenuActionsViewController.swift @@ -36,7 +36,7 @@ class MenuActionsViewController: UIViewController, MenuActionSheetDelegate { weak var delegate: MenuActionsViewControllerDelegate? @objc - public let focusedInteraction: TSInteraction? + public let focusedInteraction: TSInteraction private let focusedView: UIView private let actionSheetView: MenuActionSheetView @@ -46,7 +46,7 @@ class MenuActionsViewController: UIViewController, MenuActionSheetDelegate { } @objc - required init(focusedInteraction: TSInteraction?, focusedView: UIView, actions: [MenuAction]) { + required init(focusedInteraction: TSInteraction, focusedView: UIView, actions: [MenuAction]) { self.focusedView = focusedView self.focusedInteraction = focusedInteraction @@ -180,7 +180,7 @@ class MenuActionsViewController: UIViewController, MenuActionSheetDelegate { public let vSpacing: CGFloat = 10 @objc - public func focusUI() -> UIView { + public var focusUI: UIView { return actionSheetView }