Skip to content

Commit

Permalink
Stop the timeline from requesting back pagination whilst loading exis…
Browse files Browse the repository at this point in the history
…ting 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.
  • Loading branch information
pixlwave authored Jun 27, 2024
1 parent 41d40d8 commit d977efd
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 35 deletions.
2 changes: 1 addition & 1 deletion ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down
18 changes: 17 additions & 1 deletion ElementX/Sources/Services/Timeline/RoomTimelineProvider.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class RoomTimelineProvider: RoomTimelineProviderProtocol {

private var roomTimelineObservationToken: TaskHandle?

private let paginationStateSubject = CurrentValueSubject<PaginationState, Never>(.default)
private let paginationStateSubject = CurrentValueSubject<PaginationState, Never>(.initial)
var paginationState: PaginationState {
paginationStateSubject.value
}
Expand Down Expand Up @@ -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<Void, Never>?
/// 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]) {
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,20 @@ import Combine
import Foundation
import MatrixRustSDK

enum PaginationDirection: String {
case backwards, forwards
}

enum PaginationStatus {
case idle
case timelineEndReached
case paginating
}

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
}
Expand Down
99 changes: 68 additions & 31 deletions ElementX/Sources/Services/Timeline/TimelineProxy.swift
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ final class TimelineProxy: TimelineProxyProtocol {

private var backPaginationStatusObservationToken: TaskHandle?

private let backPaginationSubscriptionSubject = CurrentValueSubject<PaginationStatus, Never>(.idle)
// The default values don't matter here, they will be updated when calling subscribeToPagination.
private let backPaginationStatusSubject = CurrentValueSubject<PaginationStatus, Never>(.timelineEndReached)
private let forwardPaginationStatusSubject = CurrentValueSubject<PaginationStatus, Never>(.timelineEndReached)

let isLive: Bool
Expand All @@ -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) {
Expand All @@ -75,6 +81,22 @@ final class TimelineProxy: TimelineProxyProtocol {
}

func paginateBackwards(requestSize: UInt16) async -> Result<Void, TimelineProxyError> {
// 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<Void, TimelineProxyError> {
await focussedPaginate(.forwards, requestSize: requestSize)
}

/// Paginate backwards using the subscription from Rust to drive the pagination state.
private func paginateBackwardsOnLive(requestSize: UInt16) async -> Result<Void, TimelineProxyError> {
MXLog.info("Paginating backwards")

do {
Expand All @@ -88,26 +110,36 @@ final class TimelineProxy: TimelineProxyProtocol {
}
}

func paginateForwards(requestSize: UInt16) async -> Result<Void, TimelineProxyError> {
// 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<Void, TimelineProxyError> {
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))
}
}
Expand Down Expand Up @@ -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)
}
}
Expand Down

0 comments on commit d977efd

Please sign in to comment.