From 37e9c7094d61837aa5f19046709f0acd199639fc Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Fri, 9 Oct 2026 19:18:10 +0000 Subject: [PATCH 1/5] fix(chat): do not send a read marker below the user's own message The server moves the read marker of the sender to a message when it is sent (ChatManager::addChatMessage) but accepts any later POST chat//read, including a lower value. The chat sent its local marker on leaving the chat while that marker still pointed below the own, just sent message. lastCommonRead is the minimum over the participants, so the recipient kept seeing a single check mark after a reply. Advance the local marker to the newest accepted own message, never send a marker below it, and skip the request while the sent message is not yet confirmed by the server (its temporary copy is still shown). Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/viewmodels/ChatViewModel.kt | 48 ++++++++++++- .../talk/chat/viewmodels/ChatViewModelTest.kt | 67 ++++++++++++++++++- 2 files changed, 111 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 140f2f8b0e..75dc7e1f3a 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -1383,6 +1383,7 @@ class ChatViewModel @AssistedInject constructor( _uiState.update { current -> current.copy(items = items) } + newestOwnMessageId(items)?.let(::advanceLocalLastReadMessageIfNeeded) } .launchIn(viewModelScope) } @@ -2122,10 +2123,17 @@ class ChatViewModel @AssistedInject constructor( val conversationLastReadMessage = _uiState.value.conversation?.lastReadMessage ?: return Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, conversation.lastReadMessage: $conversationLastReadMessage") - if (localLastReadMessage > conversationLastReadMessage) { + val items = _uiState.value.items + val marker = readMarkerToSend( + localLastReadMessage = localLastReadMessage, + conversationLastReadMessage = conversationLastReadMessage, + newestOwnMessageId = newestOwnMessageId(items), + hasUnconfirmedOwnMessage = hasUnconfirmedOwnMessage(items) + ) + if (marker != null) { Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, setChatReadMessage...") - setChatReadMessage(localLastReadMessage) + setChatReadMessage(marker) } } @@ -2804,6 +2812,42 @@ class ChatViewModel @AssistedInject constructor( fun holdsShare(messages: List?, referenceId: String): Boolean? = messages?.any { it.referenceId == referenceId } + /** + * Id of the newest message sent by the user that the server has already accepted, or null; the + * server marks everything up to it as read. Temporary messages (negative ids) are not accepted yet + * and are ignored. + */ + internal fun newestOwnMessageId(items: List): Int? = + items.mapNotNull { it.messageOrNull() } + .filter { !it.incoming && it.id > 0 } + .maxOfOrNull { it.id } + + /** + * True while a message of the user was sent, but its server copy is not in [items] yet. The + * server has already moved the user's read marker to it by then, so this client must not + * send an older marker over it. + */ + internal fun hasUnconfirmedOwnMessage(items: List): Boolean = + items.mapNotNull { it.messageOrNull() } + .any { !it.incoming && it.id < 0 && it.statusIcon == MessageStatusIcon.SENT } + + /** + * The read marker to send to the server, or null when nothing has to be sent. The server sets the + * marker to a message at the moment the user sends it, so the client never sends a marker + * below the newest own message: that would move the marker back and leave the message with a + * single check mark for the other participants. + */ + internal fun readMarkerToSend( + localLastReadMessage: Int, + conversationLastReadMessage: Int, + newestOwnMessageId: Int?, + hasUnconfirmedOwnMessage: Boolean + ): Int? { + if (hasUnconfirmedOwnMessage) return null + val marker = maxOf(localLastReadMessage, newestOwnMessageId ?: 0) + return marker.takeIf { it > conversationLastReadMessage } + } + /** * Ids of the messages a delete of [messageId] applies to: every already-uploaded message of the * media group the message is shown in, or just the message itself. diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt index 562bfb1538..64b6439505 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt @@ -432,8 +432,71 @@ class ChatViewModelTest { assertNull(ChatViewModel.readMarkerForMarkingUnread(items, messageId = 77)) } - private fun messageItem(id: Int): ChatViewModel.ChatItem.MessageItem = - ChatViewModel.ChatItem.MessageItem(uiMessage(id)) + // Read marker after the user sent a message: the server moves the marker to the sent message, so the + // client must never send an older one. Sequence from the server log of a real chat: the user sent + // message 2624 while the client's local marker still was 2623. + + @Test + fun `read marker is not sent below the own message just sent`() { + val items = listOf(messageItem(2624, incoming = false), messageItem(2623)) + + val marker = ChatViewModel.readMarkerToSend( + localLastReadMessage = 2623, + conversationLastReadMessage = 2620, + newestOwnMessageId = ChatViewModel.newestOwnMessageId(items), + hasUnconfirmedOwnMessage = ChatViewModel.hasUnconfirmedOwnMessage(items) + ) + + assertEquals(2624, marker) + } + + @Test + fun `read marker is not sent while the sent message is not confirmed by the server yet`() { + val items = listOf(messageItem(-77, incoming = false), messageItem(2623)) + + val marker = ChatViewModel.readMarkerToSend( + localLastReadMessage = 2623, + conversationLastReadMessage = 2620, + newestOwnMessageId = ChatViewModel.newestOwnMessageId(items), + hasUnconfirmedOwnMessage = ChatViewModel.hasUnconfirmedOwnMessage(items) + ) + + assertNull(marker) + } + + @Test + fun `read marker of the incoming messages is sent when the user sent nothing`() { + val items = listOf(messageItem(2623), messageItem(2622)) + + val marker = ChatViewModel.readMarkerToSend( + localLastReadMessage = 2623, + conversationLastReadMessage = 2620, + newestOwnMessageId = ChatViewModel.newestOwnMessageId(items), + hasUnconfirmedOwnMessage = ChatViewModel.hasUnconfirmedOwnMessage(items) + ) + + assertEquals(2623, marker) + } + + @Test + fun `read marker is not sent when the server already has it`() { + assertNull(ChatViewModel.readMarkerToSend(2624, 2624, newestOwnMessageId = 2624, false)) + } + + @Test + fun `newest own message ignores incoming and temporary messages`() { + val items = listOf( + messageItem(2630), + messageItem(-5, incoming = false), + messageItem(2624, incoming = false), + messageItem(2610, incoming = false) + ) + + assertEquals(2624, ChatViewModel.newestOwnMessageId(items)) + } + + private fun messageItem(id: Int, incoming: Boolean = true): ChatViewModel.ChatItem.MessageItem = + ChatViewModel.ChatItem.MessageItem(uiMessage(id).copy(incoming = incoming)) private fun uiMessage(id: Int): ChatMessageUi = ChatMessageUi( From 2647bdb7dc3140f11b2755382c42b112e85fd8c1 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Sun, 11 Oct 2026 14:03:12 +0000 Subject: [PATCH 2/5] fix(chat): decide the own-message read marker from the stored messages The check that keeps the read marker from going below the user's own message looked at the rendered items. They lag behind the database (throttled rebuild), so a message the server had already accepted could be missing there when the chat was left right after sending, and the older local marker was sent over the marker the server had moved to that message. Take the decision from the message list of the database instead, updated before the throttle: a stored temporary copy of the user's text message that did not fail is treated as possibly accepted, whether the request is still in flight or only its server copy is missing. A file placeholder counts only after the file was shared, as the server has no message for it before that. Assisted-by: Claude-Code:claude-opus-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/viewmodels/ChatViewModel.kt | 52 +++++++--- .../talk/chat/viewmodels/ChatViewModelTest.kt | 97 ++++++++++++------- 2 files changed, 98 insertions(+), 51 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 75dc7e1f3a..2063640e5a 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -44,6 +44,7 @@ import com.nextcloud.talk.conversationlist.viewmodels.ConversationsListViewModel import com.nextcloud.talk.dagger.modules.ApplicationScope import com.nextcloud.talk.data.database.mappers.toDomainModel import com.nextcloud.talk.data.database.model.ChatMessageEntity +import com.nextcloud.talk.data.database.model.SendStatus import com.nextcloud.talk.data.user.model.User import com.nextcloud.talk.jobs.ReadMarkerSyncWorker import com.nextcloud.talk.jobs.SendMessageWorker @@ -362,6 +363,12 @@ class ChatViewModel @AssistedInject constructor( private var localLastReadMessage: Int = 0 + // Newest message list from the database, updated before the throttled rebuild of the UI items. It is the + // source of truth for the user's own sends (temporary copy with its send status, then the accepted + // message) when the read marker is decided, see updateRemoteLastReadMessageIfNeeded. + @Volatile + private var latestMessages: List = emptyList() + /** * Set once the user marked a message as unread, so the automatic read marker handling keeps its * hands off the conversation for the rest of this visit. @@ -1308,7 +1315,9 @@ class ChatViewModel @AssistedInject constructor( // getLastCommonReadFlow provides the "last read by all" value for read-receipt checkmarks. // These are two different concepts that must not be conflated. combine( - messagesFlow, + // the raw message list is tapped before the throttle and the UI mapping below: the read marker + // decisions must not depend on how far the rendered items lag behind the database + messagesFlow.onEach(::onMessagesChanged), getLastCommonReadFlow.onStart { emit(0) }, parentMessagesFlow, combine(conversationFlow, _spreedCapabilities) { conv, caps -> conv to caps }, @@ -1383,7 +1392,6 @@ class ChatViewModel @AssistedInject constructor( _uiState.update { current -> current.copy(items = items) } - newestOwnMessageId(items)?.let(::advanceLocalLastReadMessageIfNeeded) } .launchIn(viewModelScope) } @@ -2096,6 +2104,13 @@ class ChatViewModel @AssistedInject constructor( } } + // Runs on the background dispatcher of the rebuild pipeline (flowOn in observeMessages): it only + // publishes the immutable list through a volatile field. localLastReadMessage is written on the main + // thread only, so the own message is combined with it when the marker is decided (readMarkerToSend). + private fun onMessagesChanged(messages: List) { + latestMessages = messages + } + fun advanceLocalLastReadMessageIfNeeded(messageId: Int) { Log.d(TAG, "advanceLocalLastReadMessageIfNeeded, messageId: $messageId") Log.d(TAG, "advanceLocalLastReadMessageIfNeeded, localLastReadMessage: $localLastReadMessage") @@ -2123,12 +2138,12 @@ class ChatViewModel @AssistedInject constructor( val conversationLastReadMessage = _uiState.value.conversation?.lastReadMessage ?: return Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, conversation.lastReadMessage: $conversationLastReadMessage") - val items = _uiState.value.items + val messages = latestMessages val marker = readMarkerToSend( localLastReadMessage = localLastReadMessage, conversationLastReadMessage = conversationLastReadMessage, - newestOwnMessageId = newestOwnMessageId(items), - hasUnconfirmedOwnMessage = hasUnconfirmedOwnMessage(items) + newestOwnMessageId = newestOwnMessageId(messages), + hasUnconfirmedOwnMessage = hasUnconfirmedOwnMessage(messages) ) if (marker != null) { Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, setChatReadMessage...") @@ -2817,19 +2832,26 @@ class ChatViewModel @AssistedInject constructor( * server marks everything up to it as read. Temporary messages (negative ids) are not accepted yet * and are ignored. */ - internal fun newestOwnMessageId(items: List): Int? = - items.mapNotNull { it.messageOrNull() } - .filter { !it.incoming && it.id > 0 } - .maxOfOrNull { it.id } + internal fun newestOwnMessageId(messages: List): Int? = + messages.filter { !it.incoming && it.jsonMessageId > 0 } + .maxOfOrNull { it.jsonMessageId } /** - * True while a message of the user was sent, but its server copy is not in [items] yet. The - * server has already moved the user's read marker to it by then, so this client must not - * send an older marker over it. + * True while a text message of the user is on its way to the server: its temporary copy is stored + * and not failed, so the server may have accepted it (and moved the user's read marker to it) + * already, while its server copy is not in [messages] yet. This client must not send an older + * marker over it. A file placeholder is not counted before its upload is finished: the server has + * no message for it until then. */ - internal fun hasUnconfirmedOwnMessage(items: List): Boolean = - items.mapNotNull { it.messageOrNull() } - .any { !it.incoming && it.id < 0 && it.statusIcon == MessageStatusIcon.SENT } + internal fun hasUnconfirmedOwnMessage(messages: List): Boolean = + messages.any { message -> + val isFileUploading = message.sendStatus != SendStatus.SENT_PENDING_ACK && + message.messageParameters.orEmpty().containsKey("file") + !message.incoming && + message.isTemporary && + message.sendStatus != SendStatus.FAILED && + !isFileUploading + } /** * The read marker to send to the server, or null when nothing has to be sent. The server sets the diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt index 64b6439505..3a5865f430 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt @@ -7,9 +7,11 @@ package com.nextcloud.talk.chat.viewmodels +import com.nextcloud.talk.chat.data.model.ChatMessage import com.nextcloud.talk.chat.ui.model.ChatMessageUi import com.nextcloud.talk.chat.ui.model.MessageStatusIcon import com.nextcloud.talk.chat.ui.model.MessageTypeContent +import com.nextcloud.talk.data.database.model.SendStatus import com.nextcloud.talk.models.json.chat.ChatMessageDto import com.nextcloud.talk.utils.message.SendMessageUtils import org.junit.Assert.assertEquals @@ -434,48 +436,57 @@ class ChatViewModelTest { // Read marker after the user sent a message: the server moves the marker to the sent message, so the // client must never send an older one. Sequence from the server log of a real chat: the user sent - // message 2624 while the client's local marker still was 2623. + // message 2624 while the client's local marker still was 2623. The decision is made on the message + // list from the database, not on the rendered items, which lag behind it. + + private fun readMarker(messages: List, local: Int = 2623, conversation: Int = 2620): Int? = + ChatViewModel.readMarkerToSend( + localLastReadMessage = local, + conversationLastReadMessage = conversation, + newestOwnMessageId = ChatViewModel.newestOwnMessageId(messages), + hasUnconfirmedOwnMessage = ChatViewModel.hasUnconfirmedOwnMessage(messages) + ) @Test fun `read marker is not sent below the own message just sent`() { - val items = listOf(messageItem(2624, incoming = false), messageItem(2623)) + assertEquals(2624, readMarker(listOf(ownMessage(2624), incomingMessage(2623)))) + } - val marker = ChatViewModel.readMarkerToSend( - localLastReadMessage = 2623, - conversationLastReadMessage = 2620, - newestOwnMessageId = ChatViewModel.newestOwnMessageId(items), - hasUnconfirmedOwnMessage = ChatViewModel.hasUnconfirmedOwnMessage(items) - ) + @Test + fun `read marker is not sent while the own message is on its way to the server`() { + // the request is in flight: the server may have accepted the message, the answer is not processed yet + assertNull(readMarker(listOf(ownMessage(-77, SendStatus.PENDING), incomingMessage(2623)))) + } - assertEquals(2624, marker) + @Test + fun `read marker is not sent while the own message waits for its server copy`() { + assertNull(readMarker(listOf(ownMessage(-77, SendStatus.SENT_PENDING_ACK), incomingMessage(2623)))) } @Test - fun `read marker is not sent while the sent message is not confirmed by the server yet`() { - val items = listOf(messageItem(-77, incoming = false), messageItem(2623)) + fun `read marker is sent when the own message failed to send`() { + assertEquals(2623, readMarker(listOf(ownMessage(-77, SendStatus.FAILED), incomingMessage(2623)))) + } - val marker = ChatViewModel.readMarkerToSend( - localLastReadMessage = 2623, - conversationLastReadMessage = 2620, - newestOwnMessageId = ChatViewModel.newestOwnMessageId(items), - hasUnconfirmedOwnMessage = ChatViewModel.hasUnconfirmedOwnMessage(items) - ) + @Test + fun `read marker is sent while a file of the user is still uploading`() { + val upload = ownMessage(-5, SendStatus.PENDING).apply { + messageParameters = hashMapOf("file" to hashMapOf("type" to "file")) + } + assertEquals(2623, readMarker(listOf(upload, incomingMessage(2623)))) + } - assertNull(marker) + @Test + fun `read marker waits for a file of the user that was shared`() { + val shared = ownMessage(-5, SendStatus.SENT_PENDING_ACK).apply { + messageParameters = hashMapOf("file" to hashMapOf("type" to "file")) + } + assertNull(readMarker(listOf(shared, incomingMessage(2623)))) } @Test fun `read marker of the incoming messages is sent when the user sent nothing`() { - val items = listOf(messageItem(2623), messageItem(2622)) - - val marker = ChatViewModel.readMarkerToSend( - localLastReadMessage = 2623, - conversationLastReadMessage = 2620, - newestOwnMessageId = ChatViewModel.newestOwnMessageId(items), - hasUnconfirmedOwnMessage = ChatViewModel.hasUnconfirmedOwnMessage(items) - ) - - assertEquals(2623, marker) + assertEquals(2623, readMarker(listOf(incomingMessage(2623), incomingMessage(2622)))) } @Test @@ -485,18 +496,32 @@ class ChatViewModelTest { @Test fun `newest own message ignores incoming and temporary messages`() { - val items = listOf( - messageItem(2630), - messageItem(-5, incoming = false), - messageItem(2624, incoming = false), - messageItem(2610, incoming = false) + val messages = listOf( + incomingMessage(2630), + ownMessage(-5, SendStatus.PENDING), + ownMessage(2624), + ownMessage(2610) ) - assertEquals(2624, ChatViewModel.newestOwnMessageId(items)) + assertEquals(2624, ChatViewModel.newestOwnMessageId(messages)) } - private fun messageItem(id: Int, incoming: Boolean = true): ChatViewModel.ChatItem.MessageItem = - ChatViewModel.ChatItem.MessageItem(uiMessage(id).copy(incoming = incoming)) + private fun incomingMessage(id: Int): ChatMessage = + ChatMessage().apply { + jsonMessageId = id + incoming = true + } + + private fun ownMessage(id: Int, status: SendStatus? = null): ChatMessage = + ChatMessage().apply { + jsonMessageId = id + incoming = false + isTemporary = id < 0 + sendStatus = status + } + + private fun messageItem(id: Int): ChatViewModel.ChatItem.MessageItem = + ChatViewModel.ChatItem.MessageItem(uiMessage(id)) private fun uiMessage(id: Int): ChatMessageUi = ChatMessageUi( From 82fc8244497dad874011d967d582b8daeee68a46 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Wed, 14 Oct 2026 18:46:14 +0000 Subject: [PATCH 3/5] fix(chat): keep updating the local read state while an own message is queued A text message queued offline (SendStatus.PENDING) made readMarkerToSend return null, so setChatReadMessage was skipped as a whole, including the local read state update. The conversation list kept showing the messages as unread until the message was sent and the next sync. Write the local read state whenever the conversation lacks it and skip only the server request while an own message is on its way, so the marker is still never sent below the own message. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/viewmodels/ChatViewModel.kt | 40 ++++++++++++++----- .../talk/chat/viewmodels/ChatViewModelTest.kt | 35 ++++++++++++++++ 2 files changed, 64 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 2063640e5a..66b8fbb87a 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -2139,16 +2139,24 @@ class ChatViewModel @AssistedInject constructor( Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, conversation.lastReadMessage: $conversationLastReadMessage") val messages = latestMessages - val marker = readMarkerToSend( + val newestOwnMessageId = newestOwnMessageId(messages) + val marker = localReadMarker( localLastReadMessage = localLastReadMessage, conversationLastReadMessage = conversationLastReadMessage, - newestOwnMessageId = newestOwnMessageId(messages), - hasUnconfirmedOwnMessage = hasUnconfirmedOwnMessage(messages) + newestOwnMessageId = newestOwnMessageId ) if (marker != null) { - Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, setChatReadMessage...") + // The local read state is always written; only the server request waits for an own message + // that is still on its way (offline queue included), see readMarkerToSend. + val sendToServer = readMarkerToSend( + localLastReadMessage = localLastReadMessage, + conversationLastReadMessage = conversationLastReadMessage, + newestOwnMessageId = newestOwnMessageId, + hasUnconfirmedOwnMessage = hasUnconfirmedOwnMessage(messages) + ) != null + Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, setChatReadMessage, sendToServer: $sendToServer") - setChatReadMessage(marker) + setChatReadMessage(marker, sendToServer) } } @@ -2191,7 +2199,7 @@ class ChatViewModel @AssistedInject constructor( ) } - fun setChatReadMessage(lastReadMessage: Int) { + fun setChatReadMessage(lastReadMessage: Int, sendToServer: Boolean = true) { // marking as read is the explicit counterpart of marking as unread and hands the read marker // back to the automatic handling keepMarkedAsUnread = false @@ -2200,6 +2208,7 @@ class ChatViewModel @AssistedInject constructor( viewModelScope.launch { chatRepository.updateLocalReadState(lastReadMessage) } + if (!sendToServer) return ReadMarkerSyncWorker.enqueue( context = NextcloudTalkApplication.sharedApplication!!.applicationContext, userId = currentUser.id!!, @@ -2864,11 +2873,20 @@ class ChatViewModel @AssistedInject constructor( conversationLastReadMessage: Int, newestOwnMessageId: Int?, hasUnconfirmedOwnMessage: Boolean - ): Int? { - if (hasUnconfirmedOwnMessage) return null - val marker = maxOf(localLastReadMessage, newestOwnMessageId ?: 0) - return marker.takeIf { it > conversationLastReadMessage } - } + ): Int? = + localReadMarker(localLastReadMessage, conversationLastReadMessage, newestOwnMessageId) + .takeUnless { hasUnconfirmedOwnMessage } + + /** + * The read marker to write into the local conversation state, or null when it has it already. + * Unlike [readMarkerToSend] it does not wait for an own message that is on its way: the local state + * is not sent anywhere, so the conversation list loses its unread count right away, also offline. + */ + internal fun localReadMarker( + localLastReadMessage: Int, + conversationLastReadMessage: Int, + newestOwnMessageId: Int? + ): Int? = maxOf(localLastReadMessage, newestOwnMessageId ?: 0).takeIf { it > conversationLastReadMessage } /** * Ids of the messages a delete of [messageId] applies to: every already-uploaded message of the diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt index 3a5865f430..4c025992cb 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt @@ -463,6 +463,41 @@ class ChatViewModelTest { assertNull(readMarker(listOf(ownMessage(-77, SendStatus.SENT_PENDING_ACK), incomingMessage(2623)))) } + private fun localReadMarker(messages: List, local: Int = 2623, conversation: Int = 2620): Int? = + ChatViewModel.localReadMarker( + localLastReadMessage = local, + conversationLastReadMessage = conversation, + newestOwnMessageId = ChatViewModel.newestOwnMessageId(messages) + ) + + @Test + fun `local read state is still written while the own message waits to be sent offline`() { + // 5 unread messages read offline, then a reply that is only queued (PENDING): the server request + // has to wait, the conversation list must not keep showing the messages as unread + val messages = listOf(ownMessage(-77, SendStatus.PENDING), incomingMessage(2623)) + + assertNull(readMarker(messages)) + assertEquals(2623, localReadMarker(messages)) + } + + @Test + fun `local read state is still written while the own message waits for its server copy`() { + val messages = listOf(ownMessage(-77, SendStatus.SENT_PENDING_ACK), incomingMessage(2623)) + + assertNull(readMarker(messages)) + assertEquals(2623, localReadMarker(messages)) + } + + @Test + fun `local read state is not written below the own message just sent`() { + assertEquals(2624, localReadMarker(listOf(ownMessage(2624), incomingMessage(2623)))) + } + + @Test + fun `local read state is not written when the conversation already has it`() { + assertNull(localReadMarker(listOf(incomingMessage(2623)), local = 2620, conversation = 2620)) + } + @Test fun `read marker is sent when the own message failed to send`() { assertEquals(2623, readMarker(listOf(ownMessage(-77, SendStatus.FAILED), incomingMessage(2623)))) From 79c2f6b98560e9bcfc794e36397ba8686a7036ef Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Wed, 14 Oct 2026 19:07:36 +0000 Subject: [PATCH 4/5] fix(chat): do not lose the read marker held back by a queued own message The previous commit wrote the full local read state while an own message was queued. The stored marker then equalled the held-back one, so the "newer than the conversation marker" check suppressed the send for good: after the message failed, the server stayed unread and the pending marker guarded the local state. While the marker has to wait, lower only the unread count of the conversation entry. The stored marker stays the one the server knows and nothing is registered as pending, so the marker is decided again by the next call and sent once the message is confirmed or failed. Add a test on the real ChatViewModel for the path updateRemoteLastReadMessageIfNeeded -> repository -> ReadMarkerSyncWorker. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/data/ChatMessageRepository.kt | 8 + .../network/OfflineFirstChatRepository.kt | 4 + .../talk/chat/viewmodels/ChatViewModel.kt | 10 +- .../data/network/ConversationListUpdater.kt | 18 ++ .../viewmodels/ChatViewModelReadMarkerTest.kt | 182 ++++++++++++++++++ ...onversationListFreshnessIntegrationTest.kt | 18 ++ 6 files changed, 239 insertions(+), 1 deletion(-) create mode 100644 app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt index 1d8fc219b4..11a2a806c1 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/ChatMessageRepository.kt @@ -79,6 +79,14 @@ interface ChatMessageRepository : LifecycleAwareManager { */ suspend fun updateLocalReadState(lastReadMessage: Int) + /** + * Like [updateLocalReadState], but for a marker that cannot be sent yet (an own message is still on its + * way): only the unread count of the conversation entry is lowered, the stored read marker stays the + * one the server knows and no pending marker is registered. So the server stays the authority for the + * read state, and the marker is still sent as soon as the own message is confirmed or failed. + */ + suspend fun updateLocalUnreadCountOnly(lastReadMessage: Int) + /** * Writes the local read state for a message the user marked as unread: the read marker moves back * to [lastReadMessage] and the conversation keeps at least one unread message until the server diff --git a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt index 180f3cb17e..6fe4bd45b8 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/data/network/OfflineFirstChatRepository.kt @@ -430,6 +430,10 @@ class OfflineFirstChatRepository @Inject constructor( conversationListUpdater.updateLocalReadState(syncTarget, lastReadMessage) } + override suspend fun updateLocalUnreadCountOnly(lastReadMessage: Int) { + conversationListUpdater.updateLocalUnreadCountOnly(syncTarget, lastReadMessage) + } + override suspend fun updateLocalUnreadState(lastReadMessage: Int) { conversationListUpdater.updateLocalUnreadState(syncTarget, lastReadMessage) } diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 66b8fbb87a..203b0b5ca4 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -2204,11 +2204,19 @@ class ChatViewModel @AssistedInject constructor( // back to the automatic handling keepMarkedAsUnread = false _uiState.update { it.copy(markedAsUnreadByUser = false) } + if (!sendToServer) { + // The marker waits for an own message that is on its way: show the lower unread count, but keep + // the stored marker and register nothing pending, so the marker is decided again (and sent) by + // the next call once the message is confirmed or failed. + viewModelScope.launch { + chatRepository.updateLocalUnreadCountOnly(lastReadMessage) + } + return + } chatRepository.markPendingReadMarker(lastReadMessage) viewModelScope.launch { chatRepository.updateLocalReadState(lastReadMessage) } - if (!sendToServer) return ReadMarkerSyncWorker.enqueue( context = NextcloudTalkApplication.sharedApplication!!.applicationContext, userId = currentUser.id!!, diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt index 8866b601d4..caee217275 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt @@ -160,6 +160,24 @@ class ConversationListUpdater @Inject constructor( Log.d(TAG, "Local read state for room ${target.roomToken}: lastRead=$lastReadMessage, unread=$unreadMessages") } + /** + * Lowers only the unread count of the conversation entry to what lies above [lastReadMessage]. The stored + * read marker stays and no pending marker is registered: the marker has not been sent, so the server + * state remains the authority and a room list sync may restore it. Used while an own message is on + * its way and the marker has to wait for it. + */ + suspend fun updateLocalUnreadCountOnly(target: ChatMessageSyncer.SyncTarget, lastReadMessage: Int) { + val conversation = + conversationsDao.getConversationForUser(target.accountId, target.roomToken).first() ?: return + val unreadMessages = chatDao.countMessagesNewerThan( + internalConversationId = target.internalConversationId, + messageId = lastReadMessage.toLong(), + excludedActorId = target.user.userId!! + ) + conversationsDao.updateReadState(conversation.internalId, conversation.lastReadMessage, unreadMessages) + Log.d(TAG, "Local unread count for room ${target.roomToken}: unread=$unreadMessages (marker not sent yet)") + } + /** * Writes the read state for a message the user marked as unread: the read marker moves back to * [lastReadMessage] and the conversation is left with at least one unread message, so the entry diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt new file mode 100644 index 0000000000..dac0887057 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt @@ -0,0 +1,182 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.chat.viewmodels + +import android.os.Looper +import androidx.work.WorkManager +import androidx.work.testing.WorkManagerTestInitHelper +import com.nextcloud.talk.application.NextcloudTalkApplication +import com.nextcloud.talk.chat.data.ChatMessageRepository +import com.nextcloud.talk.chat.data.model.ChatMessage +import com.nextcloud.talk.data.database.model.SendStatus +import com.nextcloud.talk.data.user.model.User +import com.nextcloud.talk.models.domain.ConversationModel +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.MutableStateFlow +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mockito +import org.mockito.Mockito.RETURNS_DEEP_STUBS +import org.mockito.invocation.InvocationOnMock +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import org.robolectric.annotation.Config + +/** + * The read marker path of the real [ChatViewModel] when the chat is left: updateRemoteLastReadMessageIfNeeded -> + * local read state in the repository -> ReadMarkerSyncWorker. The repository is a fake that keeps the stored + * read marker like Room does: the view model sees it as conversation.lastReadMessage afterwards. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33]) +class ChatViewModelReadMarkerTest { + + private val calls = mutableListOf>() + private lateinit var viewModel: ChatViewModel + + // the setter of the singleton is protected: only the real application sets it + private val sharedApplicationField = NextcloudTalkApplication::class.java + .getDeclaredField("sharedApplication") + .apply { isAccessible = true } + private var previousApplication: NextcloudTalkApplication? = null + + @Before + fun setUp() { + val context = RuntimeEnvironment.getApplication() + WorkManagerTestInitHelper.initializeTestWorkManager(context) + previousApplication = sharedApplicationField.get(null) as NextcloudTalkApplication? + val application = mock() + whenever(application.applicationContext).thenReturn(context) + sharedApplicationField.set(null, application) + + viewModel = newViewModel() + viewModel.advanceLocalLastReadMessageIfNeeded(LOCAL_MARKER) + storedMarker(STORED_MARKER) + } + + @After + fun tearDown() { + sharedApplicationField.set(null, previousApplication) + } + + @Test + fun `leaving the chat with an own message queued still updates the local read state`() { + messages(ownMessage(-77, SendStatus.PENDING), incomingMessage(LOCAL_MARKER)) + + viewModel.updateRemoteLastReadMessageIfNeeded() + shadowOf(Looper.getMainLooper()).idle() + + assertTrue("local read state written: $calls", calls.any { it.first.startsWith("updateLocal") }) + assertEquals("no marker is sent to the server", 0, queuedMarkers()) + } + + @Test + fun `a marker held back by a queued own message is sent once that message failed`() { + messages(ownMessage(-77, SendStatus.PENDING), incomingMessage(LOCAL_MARKER)) + viewModel.updateRemoteLastReadMessageIfNeeded() + shadowOf(Looper.getMainLooper()).idle() + assertEquals(0, queuedMarkers()) + + messages(ownMessage(-77, SendStatus.FAILED), incomingMessage(LOCAL_MARKER)) + viewModel.updateRemoteLastReadMessageIfNeeded() + shadowOf(Looper.getMainLooper()).idle() + + assertEquals("the marker reaches the server after the failure", 1, queuedMarkers()) + } + + @Test + fun `a confirmed own message moves the marker to the message and sends it`() { + messages(ownMessage(OWN_MESSAGE_ID, null), incomingMessage(LOCAL_MARKER)) + + viewModel.updateRemoteLastReadMessageIfNeeded() + shadowOf(Looper.getMainLooper()).idle() + + assertEquals("marker is sent", 1, queuedMarkers()) + assertEquals("never below the own message", OWN_MESSAGE_ID, storedMarkerValue) + } + + private var storedMarkerValue = STORED_MARKER + + // what Room does: the conversation entry carries the stored marker, the view model observes it + private fun storedMarker(value: Int) { + storedMarkerValue = value + val conversation = mock() + whenever(conversation.lastReadMessage).thenReturn(value) + @Suppress("UNCHECKED_CAST") + val state = viewModel.uiState as MutableStateFlow + state.value = state.value.copy(conversation = conversation) + } + + private fun messages(vararg messages: ChatMessage) { + ChatViewModel::class.java.getDeclaredField("latestMessages") + .apply { isAccessible = true } + .set(viewModel, messages.toList()) + } + + private fun queuedMarkers(): Int = + WorkManager.getInstance(RuntimeEnvironment.getApplication()) + .getWorkInfosForUniqueWork("read-marker-sync-1@room") + .get() + .size + + private fun repositoryAnswer(invocation: InvocationOnMock): Any? { + val name = invocation.method.name + calls.add(name to invocation.arguments.firstOrNull()) + if (name == "updateLocalReadState") { + storedMarker(invocation.arguments[0] as Int) + return Unit + } + return Mockito.RETURNS_DEFAULTS.answer(invocation) + } + + private fun newViewModel(): ChatViewModel = + ChatViewModel( + logger = mock(), + appPreferences = mock(defaultAnswer = RETURNS_DEEP_STUBS), + chatNetworkDataSource = mock(defaultAnswer = RETURNS_DEEP_STUBS), + chatRepository = Mockito.mock(ChatMessageRepository::class.java) { repositoryAnswer(it) }, + threadsRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + conversationRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + reactionsRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + unifiedSearchRepository = mock(defaultAnswer = RETURNS_DEEP_STUBS), + mediaRecorderManager = mock(defaultAnswer = RETURNS_DEEP_STUBS), + audioFocusRequestManager = mock(defaultAnswer = RETURNS_DEEP_STUBS), + userManager = mock(defaultAnswer = RETURNS_DEEP_STUBS), + appScope = CoroutineScope(Dispatchers.Unconfined), + initialUser = User().apply { id = 1L }, + chatRoomToken = "room", + conversationThreadId = null + ) + + private fun incomingMessage(id: Int): ChatMessage = + ChatMessage().apply { + jsonMessageId = id + incoming = true + } + + private fun ownMessage(id: Int, status: SendStatus?): ChatMessage = + ChatMessage().apply { + jsonMessageId = id + incoming = false + isTemporary = id < 0 + sendStatus = status + } + + companion object { + private const val STORED_MARKER = 2620 + private const val LOCAL_MARKER = 2623 + private const val OWN_MESSAGE_ID = 2624 + } +} diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt index d5f86bc33f..a70d936dd3 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt @@ -250,6 +250,24 @@ class ConversationListFreshnessIntegrationTest { } } + @Test + fun `unread count only write keeps the stored marker and registers nothing pending`() { + runBlocking { + seedConversation(lastActivity = 12, lastReadMessage = 10, unreadMessages = 2) + wheneverBlocking { chatNetwork.pullChatMessages(any(), any(), any()) } + .thenReturn( + Response.success(overall(message(10), message(11), message(12))) + ) + syncer.catchUpRoom(target(), lastReadMessage = 10, unreadMessages = 2) + + conversationListUpdater.updateLocalUnreadCountOnly(target(), lastReadMessage = 12) + + assertEquals("the marker the server knows stays", 10, conversationEntity().lastReadMessage) + assertEquals("the list shows the messages as read", 0, conversationEntity().unreadMessages) + assertEquals(null, conversationListUpdater.pendingReadMarker(INTERNAL_CONVERSATION_ID)) + } + } + @Test fun `a sync cannot revert the read state while its marker is pending`() { val user = user(withKeepNotificationsCapability = false) From cc98e552092e273ab393e37536aaa319d837a66e Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Wed, 14 Oct 2026 19:26:44 +0000 Subject: [PATCH 5/5] fix(chat): keep the read marker state when counting and when a message window is shown Review findings on the held-back read marker: - updateLocalUnreadCountOnly read the conversation, suspended on the count and then wrote the stored marker back with updateReadState, so a marker written meanwhile was overwritten with the old one. Write only the unread count (ConversationsDao.updateUnreadMessages). - The decision on the read marker used the last list the view model received. In search and old chat block mode that is a window around a quoted message, which can miss the own message just sent: leaving the chat from there sent a marker below it. Keep the list of the newest messages and ignore anchored windows. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb --- .../talk/chat/viewmodels/ChatViewModel.kt | 8 +++- .../data/network/ConversationListUpdater.kt | 3 +- .../data/database/dao/ConversationsDao.kt | 9 +++++ .../utils/preview/ComposePreviewUtilsDaos.kt | 4 ++ .../viewmodels/ChatViewModelReadMarkerTest.kt | 40 +++++++++++++++++++ ...onversationListFreshnessIntegrationTest.kt | 21 ++++++++++ 6 files changed, 83 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt index 203b0b5ca4..2646f7e2bf 100644 --- a/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt @@ -2107,8 +2107,14 @@ class ChatViewModel @AssistedInject constructor( // Runs on the background dispatcher of the rebuild pipeline (flowOn in observeMessages): it only // publishes the immutable list through a volatile field. localLastReadMessage is written on the main // thread only, so the own message is combined with it when the marker is decided (readMarkerToSend). + // + // Only the list of the newest messages counts. A window around a quoted or searched message (search and + // old chat block mode) can lie far from them and miss the own message just sent, which must still be + // known when the chat is left from there. private fun onMessagesChanged(messages: List) { - latestMessages = messages + if (chatMode.value == ChatMode.DEFAULT_MODE || contextAnchorMessageId.value == null) { + latestMessages = messages + } } fun advanceLocalLastReadMessageIfNeeded(messageId: Int) { diff --git a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt index caee217275..8ce116afff 100644 --- a/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt +++ b/app/src/main/java/com/nextcloud/talk/conversationlist/data/network/ConversationListUpdater.kt @@ -174,7 +174,8 @@ class ConversationListUpdater @Inject constructor( messageId = lastReadMessage.toLong(), excludedActorId = target.user.userId!! ) - conversationsDao.updateReadState(conversation.internalId, conversation.lastReadMessage, unreadMessages) + // only the count: the marker read above must not be written back over one stored while counting + conversationsDao.updateUnreadMessages(conversation.internalId, unreadMessages) Log.d(TAG, "Local unread count for room ${target.roomToken}: unread=$unreadMessages (marker not sent yet)") } diff --git a/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt b/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt index bbf19adb22..286a830f48 100644 --- a/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt +++ b/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt @@ -104,6 +104,15 @@ interface ConversationsDao { ) suspend fun updateReadState(internalId: String, lastReadMessage: Int, unreadMessages: Int) + @Query( + """ + UPDATE Conversations + SET unreadMessages = :unreadMessages + WHERE internalId = :internalId + """ + ) + suspend fun updateUnreadMessages(internalId: String, unreadMessages: Int) + /** * Deletes rows in the db matching the specified [conversationIds] */ diff --git a/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt b/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt index 2ce7abc503..40146986ce 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt +++ b/app/src/main/java/com/nextcloud/talk/utils/preview/ComposePreviewUtilsDaos.kt @@ -320,6 +320,10 @@ class DummyConversationDaoImpl : ConversationsDao { /* */ } + override suspend fun updateUnreadMessages(internalId: String, unreadMessages: Int) { + /* */ + } + override fun insertConversation(conversation: ConversationEntity) { /* */ } diff --git a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt index dac0887057..853c09fa86 100644 --- a/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt @@ -39,6 +39,7 @@ import org.robolectric.annotation.Config * local read state in the repository -> ReadMarkerSyncWorker. The repository is a fake that keeps the stored * read marker like Room does: the view model sees it as conversation.lastReadMessage afterwards. */ +@Suppress("TooManyFunctions") @RunWith(RobolectricTestRunner::class) @Config(sdk = [33]) class ChatViewModelReadMarkerTest { @@ -107,6 +108,44 @@ class ChatViewModelReadMarkerTest { assertEquals("never below the own message", OWN_MESSAGE_ID, storedMarkerValue) } + @Test + fun `a window around an old message does not make the view model forget a queued own message`() { + messages(ownMessage(-77, SendStatus.PENDING), incomingMessage(LOCAL_MARKER)) + openOldMessageWindow(incomingMessage(OLD_MESSAGE_ID)) + + viewModel.updateRemoteLastReadMessageIfNeeded() + shadowOf(Looper.getMainLooper()).idle() + + assertEquals("nothing is sent below the queued own message", 0, queuedMarkers()) + } + + @Test + fun `a window around an old message does not make the view model forget the own message just sent`() { + messages(ownMessage(OWN_MESSAGE_ID, null), incomingMessage(LOCAL_MARKER)) + openOldMessageWindow(incomingMessage(OLD_MESSAGE_ID)) + + viewModel.updateRemoteLastReadMessageIfNeeded() + shadowOf(Looper.getMainLooper()).idle() + + assertEquals("marker is sent", 1, queuedMarkers()) + assertEquals("never below the own message", OWN_MESSAGE_ID, storedMarkerValue) + } + + // the chat shows a window around a quoted or searched message that is far from the newest messages; + // the database then delivers that window instead of the newest messages + private fun openOldMessageWindow(vararg window: ChatMessage) { + fun field(name: String) = ChatViewModel::class.java.getDeclaredField(name).apply { isAccessible = true } + + @Suppress("UNCHECKED_CAST") + (field("contextAnchorMessageId").get(viewModel) as MutableStateFlow).value = OLD_MESSAGE_ID.toLong() + @Suppress("UNCHECKED_CAST") + (field("_chatMode").get(viewModel) as MutableStateFlow).value = + ChatViewModel.ChatMode.OLD_CHATBLOCK_MODE + ChatViewModel::class.java.getDeclaredMethod("onMessagesChanged", List::class.java) + .apply { isAccessible = true } + .invoke(viewModel, window.toList()) + } + private var storedMarkerValue = STORED_MARKER // what Room does: the conversation entry carries the stored marker, the view model observes it @@ -178,5 +217,6 @@ class ChatViewModelReadMarkerTest { private const val STORED_MARKER = 2620 private const val LOCAL_MARKER = 2623 private const val OWN_MESSAGE_ID = 2624 + private const val OLD_MESSAGE_ID = 1500 } } diff --git a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt index a70d936dd3..8c6c8cced5 100644 --- a/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt +++ b/app/src/test/java/com/nextcloud/talk/conversationlist/data/network/ConversationListFreshnessIntegrationTest.kt @@ -16,6 +16,7 @@ import com.nextcloud.talk.arbitrarystorage.ArbitraryStorageManager import com.nextcloud.talk.chat.data.model.ChatMessage import com.nextcloud.talk.chat.data.network.ChatMessageSyncer import com.nextcloud.talk.chat.data.network.ChatNetworkDataSource +import com.nextcloud.talk.data.database.dao.ChatMessagesDao import com.nextcloud.talk.data.database.mappers.asEntity import com.nextcloud.talk.data.database.model.ConversationEntity import com.nextcloud.talk.data.network.NetworkMonitor @@ -50,6 +51,7 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull +import org.mockito.kotlin.doSuspendableAnswer import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import org.mockito.kotlin.wheneverBlocking @@ -268,6 +270,25 @@ class ConversationListFreshnessIntegrationTest { } } + @Test + fun `unread count only write does not overwrite a marker written while it counts`() { + // a full read state write (the marker of a sent message) lands while the count is being made + val countingChatDao = mock() + val updater = ConversationListUpdater(countingChatDao, db.chatBlocksDao(), db.conversationsDao()) + runBlocking { + seedConversation(lastActivity = 12, lastReadMessage = 10, unreadMessages = 2) + wheneverBlocking { countingChatDao.countMessagesNewerThan(any(), any(), any()) } + .doSuspendableAnswer { + db.conversationsDao().updateReadState(INTERNAL_CONVERSATION_ID, 15, 0) + 0 + } + + updater.updateLocalUnreadCountOnly(target(), lastReadMessage = 12) + + assertEquals("the marker written meanwhile stays", 15, conversationEntity().lastReadMessage) + } + } + @Test fun `a sync cannot revert the read state while its marker is pending`() { val user = user(withKeepNotificationsCapability = false)