From cac29a573aebd5fdc25971607b15e1aa3221712d Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 9 Jan 2024 12:58:20 +1100 Subject: [PATCH] Optimised jumping between messages to avoid excessive loading --- .../ConversationVC+Interaction.swift | 6 +- Session/Conversations/ConversationVC.swift | 161 +++++++++++------- .../Conversations/ConversationViewModel.swift | 29 ++-- .../Types/PagedDatabaseObserver.swift | 36 ++-- 4 files changed, 135 insertions(+), 97 deletions(-) diff --git a/Session/Conversations/ConversationVC+Interaction.swift b/Session/Conversations/ConversationVC+Interaction.swift index 6bceb2a12..78579e6b8 100644 --- a/Session/Conversations/ConversationVC+Interaction.swift +++ b/Session/Conversations/ConversationVC+Interaction.swift @@ -1077,7 +1077,11 @@ extension ConversationVC: return } - self.scrollToInteractionIfNeeded(with: interactionInfo, focusBehaviour: .highlight) + self.scrollToInteractionIfNeeded( + with: interactionInfo, + focusBehaviour: .highlight, + originalIndexPath: self.tableView.indexPath(for: cell) + ) // If the message contains both links and a LinkPreview, and the user tapped on // the LinkPreview; OR the message only contained a LinkPreview, then open the link diff --git a/Session/Conversations/ConversationVC.swift b/Session/Conversations/ConversationVC.swift index af0b20a24..7e56f524a 100644 --- a/Session/Conversations/ConversationVC.swift +++ b/Session/Conversations/ConversationVC.swift @@ -658,7 +658,8 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers Storage.shared.removeObserver(self?.viewModel.pagedDataObserver) // Swap the observing to the updated thread - self?.viewModel.swapToThread(updatedThreadId: unblindedId) + let newestVisibleMessageId: Int64? = self?.fullyVisibleCellViewModels()?.last?.id + self?.viewModel.swapToThread(updatedThreadId: unblindedId, focussedMessageId: newestVisibleMessageId) // Start observing changes again Storage.shared.addObserver(self?.viewModel.pagedDataObserver) @@ -961,14 +962,14 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers struct ItemChangeInfo { let isInsertAtTop: Bool let firstIndexIsVisible: Bool - let visibleIndexPath: IndexPath - let oldVisibleIndexPath: IndexPath + let visibleIndexPath: IndexPath? + let oldVisibleIndexPath: IndexPath? init( isInsertAtTop: Bool = false, firstIndexIsVisible: Bool = false, - visibleIndexPath: IndexPath = IndexPath(row: 0, section: 0), - oldVisibleIndexPath: IndexPath = IndexPath(row: 0, section: 0) + visibleIndexPath: IndexPath? = nil, + oldVisibleIndexPath: IndexPath? = nil ) { self.isInsertAtTop = isInsertAtTop self.firstIndexIsVisible = firstIndexIsVisible @@ -982,22 +983,39 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers let wasLoadingMore: Bool = self.isLoadingMore let wasOffsetCloseToBottom: Bool = self.isCloseToBottom let numItemsInUpdatedData: [Int] = updatedData.map { $0.elements.count } - let didSwapAllContent: Bool = (updatedData - .first(where: { $0.model == .messages })? - .elements - .contains(where: { - $0.id == self.viewModel.interactionData - .first(where: { $0.model == .messages })? + let didSwapAllContent: Bool = { + // The dynamic headers use negative id values so by using `compactMap` and returning + // null in those cases allows us to exclude them without another iteration via `filter` + let currentIds: Set = (self.viewModel.interactionData + .first { $0.model == .messages }? + .elements + .compactMap { $0.id > 0 ? $0.id : nil } + .asSet()) + .defaulting(to: []) + let updatedIds: Set = (updatedData + .first { $0.model == .messages }? .elements - .first? - .id - })) - .defaulting(to: false) - let itemChangeInfo: ItemChangeInfo? = { + .compactMap { $0.id > 0 ? $0.id : nil } + .asSet()) + .defaulting(to: []) + + return updatedIds.isDisjoint(with: currentIds) + }() + let itemChangeInfo: ItemChangeInfo = { guard isInsert, let oldSectionIndex: Int = self.viewModel.interactionData.firstIndex(where: { $0.model == .messages }), let newSectionIndex: Int = updatedData.firstIndex(where: { $0.model == .messages }), + let firstVisibleIndexPath: IndexPath = self.tableView.indexPathsForVisibleRows? + .filter({ + $0.section == oldSectionIndex && + self.viewModel.interactionData[$0.section].elements[$0.row].cellType != .dateHeader + }) + .sorted() + .first + else { return ItemChangeInfo() } + + guard let newFirstItemIndex: Int = updatedData[newSectionIndex].elements .firstIndex(where: { item -> Bool in // Since the first item is probably a `DateHeaderCell` (which would likely @@ -1011,20 +1029,28 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers item.id == messages[safe: 1]?.id ) }), - let firstVisibleIndexPath: IndexPath = self.tableView.indexPathsForVisibleRows? - .filter({ - $0.section == oldSectionIndex && - self.viewModel.interactionData[$0.section].elements[$0.row].cellType != .dateHeader - }) - .sorted() - .first, let newVisibleIndex: Int = updatedData[newSectionIndex].elements .firstIndex(where: { item in item.id == self.viewModel.interactionData[oldSectionIndex] .elements[firstVisibleIndexPath.row] .id }) - else { return nil } + else { + let oldTimestamps: [Int64] = self.viewModel.interactionData[oldSectionIndex] + .elements + .filter { $0.cellType != .dateHeader } + .map { $0.timestampMs } + let newTimestamps: [Int64] = updatedData[newSectionIndex] + .elements + .filter { $0.cellType != .dateHeader } + .map { $0.timestampMs } + + return ItemChangeInfo( + isInsertAtTop: ((newTimestamps.max() ?? Int64.max) < (oldTimestamps.min() ?? Int64.min)), + firstIndexIsVisible: (firstVisibleIndexPath.row == 0), + oldVisibleIndexPath: firstVisibleIndexPath + ) + } return ItemChangeInfo( isInsertAtTop: ( @@ -1039,25 +1065,25 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers ) }() - guard !isInsert || itemChangeInfo?.isInsertAtTop == true else { + guard !isInsert || (!didSwapAllContent && itemChangeInfo.isInsertAtTop) else { self.viewModel.updateInteractionData(updatedData) self.tableView.reloadData() - // Animate to the target interaction (or the bottom) after a slightly delay to prevent buggy - // animation conflicts + // If we had a focusedInteractionInfo then scroll to it (and hide the search + // result bar loading indicator) if let focusedInteractionInfo: Interaction.TimestampInfo = self.focusedInteractionInfo { - // If we had a focusedInteractionInfo then scroll to it (and hide the search - // result bar loading indicator) - let delay: DispatchTime = (didSwapAllContent ? - .now() : - (.now() + .milliseconds(100)) - ) - - DispatchQueue.main.asyncAfter(deadline: delay) { [weak self] in + self.tableView.afterNextLayoutSubviews(when: { _, _, _ in true }, then: { [weak self] in self?.searchController.resultsBar.stopLoading() self?.scrollToInteractionIfNeeded( with: focusedInteractionInfo, focusBehaviour: (self?.focusBehaviour ?? .none), + contentSwapLocation: { + switch (didSwapAllContent, itemChangeInfo.isInsertAtTop) { + case (true, true): return .earlier + case (true, false): return .later + default: return .none + } + }(), isAnimated: true ) @@ -1066,7 +1092,7 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers self?.isLoadingMore = false self?.autoLoadNextPageIfNeeded() } - } + }) } else if wasOffsetCloseToBottom && !wasLoadingMore && numItemsInserted < 5 { /// Scroll to the bottom if an interaction was just inserted and we either just sent a message or are close enough to the @@ -1096,8 +1122,8 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers /// /// Unfortunately the UITableView also does some weird things when updating (where it won't have updated it's internal data until /// after it performs the next layout); the below code checks a condition on layout and if it passes it calls a closure - if let itemChangeInfo: ItemChangeInfo = itemChangeInfo, itemChangeInfo.isInsertAtTop { - let oldCellRect: CGRect = self.tableView.rectForRow(at: itemChangeInfo.oldVisibleIndexPath) + if itemChangeInfo.isInsertAtTop, let visibleIndexPath: IndexPath = itemChangeInfo.visibleIndexPath, let oldVisibleIndexPath: IndexPath = itemChangeInfo.oldVisibleIndexPath { + let oldCellRect: CGRect = self.tableView.rectForRow(at: oldVisibleIndexPath) let oldCellTopOffset: CGFloat = (self.tableView.frame.minY - self.tableView.convert(oldCellRect, to: self.tableView.superview).minY) // The the user triggered the 'scrollToTop' animation (by tapping in the nav bar) then we @@ -1119,11 +1145,7 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers // a large number of cells when getting search results which are very far away // only to instantly start scrolling making the calculation redundant) UIView.performWithoutAnimation { - self?.tableView.scrollToRow( - at: itemChangeInfo.visibleIndexPath, - at: .top, - animated: false - ) + self?.tableView.scrollToRow(at: visibleIndexPath, at: .top, animated: false) self?.tableView.contentOffset.y += oldCellTopOffset } @@ -1179,7 +1201,7 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers deleteRowsAnimation: .fade, insertRowsAnimation: .none, reloadRowsAnimation: .none, - interrupt: { itemChangeInfo?.isInsertAtTop == true || $0.changeCount > ConversationViewModel.pageSize } + interrupt: { itemChangeInfo.isInsertAtTop || $0.changeCount > ConversationViewModel.pageSize } ) { [weak self] updatedData in self?.viewModel.updateInteractionData(updatedData) } @@ -1618,7 +1640,6 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers self.scrollToInteractionIfNeeded( with: lastInteractionInfo, position: .bottom, - isJumpingToLastInteraction: true, isAnimated: true ) return @@ -1827,7 +1848,8 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers with interactionInfo: Interaction.TimestampInfo, focusBehaviour: ConversationViewModel.FocusBehaviour = .none, position: UITableView.ScrollPosition = .middle, - isJumpingToLastInteraction: Bool = false, + contentSwapLocation: ConversationViewModel.ContentSwapLocation = .none, + originalIndexPath: IndexPath? = nil, isAnimated: Bool = true ) { // Store the info incase we need to load more data (call will be re-triggered) @@ -1850,18 +1872,10 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers self.searchController.resultsBar.startLoading() DispatchQueue.global(qos: .userInitiated).async { [weak self] in - if isJumpingToLastInteraction { - self?.viewModel.pagedDataObserver?.load(.jumpTo( - id: interactionInfo.id, - paddingForInclusive: 5 - )) - } - else { - self?.viewModel.pagedDataObserver?.load(.untilInclusive( - id: interactionInfo.id, - padding: 5 - )) - } + self?.viewModel.pagedDataObserver?.load(.jumpTo( + id: interactionInfo.id, + paddingForInclusive: 5 + )) } return } @@ -1938,12 +1952,41 @@ final class ConversationVC: BaseVC, SessionUtilRespondingViewController, Convers return } + // As an optimisation if the target cell is too far away we just reload the entire table instead of loading + // all intermediate messages, as a result the scroll animation can be buggy (as the contentOffset could + // actually end up on the wrong side of the destination before the scroll animation starts) + // + // To get around this we immediately jump to a position 10 cells above/below the destination and then scroll + // which appears as though the screen has properly scrolled between the messages + switch contentSwapLocation { + case .none: + if let originalIndexPath: IndexPath = originalIndexPath { + // Since we use `estimatedRowHeight` instead of an explicit height there is an annoying issue + // where the cells won't have their heights calculated correctly so jumping between cells can + // result in a scroll animation going the wrong direction - by jumping to the destination and + // back to the current cell all of the relevant cells will have their frames calculated correctly + // and the animation will look correct + self.tableView.scrollToRow(at: targetIndexPath, at: targetPosition, animated: false) + self.tableView.scrollToRow(at: originalIndexPath, at: targetPosition, animated: false) + } + + case .earlier: + let targetRow: Int = min(targetIndexPath.row + 10, self.viewModel.interactionData[messageSectionIndex].elements.count - 1) + + self.tableView.contentOffset = CGPoint(x: 0, y: self.tableView.rectForRow(at: IndexPath(row: targetRow, section: targetIndexPath.section)).midY) + + case .later: + let targetRow: Int = min(targetIndexPath.row - 10, 0) + + self.tableView.contentOffset = CGPoint(x: 0, y: self.tableView.rectForRow(at: IndexPath(row: targetRow, section: targetIndexPath.section)).midY) + } + self.tableView.scrollToRow(at: targetIndexPath, at: targetPosition, animated: true) } func fullyVisibleCellViewModels() -> [MessageViewModel]? { // We remove the 'Values.mediumSpacing' as that is the distance the table content appears above the input view - let tableVisualTop: CGFloat = tableView.frame.minY//(tableView.frame.minY - (tableView.contentInset.bottom - Values.mediumSpacing)) + let tableVisualTop: CGFloat = tableView.frame.minY let tableVisualBottom: CGFloat = (tableView.frame.maxY - (tableView.contentInset.bottom - Values.mediumSpacing)) guard diff --git a/Session/Conversations/ConversationViewModel.swift b/Session/Conversations/ConversationViewModel.swift index ad84f5090..a1efd8f6f 100644 --- a/Session/Conversations/ConversationViewModel.swift +++ b/Session/Conversations/ConversationViewModel.swift @@ -17,6 +17,14 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { case highlight } + // MARK: - ContentSwapLocation + + public enum ContentSwapLocation { + case none + case earlier + case later + } + // MARK: - Action public enum Action { @@ -172,12 +180,10 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { DispatchQueue.global(qos: .userInitiated).async { [weak self] in // If we don't have a `initialFocusedInfo` then default to `.pageBefore` (it'll query // from a `0` offset) - guard let initialFocusedInfo: Interaction.TimestampInfo = (focusedInteractionInfo ?? initialData?.initialUnreadInteractionInfo) else { - self?.pagedDataObserver?.load(.pageBefore) - return + switch (focusedInteractionInfo ?? initialData?.initialUnreadInteractionInfo) { + case .some(let info): self?.pagedDataObserver?.load(.initialPageAround(id: info.id)) + case .none: self?.pagedDataObserver?.load(.pageBefore) } - - self?.pagedDataObserver?.load(.initialPageAround(id: initialFocusedInfo.id)) } } @@ -785,14 +791,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { markAsReadTrigger.send((target, timestampMs)) } - public func swapToThread(updatedThreadId: String) { - let oldestMessageId: Int64? = self.interactionData - .filter { $0.model == .messages } - .first? - .elements - .first? - .id - + public func swapToThread(updatedThreadId: String, focussedMessageId: Int64?) { self.threadId = updatedThreadId self.observableThreadData = self.setupObservableThreadData(for: updatedThreadId) self.pagedDataObserver = self.setupPagedObserver( @@ -804,8 +803,8 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { // Try load everything up to the initial visible message, fallback to just the initial page of messages // if we don't have one - switch oldestMessageId { - case .some(let id): self.pagedDataObserver?.load(.untilInclusive(id: id, padding: 0)) + switch focussedMessageId { + case .some(let id): self.pagedDataObserver?.load(.initialPageAround(id: id)) case .none: self.pagedDataObserver?.load(.pageBefore) } } diff --git a/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift b/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift index 852e1e9b8..9612cea8f 100644 --- a/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift +++ b/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift @@ -633,11 +633,13 @@ public class PagedDatabaseObserver: TransactionObserver where ) else { return (nil, nil) } - // If the target id is within a single page of the current cached data - // then trigger the `untilInclusive` behaviour instead + // If the targetIndex is over a page before the current content or more than a page + // after the current content then we want to reload the entire content (to avoid + // loading an excessive amount of data), otherwise we should load all messages between + // the current content and the targetIndex (plus padding) guard - abs(targetIndex - cacheCurrentEndIndex) > currentPageInfo.pageSize || - abs(targetIndex - currentPageInfo.pageOffset) > currentPageInfo.pageSize + (targetIndex < (currentPageInfo.pageOffset - currentPageInfo.pageSize)) || + (targetIndex > (cacheCurrentEndIndex + currentPageInfo.pageSize)) else { let callback: () -> () = { self?.load(.untilInclusive(id: targetId, padding: paddingForInclusive)) @@ -650,14 +652,7 @@ public class PagedDatabaseObserver: TransactionObserver where let callback: () -> () = { self?.dataCache.mutate { $0 = DataCache() } self?.associatedRecords.forEach { $0.clearCache(db) } - self?.pageInfo.mutate { - $0 = PagedData.PageInfo( - pageSize: currentPageInfo.pageSize, - pageOffset: 0, - currentCount: 0, - totalCount: 0 - ) - } + self?.pageInfo.mutate { $0 = PagedData.PageInfo(pageSize: currentPageInfo.pageSize) } self?.load(.initialPageAround(id: targetId)) } @@ -908,9 +903,15 @@ public enum PagedData { case initialPageAround(id: SQLExpression) case pageBefore case pageAfter - case untilInclusive(id: SQLExpression, padding: Int) case jumpTo(id: SQLExpression, paddingForInclusive: Int) case reloadCurrent + + /// This will be used when `jumpTo` is called and the `id` is within a single `pageSize` of the currently + /// cached data (plus the padding amount) + /// + /// **Note:** If the id is already within the cache then this will do nothing (even if + /// the padding would mean more data should be loaded) + case untilInclusive(id: SQLExpression, padding: Int) } public enum Target { @@ -925,13 +926,6 @@ public enum PagedData { /// This will attempt to load a page of data after the last item in the cache case pageAfter - /// This will attempt to load all data between what is currently in the cache until the - /// specified id (plus the padding amount) - /// - /// **Note:** If the id is already within the cache then this will do nothing (even if - /// the padding would mean more data should be loaded) - case untilInclusive(id: ID, padding: Int) - /// This will jump to the specified id, loading a page around it and clearing out any /// data that was previously cached /// @@ -944,8 +938,6 @@ public enum PagedData { case .initialPageAround(let id): return .initialPageAround(id: id.sqlExpression) case .pageBefore: return .pageBefore case .pageAfter: return .pageAfter - case .untilInclusive(let id, let padding): - return .untilInclusive(id: id.sqlExpression, padding: padding) case .jumpTo(let id, let paddingForInclusive): return .jumpTo(id: id.sqlExpression, paddingForInclusive: paddingForInclusive)