Skip to content

Commit

Permalink
Fixes #3088 - Update subscriptions if visible range rooms change with…
Browse files Browse the repository at this point in the history
…out the range itself changing

- we switched from range based subscribing with duplicate suppression to id based, but ids can change without ranges changing
- the all rooms list fetches at least 1 event per rooms and can cause rooms to come in and out of the visible range without user interaction
  • Loading branch information
stefanceriu committed Sep 2, 2024
1 parent 260777c commit f42b9fc
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 77 deletions.
2 changes: 1 addition & 1 deletion ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ enum HomeScreenViewAction {
case startChat
case confirmRecoveryKey
case skipRecoveryKeyConfirmation
case updateVisibleItemRange(range: Range<Int>, isScrolling: Bool)
case updateVisibleItemRange(Range<Int>)
case globalSearch
case markRoomAsUnread(roomIdentifier: String)
case markRoomAsRead(roomIdentifier: String)
Expand Down
45 changes: 2 additions & 43 deletions ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol

private var migrationCancellable: AnyCancellable?

private var visibleItemRangeObservationToken: AnyCancellable?
private let visibleItemRangePublisher = CurrentValueSubject<(range: Range<Int>, isScrolling: Bool), Never>((0..<0, false))

private var actionsSubject: PassthroughSubject<HomeScreenViewModelAction, Never> = .init()
var actions: AnyPublisher<HomeScreenViewModelAction, Never> {
actionsSubject.eraseToAnyPublisher()
Expand Down Expand Up @@ -151,8 +148,8 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol
actionsSubject.send(.presentSecureBackupSettings)
case .skipRecoveryKeyConfirmation:
state.securityBannerMode = .dismissed
case .updateVisibleItemRange(let range, let isScrolling):
visibleItemRangePublisher.send((range, isScrolling))
case .updateVisibleItemRange(let range):
roomSummaryProvider?.updateVisibleRange(range)
case .startChat:
actionsSubject.send(.presentStartChatScreen)
case .globalSearch:
Expand Down Expand Up @@ -266,14 +263,9 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol
.store(in: &cancellables)

roomSummaryProvider.roomListPublisher
.dropFirst(1) // We don't care about its initial value
.receive(on: DispatchQueue.main)
.sink { [weak self] _ in
self?.updateRooms()

// Wait for the all rooms view to receive its first update before installing
// dynamic timeline modifiers
self?.installListRangeModifiers()
}
.store(in: &cancellables)
}
Expand Down Expand Up @@ -315,26 +307,6 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol
}
}
}

private func installListRangeModifiers() {
guard visibleItemRangeObservationToken == nil else {
return
}

visibleItemRangeObservationToken = visibleItemRangePublisher
.filter { $0.isScrolling == false }
.throttle(for: 0.5, scheduler: DispatchQueue.main, latest: true)
.sink { [weak self] value in
guard let self else { return }

// Ignore scrolling while filtering rooms
guard self.state.bindings.searchQuery.isEmpty else {
return
}

self.updateVisibleRange(value.range)
}
}

private func updateRooms() {
guard let roomSummaryProvider else {
Expand All @@ -351,20 +323,7 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol

state.rooms = rooms
}

private func updateVisibleRange(_ range: Range<Int>) {
guard !range.isEmpty else {
return
}

guard let roomSummaryProvider else {
MXLog.error("Visible rooms summary provider unavailable")
return
}

roomSummaryProvider.updateVisibleRange(range)
}

private func markRoomAsFavourite(_ roomID: String, isFavourite: Bool) async {
guard case let .joined(roomProxy) = await userSession.clientProxy.roomForIdentifier(roomID) else {
MXLog.error("Failed retrieving room for identifier: \(roomID)")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,8 @@ struct HomeScreenContent: View {

private func delayedUpdateVisibleRange() {
guard let scrollView = scrollViewAdapter.scrollView,
scrollViewAdapter.isScrolling.value == false, // Ignore while scrolling
context.searchQuery.isEmpty == true, // Ignore while filtering
context.viewState.visibleRooms.count > 0 else {
return
}
Expand All @@ -215,6 +217,6 @@ struct HomeScreenContent: View {
let lastIndex = Int(max(0.0, scrollView.contentOffset.y + scrollView.bounds.height) / cellHeight)

// This will be deduped and throttled on the view model layer
context.send(viewAction: .updateVisibleItemRange(range: firstIndex..<lastIndex, isScrolling: scrollViewAdapter.isScrolling.value))
context.send(viewAction: .updateVisibleItemRange(firstIndex..<lastIndex))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol {

private let serialDispatchQueue: DispatchQueue

private let visibleItemRangePublisher = CurrentValueSubject<Range<Int>, Never>(0..<0)

// periphery:ignore - retaining purpose
private var roomList: RoomListProtocol?

Expand Down Expand Up @@ -80,6 +82,8 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol {
.sink { [weak self] in self?.updateRoomsWithDiffs($0) }
.store(in: &cancellables)

setupVisibleRangeObservers()

setupNotificationSettingsSubscription()
}

Expand Down Expand Up @@ -114,38 +118,7 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol {
}

func updateVisibleRange(_ range: Range<Int>) {
if range.upperBound >= rooms.count {
listUpdatesSubscriptionResult?.controller().addOnePage()
} else if range.lowerBound == 0 {
listUpdatesSubscriptionResult?.controller().resetToOnePage()
}

guard shouldUpdateVisibleRange else {
return
}

// The scroll view content size based visible range calculations might create large ranges
// This is just a safety check to not overload the backend
var range = range
if range.upperBound - range.lowerBound > SlidingSyncConstants.maximumVisibleRangeSize {
let upperBound = range.lowerBound + SlidingSyncConstants.maximumVisibleRangeSize
range = range.lowerBound..<upperBound
}

MXLog.info("\(name): Requesting subscriptions for visible range: \(range)")

let roomIDs = range
.filter { $0 < rooms.count }
.map { rooms[$0].id }

do {
try roomListService.subscribeToRooms(roomIds: roomIDs,
settings: .init(requiredState: SlidingSyncConstants.defaultRequiredState,
timelineLimit: SlidingSyncConstants.defaultTimelineLimit,
includeHeroes: false))
} catch {
MXLog.error("Failed subscribing to rooms with error: \(error)")
}
visibleItemRangePublisher.send(range)
}

func setFilter(_ filter: RoomSummaryProviderFilter) {
Expand All @@ -167,7 +140,63 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol {
}

// MARK: - Private

private func setupVisibleRangeObservers() {
visibleItemRangePublisher
.throttle(for: 0.5, scheduler: DispatchQueue.main, latest: true)
.removeDuplicates()
.sink { [weak self] range in
guard let self else { return }

MXLog.info("\(self.name): Updating visible range: \(range)")

if range.upperBound >= rooms.count {
listUpdatesSubscriptionResult?.controller().addOnePage()
} else if range.lowerBound == 0 {
listUpdatesSubscriptionResult?.controller().resetToOnePage()
}
}
.store(in: &cancellables)

visibleItemRangePublisher
.throttle(for: 0.5, scheduler: DispatchQueue.main, latest: true)
.filter { [weak self] range in
guard let self else { return false }
return !range.isEmpty && shouldUpdateVisibleRange
}
.compactMap { [weak self] (range: Range) -> [String]? in
guard let self else { return nil }

// The scroll view content size based visible range calculations might create large ranges
// This is just a safety check to not overload the backend
var range = range
if range.upperBound - range.lowerBound > SlidingSyncConstants.maximumVisibleRangeSize {
let upperBound = range.lowerBound + SlidingSyncConstants.maximumVisibleRangeSize
range = range.lowerBound..<upperBound
}

return range
.filter { $0 < self.rooms.count }
.map { self.rooms[$0].id }
}
.removeDuplicates()
.sink { [weak self] roomIDs in
guard let self else { return }

MXLog.debug("\(self.name): Updating subscriptions for visible rooms: \(roomIDs)")

do {
try roomListService.subscribeToRooms(roomIds: roomIDs,
settings: .init(requiredState: SlidingSyncConstants.defaultRequiredState,
timelineLimit: SlidingSyncConstants.defaultTimelineLimit,
includeHeroes: false))
} catch {
MXLog.error("Failed subscribing to rooms with error: \(error)")
}
}
.store(in: &cancellables)
}

fileprivate func updateRoomsWithDiffs(_ diffs: [RoomListEntriesUpdate]) {
let span = MXLog.createSpan("\(name).process_room_list_diffs")
span.enter()
Expand Down

0 comments on commit f42b9fc

Please sign in to comment.