From 288b1e78963127d933ffe467ec7ce388308f8dcc Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Mon, 2 Sep 2024 18:35:31 +0300 Subject: [PATCH] Update subscriptions if visible range rooms change without the range itself changing (#3220) * Remove room list range deduplication and allow subscribing to rooms when then update to come into the visible range. * Fixes #3088 - Update subscriptions if visible range rooms change without 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 --- .../Screens/HomeScreen/HomeScreenModels.swift | 2 +- .../HomeScreen/HomeScreenViewModel.swift | 46 +-------- .../HomeScreen/View/HomeScreenContent.swift | 4 +- .../RoomSummary/RoomSummaryProvider.swift | 93 ++++++++++++------- 4 files changed, 67 insertions(+), 78 deletions(-) diff --git a/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift b/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift index 7e20d0037..fd2b212ea 100644 --- a/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift +++ b/ElementX/Sources/Screens/HomeScreen/HomeScreenModels.swift @@ -40,7 +40,7 @@ enum HomeScreenViewAction { case startChat case confirmRecoveryKey case skipRecoveryKeyConfirmation - case updateVisibleItemRange(range: Range, isScrolling: Bool) + case updateVisibleItemRange(Range) case globalSearch case markRoomAsUnread(roomIdentifier: String) case markRoomAsRead(roomIdentifier: String) diff --git a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift index 16bedbfd8..2b4bd8b25 100644 --- a/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift +++ b/ElementX/Sources/Screens/HomeScreen/HomeScreenViewModel.swift @@ -30,9 +30,6 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol private var migrationCancellable: AnyCancellable? - private var visibleItemRangeObservationToken: AnyCancellable? - private let visibleItemRangePublisher = CurrentValueSubject<(range: Range, isScrolling: Bool), Never>((0..<0, false)) - private var actionsSubject: PassthroughSubject = .init() var actions: AnyPublisher { actionsSubject.eraseToAnyPublisher() @@ -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: @@ -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) } @@ -315,27 +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) - .removeDuplicates(by: { $0.isScrolling == $1.isScrolling && $0.range == $1.range }) - .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 { @@ -352,20 +323,7 @@ class HomeScreenViewModel: HomeScreenViewModelType, HomeScreenViewModelProtocol state.rooms = rooms } - - private func updateVisibleRange(_ range: Range) { - 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)") diff --git a/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift b/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift index bd3305b4c..7d4ad1c46 100644 --- a/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift +++ b/ElementX/Sources/Screens/HomeScreen/View/HomeScreenContent.swift @@ -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 } @@ -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.., Never>(0..<0) + // periphery:ignore - retaining purpose private var roomList: RoomListProtocol? @@ -80,6 +82,8 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol { .sink { [weak self] in self?.updateRoomsWithDiffs($0) } .store(in: &cancellables) + setupVisibleRangeObservers() + setupNotificationSettingsSubscription() } @@ -114,38 +118,7 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol { } func updateVisibleRange(_ range: Range) { - 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..= 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..