From 1609ae2036aac469114ddc18f5411131b067e432 Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Tue, 1 Aug 2023 11:53:02 +0100 Subject: [PATCH] Send read receipts as messages are displayed. (#1426) --- .../RoomScreen/RoomScreenCoordinator.swift | 4 - .../Screens/RoomScreen/RoomScreenModels.swift | 5 +- .../RoomScreen/RoomScreenViewModel.swift | 49 +++++- .../RoomScreen/View/RoomHeaderView.swift | 2 +- .../Screens/RoomScreen/View/RoomScreen.swift | 9 +- .../TimelineTableViewController.swift | 23 +++ .../MockRoomTimelineController.swift | 13 +- .../RoomTimelineController.swift | 4 +- .../RoomTimelineControllerProtocol.swift | 2 +- .../Sources/RoomScreenViewModelTests.swift | 162 ++++++++++++++++-- changelog.d/639.bugfix | 1 + 11 files changed, 234 insertions(+), 40 deletions(-) create mode 100644 changelog.d/639.bugfix diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift index 85be387e5..157d6779f 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift +++ b/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)) } diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift index d21b2349d..5fc646acd 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift +++ b/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 markRoomAsRead + case sendReadReceiptIfNeeded(TimelineItemIdentifier) 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] = [:] diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift index fe15e1042..86fc1317f 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift +++ b/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: - Task { await markRoomAsRead() } + case .sendReadReceiptIfNeeded(let lastVisibleItemID): + 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 { - notificationCenterProtocol.post(name: .roomMarkedAsRead, object: roomProxy.id) - _ = await timelineController.markRoomAsRead() + /// The ID of the newest item in the room that the user has seen. + /// This includes both event based items and virtual items. + private var lastReadItemID: TimelineItemIdentifier? + private func sendReadReceiptIfNeeded(for lastVisibleItemID: TimelineItemIdentifier) async -> Result { + 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[.. 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) diff --git a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift b/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift index a3544fff0..3828563a5 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Timeline/TimelineTableViewController.swift +++ b/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 diff --git a/ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift b/ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift index 1ed0ef6a5..c394b721b 100644 --- a/ElementX/Sources/Services/Timeline/TimelineController/MockRoomTimelineController.swift +++ b/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() @@ -49,7 +50,15 @@ class MockRoomTimelineController: RoomTimelineControllerProtocol { return .success(()) } - func markRoomAsRead() async -> Result { .success(()) } + func sendReadReceipt(for itemID: TimelineItemIdentifier) async -> Result { + 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 { } diff --git a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift index d2b3c80d3..880d3ca31 100644 --- a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift +++ b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineController.swift @@ -82,9 +82,9 @@ class RoomTimelineController: RoomTimelineControllerProtocol { } } - func markRoomAsRead() async -> Result { + func sendReadReceipt(for itemID: TimelineItemIdentifier) async -> Result { guard roomProxy.hasUnreadNotifications, - let eventID = timelineItems.last?.id.eventID + let eventID = itemID.eventID else { return .success(()) } switch await roomProxy.sendReadReceipt(for: eventID) { diff --git a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift index c31139110..f0d1faf4a 100644 --- a/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift +++ b/ElementX/Sources/Services/Timeline/TimelineController/RoomTimelineControllerProtocol.swift @@ -49,7 +49,7 @@ protocol RoomTimelineControllerProtocol { func paginateBackwards(requestSize: UInt, untilNumberOfItems: UInt) async -> Result - func markRoomAsRead() async -> Result + func sendReadReceipt(for itemID: TimelineItemIdentifier) async -> Result func sendMessage(_ message: String, inReplyTo itemID: TimelineItemIdentifier?) async diff --git a/UnitTests/Sources/RoomScreenViewModelTests.swift b/UnitTests/Sources/RoomScreenViewModelTests.swift index 1b45a27e7..78573d781 100644 --- a/UnitTests/Sources/RoomScreenViewModelTests.swift +++ b/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 { - // Setup - let notificationCenterMock = NotificationCenterMock() + + // MARK: - Read Receipts + + // 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) - - viewModel.context.send(viewAction: .markRoomAsRead) - try await Task.sleep(for: .microseconds(100)) - XCTAssertEqual(notificationCenterMock.postNameObjectReceivedArguments?.aName, .roomMarkedAsRead) - let roomID = notificationCenterMock.postNameObjectReceivedArguments?.anObject as? String - XCTAssertEqual(roomID, roomProxyMock.id) + notificationCenterProtocol: notificationCenter) + + return (viewModel, roomProxy, timelineController, notificationCenter) } } @@ -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!")) + } +} diff --git a/changelog.d/639.bugfix b/changelog.d/639.bugfix new file mode 100644 index 000000000..ab1eda069 --- /dev/null +++ b/changelog.d/639.bugfix @@ -0,0 +1 @@ +Send read receipts as messages are displayed instead of on opening/closing rooms. \ No newline at end of file