From 90d8fb3d14e20a39da2b4d10903d957de8dcddfd Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 10 Dec 2018 11:59:00 -0500 Subject: [PATCH 1/2] Refine view model diffing. --- .../ColorPickerViewController.swift | 1 + .../Cells/OWSMessageBubbleView.m | 26 ++++++++- .../ConversationView/ConversationViewItem.h | 2 + .../ConversationView/ConversationViewItem.m | 4 ++ .../ConversationView/ConversationViewModel.m | 57 ++++++++++--------- 5 files changed, 61 insertions(+), 29 deletions(-) diff --git a/Signal/src/ViewControllers/ColorPickerViewController.swift b/Signal/src/ViewControllers/ColorPickerViewController.swift index beaff2faf..429a80cc7 100644 --- a/Signal/src/ViewControllers/ColorPickerViewController.swift +++ b/Signal/src/ViewControllers/ColorPickerViewController.swift @@ -340,6 +340,7 @@ private class MockConversationViewItem: NSObject, ConversationViewItem { var hasBodyTextActionContent: Bool = false var hasMediaActionContent: Bool = false var mediaAlbumItems: [ConversationMediaAlbumItem]? + var hasCachedLayoutState: Bool = false override init() { super.init() diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageBubbleView.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageBubbleView.m index 4b9adb061..73adb67f4 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageBubbleView.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSMessageBubbleView.m @@ -577,7 +577,6 @@ const UIDataDetectorTypes kOWSAllowedDataDetectorTypes case OWSMessageCellType_Unknown: case OWSMessageCellType_TextMessage: case OWSMessageCellType_OversizeTextMessage: - return NO; case OWSMessageCellType_Audio: case OWSMessageCellType_GenericAttachment: case OWSMessageCellType_DownloadingAttachment: @@ -588,6 +587,21 @@ const UIDataDetectorTypes kOWSAllowedDataDetectorTypes } } +- (BOOL)hasBodyMediaView { + switch (self.cellType) { + case OWSMessageCellType_Unknown: + case OWSMessageCellType_TextMessage: + case OWSMessageCellType_OversizeTextMessage: + return NO; + case OWSMessageCellType_Audio: + case OWSMessageCellType_GenericAttachment: + case OWSMessageCellType_DownloadingAttachment: + case OWSMessageCellType_ContactShare: + case OWSMessageCellType_MediaAlbum: + return YES; + } +} + - (BOOL)hasFullWidthMediaView { return (self.hasBodyMediaWithThumbnail || self.cellType == OWSMessageCellType_ContactShare @@ -601,8 +615,14 @@ const UIDataDetectorTypes kOWSAllowedDataDetectorTypes - (BOOL)hasBottomFooter { - BOOL shouldFooterOverlayMedia = (self.canFooterOverlayMedia && !self.hasBodyText); - return !self.viewItem.shouldHideFooter && !shouldFooterOverlayMedia; + BOOL shouldFooterOverlayMedia = (self.canFooterOverlayMedia && self.hasBodyMediaView && !self.hasBodyText); + if (self.viewItem.shouldHideFooter) { + return NO; + } else if (shouldFooterOverlayMedia) { + return NO; + } else { + return YES; + } } - (BOOL)insertAnyTextViewsIntoStackView:(NSArray *)textViews diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h index 048c191a1..d106c8249 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.h @@ -91,6 +91,8 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType); - (void)clearCachedLayoutState; +@property (nonatomic, readonly) BOOL hasCachedLayoutState; + #pragma mark - Audio Playback @property (nonatomic, weak) OWSAudioMessageView *lastAudioMessageView; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m index 7a2c75cfd..610fe74f2 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewItem.m @@ -286,6 +286,10 @@ NSString *NSStringForOWSMessageCellType(OWSMessageCellType cellType) self.cachedCellSize = nil; } +- (BOOL)hasCachedLayoutState { + return self.cachedCellSize != nil; +} + - (CGSize)cellSize { OWSAssertIsOnMainThread(); diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m index 07fca0c5c..26730524b 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m @@ -569,7 +569,6 @@ static const int kYapDatabaseRangeMinLength = 0; // reloadViewItems. BOOL hasMalformedRowChange = NO; NSMutableSet *updatedItemSet = [NSMutableSet new]; - NSMutableSet *updatedNeighborItemSet = [NSMutableSet new]; for (YapDatabaseViewRowChange *rowChange in rowChanges) { switch (rowChange.type) { case YapDatabaseViewChangeUpdate: { @@ -579,17 +578,10 @@ static const int kYapDatabaseRangeMinLength = 0; if (viewItem) { [self reloadInteractionForViewItem:viewItem]; [updatedItemSet addObject:viewItem.itemId]; - if (rowChange.changes == YapDatabaseViewChangedDependency) { - [updatedNeighborItemSet addObject:viewItem.itemId]; - } } else { OWSLogError(@"Update is missing view item"); hasMalformedRowChange = YES; } - } else if (rowChange.indexPath && rowChange.originalIndex < self.viewItems.count) { - // Do nothing, this is a pseudo-update generated due to - // setCellDrawingDependencyOffsets. - OWSAssertDebug(rowChange.changes == YapDatabaseViewChangedDependency); } else { OWSFailDebug(@"Update is missing collection key"); hasMalformedRowChange = YES; @@ -634,9 +626,7 @@ static const int kYapDatabaseRangeMinLength = 0; OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewItems.count); - [self updateViewWithOldItemIdList:oldItemIdList - updatedItemSet:updatedItemSet - updatedNeighborItemSet:updatedNeighborItemSet]; + [self updateViewWithOldItemIdList:oldItemIdList updatedItemSet:updatedItemSet]; } // A simpler version of the update logic we use when @@ -662,15 +652,13 @@ static const int kYapDatabaseRangeMinLength = 0; OWSLogVerbose(@"self.viewItems.count: %zd -> %zd", oldItemIdList.count, self.viewItems.count); - [self updateViewWithOldItemIdList:oldItemIdList updatedItemSet:[NSSet set] updatedNeighborItemSet:nil]; + [self updateViewWithOldItemIdList:oldItemIdList updatedItemSet:[NSSet set]]; } - (void)updateViewWithOldItemIdList:(NSArray *)oldItemIdList - updatedItemSet:(NSSet *)updatedItemSet - updatedNeighborItemSet:(nullable NSMutableSet *)updatedNeighborItemSet -{ + updatedItemSet:(NSSet *)updatedItemSetParam { OWSAssertDebug(oldItemIdList); - OWSAssertDebug(updatedItemSet); + OWSAssertDebug(updatedItemSetParam); if (!self.delegate.isObservingVMUpdates) { OWSLogVerbose(@"Skipping VM update."); @@ -771,6 +759,33 @@ static const int kYapDatabaseRangeMinLength = 0; return [self.delegate conversationViewModelDidUpdate:ConversationUpdate.reloadUpdate]; } + NSMutableSet *updatedItemSet = [updatedItemSetParam mutableCopy]; + NSMutableSet *updatedNeighborItemSet = [NSMutableSet new]; + for (NSString *itemId in newItemIdSet) { + if (![oldItemIdSet containsObject:itemId]) { + continue; + } + if ([insertedItemIdSet containsObject:itemId] || [updatedItemSet containsObject:itemId]) { + continue; + } + OWSAssertDebug(![deletedItemIdSet containsObject:itemId]); + + NSUInteger newIndex = [newItemIdList indexOfObject:itemId]; + if (newIndex == NSNotFound) { + OWSFailDebug(@"Can't find index of holdover view item."); + return [self.delegate conversationViewModelDidUpdate:ConversationUpdate.reloadUpdate]; + } + id _Nullable viewItem = newViewItemMap[itemId]; + if (!viewItem) { + OWSFailDebug(@"Can't find holdover view item."); + return [self.delegate conversationViewModelDidUpdate:ConversationUpdate.reloadUpdate]; + } + if (!viewItem.hasCachedLayoutState) { + [updatedItemSet addObject:itemId]; + [updatedNeighborItemSet addObject:itemId]; + } + } + // 3. Updates. // // NOTE: Order doesn't matter. @@ -867,16 +882,6 @@ static const int kYapDatabaseRangeMinLength = 0; [[YapDatabaseViewMappings alloc] initWithGroups:@[] view:TSMessageDatabaseViewExtensionName]; } - // Cells' appearance can depend on adjacent cells in both directions. - // - // TODO: We could do the same thing in our logic to generate "update items" - // in updateViewWithOldItemIdList. - [self.messageMappings setCellDrawingDependencyOffsets:[NSSet setWithArray:@[ - @(-1), - @(+1), - ]] - forGroup:self.thread.uniqueId]; - [self.uiDatabaseConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) { [self.messageMappings updateWithTransaction:transaction]; }]; From 85f6d05e0f6da2d2f5b642c4dc59c1bc717da983 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 10 Dec 2018 12:02:18 -0500 Subject: [PATCH 2/2] Refine view model diffing. --- .../ConversationView/ConversationViewModel.m | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m index 26730524b..c9720df78 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationViewModel.m @@ -759,6 +759,17 @@ static const int kYapDatabaseRangeMinLength = 0; return [self.delegate conversationViewModelDidUpdate:ConversationUpdate.reloadUpdate]; } + // In addition to "update" items from the database change notification, + // we may need to update other items. One example is neighbors of modified + // cells. Another is cells whose appearance has changed due to the passage + // of time. We detect "dirty" items by whether or not they have cached layout + // state, since that is cleared whenever we change the properties of the + // item that affect its appearance. + // + // This replaces the setCellDrawingDependencyOffsets/ + // YapDatabaseViewChangedDependency logic offered by YDB mappings, + // which only reflects changes in the data store, not at the view + // level. NSMutableSet *updatedItemSet = [updatedItemSetParam mutableCopy]; NSMutableSet *updatedNeighborItemSet = [NSMutableSet new]; for (NSString *itemId in newItemIdSet) {