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

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()
         }
         

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
     }

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"]
 }

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 {

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..<roomCount)
+        default:
+            break
+        }
+    }
 }

ElementX/Sources/Screens/MessageForwardingScreen/View/MessageForwardingScreen.swift

@@ -22,10 +22,19 @@ struct MessageForwardingScreen: View {
     var body: some View {
         Form {
             Section {
-                ForEach(context.viewState.visibleRooms) { room in
+                ForEach(context.viewState.rooms) { room in
                     MessageForwardingRoomCell(room: room, context: context)
                         .buttonStyle(FormButtonStyle(accessory: .singleSelection(isSelected: context.viewState.selectedRoomID == room.id)))
                 }
+                // Replace these with ScrollView's `scrollPosition` when dropping iOS 16.
+            } header: {
+                Rectangle().hidden().onAppear {
+                    context.send(viewAction: .reachedTop)
+                }
+            } footer: {
+                Rectangle().hidden().onAppear {
+                    context.send(viewAction: .reachedBottom)
+                }
             }
             .compoundFormSection()
         }

ElementX/Sources/Services/Client/ClientProxy.swift

@@ -35,10 +35,14 @@ class ClientProxy: ClientProxyProtocol {
     private var syncService: SyncService?
     private var syncServiceStateUpdateTaskHandle: TaskHandle?
     
-    var roomSummaryProvider: RoomSummaryProviderProtocol?
-    var inviteSummaryProvider: RoomSummaryProviderProtocol?
+    // These following summary providers both operate on the same allRooms() list but
+    // can apply their own filtering and pagination
+    private(set) var roomSummaryProvider: RoomSummaryProviderProtocol?
+    private(set) var messageForwardingRoomSummaryProvider: RoomSummaryProviderProtocol?
     
-    var notificationSettings: NotificationSettingsProxyProtocol
+    private(set) var inviteSummaryProvider: RoomSummaryProviderProtocol?
+    
+    let notificationSettings: NotificationSettingsProxyProtocol
 
     private let roomListRecencyOrderingAllowedEventTypes = ["m.room.message", "m.room.encrypted", "m.sticker"]
 
@@ -451,10 +455,18 @@ class ClientProxy: ClientProxyProtocol {
             roomSummaryProvider = RoomSummaryProvider(roomListService: roomListService,
                                                       eventStringBuilder: eventStringBuilder,
                                                       name: "AllRooms",
+                                                      shouldUpdateVisibleRange: true,
                                                       notificationSettings: notificationSettings,
                                                       backgroundTaskService: backgroundTaskService)
             try await roomSummaryProvider?.setRoomList(roomListService.allRooms())
             
+            messageForwardingRoomSummaryProvider = RoomSummaryProvider(roomListService: roomListService,
+                                                                       eventStringBuilder: eventStringBuilder,
+                                                                       name: "MessageForwarding",
+                                                                       notificationSettings: notificationSettings,
+                                                                       backgroundTaskService: backgroundTaskService)
+            try await messageForwardingRoomSummaryProvider?.setRoomList(roomListService.allRooms())
+            
             inviteSummaryProvider = RoomSummaryProvider(roomListService: roomListService,
                                                         eventStringBuilder: eventStringBuilder,
                                                         name: "Invites",

ElementX/Sources/Services/Client/ClientProxyProtocol.swift

@@ -89,6 +89,8 @@ protocol ClientProxyProtocol: AnyObject, MediaLoaderProtocol {
     
     var roomSummaryProvider: RoomSummaryProviderProtocol? { get }
     
+    var messageForwardingRoomSummaryProvider: RoomSummaryProviderProtocol? { get }
+    
     var inviteSummaryProvider: RoomSummaryProviderProtocol? { get }
     
     var notificationSettings: NotificationSettingsProxyProtocol { get }

ElementX/Sources/Services/Client/MockClientProxy.swift

@@ -30,6 +30,8 @@ class MockClientProxy: ClientProxyProtocol {
     
     var roomSummaryProvider: RoomSummaryProviderProtocol? = MockRoomSummaryProvider()
     
+    var messageForwardingRoomSummaryProvider: RoomSummaryProviderProtocol? = MockRoomSummaryProvider()
+    
     var inviteSummaryProvider: RoomSummaryProviderProtocol? = MockRoomSummaryProvider()
 
     var userAvatarURL: CurrentValuePublisher<URL?, Never> { CurrentValueSubject<URL?, Never>(nil).asCurrentValuePublisher() }

ElementX/Sources/Services/Room/RoomSummary/MockRoomSummaryProvider.swift

@@ -24,8 +24,17 @@ enum MockRoomSummaryProviderState {
 }
 
 class MockRoomSummaryProvider: RoomSummaryProviderProtocol {
-    let roomListPublisher: CurrentValuePublisher<[RoomSummary], Never>
-    let statePublisher: CurrentValuePublisher<RoomSummaryProviderState, Never>
+    private let initialRooms: [RoomSummary]
+    
+    private let roomListSubject: CurrentValueSubject<[RoomSummary], Never>
+    var roomListPublisher: CurrentValuePublisher<[RoomSummary], Never> {
+        roomListSubject.asCurrentValuePublisher()
+    }
+    
+    private let stateSuject: CurrentValueSubject<RoomSummaryProviderState, Never>
+    var statePublisher: CurrentValuePublisher<RoomSummaryProviderState, Never> {
+        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<Int>) { }
     
-    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 {

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<Int>) {
+        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) {

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() {

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

changelog.d/1864.bugfix

@@ -0,0 +1 @@
+Fix message forwarding room list filtering and pagination problems