From d977efdf3175de621c61b9674edecc19fdea6079 Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Thu, 27 Jun 2024 14:20:44 +0100 Subject: [PATCH] Stop the timeline from requesting back pagination whilst loading existing items. (#2969) * Handle Rust error "can't subscribe to the back-pagination status on a focused timeline" * Fix a bug where the timeline was attempting to paginate before adding the items. * Stop the timeline from paginating whilst loading existing items. --- .../Screens/RoomScreen/RoomScreenModels.swift | 2 +- .../TimelineTableViewController.swift | 2 +- .../Timeline/RoomTimelineProvider.swift | 18 +++- .../RoomTimelineProviderProtocol.swift | 8 +- .../Services/Timeline/TimelineProxy.swift | 99 +++++++++++++------ 5 files changed, 94 insertions(+), 35 deletions(-) diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift index 257f84582..d4221e698 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift @@ -229,7 +229,7 @@ struct RoomMemberState { /// Is also nice to have this as a wrapper for any state that is directly connected to the timeline. struct TimelineViewState { var isLive = true - var paginationState = PaginationState.default + var paginationState = PaginationState.initial /// The room is in the process of loading items from a new timeline (switching to/from a detached timeline). var isSwitchingTimelines = false diff --git a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift b/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift index 54b2a9bae..8cc07bae6 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift @@ -117,7 +117,7 @@ class TimelineTableViewController: UIViewController { } /// The state of pagination (in both directions) of the current timeline. - var paginationState: PaginationState = .default { + var paginationState: PaginationState = .initial { didSet { // Paginate again if the threshold hasn't been satisfied. paginatePublisher.send(()) diff --git a/ElementX/Sources/Services/Timeline/RoomTimelineProvider.swift b/ElementX/Sources/Services/Timeline/RoomTimelineProvider.swift index f27776f72..0df378223 100644 --- a/ElementX/Sources/Services/Timeline/RoomTimelineProvider.swift +++ b/ElementX/Sources/Services/Timeline/RoomTimelineProvider.swift @@ -24,7 +24,7 @@ class RoomTimelineProvider: RoomTimelineProviderProtocol { private var roomTimelineObservationToken: TaskHandle? - private let paginationStateSubject = CurrentValueSubject(.default) + private let paginationStateSubject = CurrentValueSubject(.initial) var paginationState: PaginationState { paginationStateSubject.value } @@ -74,6 +74,17 @@ class RoomTimelineProvider: RoomTimelineProviderProtocol { } } + /// A continuation to signal whether the initial timeline items have been loaded and processed. + private var hasLoadedInitialItemsContinuation: CheckedContinuation? + /// A method that allows `await`ing the first update of timeline items from the listener, as the items + /// aren't added directly to the provider upon initialisation and may take some time to come in. + func waitForInitialItems() async { + guard itemProxies.isEmpty else { return } + return await withCheckedContinuation { continuation in + hasLoadedInitialItemsContinuation = continuation + } + } + // MARK: - Private private func updateItemsWithDiffs(_ diffs: [TimelineDiff]) { @@ -100,6 +111,11 @@ class RoomTimelineProvider: RoomTimelineProviderProtocol { } MXLog.verbose("Finished applying diffs, current items (\(itemProxies.count)) : \(itemProxies.map(\.debugIdentifier))") + + if let hasLoadedInitialItemsContinuation { + hasLoadedInitialItemsContinuation.resume() + self.hasLoadedInitialItemsContinuation = nil + } } // swiftlint:disable:next cyclomatic_complexity diff --git a/ElementX/Sources/Services/Timeline/RoomTimelineProviderProtocol.swift b/ElementX/Sources/Services/Timeline/RoomTimelineProviderProtocol.swift index 0975f60a9..ee84e7fb8 100644 --- a/ElementX/Sources/Services/Timeline/RoomTimelineProviderProtocol.swift +++ b/ElementX/Sources/Services/Timeline/RoomTimelineProviderProtocol.swift @@ -18,6 +18,10 @@ import Combine import Foundation import MatrixRustSDK +enum PaginationDirection: String { + case backwards, forwards +} + enum PaginationStatus { case idle case timelineEndReached @@ -25,7 +29,9 @@ enum PaginationStatus { } struct PaginationState: Equatable { - static var `default` = PaginationState(backward: .idle, forward: .timelineEndReached) + /// An initial state that is used to prevent pagination whilst loading the timeline. + /// Once the initial items are loaded the TimelineProxy will publish the correct value. + static var initial = PaginationState(backward: .timelineEndReached, forward: .timelineEndReached) let backward: PaginationStatus let forward: PaginationStatus } diff --git a/ElementX/Sources/Services/Timeline/TimelineProxy.swift b/ElementX/Sources/Services/Timeline/TimelineProxy.swift index cac6aaaab..d5258e906 100644 --- a/ElementX/Sources/Services/Timeline/TimelineProxy.swift +++ b/ElementX/Sources/Services/Timeline/TimelineProxy.swift @@ -23,7 +23,8 @@ final class TimelineProxy: TimelineProxyProtocol { private var backPaginationStatusObservationToken: TaskHandle? - private let backPaginationSubscriptionSubject = CurrentValueSubject(.idle) + // The default values don't matter here, they will be updated when calling subscribeToPagination. + private let backPaginationStatusSubject = CurrentValueSubject(.timelineEndReached) private let forwardPaginationStatusSubject = CurrentValueSubject(.timelineEndReached) let isLive: Bool @@ -48,14 +49,19 @@ final class TimelineProxy: TimelineProxyProtocol { return } - let paginationStatePublisher = backPaginationSubscriptionSubject + let paginationStatePublisher = backPaginationStatusSubject .combineLatest(forwardPaginationStatusSubject) .map { PaginationState(backward: $0.0, forward: $0.1) } .eraseToAnyPublisher() await subscribeToPagination() - innerTimelineProvider = await RoomTimelineProvider(timeline: timeline, isLive: isLive, paginationStatePublisher: paginationStatePublisher) + let provider = await RoomTimelineProvider(timeline: timeline, isLive: isLive, paginationStatePublisher: paginationStatePublisher) + // Make sure the existing items are built so that we have content in the timeline before + // determining whether or not the timeline should paginate to load more items. + await provider.waitForInitialItems() + + innerTimelineProvider = provider } func fetchDetails(for eventID: String) { @@ -75,6 +81,22 @@ final class TimelineProxy: TimelineProxyProtocol { } func paginateBackwards(requestSize: UInt16) async -> Result { + // We can't subscribe to back pagination on detached timelines and as live timelines + // can be shared between multiple instances of the same room on the stack, it is + // safer to still use the subscription logic for back pagination when live. + await if isLive { + paginateBackwardsOnLive(requestSize: requestSize) + } else { + focussedPaginate(.backwards, requestSize: requestSize) + } + } + + func paginateForwards(requestSize: UInt16) async -> Result { + await focussedPaginate(.forwards, requestSize: requestSize) + } + + /// Paginate backwards using the subscription from Rust to drive the pagination state. + private func paginateBackwardsOnLive(requestSize: UInt16) async -> Result { MXLog.info("Paginating backwards") do { @@ -88,26 +110,36 @@ final class TimelineProxy: TimelineProxyProtocol { } } - func paginateForwards(requestSize: UInt16) async -> Result { - // This extra check is necessary as forwards pagination status doesn't support subscribing. + /// Paginate forward or backwards using our own logic to drive the pagination state as the + /// Rust subscription isn't allowed on focussed/detached timelines. + private func focussedPaginate(_ direction: PaginationDirection, requestSize: UInt16) async -> Result { + let subject = switch direction { + case .backwards: backPaginationStatusSubject + case .forwards: forwardPaginationStatusSubject + } + + // This extra check is necessary as detached timelines don't support subscribing to pagination status. // We need it to make sure we send a valid status after a failure. - guard forwardPaginationStatusSubject.value == .idle else { - MXLog.error("Attempting to paginate forwards when already at the end.") + guard subject.value == .idle else { + MXLog.error("Attempting to paginate \(direction.rawValue) when already at the end.") return .failure(.failedPaginatingEndReached) } - - MXLog.info("Paginating forwards") - forwardPaginationStatusSubject.send(.paginating) - + + MXLog.info("Paginating \(direction.rawValue)") + subject.send(.paginating) + do { - let timelineEndReached = try await timeline.focusedPaginateForwards(numEvents: requestSize) - MXLog.info("Finished paginating forwards") + let timelineEndReached = try await switch direction { + case .backwards: timeline.paginateBackwards(numEvents: requestSize) + case .forwards: timeline.focusedPaginateForwards(numEvents: requestSize) + } + MXLog.info("Finished paginating \(direction.rawValue)") - forwardPaginationStatusSubject.send(timelineEndReached ? .timelineEndReached : .idle) + subject.send(timelineEndReached ? .timelineEndReached : .idle) return .success(()) } catch { - MXLog.error("Failed paginating forwards with error: \(error)") - forwardPaginationStatusSubject.send(.idle) + MXLog.error("Failed paginating \(direction.rawValue) with error: \(error)") + subject.send(.idle) return .failure(.sdkError(error)) } } @@ -499,26 +531,31 @@ final class TimelineProxy: TimelineProxyProtocol { } private func subscribeToPagination() async { - let backPaginationListener = RoomPaginationStatusListener { [weak self] status in - guard let self else { - return + if isLive { + let backPaginationListener = RoomPaginationStatusListener { [weak self] status in + guard let self else { + return + } + + switch status { + case .idle(let hitStartOfTimeline): + backPaginationStatusSubject.send(hitStartOfTimeline ? .timelineEndReached : .idle) + case .paginating: + backPaginationStatusSubject.send(.paginating) + } } - switch status { - case .idle(let hitStartOfTimeline): - backPaginationSubscriptionSubject.send(hitStartOfTimeline ? .timelineEndReached : .idle) - case .paginating: - backPaginationSubscriptionSubject.send(.paginating) + do { + backPaginationStatusObservationToken = try await timeline.subscribeToBackPaginationStatus(listener: backPaginationListener) + } catch { + MXLog.error("Failed to subscribe to back pagination status with error: \(error)") } + } else { + // Detached timelines don't support observation, set the initial state ourself. + backPaginationStatusSubject.send(.idle) } - do { - backPaginationStatusObservationToken = try await timeline.subscribeToBackPaginationStatus(listener: backPaginationListener) - } catch { - MXLog.error("Failed to subscribe to back pagination status with error: \(error)") - } - - // Forward pagination doesn't support observation, set the initial state ourself. + // Detached timelines don't support observation, set the initial state ourself. forwardPaginationStatusSubject.send(isLive ? .timelineEndReached : .idle) } }