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 1d8fc219b46..11a2a806c18 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 180f3cb17eb..6fe4bd45b8c 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 140f2f8b0ea..2646f7e2bf3 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 }, @@ -2095,6 +2104,19 @@ 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) { + if (chatMode.value == ChatMode.DEFAULT_MODE || contextAnchorMessageId.value == null) { + latestMessages = messages + } + } + fun advanceLocalLastReadMessageIfNeeded(messageId: Int) { Log.d(TAG, "advanceLocalLastReadMessageIfNeeded, messageId: $messageId") Log.d(TAG, "advanceLocalLastReadMessageIfNeeded, localLastReadMessage: $localLastReadMessage") @@ -2122,10 +2144,25 @@ class ChatViewModel @AssistedInject constructor( val conversationLastReadMessage = _uiState.value.conversation?.lastReadMessage ?: return Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, conversation.lastReadMessage: $conversationLastReadMessage") - if (localLastReadMessage > conversationLastReadMessage) { - Log.d(TAG, "updateRemoteLastReadMessageIfNeeded, setChatReadMessage...") + val messages = latestMessages + val newestOwnMessageId = newestOwnMessageId(messages) + val marker = localReadMarker( + localLastReadMessage = localLastReadMessage, + conversationLastReadMessage = conversationLastReadMessage, + newestOwnMessageId = newestOwnMessageId + ) + if (marker != null) { + // 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(localLastReadMessage) + setChatReadMessage(marker, sendToServer) } } @@ -2168,11 +2205,20 @@ 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 _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) @@ -2804,6 +2850,58 @@ 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(messages: List): Int? = + messages.filter { !it.incoming && it.jsonMessageId > 0 } + .maxOfOrNull { it.jsonMessageId } + + /** + * 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(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 + * 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? = + 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 * media group the message is shown in, or just the message itself. 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 8866b601d44..8ce116afffc 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,25 @@ 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!! + ) + // 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)") + } + /** * 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/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt b/app/src/main/java/com/nextcloud/talk/data/database/dao/ConversationsDao.kt index bbf19adb224..286a830f48a 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 2ce7abc5034..40146986ceb 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 new file mode 100644 index 00000000000..853c09fa864 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelReadMarkerTest.kt @@ -0,0 +1,222 @@ +/* + * 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. + */ +@Suppress("TooManyFunctions") +@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) + } + + @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 + 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 + private const val OLD_MESSAGE_ID = 1500 + } +} 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 562bfb1538d..4c025992cb9 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 @@ -432,6 +434,127 @@ class ChatViewModelTest { assertNull(ChatViewModel.readMarkerForMarkingUnread(items, messageId = 77)) } + // 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. 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`() { + assertEquals(2624, readMarker(listOf(ownMessage(2624), incomingMessage(2623)))) + } + + @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)))) + } + + @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)))) + } + + 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)))) + } + + @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)))) + } + + @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`() { + assertEquals(2623, readMarker(listOf(incomingMessage(2623), incomingMessage(2622)))) + } + + @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 messages = listOf( + incomingMessage(2630), + ownMessage(-5, SendStatus.PENDING), + ownMessage(2624), + ownMessage(2610) + ) + + assertEquals(2624, ChatViewModel.newestOwnMessageId(messages)) + } + + 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)) 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 d5f86bc33fd..8c6c8cced5f 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 @@ -250,6 +252,43 @@ 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 `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)