Send read receipts as messages are displayed. (#1426)

ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift

@@ -91,10 +91,6 @@ final class RoomScreenCoordinator: CoordinatorProtocol {
         }
     }
     
-    func stop() {
-        viewModel.context.send(viewAction: .markRoomAsRead)
-    }
-    
     func toPresentable() -> AnyView {
         AnyView(RoomScreen(context: viewModel.context))
     }

ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift

@@ -59,8 +59,7 @@ enum RoomScreenViewAction {
     case toggleReaction(key: String, itemID: TimelineItemIdentifier)
     case cancelReply
     case cancelEdit
-    /// Mark the entire room as read - this is heavy handed as a starting point for now.
+    case sendReadReceiptIfNeeded(TimelineItemIdentifier)
-    case markRoomAsRead
     case paginateBackwards
     
     case timelineItemMenu(itemID: TimelineItemIdentifier)
@@ -85,7 +84,7 @@ enum RoomScreenViewAction {
 }
 
 struct RoomScreenViewState: BindableState {
-    var roomId: String
+    var roomID: String
     var roomTitle = ""
     var roomAvatarURL: URL?
     var members: [String: RoomMemberState] = [:]

ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift

@@ -53,7 +53,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
         self.userIndicatorController = userIndicatorController
         self.notificationCenterProtocol = notificationCenterProtocol
         
-        super.init(initialViewState: RoomScreenViewState(roomId: timelineController.roomID,
+        super.init(initialViewState: RoomScreenViewState(roomID: timelineController.roomID,
                                                          roomTitle: roomProxy.roomTitle,
                                                          roomAvatarURL: roomProxy.avatarURL,
                                                          timelineStyle: appSettings.timelineStyle,
@@ -103,8 +103,8 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
         case .cancelEdit:
             setComposerMode(.default)
             state.bindings.composerText = ""
-        case .markRoomAsRead:
+        case .sendReadReceiptIfNeeded(let lastVisibleItemID):
-            Task { await markRoomAsRead() }
+            Task { await sendReadReceiptIfNeeded(for: lastVisibleItemID) }
         case .timelineItemMenu(let itemID):
             Task {
                 if case let .success(value) = await roomProxy.canUserRedact(userID: roomProxy.ownUserID) {
@@ -245,9 +245,46 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol
         }
     }
     
-    private func markRoomAsRead() async {
+    /// The ID of the newest item in the room that the user has seen.
-        notificationCenterProtocol.post(name: .roomMarkedAsRead, object: roomProxy.id)
+    /// This includes both event based items and virtual items.
-        _ = await timelineController.markRoomAsRead()
+    private var lastReadItemID: TimelineItemIdentifier?
+    private func sendReadReceiptIfNeeded(for lastVisibleItemID: TimelineItemIdentifier) async -> Result<Void, RoomTimelineControllerError> {
+        guard lastReadItemID != lastVisibleItemID,
+              let eventItemID = eventBasedItem(nearest: lastVisibleItemID)
+        else { return .success(()) }
+        
+        // Make sure the item is newer than the item that was last marked as read.
+        if let lastReadItemIndex = state.timelineViewState.timelineIDs.firstIndex(of: lastReadItemID?.timelineID ?? ""),
+           let lastVisibleItemIndex = state.timelineViewState.timelineIDs.firstIndex(of: eventItemID.timelineID),
+           lastReadItemIndex > lastVisibleItemIndex {
+            return .success(())
+        }
+        
+        // Update the last read item ID to avoid attempting duplicate requests.
+        lastReadItemID = lastVisibleItemID
+        
+        // Clear any notifications from notification center.
+        if lastVisibleItemID.timelineID == state.timelineViewState.timelineIDs.last {
+            notificationCenterProtocol.post(name: .roomMarkedAsRead, object: roomProxy.id)
+        }
+        
+        switch await timelineController.sendReadReceipt(for: eventItemID) {
+        case .success:
+            return .success(())
+        case .failure:
+            return .failure(.generic)
+        }
+    }
+    
+    /// Returns the first item ID that contains an `eventID` starting from the supplied ID, working backwards through the timeline.
+    private func eventBasedItem(nearest itemID: TimelineItemIdentifier) -> TimelineItemIdentifier? {
+        guard itemID.eventID == nil else { return itemID }
+        
+        let timelineIDs = state.timelineViewState.itemViewStates.map(\.identifier)
+        guard let index = timelineIDs.firstIndex(of: itemID) else { return nil }
+        
+        let nearestItemID = timelineIDs[..<index].last(where: { $0.eventID != nil })
+        return nearestItemID
     }
 
     private func itemTapped(with itemID: TimelineItemIdentifier) async {

ElementX/Sources/Screens/RoomScreen/View/RoomHeaderView.swift

@@ -40,7 +40,7 @@ struct RoomHeaderView: View {
     @ViewBuilder private var roomAvatar: some View {
         LoadableAvatarImage(url: context.viewState.roomAvatarURL,
                             name: context.viewState.roomTitle,
-                            contentID: context.viewState.roomId,
+                            contentID: context.viewState.roomID,
                             avatarSize: .room(on: .timeline),
                             imageProvider: context.imageProvider)
             .accessibilityIdentifier(A11yIdentifiers.roomScreen.avatar)

ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift

@@ -56,13 +56,6 @@ struct RoomScreen: View {
             }
             .interactiveQuickLook(item: $context.mediaPreviewItem)
             .track(screen: .room)
-            .task(id: context.viewState.roomId) {
-                // Give a couple of seconds for items to load and to see them.
-                try? await Task.sleep(for: .seconds(2))
-                
-                guard !Task.isCancelled else { return }
-                context.send(viewAction: .markRoomAsRead)
-            }
             .onDrop(of: ["public.item"], isTargeted: $dragOver) { providers -> Bool in
                 guard let provider = providers.first,
                       provider.isSupportedForPasteOrDrop else {
@@ -89,7 +82,7 @@ struct RoomScreen: View {
 
     private var timeline: some View {
         timelineSwitch
-            .id(context.viewState.roomId)
+            .id(context.viewState.roomID)
             .environmentObject(context)
             .environment(\.timelineStyle, context.viewState.timelineStyle)
             .environment(\.readReceiptsEnabled, context.viewState.readReceiptsEnabled)

ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift

@@ -48,6 +48,8 @@ class TimelineTableViewController: UIViewController {
             if timelineItemsDictionary.isEmpty {
                 paginateBackwardsPublisher.send()
             }
+            
+            sendReadReceiptIfNeeded()
         }
     }
 
@@ -241,6 +243,15 @@ class TimelineTableViewController: UIViewController {
         
         coordinator.send(viewAction: .paginateBackwards)
     }
+    
+    private func sendReadReceiptIfNeeded() {
+        guard let lastVisibleItemIndexPath = tableView.indexPathsForVisibleRows?.first,
+              let lastVisibleItemTimelineID = dataSource?.itemIdentifier(for: lastVisibleItemIndexPath),
+              let lastVisibleItemID = timelineItemsDictionary[lastVisibleItemTimelineID]?.identifier
+        else { return }
+        
+        coordinator.send(viewAction: .sendReadReceiptIfNeeded(lastVisibleItemID))
+    }
 }
 
 // MARK: - UITableViewDelegate
@@ -271,6 +282,18 @@ extension TimelineTableViewController: UITableViewDelegate {
         scrollToTop(animated: true)
         return false
     }
+    
+    func scrollViewDidEndDragging(_ scrollView: UIScrollView, willDecelerate decelerate: Bool) {
+        sendReadReceiptIfNeeded()
+    }
+    
+    func scrollViewDidEndDecelerating(_ scrollView: UIScrollView) {
+        sendReadReceiptIfNeeded()
+    }
+    
+    func scrollViewDidEndScrollingAnimation(_ scrollView: UIScrollView) {
+        sendReadReceiptIfNeeded()
+    }
 }
 
 // MARK: - Layout Types

ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift

@@ -26,7 +26,8 @@ class MockRoomTimelineController: RoomTimelineControllerProtocol {
     /// An array of timeline items that will be appended in order when ``simulateIncomingItems()`` is called.
     var incomingItems: [RoomTimelineItemProtocol] = []
     
-    let roomID = "MockRoomIdentifier"
+    var roomProxy: RoomProxyProtocol?
+    var roomID: String { roomProxy?.id ?? "MockRoomIdentifier" }
     
     let callbacks = PassthroughSubject<RoomTimelineControllerCallback, Never>()
     
@@ -49,7 +50,15 @@ class MockRoomTimelineController: RoomTimelineControllerProtocol {
         return .success(())
     }
     
-    func markRoomAsRead() async -> Result<Void, RoomTimelineControllerError> { .success(()) }
+    func sendReadReceipt(for itemID: TimelineItemIdentifier) async -> Result<Void, RoomTimelineControllerError> {
+        guard let roomProxy, let eventID = itemID.eventID else { return .failure(.generic) }
+        switch await roomProxy.sendReadReceipt(for: eventID) {
+        case .success:
+            return .success(())
+        case .failure:
+            return .failure(.generic)
+        }
+    }
     
     func processItemAppearance(_ itemID: TimelineItemIdentifier) async { }
     

ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift

@@ -82,9 +82,9 @@ class RoomTimelineController: RoomTimelineControllerProtocol {
         }
     }
     
-    func markRoomAsRead() async -> Result<Void, RoomTimelineControllerError> {
+    func sendReadReceipt(for itemID: TimelineItemIdentifier) async -> Result<Void, RoomTimelineControllerError> {
         guard roomProxy.hasUnreadNotifications,
-              let eventID = timelineItems.last?.id.eventID
+              let eventID = itemID.eventID
         else { return .success(()) }
         
         switch await roomProxy.sendReadReceipt(for: eventID) {

ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift

@@ -49,7 +49,7 @@ protocol RoomTimelineControllerProtocol {
     
     func paginateBackwards(requestSize: UInt, untilNumberOfItems: UInt) async -> Result<Void, RoomTimelineControllerError>
     
-    func markRoomAsRead() async -> Result<Void, RoomTimelineControllerError>
+    func sendReadReceipt(for itemID: TimelineItemIdentifier) async -> Result<Void, RoomTimelineControllerError>
     
     func sendMessage(_ message: String, inReplyTo itemID: TimelineItemIdentifier?) async
 

UnitTests/Sources/RoomScreenViewModelTests.swift

@@ -28,6 +28,8 @@ class RoomScreenViewModelTests: XCTestCase {
     override func tearDown() async throws {
         userIndicatorControllerMock = nil
     }
+    
+    // MARK: - Message Grouping
 
     func testMessageGrouping() {
         // Given 3 messages from Bob.
@@ -175,6 +177,8 @@ class RoomScreenViewModelTests: XCTestCase {
         XCTAssertEqual(viewModel.state.timelineViewState.itemViewStates[1].groupStyle, .middle, "Nothing should prevent the second message from being grouped.")
         XCTAssertEqual(viewModel.state.timelineViewState.itemViewStates[2].groupStyle, .last, "Reactions on the last message should not prevent it from being grouped.")
     }
+    
+    // MARK: - User Details
 
     func testGoToUserDetailsSuccessNoDelay() async {
         // Setup
@@ -277,6 +281,8 @@ class RoomScreenViewModelTests: XCTestCase {
         XCTAssert(roomProxyMock.getMemberUserIDCallsCount == 1)
         XCTAssertEqual(roomProxyMock.getMemberUserIDReceivedUserID, "bob")
     }
+    
+    // MARK: - Sending
 
     func testRetrySend() async throws {
         // Setup
@@ -352,26 +358,139 @@ class RoomScreenViewModelTests: XCTestCase {
         await Task.yield()
         XCTAssert(roomProxyMock.cancelSendTransactionIDCallsCount == 0)
     }
-
+    
-    func testMarkAsRead() async throws {
+    // MARK: - Read Receipts
-        // Setup
+    
-        let notificationCenterMock = NotificationCenterMock()
+    // swiftlint:disable force_unwrapping
+    
+    func testSendReadReceipt() async throws {
+        // Given a room with only text items in the timeline
+        let items = [TextRoomTimelineItem(eventID: "t1"),
+                     TextRoomTimelineItem(eventID: "t2"),
+                     TextRoomTimelineItem(eventID: "t3")]
+        let (viewModel, roomProxy, _, notificationCenter) = readReceiptsConfiguration(with: items)
+        
+        // When sending a read receipt for the last item.
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(items.last!.id))
+        try await Task.sleep(for: .microseconds(100))
+        
+        // Then the receipt should be sent.
+        XCTAssertEqual(roomProxy.sendReadReceiptForCalled, true)
+        XCTAssertEqual(roomProxy.sendReadReceiptForReceivedEventID, "t3")
+        
+        // And the notifications should be cleared.
+        XCTAssertEqual(notificationCenter.postNameObjectReceivedArguments?.aName, .roomMarkedAsRead)
+        let roomID = notificationCenter.postNameObjectReceivedArguments?.anObject as? String
+        XCTAssertEqual(roomID, roomProxy.id)
+    }
+    
+    func testSendMoreReadReceipts() async throws {
+        // Given a room with only text items in the timeline that are all read.
+        let items = [TextRoomTimelineItem(eventID: "t1"),
+                     TextRoomTimelineItem(eventID: "t2"),
+                     TextRoomTimelineItem(eventID: "t3")]
+        let (viewModel, roomProxy, timelineController, _) = readReceiptsConfiguration(with: items)
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(items.last!.id))
+        try await Task.sleep(for: .microseconds(100))
+        XCTAssertEqual(roomProxy.sendReadReceiptForCallsCount, 1)
+        XCTAssertEqual(roomProxy.sendReadReceiptForReceivedEventID, "t3")
+        
+        // When sending a receipt for the first item in the timeline.
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(items.first!.id))
+        try await Task.sleep(for: .microseconds(100))
+        
+        // Then the request should be ignored.
+        XCTAssertEqual(roomProxy.sendReadReceiptForCallsCount, 1)
+        XCTAssertEqual(roomProxy.sendReadReceiptForReceivedEventID, "t3")
+        
+        // When a new message is received and marked as read.
+        let newMessage = TextRoomTimelineItem(eventID: "t4")
+        timelineController.timelineItems.append(newMessage)
+        timelineController.callbacks.send(.updatedTimelineItems)
+        try await Task.sleep(for: .microseconds(500))
+        
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(newMessage.id))
+        try await Task.sleep(for: .microseconds(100))
+        
+        // Then the request should be made.
+        XCTAssertEqual(roomProxy.sendReadReceiptForCallsCount, 2)
+        XCTAssertEqual(roomProxy.sendReadReceiptForReceivedEventID, "t4")
+    }
+    
+    func testSendReadReceiptWithoutEvents() async throws {
+        // Given a room with only virtual items.
+        let items = [SeparatorRoomTimelineItem(timelineID: "v1"),
+                     SeparatorRoomTimelineItem(timelineID: "v2"),
+                     SeparatorRoomTimelineItem(timelineID: "v3")]
+        let (viewModel, roomProxy, _, _) = readReceiptsConfiguration(with: items)
+        
+        // When sending a read receipt for the last item.
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(items.last!.id))
+        try await Task.sleep(for: .microseconds(100))
+        
+        // Then nothing should be sent.
+        XCTAssertEqual(roomProxy.sendReadReceiptForCalled, false)
+    }
+    
+    func testSendReadReceiptVirtualLast() async throws {
+        // Given a room where the last event is a virtual item.
+        let items: [RoomTimelineItemProtocol] = [TextRoomTimelineItem(eventID: "t1"),
+                                                 TextRoomTimelineItem(eventID: "t2"),
+                                                 SeparatorRoomTimelineItem(timelineID: "v3")]
+        let (viewModel, roomProxy, _, _) = readReceiptsConfiguration(with: items)
+        
+        // When sending a read receipt for the last item.
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(items.last!.id))
+        try await Task.sleep(for: .microseconds(100))
+        
+        // Then a read receipt should be sent for the item before it.
+        XCTAssertEqual(roomProxy.sendReadReceiptForCalled, true)
+        XCTAssertEqual(roomProxy.sendReadReceiptForReceivedEventID, "t2")
+    }
+    
+    func testSendReadReceiptMultipleRequests() async throws {
+        // Given a room where the last event is a virtual item which was already read.
+        let items: [RoomTimelineItemProtocol] = [TextRoomTimelineItem(eventID: "t1"),
+                                                 TextRoomTimelineItem(eventID: "t2"),
+                                                 SeparatorRoomTimelineItem(timelineID: "v3")]
+        let (viewModel, roomProxy, _, _) = readReceiptsConfiguration(with: items)
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(items.last!.id))
+        try await Task.sleep(for: .microseconds(100))
+        XCTAssertEqual(roomProxy.sendReadReceiptForCallsCount, 1)
+        XCTAssertEqual(roomProxy.sendReadReceiptForReceivedEventID, "t2")
+        
+        // When sending the same receipt again
+        viewModel.context.send(viewAction: .sendReadReceiptIfNeeded(items.last!.id))
+        try await Task.sleep(for: .microseconds(100))
+        
+        // Then the second call should be ignored.
+        XCTAssertEqual(roomProxy.sendReadReceiptForCallsCount, 1)
+    }
+    
+    // swiftlint:enable force_unwrapping
+    // swiftlint:disable:next large_tuple
+    private func readReceiptsConfiguration(with items: [RoomTimelineItemProtocol]) -> (RoomScreenViewModel,
+                                                                                       RoomProxyMock,
+                                                                                       MockRoomTimelineController,
+                                                                                       NotificationCenterMock) {
+        let notificationCenter = NotificationCenterMock()
+        let roomProxy = RoomProxyMock(with: .init(displayName: ""))
         let timelineController = MockRoomTimelineController()
-        let roomProxyMock = RoomProxyMock(with: .init(displayName: ""))
+        
+        roomProxy.sendReadReceiptForReturnValue = .success(())
+        roomProxy.underlyingHasUnreadNotifications = true
+        timelineController.timelineItems = items
+        timelineController.roomProxy = roomProxy
 
         let viewModel = RoomScreenViewModel(timelineController: timelineController,
                                             mediaProvider: MockMediaProvider(),
-                                            roomProxy: roomProxyMock,
+                                            roomProxy: roomProxy,
                                             appSettings: ServiceLocator.shared.settings,
                                             analytics: ServiceLocator.shared.analytics,
                                             userIndicatorController: userIndicatorControllerMock,
-                                            notificationCenterProtocol: notificationCenterMock)
+                                            notificationCenterProtocol: notificationCenter)
-
+        
-        viewModel.context.send(viewAction: .markRoomAsRead)
+        return (viewModel, roomProxy, timelineController, notificationCenter)
-        try await Task.sleep(for: .microseconds(100))
-        XCTAssertEqual(notificationCenterMock.postNameObjectReceivedArguments?.aName, .roomMarkedAsRead)
-        let roomID = notificationCenterMock.postNameObjectReceivedArguments?.anObject as? String
-        XCTAssertEqual(roomID, roomProxyMock.id)
     }
 }
 
@@ -387,3 +506,20 @@ private extension TextRoomTimelineItem {
                   properties: RoomTimelineItemProperties(reactions: reactions))
     }
 }
+
+private extension SeparatorRoomTimelineItem {
+    init(timelineID: String) {
+        self.init(id: .init(timelineID: timelineID), text: "")
+    }
+}
+
+private extension TextRoomTimelineItem {
+    init(eventID: String) {
+        self.init(id: .init(timelineID: UUID().uuidString, eventID: eventID),
+                  timestamp: "",
+                  isOutgoing: false,
+                  isEditable: false,
+                  sender: .init(id: ""),
+                  content: .init(body: "Hello, World!"))
+    }
+}

changelog.d/639.bugfix

@@ -0,0 +1 @@
+Send read receipts as messages are displayed instead of on opening/closing rooms.