From 1f12a7cb2f1da834329dc536039c7bad0dbfee78 Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Mon, 9 Oct 2023 14:59:31 +0300 Subject: [PATCH] Fixes 1864 - Message forwarding room list filtering and pagination problems (#1867) * Fixes 1864 - Message forwarding room list filtering and pagination problems * Fix SwiftFormat redundant closures body error * Prevent crashes when writing to the localization bundle cache from different threads * Fix unit tests * Fix unit tests * Address PR comments --- .../RoomFlowCoordinator.swift | 2 +- .../Sources/Other/Extensions/Bundle.swift | 5 ++- ...THTMLElement+AttributedStringBuilder.swift | 16 ++++---- .../MessageForwardingScreenModels.swift | 11 +---- .../MessageForwardingScreenViewModel.swift | 35 ++++++++++++++++ .../View/MessageForwardingScreen.swift | 11 ++++- .../Sources/Services/Client/ClientProxy.swift | 18 +++++++-- .../Services/Client/ClientProxyProtocol.swift | 2 + .../Services/Client/MockClientProxy.swift | 2 + .../RoomSummary/MockRoomSummaryProvider.swift | 40 +++++++++++++++---- .../RoomSummary/RoomSummaryProvider.swift | 24 ++++++++--- ...essageForwardingScreenViewModelTests.swift | 9 ++++- .../test_messageForwardingScreen.1.png | 4 +- changelog.d/1864.bugfix | 1 + 14 files changed, 139 insertions(+), 41 deletions(-) create mode 100644 changelog.d/1864.bugfix diff --git a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift index 2d2b48425..7e7002ef9 100644 --- a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift @@ -684,7 +684,7 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { } private func presentMessageForwarding(for itemID: TimelineItemIdentifier) { - guard let roomProxy, let roomSummaryProvider = userSession.clientProxy.roomSummaryProvider, let eventID = itemID.eventID else { + guard let roomProxy, let roomSummaryProvider = userSession.clientProxy.messageForwardingRoomSummaryProvider, let eventID = itemID.eventID else { fatalError() } diff --git a/ElementX/Sources/Other/Extensions/Bundle.swift b/ElementX/Sources/Other/Extensions/Bundle.swift index 9ddaa8b4a..f41fc3e50 100644 --- a/ElementX/Sources/Other/Extensions/Bundle.swift +++ b/ElementX/Sources/Other/Extensions/Bundle.swift @@ -47,7 +47,10 @@ public extension Bundle { } let bundle = Bundle(url: lprojURL) - cachedLocalizationBundles[language] = bundle + + DispatchQueue.main.async { + cachedLocalizationBundles[language] = bundle + } return bundle } diff --git a/ElementX/Sources/Other/HTMLParsing/DTHTMLElement+AttributedStringBuilder.swift b/ElementX/Sources/Other/HTMLParsing/DTHTMLElement+AttributedStringBuilder.swift index 7b7d7da71..be8bb482b 100644 --- a/ElementX/Sources/Other/HTMLParsing/DTHTMLElement+AttributedStringBuilder.swift +++ b/ElementX/Sources/Other/HTMLParsing/DTHTMLElement+AttributedStringBuilder.swift @@ -64,13 +64,11 @@ public extension DTHTMLElement { } } - private static var allowedHTMLTags = { - ["font", // custom to matrix for IRC-style font coloring - "del", // for markdown - "body", // added internally by DTCoreText - "mx-reply", - "h1", "h2", "h3", "h4", "h5", "h6", "blockquote", "p", "a", "ul", "ol", - "nl", "li", "b", "i", "u", "strong", "em", "strike", "code", "hr", "br", "div", - "table", "thead", "caption", "tbody", "tr", "th", "td", "pre"] - }() + private static let allowedHTMLTags = ["font", // custom to matrix for IRC-style font coloring + "del", // for markdown + "body", // added internally by DTCoreText + "mx-reply", + "h1", "h2", "h3", "h4", "h5", "h6", "blockquote", "p", "a", "ul", "ol", + "nl", "li", "b", "i", "u", "strong", "em", "strike", "code", "hr", "br", "div", + "table", "thead", "caption", "tbody", "tr", "th", "td", "pre"] } diff --git a/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenModels.swift b/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenModels.swift index ff2e3dd45..bd67b5d25 100644 --- a/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenModels.swift +++ b/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenModels.swift @@ -24,15 +24,6 @@ enum MessageForwardingScreenViewModelAction { struct MessageForwardingScreenViewState: BindableState { var rooms: [MessageForwardingRoom] = [] var selectedRoomID: String? - - var visibleRooms: [MessageForwardingRoom] { - if bindings.searchQuery.isEmpty { - return rooms - } - - return rooms.filter { $0.name.localizedStandardContains(bindings.searchQuery) } - } - var bindings = MessageForwardingScreenViewStateBindings() } @@ -44,6 +35,8 @@ enum MessageForwardingScreenViewAction { case cancel case send case selectRoom(roomID: String) + case reachedTop + case reachedBottom } struct MessageForwardingRoom: Identifiable, Equatable { diff --git a/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenViewModel.swift b/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenViewModel.swift index 36d35f728..d0ae03c79 100644 --- a/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenViewModel.swift +++ b/ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenViewModel.swift @@ -43,6 +43,15 @@ class MessageForwardingScreenViewModel: MessageForwardingScreenViewModelType, Me } .store(in: &cancellables) + context.$viewState + .map(\.bindings.searchQuery) + .removeDuplicates() + .sink { [weak self] searchQuery in + guard let self else { return } + self.roomSummaryProvider?.setFilter(.normalizedMatchRoomName(searchQuery)) + } + .store(in: &cancellables) + updateRooms() } @@ -58,6 +67,10 @@ class MessageForwardingScreenViewModel: MessageForwardingScreenViewModelType, Me actionsSubject.send(.send(roomID: roomID)) case .selectRoom(let roomID): state.selectedRoomID = roomID + case .reachedTop: + updateVisibleRange(edge: .top) + case .reachedBottom: + updateVisibleRange(edge: .bottom) } } @@ -91,4 +104,26 @@ class MessageForwardingScreenViewModel: MessageForwardingScreenViewModelType, Me MXLog.verbose("Finished updating rooms") } + + /// The actual range values don't matter as long as they contain the lower + /// or upper bounds. updateVisibleRange is a hybrid API that powers both + /// sliding sync visible range update and list paginations + /// For lists other than the home screen one we don't care about visible ranges, + /// we just need the respective bounds to be there to trigger a next page load or + /// a reset to just one page + private func updateVisibleRange(edge: UIRectEdge) { + guard let roomSummaryProvider else { + return + } + + switch edge { + case .top: + roomSummaryProvider.updateVisibleRange(0..<0) + case .bottom: + let roomCount = roomSummaryProvider.roomListPublisher.value.count + roomSummaryProvider.updateVisibleRange(roomCount.. { CurrentValueSubject(nil).asCurrentValuePublisher() } diff --git a/ElementX/Sources/Services/Room/RoomSummary/MockRoomSummaryProvider.swift b/ElementX/Sources/Services/Room/RoomSummary/MockRoomSummaryProvider.swift index 324b9c8e4..6d6f3b710 100644 --- a/ElementX/Sources/Services/Room/RoomSummary/MockRoomSummaryProvider.swift +++ b/ElementX/Sources/Services/Room/RoomSummary/MockRoomSummaryProvider.swift @@ -24,8 +24,17 @@ enum MockRoomSummaryProviderState { } class MockRoomSummaryProvider: RoomSummaryProviderProtocol { - let roomListPublisher: CurrentValuePublisher<[RoomSummary], Never> - let statePublisher: CurrentValuePublisher + private let initialRooms: [RoomSummary] + + private let roomListSubject: CurrentValueSubject<[RoomSummary], Never> + var roomListPublisher: CurrentValuePublisher<[RoomSummary], Never> { + roomListSubject.asCurrentValuePublisher() + } + + private let stateSuject: CurrentValueSubject + var statePublisher: CurrentValuePublisher { + stateSuject.asCurrentValuePublisher() + } convenience init() { self.init(state: .loading) @@ -34,11 +43,15 @@ class MockRoomSummaryProvider: RoomSummaryProviderProtocol { init(state: MockRoomSummaryProviderState) { switch state { case .loading: - roomListPublisher = .init([]) - statePublisher = .init(.notLoaded) + initialRooms = [] + roomListSubject = .init(initialRooms) + roomListSubject.send(initialRooms) + stateSuject = .init(.notLoaded) case .loaded(let rooms): - roomListPublisher = .init(rooms) - statePublisher = .init(.loaded(totalNumberOfRooms: UInt(rooms.count))) + initialRooms = rooms + roomListSubject = .init(initialRooms) + roomListSubject.send(initialRooms) + stateSuject = .init(.loaded(totalNumberOfRooms: UInt(initialRooms.count))) } } @@ -46,7 +59,20 @@ class MockRoomSummaryProvider: RoomSummaryProviderProtocol { func updateVisibleRange(_ range: Range) { } - func setFilter(_ filter: RoomSummaryProviderFilter) { } + func setFilter(_ filter: RoomSummaryProviderFilter) { + switch filter { + case .all: + roomListSubject.send(initialRooms) + case .none: + roomListSubject.send([]) + case .normalizedMatchRoomName(let filter): + if filter.isEmpty { + roomListSubject.send(initialRooms) + } else { + roomListSubject.send(initialRooms.filter { $0.name?.localizedCaseInsensitiveContains(filter) ?? false }) + } + } + } } extension Array where Element == RoomSummary { diff --git a/ElementX/Sources/Services/Room/RoomSummary/RoomSummaryProvider.swift b/ElementX/Sources/Services/Room/RoomSummary/RoomSummaryProvider.swift index 03af1b8e1..968791ea8 100644 --- a/ElementX/Sources/Services/Room/RoomSummary/RoomSummaryProvider.swift +++ b/ElementX/Sources/Services/Room/RoomSummary/RoomSummaryProvider.swift @@ -22,6 +22,7 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol { private let roomListService: RoomListServiceProtocol private let eventStringBuilder: RoomEventStringBuilder private let name: String + private let shouldUpdateVisibleRange: Bool private let notificationSettings: NotificationSettingsProxyProtocol private let backgroundTaskService: BackgroundTaskServiceProtocol @@ -56,15 +57,22 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol { } } + /// Build a new summary provider with the given parameters + /// - Parameters: + /// - shouldUpdateVisibleRange: whether this summary provider should foward visible ranges + /// to the room list service through the `applyInput(input: .viewport(ranges` api. Only useful for + /// lists that need to update the visible range on Sliding Sync init(roomListService: RoomListServiceProtocol, eventStringBuilder: RoomEventStringBuilder, name: String, + shouldUpdateVisibleRange: Bool = false, notificationSettings: NotificationSettingsProxyProtocol, backgroundTaskService: BackgroundTaskServiceProtocol) { self.roomListService = roomListService serialDispatchQueue = DispatchQueue(label: "io.element.elementx.roomsummaryprovider", qos: .default) self.eventStringBuilder = eventStringBuilder self.name = name + self.shouldUpdateVisibleRange = shouldUpdateVisibleRange self.notificationSettings = notificationSettings self.backgroundTaskService = backgroundTaskService @@ -110,6 +118,16 @@ 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 + } + Task { do { // The scroll view content size based visible range calculations might create large ranges @@ -126,12 +144,6 @@ class RoomSummaryProvider: RoomSummaryProviderProtocol { MXLog.error("Failed updating visible range with error: \(error)") } } - - if range.upperBound >= rooms.count { - listUpdatesSubscriptionResult?.controller.addOnePage() - } else if range.lowerBound == 0 { - listUpdatesSubscriptionResult?.controller.resetToOnePage() - } } func setFilter(_ filter: RoomSummaryProviderFilter) { diff --git a/UnitTests/Sources/MessageForwardingScreenViewModelTests.swift b/UnitTests/Sources/MessageForwardingScreenViewModelTests.swift index 715763310..890be87c5 100644 --- a/UnitTests/Sources/MessageForwardingScreenViewModelTests.swift +++ b/UnitTests/Sources/MessageForwardingScreenViewModelTests.swift @@ -40,9 +40,14 @@ class MessageForwardingScreenViewModelTests: XCTestCase { XCTAssertEqual(context.viewState.selectedRoomID, "2") } - func testSearching() { + func testSearching() async throws { + let defered = deferFulfillment(context.$viewState) { state in + state.rooms.count == 1 + } + context.searchQuery = "Second" - XCTAssertEqual(context.viewState.visibleRooms.count, 1) + + try await defered.fulfill() } func testForwarding() { diff --git a/UnitTests/__Snapshots__/PreviewTests/test_messageForwardingScreen.1.png b/UnitTests/__Snapshots__/PreviewTests/test_messageForwardingScreen.1.png index 9d32b19b8..4fc629892 100644 --- a/UnitTests/__Snapshots__/PreviewTests/test_messageForwardingScreen.1.png +++ b/UnitTests/__Snapshots__/PreviewTests/test_messageForwardingScreen.1.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:345aefea5a04ab6ba25c84a84cde9780a5061fdf4dee59e7cf7d3a61e821ea4b -size 103346 +oid sha256:7c1ca96074ce2549180efe3350a9fdf2e1a9654042b48975e0226222fb6a6c7e +size 103256 diff --git a/changelog.d/1864.bugfix b/changelog.d/1864.bugfix new file mode 100644 index 000000000..b2c0c5cff --- /dev/null +++ b/changelog.d/1864.bugfix @@ -0,0 +1 @@ +Fix message forwarding room list filtering and pagination problems \ No newline at end of file